#15398 closed bug (fixed)

GADT deriving Ord generates inaccessible code in a pattern with constructor.

Reported by: philderbeast Owned by:
Priority: normal Milestone: 8.6.1
Component: Compiler Version: 8.2.2
Keywords: deriving, PatternMatchWarnings Cc:
Operating System: MacOS X Architecture: x86_64 (amd64)
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #8128 #8740 Differential Rev(s): Phab:D4993
Wiki Page:

Description

I added a second type parameter k to a GADT Zone and found that when deriving Ord with standalone deriving, the generated code has errors in ghc-8.2.2. There's a reproduction repo for this;

https://github.com/BlockScope/zone-inaccessible-code-deriving-ord.

I found I needed at least three constructors in Zone to get this error.

#8128 seemed relevant for being about standalone deriving of a GADT. That being fixed, I tried with ghc-head that I built from source. With this version I found that the same code faults exist in the generated code but are treated as warnings by ghc-8.7.2

> inplace/bin/ghc-stage2 --version
The Glorious Glasgow Haskell Compilation System, version 8.7.20180715
{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE GADTs #-}
{-# LANGUAGE StandaloneDeriving #-}

module Flight.Zone where

newtype Radius a = Radius a deriving (Eq, Ord)

data CourseLine
data OpenDistance
data EndOfSpeedSection

-- TODO: Remove standalone deriving Eq & Ord for empty data after GHC 8.4.1
-- SEE: https://ghc.haskell.org/trac/ghc/ticket/7401
deriving instance Eq CourseLine
deriving instance Eq OpenDistance
deriving instance Eq EndOfSpeedSection

deriving instance Ord CourseLine
deriving instance Ord OpenDistance
deriving instance Ord EndOfSpeedSection

data Zone k a where
    Point :: (Eq a, Ord a) => Zone CourseLine a
    Vector :: (Eq a, Ord a) => Zone OpenDistance a
    Conical :: (Eq a, Ord a) => Radius a -> Zone EndOfSpeedSection a

deriving instance Eq a => Eq (Zone k a)
deriving instance (Eq a, Ord a) => Ord (Zone k a)

The error;

/.../Zone.hs:25:1: error:
    • Couldn't match type ‘OpenDistance’ with ‘CourseLine’
      Inaccessible code in
        a pattern with constructor:
          Point :: forall a. (Eq a, Ord a) => Zone CourseLine a,
        in a case alternative
    • In the pattern: Point {}
      In a case alternative: Point {} -> GT
      In the expression:
        case b of
          Point {} -> GT
          Vector -> EQ
          _ -> LT
      When typechecking the code for ‘compare’
        in a derived instance for ‘Ord (Zone k a)’:
        To see the code I am typechecking, use -ddump-deriv
   |
25 | deriving instance (Eq a, Ord a) => Ord (Zone k a)
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/.../Zone.hs:25:1: error:
    • Couldn't match type ‘OpenDistance’ with ‘CourseLine’
      Inaccessible code in
        a pattern with constructor:
          Point :: forall a. (Eq a, Ord a) => Zone CourseLine a,
        in a case alternative
    • In the pattern: Point {}
      In a case alternative: Point {} -> False
      In the expression:
        case b of
          Point {} -> False
          Vector -> False
          _ -> True
      When typechecking the code for ‘<’
        in a derived instance for ‘Ord (Zone k a)’:
        To see the code I am typechecking, use -ddump-deriv
   |
25 | deriving instance (Eq a, Ord a) => Ord (Zone k a)
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The generated code with these replacements;

:%s/GHC\.Classes\.//
:%s/GHC\.Types\.//
:%s/Flight\.Zone\.//
instance (Eq a, Ord a) =>
           Ord (Zone k a) where
    compare a_a2hw b_a2hx
      = case a_a2hw of
          Point
            -> case b_a2hx of
                 Point -> EQ
                 _ -> LT
          Vector
            -> case b_a2hx of
                 Point {} -> GT
                 Vector -> EQ
                 _ -> LT
          Conical a1_a2hy
            -> case b_a2hx of
                 Conical b1_a2hz
                   -> (a1_a2hy `compare` b1_a2hz)
                 _ -> GT
    (<) a_a2hA b_a2hB
      = case a_a2hA of
          Point
            -> case b_a2hB of
                 Point -> False
                 _ -> True
          Vector
            -> case b_a2hB of
                 Point {} -> False
                 Vector -> False
                 _ -> True
          Conical a1_a2hC
            -> case b_a2hB of
                 Conical b1_a2hD -> (a1_a2hC < b1_a2hD)
                 _ -> False
    (<=) a_a2hE b_a2hF
      = not ((<) b_a2hF a_a2hE)
    (>) a_a2hG b_a2hH = (<) b_a2hH a_a2hG
    (>=) a_a2hI b_a2hJ
      = not ((<) a_a2hI b_a2hJ)
  
instance Eq a =>
           Eq (Zone k a) where
    (==) (Point) (Point)
      = True
    (==) (Vector) (Vector)
      = True
    (==)
      (Conical a1_a2hK)
      (Conical b1_a2hL)
      = ((a1_a2hK == b1_a2hL))
    (==) _ _ = False
    (/=) a_a2hM b_a2hN
      = not ((==) a_a2hM b_a2hN)

Change History (17)

comment:1 Changed 14 months ago by RyanGlScott

I'm not sure what exactly the bug is here. The generated code genuinely has inaccessible cases, and since GHC 8.6+, this is a warning instead of an error, so you can now actually use this code.

comment:2 Changed 14 months ago by philderbeast

Is there no way for GHC to do the derivation without the generated code having warnings?

Last edited 14 months ago by philderbeast (previous) (diff)

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

Replying to philderbeast:

Is there no way for GHC to do the derivation without the generated code having warnings?

In theory this would be possible, but it would significantly complicate the algorithm that deriving Ord uses to generate code. Currently, the algorithm is implemented as a pure function over the abstract syntax tree, but changing it to not generate inaccessible cases would require it to propagate typing information in a non-trivial way. I'm skeptical that such a large change would pay its weight, especially since the workaround (just use -Wno-inaccesible-code) is so simple.

comment:4 Changed 14 months ago by simonpj

Maybe the deriving mechanism should somehow add that flag itself. It already switches off a number of warnings -- why not this one too? (With a Note, along the lines of comment:3, to explain.)

comment:5 in reply to:  4 Changed 14 months ago by RyanGlScott

I have no objection to the proposal in comment:4. That being said, it's far from clear to me how to implement it. Unlike other flags that deriving toggles, which primarily live in the renamer, -Winaccessible-code warnings are emitted during error reporting, which happens after typechecking. What's more, when -Winaccessible-code warnings are created, GHC only has access to an Implication, which gives no indication about whether it was created from derived code or not...

comment:6 Changed 14 months ago by simonpj

it's far from clear to me how to implement it

I think the right thing to do is probably to capture the DynFlags in an Implication. We already capture the TcLclEnv in ic_env; capturing the DynFlags too would be easy.

Then the DynFlags would be conveniently to hand when generating the innaccessible-code warning.

comment:7 Changed 14 months ago by simonpj

Keywords: deriving PatternMatchWarnings added

comment:8 Changed 14 months ago by RyanGlScott

I don't understand what DynFlags has to do with anything. -Wno-inaccesible-code already disables these warnings. I thought you were requesting that we never emit inaccessible code warnings for derived code (regardless of whether -Winaccessible-code is enabled or not). That's what I don't know how to do.

comment:9 Changed 14 months ago by simonpj

I thought you were requesting that we never emit inaccessible code warnings for derived code (regardless of whether -Winaccessible-code is enabled or not).

Correct. So if we switch off -Winaccessible-code when typechecking derived code, that would do job. And we try to; see

                -- Now GHC-generated derived bindings, generics, and selectors
                -- Do not generate warnings from compiler-generated code;
                -- hence the use of discardWarnings
        tc_envs@(tcg_env, tcl_env)
            <- discardWarnings (tcTopBinds deriv_binds deriv_sigs) ;

in TcRnDriver. Unfortunately the dicardWarnings only discards warnings generated while doing tcTopBinds. But tcTopBinds also creates a bunch of constraints that don't "see" that discardWarnings.

If instead switched off the warning DynFlags, and captured the DynFlags in an implication, the warning suppression would still work on the constraints.

comment:10 Changed 14 months ago by RyanGlScott

Ah, I suppose that would work. There's one annoying hiccup, though: what should we name this new field in Implication of type DynFlags? Unfortunately, ic_dflags is already taken by InteractiveContext.

comment:11 Changed 14 months ago by simonpj

Ah, I suppose that would work.

Yes.. I like the principle that we are preserving, in the constraint tree, the environment in force when the implication was built, so that delaying solving the constraint (rather than somehow solving it immediately) doesn't change the behaviour.

Unfortunately, ic_dflags is already taken by InteractiveContext.

Ha ha! I suppose the alternatives are

  • Use the same name, on the grounds that you'll seldom, if ever, want both in scope at the same time. Downside: grep won't find the right set of uses -- I used grep quite a lot
  • Use implic_dflags or something, with a short explanation about why it's different.

comment:12 in reply to:  11 Changed 14 months ago by RyanGlScott

Replying to simonpj:

  • Use the same name, on the grounds that you'll seldom, if ever, want both in scope at the same time.

Ah, if only. I actually did try adding a field named ic_dflags to Implication, and it resulted in compilation errors due to conflicting with InteractiveContext's ic_dflags in certain modules, so this is a problem in practice.

  • Use implic_dflags or something, with a short explanation about why it's different.

I'll go with that idea.

comment:13 Changed 14 months ago by RyanGlScott

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

A WIP patch for this is at Phab:D4993. I haven't added any documentation yet, since I'm not sure that this design is the one that Simon intended.

comment:14 Changed 14 months ago by goldfire

If we really want to capture the environment in an implication, we could just store the Env TcGblEnv TcLclEnv, as retrieved by getEnv. We might want to do this in CtLoc, too, for similar reasons.

comment:15 Changed 14 months ago by Krzysztof Gogolewski <krz.gogolewski@…>

In 44a7b9b/ghc:

Suppress -Winaccessible-code in derived code

Summary:
It's rather unfortunate that derived code can produce inaccessible
code warnings (as demonstrated in #8128, #8740, and #15398), since
the programmer has no control over the generated code. This patch
aims to suppress `-Winaccessible-code` in all derived code. It
accomplishes this by doing the following:

* Generalize the `ic_env :: TcLclEnv` field of `Implication` to
  be of type `Env TcGblEnc TcLclEnv` instead. This way, it also
  captures `DynFlags`, which record the flag state at the time
  the `Implication` was created.
* When typechecking derived code, turn off `-Winaccessible-code`.
  This way, any insoluble given `Implication`s that are created when
  typechecking this derived code will remember that
  `-Winaccessible-code` was disabled.
* During error reporting, consult the `DynFlags` of an
  `Implication` before making the decision to report an inaccessible
  code warning.

Test Plan: make test TEST="T8128 T8740 T15398"

Reviewers: simonpj, bgamari

Reviewed By: simonpj

Subscribers: monoidal, rwbarton, thomie, carter

GHC Trac Issues: #8128, #8740, #15398

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

comment:16 Changed 14 months ago by monoidal

Status: patchmerge

comment:17 Changed 14 months ago by bgamari

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