Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add {-# WARNING #-} to Data.List.{head,tail} #87

Closed
Bodigrim opened this issue Sep 11, 2022 · 177 comments
Closed

Add {-# WARNING #-} to Data.List.{head,tail} #87

Bodigrim opened this issue Sep 11, 2022 · 177 comments
Labels
approved Approved by CLC vote base-4.19 Implemented in base-4.19 (GHC 9.8)

Comments

@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 11, 2022

Haddocks for Data.List (technically, GHC.List) warn against head and tail on the ground of their partiality.

I propose to promote these warnings to the pragma level as per MR !9290:

{-# WARNING head "This is a partial function, it throws an error on empty lists. Use pattern matching or Data.List.uncons instead. Consider refactoring to use Data.List.NonEmpty." #-}
{-# WARNING tail "This is a partial function, it throws an error on empty lists. Replace it with drop 1, or use pattern matching or Data.List.uncons instead. Consider refactoring to use Data.List.NonEmpty." #-}

I do not propose any further steps such as deprecation or removal of these functions. This is deliberately as conservative as possible. See #70 for a wider discussion of a wider proposal.

Why only head and tail? Because these are functions, for which the widest range of replacements exist, almost always allowing for a safe, concise and local fix (see examples below). E. g., for init / last there is currently no such replacement (one must push for addition of Data.List.unsnoc first), and things like !! and maximum are even worse.

Why {-# WARNING #-} and not {-# DEPRECATED #-}? Because deprecation implies a future removal, and ambitions of this proposal are much smaller. It's already enshrined in base that these functions deserve a warning, we just promote its visibility, which should be less controversial.

The impact of the change is that users of head and tail will receive a GHC warning message. This is not an error and does not prevent from compilation, thus is not a breaking change. Users are recommended to follow the suggestion, or disable -Wno-warnings-deprecations (which is a sensible thing to do, for example, in a test suite), but they are also free to do nothing at all. Old packages will continue to work.

To avoid any confusion, -Wno-warnings-deprecations suppresses {-# WARNING #-} and {-# DEPRECATED #-}, but not any other GHC warnings. Those, who enabled -Werror, can pass -Wwarn=warnings-deprecations to downgrade this particular group back from errors to warnings. GHCi users can put :set -Wno-warnings-deprecations into their .ghci config.

There is a concern that -Wno-warnings-deprecations disables all {-# WARNING #-} and {-# DEPRECATED #-}, whatever the source. However, current Haskell ecosystem rarely makes much use of them, so I believe it is still a palatable compromise between seeing no warnings and making no changes.

Hardcore fans of head and tail, who are not satisfied with disabling warnings, are welcome to create a local file or even release a package, providing, say, Data.List.Partial, containing original definitions of head and tail without {-# WARNING #-}. I'm however opposed to introducing such Data.List.Partial into base itself: we won't be able to root it out ever.

GHC proposals ghc-proposals/ghc-proposals#454 and ghc-proposals/ghc-proposals#541 propose extensions to GHC warnings mechanism. Unfortunately, neither of them is approved or has a committed implementor, and this status does not seem to change soon, so it would be wrong to speculate on their precise nature. If and when they become a part of GHC, one can indeed ask for a review of {-# WARNING #-} pragmas.


How would you rewrite instance MonadFix [] without head and tail?

instance MonadFix [] where
    mfix f = case fix (f . head) of
               []    -> []
               (x:_) -> x : mfix (tail . f)

I'd rewrite it this way:

instance MonadFix [] where
    mfix f = case fix (take 1 >=> f) of
               []    -> []
               (x:_) -> x : mfix (drop 1 . f)

How would you rewrite this snippet?

case product xs of
  1 -> foo
  n -> bar n (head xs)

Besides options in #87 (comment), one can do this:

case (xs, product xs) of
  ([], _)    -> foo
  (_, 1)     -> foo
  (x : _, n) -> bar n x

or (if you insist on exactly two clauses):

case xs of
  x : _ | n <- product xs, n /= 1 = bar n x
  _ -> foo

How would you rewrite this snippet?

head $ filter (`notElem` hashes) $ map showt [0::Int ..]

I'd use a proper library for infinite lists aka streams: Stream, streams or infinite-list. E. g., Stream provides total head :: Stream a -> a and filter :: Stream a -> Stream a, so the snippet can be rewritten in a total way as

import Data.Stream as S 
S.head $ S.filter (`notElem` hashes) $ S.map showt $ S.iterate (+1) (0 :: Int)

infinite-list can make it even neater offering (0...) syntax to replace [0..].

@Boarders
Copy link

Boarders commented Sep 12, 2022

What do you see as the advantage of this over using a tool like hlint? I personally think there are times where functions will have internal invariants that are not easily captured by the type system and such functions can be perfectly safe. Moreover, if a user is likely to misuse head they are just as likely to misuse propagating the error forward with their own pattern match (were they to see this warning but not understand the point of it).

@brandonchinn178
Copy link

Why only head and tail? Because these are functions, for which safe concise drop-in replacements exist. E. g., for init / last there is currently no such replacement (one must push for addition of Data.List.unsnoc first), and things like !! and maximum are even worse.

Can you explain further? I dont see how head would have a safe drop-in replacement; any such drop-in replacement is necessarily partial.

All of these have a non-partial non-drop-in replacement:

headSafe = fmap NonEmpty.head . nonEmpty
lastSafe = fmap NonEmpty.last . nonEmpty
tailSafe = fmap NonEmpty.tail . nonEmpty
initSafe = fmap NonEmpty.init . nonEmpty

Note that since the flag to disable this warning is the same flag to disable deprecations, this proposal effectively deprecates head and tail, which I'm not sure we should do in a one-off operation.

We don't do this for any other partial function [in base]. I don't think head and tail are special enough to make an exception. Now, maybe if we had a push to deprecate all partial functions in base (which I'm not opposed to), I'd be on board...

@cdsmith
Copy link

cdsmith commented Sep 12, 2022

I would strongly oppose this change. It completely ignores users of GHCi, educational users, and other situations in which there's absolutely zero reason to avoid head. Even if you take it on principle that using head is somehow "bad behavior" that should be punished, it also pushes users toward the wrong solution. Uses of head/tail happen when you're certain the input is non-empty, and the default solution there, whenever possible, should be change the type to NonEmpty, not to add useless boilerplate dealing with an impossible branch in your code.

All of this might be overlooked if there were a compelling benefit. But having two fewer partial functions (and, frankly, quite possibly two of the most obviously partial and therefore least problematic) doesn't change much when there are so many more remaining.

Honestly, this would feel to me like an effort to deliberately annoy users the Haskell community disagrees with and drive them away, more than a way to help anyone.

@treeowl
Copy link

treeowl commented Sep 12, 2022

How would this affect users of Liquid Haskell?

@josephcsible
Copy link
Contributor

josephcsible commented Sep 12, 2022

It completely ignores users of GHCi, educational users, and other situations in which there's absolutely zero reason to avoid head.

I'd argue that you should still avoid it in these cases, especially education, since you don't want to teach people bad habits that they have to later unlearn when they start writing production code.

Uses of head/tail happen when you're certain the input is non-empty, and the default solution there, whenever possible, should be change the type to NonEmpty, not to add useless boilerplate dealing with an impossible branch in your code.

I agree with this, but I don't see why it's a reason not to make this change. (Or was this in regard to the specific text of the warning pragma, rather than its existence? If so, then I agree it should probably mention NonEmpty too.)

But having two fewer partial functions (and, frankly, quite possibly two of the most obviously partial and therefore least problematic) doesn't change much when there are so many more remaining.

Sure, it doesn't change much, but it's still a small step in the right direction.

@ocramz
Copy link

ocramz commented Sep 12, 2022

I agree with this proposal. It's a great way to move the language forward, because it will prod both newcomers and old hands to use safer alternatives such as pattern matching.
Anecdotally, I never use head/tail.

What do you see as the advantage of this over using a tool like hlint?

@Boarders hlint is great but does not ship with GHC and many beginners don't know about it

if a user is likely to misuse head they are just as likely to misuse propagating the error forward with their own pattern match

@Boarders really? What's the evidence for this?

maybe if we had a push to deprecate all partial functions in base I'd be on board...

@brandonchinn178 agree, let's do this!

@adamgundry
Copy link
Member

Please don't do this. There are perfectly reasonable uses for head and tail; the most common one I use is when writing tests, where the presence of an empty list should fail the test, and throwing an exception is a fine way to achieve that.

I would be fine with adding these warnings by default if there was an easy way to disable them, but -Wno-warnings-deprecations is too blunt an instrument. It would be different if we had something like ghc-proposals/ghc-proposals#454 and could attach a custom warning flag, say -Wno-incomplete-functions. (I see this was discussed somewhat before at #70 (comment).)

@chshersh
Copy link
Member

I fully support this proposal 💛

I'm yet to see a use case for head / tail which is better than NonEmpty / manual pattern matching / usage of custom error. In my experience, head and tail has this surprising property that it's always possible to painlessly rewrite their usages to NonEmpty or similar.

A warning doesn't take these functions away. It just warns about common pitfalls. So I see no harm in it.

I would strongly oppose this change. It completely ignores users of GHCi, educational users

Quite the opposite, it actually helps educational users because there's a warning about potential dangers of this function.

I personally think there are times where functions will have internal invariants that are not easily captured by the type system

Note that since the flag to disable this warning is the same flag to disable deprecations, this proposal effectively deprecates head and tail, which I'm not sure we should do in a one-off operation.

Uses of head/tail happen when you're certain the input is non-empty, and the default solution there

I would be fine with adding these warnings by default if there was an easy way to disable them, but -Wno-warnings-deprecations

Haskellers would rather suppress useful warnings than improve code maintainability 😌 We all are "Make illegal states unrepresentable" unless it's a specific partial function we have strong feelings about 😮‍💨


My suggestion to the proposal is to slightly change the wording to reflect the fact that the functions fail on empty lists (because it might not be obvious for Haskell beginners):

-{-# WARNING head "This is a partial function, use pattern matching or Data.List.uncons instead" #-}
+{-# WARNING head "This is a partial function, it throws an error on empty lists, use pattern matching or Data.List.uncons instead" #-}

@treeowl
Copy link

treeowl commented Sep 12, 2022

I'm yet to see a use case for head / tail which is better than NonEmpty / manual pattern matching / usage of custom error. In my experience, head and tail has this surprising property that it's always possible to painlessly rewrite their usages to NonEmpty or similar.

The most legitimate use I've seen is the MonadFix [] instance. There's only one of those though. Head and Tail type families are much more useful, IME, but I recognize that head and tail are helpful in GHCi sessions.

(FWIW, xs ~ (Head xs ': Tail xs) shows up relatively often.)

@googleson78
Copy link
Contributor

I think this is really highlighting the need for something like ghc-proposals/ghc-proposals#454 in order to more painlessly make subtractive improvements to the language (and libraries as well!).

@ElvishJerricco
Copy link

ElvishJerricco commented Sep 12, 2022

Sometimes whether the list is empty or not is ancillary to the logic at hand. Here's an incredibly trivial example:

case product xs of
  1 -> foo
  n -> bar n (head xs)

In the second case, we know for a certainty that the list is not empty because product [] == 1. Furthermore, in the first case, we do not know that the list is empty, because it could have been [1, 1, 1, 1]. The emptiness of the list is completely ancillary to the calculation we are interested in, and we would have had to make some strange custom version of product to include a NonEmpty in the result.

I understand that you usually shouldn't use these functions, but making it a warning seems misguided to me for three reasons.

  1. You can't disable it on a per-call basis. When you know you're using it right, you need to be able to shut the warning up. Disabling -Wno-warnings-deprecations for the whole module is bad.
  2. The warning isn't providing any information. It's not warning you about a mistake you may have missed. You know you used head or tail.
  3. Warnings generally shouldn't be complaints about coding style and they shouldn't be asking you to make fundamental changes to how you've structured your code. They should assume you generally know what you're doing and that you just need some help with things that are commonly missed. Using head or tail is not commonly missed. You either meant to do it or you didn't do it.

EDIT: To be clear, I'm not saying I think they should still be in Prelude; they definitely shouldn't. But the only warning they ought to have is a deprecation warning.

@tomjaguarpaw
Copy link
Member

when writing tests ... the presence of an empty list should fail the test, and throwing an exception is a fine way to achieve that.

It sounds like this is a good use case for a specially-named version of head provided by the testing framework in question (e.g. expectNonEmptyOrError). It doesn't seem justified to keep a partial function around in every1 Haskell module ever written just so it can be used for a small number of specialised purposes.

Footnotes

  1. barring NoImplicitPrelude etc.

@ParetoOptimalDev
Copy link

You can't disable it on a per-call basis. When you know you're using it right, you need to be able to shut the warning up. Disabling -Wno-warnings-deprecations for the whole module is bad.

What if the partial functions being deprecated were under a new flag?

Or if that's not enough... heck, give each partial function it's own flag if it clears a path to no partial functions in base.

@avanov
Copy link

avanov commented Sep 12, 2022

This will negatively affect those who use -Werror seriously

@tomjaguarpaw
Copy link
Member

This will negatively affect those who use -Werror seriously

Do you mean "this will negatively affect those who use -Werror and head/tail seriously"?

@avanov
Copy link

avanov commented Sep 12, 2022

Well yes, and those who use Liquid Haskell too.

@tomjaguarpaw
Copy link
Member

Regarding -Werror, I don't agree that the effect is negative. In fact I think it's positive. People who voluntarily use -Werror are, in effect, saying "I want the powers that be to warn me against any style of code they deem inappropriate". That's exactly what's happening here!

Regarding the interaction of head/tail and Liquid Haskell, I would like to hear more.

@avanov
Copy link

avanov commented Sep 12, 2022

It seems to me that the proposal is predicated on the assumption that there are insignificantly few cases where the logic of a computation dictates non-emptiness, and therefore, the proponents mention ad-hoc NonEmpty and ad-hoc (with never occurring cases) pattern matching as a means to tackle the issue. This is a code smell for those who rely on generic refinement annotations for guiding safety of their code, very often these annotations (non-emptiness facts) will be inferred even outside of direct call sites of head/tail.

@tomjaguarpaw
Copy link
Member

I'm not the proposer, so I can't speak for his assumptions, but that is not my understanding. My understanding is that the assumption is that those who have genuine cause to use head/tail should import it from another module, not the Prelude.

@avanov
Copy link

avanov commented Sep 12, 2022

Why isn't it the other way around? Power users already know what they're doing, and for beginners there could be ghc --beginner that enables a custom prelude with total functions only.

@tomjaguarpaw
Copy link
Member

Firstly, I'm a "power user" and I wish head and tail had never been added to Prelude. I suspect many other power users feel the same.

Secondly, power users are more likely to know how to tweak their tools to get desired behaviour, and beginners are less likely to know. That suggests that the the behaviour that requires configuration shouldn't be the one that we want beginners to be exposed to.

@avanov
Copy link

avanov commented Sep 12, 2022

Firstly, I'm a "power user" and I wish head and tail had never been added to Prelude. I suspect many other power users feel the same.

I'm a power user and I know that the future is going to be refinement/dependent where partiality of head/tail is a non-issue.

Secondly, power users are more likely to know how to tweak their tools to get desired behaviour, and beginners are less likely to know.

beginners will not benefit from this warning, because aside head/tail there are many other partial functions and many safer alternatives to choose from, and the thing that the beginners lack the most is the single entry point into the ecosystem. That includes official tutorials and guideline sections marked as Start from here. If these sections explicitly state the beginner mode, "the behaviour that requires configuration" is a non-issue either.

@chshersh
Copy link
Member

Sometimes whether the list is empty or not is ancillary to the logic at hand. Here's an incredibly trivial example:

case product xs of
 1 -> foo
 n -> bar n (head xs)

It's indeed a contrived example. I'm not surprised it's possible to create an artificial example but do people actualy write the code like that?

Here's a better way to write this particular snippet in a less "smart" way, without using head and with free performance optimization on top:

case xs of
    [] -> foo
    y : _
        | all (== 1) xs -> foo
        | otherwise     -> bar (product xs) y

For particularly pedantic people who think that the previous version could be less efficient due to traversing the list twice, here's another alternative version:

case xs of
    [] -> foo
    y : _ -> let p = product xs in
             if p == 1
             then foo
             else bar p y

@andys8
Copy link

andys8 commented Sep 12, 2022

I personally like how PureScript solved this with the Partial type class and the unsafePartial escape hatch.

Maybe ghc-proposals/ghc-proposals#454 would allow a similar solution in Haskell.

@tomjaguarpaw
Copy link
Member

I like the idea of a Partial type class but applying it to head/tail would be a breaking change. The change proposed here is not breaking (except for users of -Werror).

@Icelandjack
Copy link

I'm not a fan of this proposal but I was never bothered by the existence of basic partial functions like head and tail, not to invalidate the concerns around partiality but I suspect the discussion will become a time sink with little to show for it.

@nuttycom
Copy link

I'm enthusiastic about removing partial functions from 'base'. Partial functions aren't the future of this language, and while the best time to start "officially" discouraging their use was 10 years ago, now is the second-best time.

@ParetoOptimalDev
Copy link

ParetoOptimalDev commented Sep 12, 2022

beginners will not benefit from this warning, because aside head/tail there are many other partial functions

Beginners usually meet head and tail first @avanov .

Their relationship with partial functions start with this first impression.

I believe that pushing beginners away from partial functions also solves another common question that demonstrates a flawed mental model:

"How do I get the value out of the Monad? "

Restating things, the non-obvious benefit here is pushing beginners to look for the use in these "wrappings" rather than say "getting rid of maybeness".

@josephcsible
Copy link
Contributor

There are perfectly reasonable uses for head and tail; the most common one I use is when writing tests, where the presence of an empty list should fail the test, and throwing an exception is a fine way to achieve that.

Wouldn't using a custom pattern match with error be better here, so that the message could say which list was empty, and explain why it's expected to always contain at least one value, instead of just saying "empty list"?

@Ericson2314
Copy link
Contributor

Ericson2314 commented Sep 12, 2022

@Bodigrim are you against #70, or just see it as stalled? I did start moving things around to make a patch earlier this year, and would like to finish it. Just have been, alas, pulled in many directions.

(There is no technical difficulty making the patch, just context switched away from it :/)

@chshersh
Copy link
Member

chshersh commented Mar 24, 2023

I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all approved CLC proposals.

Field Value
Authors @Bodigrim
Status not merged
base version 4.19.0.0
Merge Request (MR) https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9290
Blocked by nothing
CHANGELOG entry present
Migration guide https://github.com/haskell/core-libraries-committee/blob/main/guides/warning-for-head-and-tail.md

Please, let me know if you find any mistakes 🙂


EDIT: This is merged now. Latest discussion:

@chshersh chshersh added base-4.19 Implemented in base-4.19 (GHC 9.8) awaits-merge Approved, but MR is still unmerged labels Mar 24, 2023
@andreasabel
Copy link
Member

andreasabel commented Sep 5, 2023

Remove -Wx-partial from -Wall. Please have a look at https://gitlab.haskell.org/ghc/ghc/-/issues/23934.

In short, because the core libraries often use plain lists where non-empty lists could be used, warnings thrown by x-partial cannot be sensibly addressed. Thus, -Wx-partial should be taken out of -Wall so that it is opt-in rather than opt-out. Once the core libraries systematically use NonEmpty lists, and -Wx-partial has passes the test of time, it can be elevated to -Wall status.

In the wild:

P.S.: Apparently -Wx-partial is even part of -Wdefault.

@andreasabel andreasabel reopened this Sep 5, 2023
@josephcsible
Copy link
Contributor

josephcsible commented Sep 5, 2023

Remove -Wx-partial from -Wall.

The result of the vote was that there should be a -Wall warning for these functions. Wouldn't doing that be unilaterally overturning the vote?

@Bodigrim
Copy link
Collaborator Author

Please refrain from reopening proposals which have reached the end of their life cycle.

The policy is that CLC is open to review past decisions, but only if there was a material change of circumstances since the original deliberation. If that's the case, feel free to open a new issue.

mnvr added a commit to mnvr/advent-of-code-2023 that referenced this issue Dec 9, 2023
Rufflewind added a commit to Rufflewind/directory that referenced this issue Jan 22, 2024
Rufflewind added a commit to Rufflewind/directory that referenced this issue Jan 22, 2024
Rufflewind added a commit to Rufflewind/directory that referenced this issue Jan 22, 2024
@Bodigrim Bodigrim removed the awaits-merge Approved, but MR is still unmerged label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote base-4.19 Implemented in base-4.19 (GHC 9.8)
Projects
None yet
Development

No branches or pull requests