haskell / core-libraries-committee

95 stars 15 forks source link

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

Closed Bodigrim closed 1 year ago

Bodigrim commented 2 years ago

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 https://github.com/haskell/core-libraries-committee/issues/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 https://github.com/ghc-proposals/ghc-proposals/pull/454 and https://github.com/ghc-proposals/ghc-proposals/pull/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 https://github.com/haskell/core-libraries-committee/issues/87#issuecomment-1243587955, 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..].

ParetoOptimalDev commented 1 year ago

Or, this approach might not be best and there's a better way to rewrite the #87 (comment) without using head that isn't too inconvenient:

The "correct" type here is an infinite non-empty stream:

return $ S.head $ S.filter (`noElem` hashes) $ S.map showT $ S.enumFrom (0 :: Int)

@simonmar objected to this earlier because it gives up list syntax.

We could get the sugar back with a source plugin I think and have the best of both worlds.

This makes me think back to a previous comment and wonder how much better things would be with if Stream were the default instead of list in most Haskell code.

tomjaguarpaw commented 1 year ago

There have been a few accusations of bad faith,

I was trying to avoid accusation of bad faith by saying:

feels like bad faith

Fair enough, in which case I hope you will forgive me my error, which was, after all, made in good faith!

Daniel-Diaz commented 1 year ago

We could get the sugar back with a source plugin I think and have the best of both worlds.

@ParetoOptimalDev There's the OverloadedLists extension. With this you could write (assuming a suitable IsList instance has been provided):

S.head $ S.filter f [0..]

It's worth noting that this is still a partial expression. If f never returns True for any of the elements in the input stream, S.head will never return.

ParetoOptimalDev commented 1 year ago

We could get the sugar back with a source plugin I think and have the best of both worlds.

@ParetoOptimalDev There's the OverloadedLists extension. With this you could write (assuming a suitable IsList instance has been provided):

S.head $ S.filter f [0..]

It's worth noting that this is still a partial expression. If f never returns True for any of the elements in the input stream, S.head will never return.

To my surprise this pushes me towards the "use lists and head" camp because an error from a partial function is less surprising than non-termination from partiality that's harder to spot.

I hope someone has a counter- argument to pull me away from this position!

Daniel-Diaz commented 1 year ago

To my surprise this pushes me towards the "use lists and head" camp because an error from a partial function is less surprising than non-termination from partiality that's harder to spot.

I hope someone has a counter- argument to pull me away from this position!

The potential non-termination wasn't introduced by using streams. Lists can also be infinite.

ParetoOptimalDev commented 1 year ago

To my surprise this pushes me towards the "use lists and head" camp because an error from a partial function is less surprising than non-termination from partiality that's harder to spot. I hope someone has a counter- argument to pull me away from this position!

The potential non-termination wasn't introduced by using streams. Lists can also be infinite.

Right... filter (const False) [0..] has the same issue. I'll have to think in this for a bit, i'm not sure what to prefer now.

josephcsible commented 1 year ago

Right... filter (const False) [0..] has the same issue. I'll have to think in this for a bit, i'm not sure what to prefer now.

Don't Agda and/or Idris make you use the type system to prove you haven't written such a thing? Although I imagine that'd also be a huge effort to ever add to Haskell.

Bodigrim commented 1 year ago

