Opened 22 months ago

Closed 16 months ago

Last modified 16 months ago

#14546 closed bug (fixed)

-Woverlapping-patterns warns on wrong patterns for Int

Reported by: Lemming Owned by: sighingnow
Priority: normal Milestone:
Component: Compiler (Type checker) Version: 8.2.2
Keywords: PatternMatchWarnings Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect error/warning at compile-time Test Case: deSugar/should_compile/T14546a, deSugar/should_compile/T14546b, deSugar/should_compile/T14546c
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D4571
Wiki Page:

Description

GHC-8.0 introduced a new warning on redundant pattern matches in case clauses. But there is something strange.

$ cat RedundantCase.hs
main :: IO ()
main = do
   case 0::Int of
      0 -> putStrLn "A"
      1 -> putStrLn "B"
      _ -> putStrLn "C"

   case 0::Int of
      0 -> putStrLn "A"
      1 -> putStrLn "B"
      2 -> putStrLn "C"

   case 0::Integer of
      0 -> putStrLn "A"
      1 -> putStrLn "B"
      _ -> putStrLn "C"

   case 0::Integer of
      0 -> putStrLn "A"
      1 -> putStrLn "B"
      2 -> putStrLn "C"

   case 0::Integer of
      1 -> putStrLn "B"
      2 -> putStrLn "C"

   let dummy :: Integer
       dummy = 0
   case dummy of
      0 -> putStrLn "A"
      1 -> putStrLn "B"
      _ -> putStrLn "C"

   case "foo" of
      "foo" -> putStrLn "A"
      "bar" -> putStrLn "B"
      "baz" -> putStrLn "C"

   if True
     then putStrLn "X"
     else putStrLn "Y"
$ ghc-8.2.2 -Wall RedundantCase.hs 
[1 of 1] Compiling Main             ( RedundantCase.hs, RedundantCase.o )

RedundantCase.hs:4:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 0 -> ...
  |
4 |       0 -> putStrLn "A"
  |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:5:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 1 -> ...
  |
5 |       1 -> putStrLn "B"
  |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:8:4: warning: [-Wincomplete-patterns]
    Pattern match(es) are non-exhaustive
    In a case alternative: Patterns not matched: 0
  |
8 |    case 0::Int of
  |    ^^^^^^^^^^^^^^...

RedundantCase.hs:9:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 0 -> ...
  |
9 |       0 -> putStrLn "A"
  |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:10:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 1 -> ...
   |
10 |       1 -> putStrLn "B"
   |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:11:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 2 -> ...
   |
11 |       2 -> putStrLn "C"
   |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:15:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 1 -> ...
   |
15 |       1 -> putStrLn "B"
   |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:16:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: _ -> ...
   |
16 |       _ -> putStrLn "C"
   |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:20:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 1 -> ...
   |
20 |       1 -> putStrLn "B"
   |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:21:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 2 -> ...
   |
21 |       2 -> putStrLn "C"
   |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:23:4: warning: [-Wincomplete-patterns]
    Pattern match(es) are non-exhaustive
    In a case alternative: Patterns not matched: 0
   |
23 |    case 0::Integer of
   |    ^^^^^^^^^^^^^^^^^^...

RedundantCase.hs:24:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 1 -> ...
   |
24 |       1 -> putStrLn "B"
   |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:25:7: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: 2 -> ...
   |
25 |       2 -> putStrLn "C"
   |       ^^^^^^^^^^^^^^^^^

RedundantCase.hs:34:4: warning: [-Wincomplete-patterns]
    Pattern match(es) are non-exhaustive
    In a case alternative:
        Patterns not matched:
            []
            (p:_) where p is not one of {'b', 'f'}
            ['b']
            ('b':p:_) where p is not one of {'a'}
            ...
   |
34 |    case "foo" of
   |    ^^^^^^^^^^^^^...
Linking RedundantCase ...

In the first case it warns about the wrong patterns, in the second case it warns about all patterns and a non-exhaustive pattern list. In the following three cases using Integer the warnings seem to be correct. The next case shows that I can suppress the warning if I hide the constant nature of the case selector a bit. Then a case on a String - GHC does not seem to try to detect redundant pattern matches but falls back to a general test on exhaustive pattern lists. Finally, in a constant case on Bool I can avoid the warning using a plain if-then-else.

Now I wonder what the purpose of the warning is at all. First I thought that it wants to warn me, that the patterns overlap, i.e. pattern _ contains the already handled case 0. This does not seem to be the purpose of the warning - and such overlapping patterns are pretty common, aren't they? Second thought: GHC wants to warn me about redundant patterns in the presence of a constant case selector. But then, it does so with varying degrees of success: For Integer it works, but not for Int or String. Is it really only about constant case selectors? Then, not only some pattern matches are redundant but the whole case clause is redundant. Then again, can't we assume that the programmer wrote such a case clause by intend? At least, this is how I use a case clause with constant selector: as a type-safe way to out-comment alternative implementations. Thus my question: Where is this warning useful?

