[GHC] #8710: Overlapping patterns warning misplaced
#8710: Overlapping patterns warning misplaced -------------------------+------------------------------------------------- Reporter: | Owner: goldfire | Status: new Type: bug | Milestone: Priority: low | Version: 7.7 Component: | Operating System: Unknown/Multiple Compiler | Type of failure: Incorrect warning at Keywords: | compile-time Architecture: | Test Case: Unknown/Multiple | Blocking: Difficulty: | Unknown | Blocked By: | Related Tickets: | -------------------------+------------------------------------------------- Consider {{{ {-# OPTIONS_GHC -fwarn-overlapping-patterns #-} len [] = 0 len (_:t) = 1 + len t len _ = error "urk" }}} GHC reports the problem with this definition at the first line of it. I think it would be more helpful to have it at the last line, which is the pattern that will never match. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by thomie): * cc: gkaracha (added) Comment: George, how hard would it be to do this? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by gkaracha): Replying to [comment:1 thomie]:
George, how hard would it be to do this?
Not very if we agree how we want the messages to look like. At the moment function `dsPmWarn` in `deSugar/Check.hs` takes the location **of the whole match**, so that it can say that "the following clauses of this match are redundant..". Every clause is an `LMatch` so we have location information available if we want to use this instead of the location of the match. My question is, do we want a separate warning for every redundant clause, or we want the 1 warning/match (what we currently have), just with more information in it? E.g. something like the following: {{{ Bug.hs:22:3: Warning: Pattern match(es) are overlapped In an equation for ‘show’: Bug.hs:24:3: len _ = ... Bug.hs:25:3: len _ = ... }}} -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:2 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by thomie): I would maybe prefer separate warnings, such that even for a single redundant clause, the warning message doesn't have to mention two different line numbers (one for the location of the whole match, and one for the location of the redundant clause). I don't know if there is precedent for multiple line numbers in a warning message. Either way is fine though. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by simonpj): * keywords: => PatternMatchWarnings -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by gkaracha): Replying to [comment:3 thomie]:
I would maybe prefer separate warnings, such that even for a single redundant clause, the warning message doesn't have to mention two different line numbers (one for the location of the whole match, and one for the location of the redundant clause).
I don't know if there is precedent for multiple line numbers in a warning message.
Either way is fine though.
I created a small patch for this (Phab:D1910). If we print a separate warning for every redundant clause it may become overwhelming so I thought that it would be best to keep them all in the same warning. The patch has the following behavior: {{{ Bug.hs:22:3: warning: Pattern match(es) are overlapped In an equation for ‘show’: len _ = ... (at Bug.hs:24:3) len _ = ... (at Bug.hs:25:3) }}} -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910 Wiki Page: | -------------------------------------+------------------------------------- Changes (by gkaracha): * differential: => Phab:D1910 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910 Wiki Page: | -------------------------------------+------------------------------------- Comment (by thomie): Looks great to me. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910 Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj):
If we print a separate warning for every redundant clause it may become overwhelming
Really? There are a few artificial examples in the testsuite, but I'd have thought that one error per redundant equation would be ok. But still, I'd really like the common case of one redundant equation to look like {{{ Bug.hs:24:3: warning: -- Note correct line number Pattern match(es) are redundant In an equation for ‘show’: len _ = ... (at Bug.hs:24:3) }}} Or maybe even {{{ Bug.hs:24:3: warning: Pattern match(es) are redundant In an equation for ‘show’: len _ = ... -- No need to duplicate here }}} If there are more than one, then adding the individual locations is good {{{ Bug.hs:24:3: warning: Pattern match(es) are redundant In an equation for ‘show’: len _ = ... (at Bug.hs:24:3) len _ = ... (at Bug.hs:25:3) }}} but the herald should mention the first. Can't be hard, I guess. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910 Wiki Page: | -------------------------------------+------------------------------------- Comment (by gkaracha): Replying to [comment:8 simonpj]:
If we print a separate warning for every redundant clause it may become overwhelming
Really? There are a few artificial examples in the testsuite, but I'd have thought that one error per redundant equation would be ok.
Hmmm, true. The number of redundant clauses (and of those with inaccessible RHS) is bound by the number of clauses a match has (it's not the case with the uncovered though). In practice I have not seen matches with many redundant cases. Hence, I created Phab:D1920 too, which implements this approach (one warning per redundant clause, each with its own location).
But still, I'd really like the common case of one redundant equation to
look like
{{{ Bug.hs:24:3: warning: -- Note correct line number Pattern match(es) are redundant In an equation for ‘show’: len _ = ... (at Bug.hs:24:3) }}} Or maybe even {{{ Bug.hs:24:3: warning: Pattern match(es) are redundant In an equation for ‘show’: len _ = ... -- No need to duplicate here }}} If there are more than one, then adding the individual locations is good {{{ Bug.hs:24:3: warning: Pattern match(es) are redundant In an equation for ‘show’: len _ = ... (at Bug.hs:24:3) len _ = ... (at Bug.hs:25:3) }}} but the herald should mention the first.
Can't be hard, I guess.
Agreed on this one too, I have updated Phab:D1910 to behave like this. Merge at will the revision you prefer. :-) -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Changes (by gkaracha): * differential: Phab:D1910 => Phab:D1910, Phab:D1920 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:10 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by simonpj): So we are agreed on one warning per equation, with the location of the equation in the warning header. George: I think you are saying that D1910 and D1920 do the same thing. So what's the difference? Which should we choose? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by gkaracha): Replying to [comment:11 simonpj]:
So we are agreed on one warning per equation, with the location of the equation in the warning header.
George: I think you are saying that D1910 and D1920 do the same thing. So what's the difference? Which should we choose?
If we agree on one warning per equation, with the location of the equation in the warning header, then Phab:D1920 is the one we want. Phab:D1910 implements what you describe in you previous [comment:8 comment]. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:12 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by bgamari): So to be clear, I believe that given a function with redundant patterns, {{{#!hs module Bug where hello _ = "world" hello 1 = "turtle" hello 2 = "frog" }}} Phab:D1910 will provide a warning like, {{{ Bug.hs:3:1: Warning: Pattern match(es) are overlapped In an equation for ‘hello’: hello 1 = ... hello 2 = ... }}} whereas Phab:D1920 will provide, {{{ Bug.hs:3:1: Warning: Pattern match is overlapped In an equation for ‘hello’: hello 1 = ... Bug.hs:4:1: Warning: Pattern match is overlapped In an equation for ‘hello’: hello 2 = ... }}} Is this right George? The former seems more user friendly to me -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:13 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by simonpj): I prefer the latter. It's rare for more than one to be overlapped, and when there is I'd like to find my way quickly to the correct source line for each. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:14 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by gkaracha): Replying to [comment:13 bgamari]: Phab:D1910 will actually provide the following: {{{ Bug.hs:3:1: Warning: Pattern match(es) are overlapped In an equation for ‘hello’: hello 1 = ... (at Bug.hs:3:1) hello 2 = ... (at Bug.hs:4:1) }}} , since there are two redundant clauses. If there was just one it would omit the (at <location>)-part. From Phab:D1920, we will indeed get the warnings you mentioned. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:15 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by simonpj): I vote for 1920. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:16 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by thomie): I flipped a coin, and it also said 1920. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:17 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by gkaracha): Replying to [comment:17 thomie]:
I flipped a coin, and it also said 1920.
I am rather indifferent too about it. I agree with Ben that the shorter warning seems more user friendly, yet I think Simon is right that it is incredibly rare to have more than one redundant clause in a match (I can only imagine it happening as a copy-paste error) so I don't think it really makes a big difference. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:18 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: new Priority: low | Milestone: Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Comment (by bgamari): I'm happy to go with the flow here. D1920 it is. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:19 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced
-------------------------------------+-------------------------------------
Reporter: goldfire | Owner:
Type: bug | Status: new
Priority: low | Milestone:
Component: Compiler | Version: 7.7
Resolution: | Keywords:
| PatternMatchWarnings
Operating System: Unknown/Multiple | Architecture:
Type of failure: Incorrect | Unknown/Multiple
warning at compile-time | Test Case:
Blocked By: | Blocking:
Related Tickets: | Differential Rev(s): Phab:D1910,
Wiki Page: | Phab:D1920
-------------------------------------+-------------------------------------
Comment (by Ben Gamari
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: merge Priority: low | Milestone: 8.0.1 Component: Compiler | Version: 7.7 Resolution: | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Changes (by bgamari): * status: new => merge * milestone: => 8.0.1 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:21 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#8710: Overlapping patterns warning misplaced -------------------------------------+------------------------------------- Reporter: goldfire | Owner: Type: bug | Status: closed Priority: low | Milestone: 8.0.1 Component: Compiler | Version: 7.7 Resolution: fixed | Keywords: | PatternMatchWarnings Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1910, Wiki Page: | Phab:D1920 -------------------------------------+------------------------------------- Changes (by bgamari): * status: merge => closed * resolution: => fixed Comment: Merged as 680557c125021a1f248ef010418b1234d9a2aa4a. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/8710#comment:22 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
participants (1)
-
GHC