Opened 16 months ago

Last modified 2 months ago

#13965 new bug

COMPLETE sets nerf redundant pattern-match warnings

Reported by: RyanGlScott Owned by:
Priority: normal Milestone:
Component: Compiler Version: 8.2.1-rc2
Keywords: PatternSynonyms, PatternMatchWarnings Cc: mpickering, ckoparkar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

If I give redundant patterns for a datatype with a COMPLETE set, then GHC doesn't warn me at all. Here's an example:

{-# LANGUAGE PatternSynonyms #-}
{-# LANGUAGE ViewPatterns #-}
module Bug (Boolean(F, TooGoodToBeTrue), catchAll) where

data Boolean = F | T
  deriving Eq

pattern TooGoodToBeTrue :: Boolean
pattern TooGoodToBeTrue <- ((== T) -> True)
  where
    TooGoodToBeTrue = T
{-# COMPLETE F, TooGoodToBeTrue #-}

catchAll :: Boolean -> Int
catchAll F               = 0
catchAll TooGoodToBeTrue = 1
catchAll F               = 2

GHC thinks this is fine and dandy:

$ /opt/ghc/8.2.1/bin/ghci Bug.hs -Wall
GHCi, version 8.2.0.20170704: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/rgscott/.ghci
[1 of 1] Compiling Bug              ( Bug.hs, interpreted )
Ok, 1 module loaded.

This problem also persists across modules:

module Foo where

import Bug

catchAll2 :: Boolean -> Int
catchAll2 F               = 0
catchAll2 TooGoodToBeTrue = 1
catchAll2 F               = 2
$ /opt/ghc/8.2.1/bin/ghci Foo.hs -Wall
GHCi, version 8.2.0.20170704: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/rgscott/.ghci
[1 of 2] Compiling Bug              ( Bug.hs, interpreted )
[2 of 2] Compiling Foo              ( Foo.hs, interpreted )

Change History (6)

comment:1 Changed 2 months ago by ckoparkar

Cc: mpickering added

It looks like this was by design when the COMPLETE pragma was added in Phab:D2669.

compiler/deSugar/Check.hs
@@ -1360,8 +1455,8 @@ wrapUpTmState (residual, (_, subst)) = (residual, flattenPmVarEnv subst)
 dsPmWarn :: DynFlags -> DsMatchContext -> PmResult -> DsM ()
 dsPmWarn dflags ctx@(DsMatchContext kind loc) pm_result
   = when (flag_i || flag_u) $ do
-      let exists_r = flag_i && notNull redundant
-          exists_i = flag_i && notNull inaccessible
+      let exists_r = flag_i && notNull redundant && onlyBuiltin
+          exists_i = flag_i && notNull inaccessible && onlyBuiltin
           exists_u = flag_u && notNull uncovered
       when exists_r $ forM_ redundant $ \(L l q) -> do
         putSrcSpanDs l (warnDs (Reason Opt_WarnOverlappingPatterns)
@@ -1373,7 +1468,8 @@ dsPmWarn dflags ctx@(DsMatchContext kind loc) pm_result

Relevant comment: compiler/deSugar/Check.hs

data PartialResult = PartialResult {
                        presultProvenance :: Provenance
                         -- keep track of provenance because we don't want
                         -- to warn about redundant matches if the result
                         -- is contaminated with a COMPLETE pragma
...}

Do we want to change this?

comment:2 Changed 2 months ago by ckoparkar

Cc: ckoparkar added

comment:3 Changed 2 months ago by mpickering

It's this way because removing a "redudant" match arising from a COMPLETE pragma can change what a function does.

comment:4 in reply to:  3 Changed 2 months ago by RyanGlScott

Replying to mpickering:

It's this way because removing a "redudant" match arising from a COMPLETE pragma can change what a function does.

Really? What is an example of this?

comment:5 Changed 2 months ago by mpickering

data  T = A | B

{-# COMPLETE A #-}

foo A = 5
foo B = 6

comment:6 Changed 2 months ago by RyanGlScott

I think this comparison is somewhat unfair. For one thing, this isn't even a situation where the COMPLETE set would kick in, since GHC would pick the original set of constructors over the COMPLETE set. Removing the "redundant" match on B wouldn't introduce any warnings either, but that is correct behavior, since now GHC uses the COMPLETE set.

The original program is of a different nature, since the only option it has is a COMPLETE set (as evidenced by module Foo). Moreover, removing the redundant match on F doesn't change the fact that GHC is using the same COMPLETE set. This is why I argue that GHC's warning behavior on the original program is mistaken, since the second match on F is truly redundant, even after factoring in the semantics of COMPLETE sets.

Note: See TracTickets for help on using tickets.