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

Extend -Wall with incomplete-uni-patterns and incomplete-record-updates #71

Merged
merged 2 commits into from Feb 8, 2018

Conversation

@tomjaguarpaw
Copy link
Contributor

tomjaguarpaw commented Aug 4, 2017

The proposal has been accepted; the following discussion is mostly of historic interest.


This suggestion received great support on Haskell Reddit.

@nomeata

This comment has been minimized.

Copy link
Contributor

nomeata commented Aug 4, 2017

@quchen

This comment has been minimized.

Copy link

quchen commented Aug 6, 2017

I think they’re both useful warnings, but I can also think of cases where the lambda warning can get in the way. Unfortunately we have no way of disabling the warnings for a certain block of code. Lambdas are fairly useful to write short case-by-case inline code like \(Just x) -> x where there are implicit invariants, but luckily needing definitions like these is so rare that they can just be pulled up to the top level as

fromJust (Just x) = x
fromJust _ = error "Nothing in my Data.Maybe.fromJust redefining example"
@tomjaguarpaw

This comment has been minimized.

Copy link
Contributor Author

tomjaguarpaw commented Aug 6, 2017

@quchen: My response would be

  1. Every warning can get in the way. The important question is which do more harm than good.

  2. It seems inconsistent that \(Just x) -> x should not warn but \case (Just x) -> x should.

@joehillen

This comment has been minimized.

Copy link

joehillen commented Aug 7, 2017

Not having -Wincomplete-record-updates in -Wall has bitten me in production. I would love to see this change. I sincerely think it should be in the default warnings. It's just an runtime exception waiting to happen if you're not paying close attention.

@ocharles

This comment has been minimized.

Copy link

ocharles commented Sep 22, 2017

@quchen:

I think they’re both useful warnings, but I can also think of cases where the lambda warning can get in the way.

Of course, and as we don't have the ability to disable warnings at a fine grained level, it's a question of: does this warning trigger more false positives than being useful? I would argue no, it's almost always a bug (in my code) if I forget a pattern match, and if something is truly impossible I should provide a specific error message alerting me that an assuming invariant doesn't hold.

Strong 👍 on this.

@nomeata

This comment has been minimized.

Copy link
Contributor

nomeata commented Sep 22, 2017

What about this code:

histogram :: Ord a => [a] -> [(a,Int)]
histogram = map (\(x:xs) -> (x,length (x:xs))) . group . sort

I would say this is fairly idiomatic Haskell, resting on the invariant that group only produces nonempty lists. Do we want to discourage people from writing such code?

@ocharles

This comment has been minimized.

Copy link

ocharles commented Sep 22, 2017

Yes, use group should use Data.List.NonEmpty and be honest, which is in base.

In the general case, there are cases where right now it's "convenient" with composition to pass an invariant-assuming lambda, but usually this can be better expressed with a proper data type that witnesses the invariant. In the long run, I think this produces better code. Of course, if you disagree you could turn off this warning in -Wall, but I do think it should be on by default.

@goldfirere

This comment has been minimized.

Copy link
Contributor

goldfirere commented Sep 22, 2017

Could we extend this to warn on incomplete record accessors, too?

data R = MkR1 { foo :: Int } | MkR2 Bool

bar :: R -> Int
bar r = foo r + 1

This generates no warnings but is dangerously partial.

@cartazio

This comment has been minimized.

Copy link

cartazio commented Sep 23, 2017

in general its a bit tricky to map "invariants" to simple data type embeddings.. I agree with all the arguments
might it be worth considering having not just -Wall but also -Wall-Agressively or -Wall-MightBeFalsePositive?

the example @nomeata demonstrates SHOULDN'T trigger warnings under wall. (it is correct, i merely haven't proven it :) )

@ghost

This comment has been minimized.

Copy link

ghost commented Sep 23, 2017

might it be worth considering having not just -Wall but also -Wall-Agressively or -Wall-MightBeFalsePositive?

Similar to -Wextra in GCC?

@ocharles

This comment has been minimized.

Copy link

ocharles commented Sep 23, 2017

