[GHC] #11316: Too many guards warning causes issues
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.11 Keywords: | Operating System: Unknown/Multiple Architecture: | Type of failure: None/Unknown Unknown/Multiple | Test Case: | Blocked By: Blocking: | Related Tickets: Differential Rev(s): | Wiki Page: -------------------------------------+------------------------------------- GHC HEAD warns if you have too many guards, for example: {{{ src/HSE/Bracket.hs:44:5: warning: Too many guards in an equation for ‘needBracket’ Guard checking has been over-simplified (Use: -Wno-too-many-guards to suppress this warning -ffull-guard-reasoning to run the full checker (may increase compilation time and memory consumption)) }}} The code in question is from https://github.com/ndmitchell/hlint/blob/master/src/HSE/Bracket.hs#L44 and reads: {{{#!hs needBracket i parent child | isAtom child = False | InfixApp{} <- parent, App{} <- child = False | isSection parent, App{} <- child = False | Let{} <- parent, App{} <- child = False | ListComp{} <- parent = False | List{} <- parent = False | Tuple{} <- parent = False | If{} <- parent, isAnyApp child = False | App{} <- parent, i == 0, App{} <- child = False | ExpTypeSig{} <- parent, i == 0, isApp child = False | Paren{} <- parent = False | isDotApp parent, isDotApp child, i == 1 = False | RecConstr{} <- parent = False | RecUpdate{} <- parent, i /= 0 = False | Case{} <- parent, i /= 0 || isAnyApp child = False | Lambda{} <- parent, i == length (universeBi parent :: [Pat_]) - 1 = False -- watch out for PViewPat | Do{} <- parent = False | otherwise = True }}} For code that runs with the default flags, and likes to have zero warnings (or warnings as errors), and compile with multiple GHC versions, that is problematic. How can I suppress this warning? * Adding {{{-Wno-too-many-guards}}} will only work with GHC 8, so I need to either use CPP to version select my warning suppression, or conditionality outside the compiler select the flags. * Or I can give up on detecting warnings as errors, which is a bit sad, as it's generally a useful criteria. * Or I can modify my code to make it harder to read in order to satisfy a checker whose semantics might change in future. * Or GHC could not warn on too many guards, because it's really a warning that the compiler can't cope, not that too many guards is necessarily bad. I suggest the last option, because warnings should generally be about code issues, not compiler issues. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
Or GHC could not warn on too many guards, because it's really a warning
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * priority: normal => high * cc: gkaracha (added) * milestone: => 8.0.1 Comment: that the compiler can't cope, not that too many guards is necessarily bad. Indeed I'm quite uncertain of the right way forward here as all options are quite bad, On one hand, remaining silent in this case means that the user may rely on the compiler's pattern checker when it can't be relied upon. In passing `-Wall` to GHC, the user has requested that we provide warnings on non- exhaustive patterns. If we can't deliver on this promise it seems to me that we have a duty to ensure that the user knows this. To remain silent will mean that users may be taken by surprise when their warning-free code nevertheless crashes. For this reason I think it is quite important that a warning of this sort is available (although perhaps not enabled by default). Of course, this is made slightly trickier by the fact that pattern match checking is a hard problem and therefore will always be approximate. Moreover, before George's work we couldn't reason about guards at all. Further, even now the check necessarily breaks down completely in the face of boolean guards. Moreover, I am sympathetic to the complaint that it is too difficult to disable this warning in the presence of multiple compiler versions. == A pragma? == Our current approach to this issue, the `-Wtoo-many-guards` warning, has always been a bit of a compromise. For one, it is far too coarse-grained. In an ideal world, I would say that this flag ought to be a pragma attached to a particular binding, which would solve several problems, * someone reading or modifying the binding will be faced with a reminder that they cannot count on full pattern checking * one doesn't need to completely give up on full pattern checking in a module on account of a single large binding Unfortunately the fact that unknown pragmas are errors with `-Werror` makes this option just as inconvenient as the current flag. == A Proposal == Given the fact that guard checking is merely nice to have and will always be approximate, I tend to think we should just disable this warning by default. Hopefully for 8.2 we will have a [[https://ghc.haskell.org/trac/ghc/wiki/Design/Warnings#ProposedChange|richer scheme]] for managing warnings, at which point we can add `-Wtoo-many- warnings` to `-Weverything`. In the meantime, we'll just need to ensure that the user's guide makes it known that the user must enable this flag if they want to be notified when the pattern checker gives up. George, what do you think? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by NeilMitchell): Note that at the moment I am not using {{{-Wall}}}, only {{{-Werror}}}, so I haven't asked for exhaustiveness checking of the guards (at least I haven't turned it on specially), so having a warning that it couldn't produce a warning I didn't even ask for seems even more dubious. Is the exhaustiveness of guards on by default? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:2 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by NeilMitchell): It seems that if the final guard is {{{| otherwise}}} you could short- circuit the entire analysis pass, since we immediately know what the result is going to be. In such circumstances, emitting a warning about a failed analysis which could be trivially avoided seems unnecessary. It would also provide a simple way for users with "excessively complex" guards to disable the warning, simply add {{{| otherwise -> error "GHC can't cope with this many guards"}}}. No need for the pragma then. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
Indeed I'm quite uncertain of the right way forward here as all options are quite bad,
On one hand, remaining silent in this case means that the user may rely on the compiler's pattern checker when it can't be relied upon. In passing `-Wall` to GHC, the user has requested that we provide warnings on non-exhaustive
deliver on this promise it seems to me that we have a duty to ensure
this. To remain silent will mean that users may be taken by surprise when their warning-free code nevertheless crashes. For this reason I think it is quite important
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by gkaracha): Replying to [comment:1 bgamari]: patterns. If we can't that the user knows that a warning of
this sort is available (although perhaps not enabled by default).
Of course, this is made slightly trickier by the fact that pattern match checking is a hard problem and therefore will always be approximate. Moreover, before George's work we couldn't reason about guards at all. Further, even now the check necessarily breaks down completely in the face of boolean guards.
Given the fact that guard checking is merely nice to have and will always be approximate, I tend to think we should just disable this warning by default.
I think after all I agree with this decision. By oversimplifying guards, we lose precision but the check is always reliable, in the sense that we always play on the safe side, no matter what is our choice about guards: * If a match is deemed exhaustive, it certainly is * If a clause is deemed redundant, it certainly is * If a clause is deemed to have inaccessible RHS, the RHS is certainly inaccessible I summary, this means that by oversimplifying guards, we simply increase the possibility for: * Detecting less matches as exhaustive * Detecting less clauses as redundant * Detecting more clauses as with inaccessible RHS instead of redundant (safe side in terms of laziness) The part of the check that is bit more than the rest with guard simplification is coverage checking: It is less likely to see that something is redundant if guards are oversimplified, but this is something we can live with I think. Hence, I also vote for turning `-Wtoo-many-guards` off by default if you feel that makes GHC too chatty. Replying to [comment:2 NeilMitchell]:
Note that at the moment I am not using {{{-Wall}}}, only {{{-Werror}}}, so I haven't asked for exhaustiveness checking of the guards (at least I haven't turned it on specially), so having a warning that it couldn't produce a warning I didn't even ask for seems even more dubious. Is the exhaustiveness of guards on by default?
Ah, I see. By default, GHC has `-fwarn-overlapping-patterns` (coverage checking) enabled. Hence, even without `-Wall`, the checker will run (but only print warnings about redundant or clauses with inaccessible right hand side). About the guards, there is a misconception in general: They are not something special that the checker can completely drop. The only thing a check can do is oversimplify (like the old checker used to do -- and that's why we would get all these wrong messages) but never drop: A guard that might fail makes the clause in question not total so we cannot just say "forget about the guard", one way or the other, a guard changes the semantics of pattern matching and has to be reasoned about. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by gkaracha): Replying to [comment:3 NeilMitchell]:
It seems that if the final guard is {{{| otherwise}}} you could short- circuit the entire analysis pass, since we immediately know what the result is going to be. In such circumstances, emitting a warning about a failed analysis which could be trivially avoided seems unnecessary. It would also provide a simple way for users with "excessively complex" guards to disable the warning, simply add {{{| otherwise -> error "GHC can't cope with this many guards"}}}. No need for the pragma then.
Hmmmm, I disagree with this: The check is not about exhaustiveness only but also about coverage (redundancy) checking, which is by the way the **expensive** part (both problems are NP-Hard but coverage checking appears to trigger exponential behaviour much more often). Hence, having an `otherwise` means that the check is exhaustive but it does not mean that everything is useful: {{{#!hs len x | [] <- x = 0 | (_:ys) <- x = 1 + len ys | otherwise = error "can't happen" }}} Of course the above is exhaustive but it would be nice to see that the last guard is redundant. Since we cannot see that a specific guard is redundant (we can only check for redundant clauses), I see your point. But in principle I always think about coverage/exhaustiveness inseparably. Additionally, in terms of the implementation, I would not like to have so many "special cases", because it becomes really fast unintuitive for the user. What I mean is that I would prefer the warning about too many guards to appear or not independently of the existence of the `otherwise` clause. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by NeilMitchell): Understood about coverage and exhaustiveness, that makes sense. As long as in default mode the warning doesn't pop up, I'm happy. However, there are some people that always run with {{{-Wall}}}, and if that also turns on {{{-Wtoo-many-guards}}} then these people are going to be left with the nasty set of choices above. In general, introducing warnings that cannot be suppressed by refactorings that improve the code is something I dislike. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by gkaracha): To wrap up, do we all agree that the warning should be suppressed by default? If yes, let me know so that I can fix it soon, one less ticket to worry about :) -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by NeilMitchell): I agree that seems a minimum starting point. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: patch Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1737 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * status: new => patch * differential: => Phab:D1737 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues
-------------------------------------+-------------------------------------
Reporter: NeilMitchell | Owner:
Type: bug | Status: patch
Priority: high | Milestone: 8.0.1
Component: Compiler | Version: 7.11
Resolution: | Keywords:
Operating System: Unknown/Multiple | Architecture:
| Unknown/Multiple
Type of failure: None/Unknown | Test Case:
Blocked By: | Blocking:
Related Tickets: | Differential Rev(s): Phab:D1737
Wiki Page: |
-------------------------------------+-------------------------------------
Comment (by Ben Gamari
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1737 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * status: patch => new Comment: The warning is now disabled by default. There are still, however, performance issues in the pattern checker in the presence of guards. See #11163 and #11276. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: #11163, #11276 | Differential Rev(s): Phab:D1737 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * related: => #11163, #11276 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:12 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: closed Priority: high | Milestone: 8.0.1 Component: Compiler | Version: 7.11 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: #11163, #11276 | Differential Rev(s): Phab:D1737 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * status: new => closed * resolution: => fixed -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:13 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues -------------------------------------+------------------------------------- Reporter: NeilMitchell | Owner: Type: bug | Status: new Priority: high | Milestone: 8.2.1 Component: Compiler | Version: 7.11 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: #11163, #11276 | Differential Rev(s): Phab:D1737 Wiki Page: | -------------------------------------+------------------------------------- Changes (by simonpj): * keywords: => PatternMatchWarnings * status: closed => new * resolution: fixed => * milestone: 8.0.1 => 8.2.1 Comment: Re-opening to improve for 8.2. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11316#comment:14 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#11316: Too many guards warning causes issues
-------------------------------------+-------------------------------------
Reporter: NeilMitchell | Owner:
Type: bug | Status: closed
Priority: high | Milestone: 8.0.1
Component: Compiler | Version: 7.11
Resolution: fixed | Keywords:
| PatternMatchWarnings
Operating System: Unknown/Multiple | Architecture:
| Unknown/Multiple
Type of failure: None/Unknown | Test Case:
Blocked By: | Blocking:
Related Tickets: #11163, #11276 | Differential Rev(s): Phab:D1737
Wiki Page: |
-------------------------------------+-------------------------------------
Changes (by thomie):
* status: new => closed
* resolution: => fixed
* milestone: 8.2.1 => 8.0.1
Comment:
Commit 28f951edfe50ea5182065144340061ec326781f5 deleted the "too many
guards" warning completely, so I think this particular ticket can be
closed now.
{{{
Author: George Karachalias
participants (1)
-
GHC