Change History (8)

comment:1 Changed 22 months ago by simonpj

Keywords: PatternMatchWarnings added

Thanks. There are quite a few tickets related to the pattern-match warning checker. It'd be great if someone wanted to take leadership on that front.

comment:2 Changed 19 months ago by sighingnow

Owner: set to sighingnow

comment:3 Changed 18 months ago by sighingnow

Differential Rev(s): Phab:4571
Status: newpatch

comment:4 Changed 18 months ago by RyanGlScott

Differential Rev(s): Phab:4571Phab:D4571

comment:5 Changed 16 months ago by Ben Gamari <ben@…>

In 1f88f54/ghc:

Improve exhaustiveness checking for literal values and patterns, fix #14546

Currently, we parse both the **integral literal** value and the patterns
as `OverLit HsIntegral`.  For example:

```
  case 0::Int of
      0 -> putStrLn "A"
      1 -> putStrLn "B"
      _ -> putStrLn "C"
```

When checking the exhaustiveness of pattern matching, we translate the
`0` in value position as `PmOLit`, but translate the `0` and `1` in
pattern position as `PmSLit`. The inconsistency leads to the failure of
`eqPmLit` to detect the equality and report warning of "Pattern match is
redundant" on pattern `0`, as reported in #14546. In this patch we
remove the specialization of `OverLit` patterns, and keep the overloaded
number literal in pattern as it is to maintain the consistency.  Now we
can capture the exhaustiveness of pattern `0` and the redundancy of
pattern `1` and `_`.

For **string literals**, we parse the string literals as `HsString`.
When  `OverloadedStrings` is enabled, it further be turned as `HsOverLit
HsIsString`, whether it's type is `String` or not. For example:

```
  case "foo" of
      "foo" -> putStrLn "A"
      "bar" -> putStrLn "B"
      "baz" -> putStrLn "C"
```

Previously, the overloaded string values are translated to `PmOLit` and
the non-overloaded string values are translated to `PmSLit`. However the
string patterns, both overloaded and non-overloaded, are translated to
list of characters. The inconsistency leads to wrong warnings about
redundant and non-exhaustive pattern matching warnings, as reported
in #14546.

In order to catch the redundant pattern in following case:

```
  case "foo" of
      ('f':_) -> putStrLn "A"
      "bar" -> putStrLn "B"
```

In this patch, we translate non-overloaded string literals, both in
value position and pattern position, as list of characters. For
overloaded string literals, we only translate it to list of characters
only when it's type is `stringTy`, since we know nothing about the
`toString` methods.  But we know that if two overloaded strings are
syntax equal, then they are equal. Then if it's type is not `stringTy`,
we just translate it to `PmOLit`. We can still capture the
exhaustiveness of pattern `"foo"` and the redundancy of pattern `"bar"`
and `"baz"` in the following code:

```
{-# LANGUAGE OverloadedStrings #-}
main = do
  case "foo" of
      "foo" -> putStrLn "A"
      "bar" -> putStrLn "B"
      "baz" -> putStrLn "C"
```

Test Plan: make test TEST="T14546"

Reviewers: bgamari, simonpj

Reviewed By: bgamari, simonpj

Subscribers: simonpj, thomie, carter

GHC Trac Issues: #14546

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

comment:6 Changed 16 months ago by RyanGlScott

Resolution: fixed
Status: patchclosed
Test Case: deSugar/should_compile/T14546a, deSugar/should_compile/T14546b, deSugar/should_compile/T14546c

comment:7 Changed 16 months ago by jrp

It is probably this patch that breaks test T14547

cd "./T14547.run" &&  "/home/jrp/Projects/ghc/inplace/test   spaces/ghc-stage2" -c T14547.hs -dcore-lint -dcmm-lint -no-user-package-db -rtsopts -fno-warn-missed-specialisations -fshow-warning-groups -fdiagnostics-color=never -fno-diagnostics-show-caret -dno-debug-output  -Wincomplete-patterns
Actual stderr output differs from expected:
diff -uw "/dev/null" "./T14547.run/T14547.comp.stderr.normalised"
--- /dev/null	2018-06-02 19:10:04.172693400 +0100
+++ ./T14547.run/T14547.comp.stderr.normalised	2018-06-03 13:14:15.645102495 +0100
@@ -0,0 +1,4 @@
+
+T14547.hs:14:9: warning: [-Wincomplete-patterns (in -Wextra)]
+    Pattern match(es) are non-exhaustive
+    In an equation for ‘foo’: Patterns not matched: []
*** unexpected failure for T14547(normal)

Unexpected results from:
TEST="T14547"

comment:8 Changed 16 months ago by sighingnow

I'm building ghc-head now. I will look at this problem. Sorry for the break.

Note: See TracTickets for help on using tickets.