@cartazio Why can't you just use -Wall -fno-warn-incomplete-uni-patterns? I don't understand the push back on getting this into -Wall. Almost none of my projects rely on incomplete-uni-patterns, is it really that common in other peoples work? And I don't see why @nomeata's example shouldn't trigger a warning. Heck, you can't even see the definition of group, so how can you make such a claim? Obviously you are making some assumptions as to what group does, but as I said before, that can be baked into an appropriate type. For harder invariants, you can either turn the warning off (maybe with {-# OPTIONS #-}, or just provide exhaustion pattern matches that call error with an appropriate error message - which I argue is more valuable than just a vague "incomplete pattern match at x:y" error message).

@goldfirere I would love that warning too, but am I right in thinking that warning doesn't even exist atm, which makes it a tad out of scope to this proposal?

@tomjaguarpaw

This comment has been minimized.

Copy link
Contributor Author

tomjaguarpaw commented Sep 23, 2017

I share @ocharles's frustration. Every warned construct has some legitimate use cases otherwise it would be an error.

@gridaphobe

This comment has been minimized.

Copy link
Contributor

gridaphobe commented Sep 23, 2017

@ocharles I think it's fine for @nomeata's example to trigger a warning, since GHC can't (yet) prove it safe. But saying the proper way to fix the warning is to either use a wrapper type for a general function like group, or to disable the warning altogether at the module level, is a bit extreme in my opinion.

I think this example really points to the need for declaration-specific warning pragmas. That way I can tell GHC that I know the function is safe, while still being warned about other possible issues.

@tomjaguarpaw

This comment has been minimized.

Copy link
Contributor Author

tomjaguarpaw commented Sep 23, 2017

I think this example really points to the need for declaration-specific warning pragmas. That way I can tell GHC that I know the function is safe, while still being warned about other possible issues.

I agree, and I think @ocharles would agree too, but the inability to disable warnings on a declaration-specific basis is a general problem and should not hold up this particular GHC proposal.

@goldfirere

This comment has been minimized.

Copy link
Contributor

goldfirere commented Sep 23, 2017

I concur that my suggestion for warning for incomplete record accessors could be out of scope. However, any changes to GHC's warning levels tends to cause disruption to users, and I thought it would be nice to make the warning changes all at once to minimize the disruption.

@nomeata

This comment has been minimized.

Copy link
Contributor

nomeata commented Sep 23, 2017

Such warnings would also preclude (or discourage) the common idiom of using laziness in where clauses. I often see code, including in GHC, like

function x y
  | some guard = …
  | some other guard = …
 where
  auxillary definitions using partial_field x

where some of the auxillary definitions are used only in some branches, and in these branches, the partial_field is fine to use.

@goldfirere

This comment has been minimized.

Copy link
Contributor

goldfirere commented Sep 23, 2017

A clever compiler could probably deal with the case you describe in the comment immediately above, but only if those guards are pattern guards. Admittedly, though, implementing the cleverness in the compiler may require a good deal of cleverness on the part of the compiler-writer.

I don't have a great suggestion here. I don't retract my desire to have warnings on accessors, but I admit that @nomeata 's example may defeat my argument.

@BardurArantsson

This comment has been minimized.

Copy link

BardurArantsson commented Sep 23, 2017

I think this example really points to the need for declaration-specific warning pragmas.

(Echoing @tomjaguarpaw, I don't think it's reasonable to hold up the proposal just because there could theoretically be a more fine-grained mechanism. I think the proposal must be evaluated as of the current state of the warning mechanism. Whether that makes people more likely to disagree/agree, I don't know.)

Not wishing to derail, and just because we have a few clever compiler-writers here: Does anyone have a clue as to how much effort such a feature would be, roughly? EDIT: Just to be clear, I'm thinking of a very simple syntactic check along the lines of "ok, I have a warning at line X, column Y. Is there any enclosing AST node which disables that warning, yes or no?"

@cartazio

This comment has been minimized.

Copy link

cartazio commented Sep 23, 2017

@BardurArantsson

This comment has been minimized.

Copy link

BardurArantsson commented Sep 23, 2017

@nomeata Just out of curoisity, how common is this idiom outside of GHC? I can't recall ever seeing it (maybe I didn't recognize it?), but as most of us, I also have a very limited vision of the ecosystem.

AFAIUI GHC is actually quite non-idiomatic when it comes to code style. (Thus, unless there's evidence to the contrary, I don't think pointing to the GHC code base is in any way useful as a "representative" sample.)

Also, as a more concrete point on the example: This seems like an incredibly error-prone 'pattern'. I would actually be in favor of discouraging this pattern and/or forcing people to add cases with explicit "error" cases/patterns/whatever. If you're trying to be "too clever"[1] then you deserve a warning!

[1] Relative to Haskell, obviously. ;)

@treeowl

This comment has been minimized.

Copy link
Contributor

treeowl commented Sep 23, 2017

@BardurArantsson

This comment has been minimized.

Copy link

BardurArantsson commented Sep 23, 2017

@treeowl There isn't a GitHub "upvote" emoji for this, so: Read my mind! :)

@gridaphobe

This comment has been minimized.

Copy link
Contributor

gridaphobe commented Sep 23, 2017

The lazy-where pattern is not confined to GHC. I’ve come across it at least in some subset of bytestring, text, and vector, since we had to add support to LiquidHaskell to reason about it (in a limited manner). I also use it regularly in my own code.

I think it’s actually a nice example of how laziness allows you to structure better, by extracting auxiliary definitions that are used in multiple branches. Obviously there’s a point where this style becomes detrimental due to the issue @treeowl brought up, but I don’t think that’s any different from the general argument to avoid long functions.

@tomjaguarpaw

This comment has been minimized.

Copy link
Contributor Author

tomjaguarpaw commented Sep 24, 2017

The absolute value of incomplete-uni-patterns is indeed very interesting, but could I please encourage discussants to instead take a relative frame of reference and consider whether it is sufficiently different from incomplete-patterns that they should be distinguished by Wall? For example, I would suggest that the utility of lazy where bindings has no relevance to the issue at hand because it applies equally well to both warnings.

As far as I can tell incomplete-uni-patterns and incomplete-patterns serve almost exactly the same purpose and should either be both in or both out of Wall.

@BardurArantsson

This comment has been minimized.

Copy link

BardurArantsson commented Sep 24, 2017

FWIW, I'm +1 on the proposal, just FTR.

@gridaphobe

This comment has been minimized.

Copy link
Contributor

gridaphobe commented Sep 24, 2017

The GHC User's Guide says

The flag -Wincomplete-uni-patterns is similar, except that it applies only to lambda-expressions and pattern bindings, constructs that only allow a single pattern.

So the difference between the two warnings is what steps the user would have to take to appease -Wall. For -Wincomplete-patterns the user can easily disable the warning by adding a catch-all pattern, no refactoring needed.

In contrast, -Wincomplete-uni-patterns only applies to syntactic constructs that allow a single pattern, so disabling the warning would require lifting the lambda to a let binder, or doing an explicit case on the RHS of a pattern binding.

I don't know if this difference is large enough to merit a distinction for -Wall, but I do think we should consider the user impact when blaming code that may be innocuous.

@treeowl

This comment has been minimized.

Copy link
Contributor

treeowl commented Sep 24, 2017

@cartazio

This comment has been minimized.

Copy link

cartazio commented Sep 26, 2017

@tomjaguarpaw

This comment has been minimized.

Copy link
Contributor Author

tomjaguarpaw commented Sep 26, 2017

@nomeata

This comment has been minimized.

Copy link
Contributor

nomeata commented Nov 10, 2017

What is the status of this proposal? Does it encompass the results of the discussion and is in a shape for review?

@BardurArantsson

This comment has been minimized.

Copy link

BardurArantsson commented Nov 21, 2017

I think the answer is (unfortunately) excessive bikeshedding :(. I don't think any actually substantive objections were raised. Maybe just move to vote by the committee? If it doesn't pass the vote it could always be revisited.

EDIT: Let's not forget that these are warnings and that any sane package manager is NOT running with -Werror on their 'production' build.

@nomeata

This comment has been minimized.

Copy link
Contributor

nomeata commented Nov 21, 2017

Allright, committee process started.

@nomeata nomeata merged commit 5df0a99 into ghc-proposals:master Feb 8, 2018
nomeata added a commit that referenced this pull request Feb 8, 2018
@gbaz

This comment has been minimized.

Copy link

gbaz commented Feb 11, 2018

so is this accepted then?

@nomeata

This comment has been minimized.

Copy link
Contributor

nomeata commented Feb 11, 2018

Correct! (It got merged and labeled 3 days ago)

@bravit bravit added the Proposal label Nov 11, 2018
@tomjaguarpaw tomjaguarpaw deleted the tomjaguarpaw:patch-3 branch Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.