Opened 3 years ago

Last modified 40 hours ago

#13363 new bug

Wildcard patterns and COMPLETE sets can lead to misleading redundant pattern-match warnings

Reported by: RyanGlScott Owned by:
Priority: normal Milestone: 8.10.1
Component: Compiler Version: 8.1
Keywords: PatternSynonyms, PatternMatchWarnings Cc: mpickering, gkaracha
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect error/warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: #15554 Differential Rev(s):
Wiki Page:

Description

Consider this program:

{-# LANGUAGE PatternSynonyms #-}
{-# LANGUAGE ViewPatterns #-}
{-# OPTIONS_GHC -Wincomplete-patterns #-}
module Bug 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

This compiles with no warnings with -Wall. But if you tweak catchAll to add a catch-all case at the end:

{-# LANGUAGE PatternSynonyms #-}
{-# LANGUAGE ViewPatterns #-}
{-# OPTIONS_GHC -Wincomplete-patterns #-}
module Bug 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 _               = error "impossible"

Then if you compile it with -Wall, you'll get a very misleading warning:

$ ~/Software/ghc2/inplace/bin/ghc-stage2 --interactive Bug.hs -Wall
GHCi, version 8.1.20170228: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/rgscott/.ghci
[1 of 1] Compiling Bug              ( Bug.hs, interpreted )

Bug.hs:17:1: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In an equation for ‘catchAll’: catchAll TooGoodToBeTrue = ...
   |
17 | catchAll TooGoodToBeTrue = 1
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I would have expected the warning to be on the catchAll _ = error "impossible" case!

Change History (14)

comment:1 Changed 3 years ago by simonpj

Cc: gkaracha added

George, this is your patch.

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

Replying to simonpj:

George, this is your patch.

Hello, I think this is Matthew's patch. If I remember mpickering's implementation correctly, both {F, TooGoodToBeTrue} and {F,T} are complete (especially in the module where everything is visible). So both combinations are exhaustive:

F, TooGoodToBeTrue -- Using COMPLETE               => _               redundant
F, _               -- Using the Boolean signature  => TooGoodToBeTrue redundant

Hence, I think this is just non-specified semantics, but not necessarily a bug. Maybe Matthew can shed some more light on this.

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

comment:3 Changed 3 years ago by gkaracha

Keywords: PatternMatchWarnings added

comment:4 Changed 3 years ago by mpickering

I think this is a bug because the match is not "redundant" as removing it will change the semantics of the function.

Similarly if you write

f T = 1
f F = 0
f _ = 3

then removing the F case leaves a complete definition but the correct thing to do is to remove the redundant wildcard clause.

comment:5 Changed 3 years ago by gkaracha

Yes, I guess you're right. This seems like the discussion we had on Phab:D2669, but I thought you actually disabled the redundancy check when COMPLETE pragmas are involved so that these discrepancies don't show up, with the commit:

  • Don't warn about redundancy when COMPLETE is involved

comment:6 Changed 3 years ago by mpickering

But that is not the problem.

This warning comes from the built-in constructor set. I think there needs to be some more clever augmentation to the intermediate result in the case that a result of allCompleteMatches does not contain the constructor which was used to do the lookup. For example, if ToGoodToBeTrue is not in T, F.

comment:7 Changed 3 years ago by dfeuer

Milestone: 8.4.1

comment:8 Changed 20 months ago by bgamari

Milestone: 8.4.18.6.1

This ticket won't be resolved in 8.4; remilestoning for 8.6. Do holler if you are affected by this or would otherwise like to work on it.

comment:9 Changed 18 months ago by RyanGlScott

Summary: Wildcarn patterns and COMPLETE sets can lead to misleading redundant pattern-match warningsWildcard patterns and COMPLETE sets can lead to misleading redundant pattern-match warnings

comment:10 Changed 15 months ago by bgamari

This will not be addressed in GHC 8.6.

comment:11 Changed 15 months ago by bgamari

Milestone: 8.6.18.8.1

These will not be addressed in GHC 8.6.

comment:12 Changed 13 months ago by RyanGlScott

comment:13 Changed 9 months ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

comment:14 Changed 40 hours ago by Marge Bot <ben+marge-bot@…>

In 7915afc/ghc:

Encode shape information in `PmOracle`

Previously, we had an elaborate mechanism for selecting the warnings to
generate in the presence of different `COMPLETE` matching groups that,
albeit finely-tuned, produced wrong results from an end user's
perspective in some cases (#13363).

The underlying issue is that at the point where the `ConVar` case has to
commit to a particular `COMPLETE` group, there's not enough information
to do so and the status quo was to just enumerate all possible complete
sets nondeterministically.  The `getResult` function would then pick the
outcome according to metrics defined in accordance to the user's guide.
But crucially, it lacked knowledge about the order in which affected
clauses appear, leading to the surprising behavior in #13363.

In !1010 we taught the term oracle to reason about literal values a
variable can certainly not take on. This MR extends that idea to
`ConLike`s and thereby fixes #13363: Instead of committing to a
particular `COMPLETE` group in the `ConVar` case, we now split off the
matching constructor incrementally and record the newly covered case as
a refutable shape in the oracle. Whenever the set of refutable shapes
covers any `COMPLETE` set, the oracle recognises vacuosity of the
uncovered set.

This patch goes a step further: Since at this point the information
in value abstractions is merely a cut down representation of what the
oracle knows, value abstractions degenerate to a single `Id`, the
semantics of which is determined by the oracle state `Delta`.
Value vectors become lists of `[Id]` given meaning to by a single
`Delta`, value set abstractions (of which the uncovered set is an
instance) correspond to a union of `Delta`s which instantiate the
same `[Id]` (akin to models of formula).

Fixes #11528 #13021, #13363, #13965, #14059, #14253, #14851, #15753, #17096, #17149

-------------------------
Metric Decrease:
    ManyAlternatives
    T11195
-------------------------
Note: See TracTickets for help on using tickets.