My apologies, I thought @nomeata had answered this well enough (#87 (comment)). About 6 months? If it helps, I volunteer to shepherd the proposal through the committee, I don't imagine it would be very controversial.

@simonmar could you please clarify whether you volunteer just to shepherd or to make a proposal and implement it upon approval?

adamgundry commented 1 year ago

In the light of this thread, I have opened a new ghc-proposal: https://github.com/ghc-proposals/ghc-proposals/pull/541

simonmar commented 1 year ago

My apologies, I thought @nomeata had answered this well enough (#87 (comment)). About 6 months? If it helps, I volunteer to shepherd the proposal through the committee, I don't imagine it would be very controversial.

@simonmar could you please clarify whether you volunteer just to shepherd or to make a proposal and implement it upon approval?

Just to shepherd.

adamgundry commented 1 year ago

There hasn't yet been a huge amount of feedback on the GHC feature proposal for categorised WARNING pragmas motivated by this discussion, so please chime in on the PR whether or not you think this is a good idea!

Bodigrim commented 1 year ago

Thanks all for the detailed discussion and scrutiny. Given that the thread was mostly silent for a month now, I hope that everyone had a chance to voice their opinion. I revised the proposal to reflect most recent developments, and in my proposer's hat I'd like to trigger a vote.

Bodigrim commented 1 year ago

(...changing hats...)

Dear CLC members, let's vote on the proposal to add {-# WARNING #-} to Data.List.{head,tail} as per MR !9290. @tomjaguarpaw @chessai @cgibbard @mixphix @emilypi


Unsurprisingly, +1 from me.

emilypi commented 1 year ago

+1, unsurprisingly. One small step on the way to purging footguns.

AndreasPK commented 1 year ago

It would be nice to see a data driven impact analysis before this get's accepted. Maybe I missed it (I skimmed the comments but there are a lot of them). But currently the impact analysis as far as I can tell can be paraphrased as: "Users of head and tail will receive a warning and have to act on that, or not", which seems a bit weak to me for a change to Prelude itself.

Things I would like to know are how many packages actually use head/tail and how often? Do any of them use -Werror? Can you build/test stackage with these changes without modifications to any packages? If not how many packages need changes?

"Users are free to not do anything" also seems like wishful thinking. Given the existence and often recommended use of -Werror this is at best inaccurate. Clearly there are a lot of projects where "compiles without warnings" or with -Werror is a requirement. If the CLC wants to dictate a certain style of coding on these projects that's fine. But no one should pretend that in general users or projects who are comfortable with the use of head can just ignore that change at large.

For one datum on impact ghc requires builds to succeed with -Werror. Some volunteers took it on themselves to removing head/tail recently. And all said and done I'm confident to say that by this point it was multiple man hours of effort spent on this between review/code changes etc. And I would expect this change to impose a similar or larger cost on any other large haskell code base.

parsonsmatt commented 1 year ago

I don't think Hackage allows -Werror in published packages, so introducing a new warning should not break any Hackage package.

Every major version of GHC causes big breaking changes that ripple throughout the ecosystem. Lately I've been doing a lot of that work. A WARNING pragma on head or tail would be so trivially easy to fix compared to the Word64 constructor, or changes to GHC library API, or Simplified Subsumption, or, well, really almost any change that GHC has done lately. One of the weirder changes in recent GHC is the switch to GHC2021 instead of Haskell2010 as default language, and the lack of the CUSKs language extension - the resulting error took way longer to diagnose than a helpful WARNING message.

Bodigrim commented 1 year ago

Do any of them use -Werror? Can you build/test stackage with these changes without modifications to any packages?

Hackage warns against -Werror enabled by default and prohibits -Wall -Werror since 2008. I grepped through Stackage Nightly 2022-11-08 and there are only two packages, which set -Werror unconditionally:

Neither of them uses Prelude.{head,tail}, so the answer to your question is: yes, one can build the entire Stackage with {-# WARNING #-} for head and tail without a single patch.

For one datum on impact ghc requires builds to succeed with -Werror.

That's pretty easy: just redefine head and tail in GHC.Prelude as suggested in the mitigation section of the proposal.

Even if it was not that trivial, the difficulty to migrate GHC codebase is not a deal breaker. We have a recent precedent at hand: GHC enabled -Wincomplete-uni-patterns -Wincomplete-record-updates by default for all Haskell packages, despite its own codebase being littered with

{-# OPTIONS_GHC -Wno-incomplete-uni-patterns   #-}
{-# OPTIONS_GHC -Wno-incomplete-record-updates #-}

And I would expect this change to impose a similar or larger cost on any other large haskell code base.

There is a discussion above about Glean, which does not confirm your expectations. As another data point, I conducted a case study on a huge project available to me, and was able to squash all warnings very quickly.

@AndreasPK I did not want to leave your contribution to the discussion ignored and I do appreciate it, but as I said in https://github.com/haskell/core-libraries-committee/issues/87#issuecomment-1304545777, there've been plenty of opportunities for everyone to speak earlier. I believe it's time for a vote.


(changing hats)

Dear CLC members, this is a gentle reminder to vote. @tomjaguarpaw @chessai @cgibbard @mixphix

tomjaguarpaw commented 1 year ago

One of the weirder changes in recent GHC is the switch to GHC2021 instead of Haskell2010 as default language, and the lack of the CUSKs language extension - the resulting error took way longer to diagnose than a helpful WARNING message.

@parsonsmatt, as a quick aside, this is very interesting. Would you mind writing up a short paragraph, on the HF Stability Working Group issue tracker, explaining what happened? As a member of that group I would welcome learning from your experience on this matter.

tomjaguarpaw commented 1 year ago

+1


This is the smallest possible step in this direction, and long overdue in my personal opinion. It's regrettable that there is no backwards-compatible option available to us, but there are plenty of straightforward mitigations.

ffaf1 commented 1 year ago

@Bodigrim

https://hackage.haskell.org/package/urbit-hob (there seems to be a bug in cabal check, because the package was uploaded fairly recently, but uses -Wall -Werror by default).

This will be fixed when #8427 gets merged.

cgibbard commented 1 year ago

+1

chessai commented 1 year ago

+1

Bodigrim commented 1 year ago

Thanks all, 5 votes in favor out of possible 6 are enough to approve. I'll raise a PR for a migration guide soon.

chshersh commented 1 year ago

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:

andreasabel commented 1 year ago

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.

josephcsible commented 1 year ago

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 commented 1 year ago

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.