Opened 6 years ago

Closed 4 years ago

#8710 closed bug (fixed)

Overlapping patterns warning misplaced

Reported by: goldfire Owned by:
Priority: low Milestone: 8.0.1
Component: Compiler Version: 7.7
Keywords: PatternMatchWarnings Cc: gkaracha
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D1910, Phab:D1920
Wiki Page:

Description

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.

Change History (22)

comment:1 Changed 4 years ago by thomie

Cc: gkaracha added

George, how hard would it be to do this?

comment:2 in reply to:  1 Changed 4 years ago by gkaracha

Replying to 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 _     = ...

comment:3 Changed 4 years ago 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.

comment:4 Changed 4 years ago by simonpj

Keywords: PatternMatchWarnings added

comment:5 in reply to:  3 Changed 4 years ago by gkaracha

Replying to 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 redundant
    In an equation for ‘show’:
        len _     = ... (at Bug.hs:24:3)
        len _     = ... (at Bug.hs:25:3)
Last edited 4 years ago by gkaracha (previous) (diff)

comment:6 Changed 4 years ago by gkaracha

Differential Rev(s): Phab:D1910

comment:7 Changed 4 years ago by thomie

Looks great to me.

comment:8 Changed 4 years ago 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.

comment:9 in reply to:  8 Changed 4 years ago by gkaracha

Replying to 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). That is, Phab:D1920 behaves as follows:

Bug.hs:24:3: warning:
    Pattern match is redundant
    In an equation for ‘show’:
        len _     = ...

Bug.hs:25:3: warning:
    Pattern match is redundant
    In an equation for ‘show’:
        len _     = ...

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. :-)

Last edited 4 years ago by gkaracha (previous) (diff)

comment:10 Changed 4 years ago by gkaracha

Differential Rev(s): Phab:D1910Phab:D1910, Phab:D1920

comment:11 Changed 4 years ago 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?

comment:12 in reply to:  11 Changed 4 years ago by gkaracha

Replying to 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 your previous comment.

Last edited 4 years ago by gkaracha (previous) (diff)

comment:13 Changed 4 years ago by bgamari

So to be clear, I believe that given a function with redundant patterns,

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 (Phab:D1910) seems more user friendly to me

Last edited 4 years ago by bgamari (previous) (diff)

comment:14 Changed 4 years ago 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.

comment:15 in reply to:  13 Changed 4 years ago by gkaracha

Replying to 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.

comment:16 Changed 4 years ago by simonpj

I vote for 1920.

comment:17 Changed 4 years ago by thomie

I flipped a coin, and it also said 1920.

comment:18 in reply to:  17 Changed 4 years ago by gkaracha

Replying to 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.

comment:19 Changed 4 years ago by bgamari

I'm happy to go with the flow here. D1920 it is.

comment:20 Changed 4 years ago by Ben Gamari <ben@…>

In 67393977/ghc:

(Alternative way to) address #8710

Issue a separate warning per redundant (or inaccessible) clause.
This way each warning can have more precice location information
(the location of the clause under consideration and not the whole
match).

I thought that this could be too much but actually the number of
such warnings is bound by the number of cases matched against (in
contrast to the non-exhaustive warnings which may be exponentially
more).

Test Plan: validate

Reviewers: simonpj, austin, bgamari

Reviewed By: bgamari

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D1920

GHC Trac Issues: #8710

comment:21 Changed 4 years ago by bgamari

Milestone: 8.0.1
Status: newmerge

comment:22 Changed 4 years ago by bgamari

Resolution: fixed
Status: mergeclosed
Note: See TracTickets for help on using tickets.