ekmett / lens

Lenses, Folds, and Traversals - Join us on web.libera.chat #haskell-lens
http://lens.github.io/
Other
2.03k stars 273 forks source link

makeClassyPrisms generates code with redundant catch-all cases #866

Closed RyanGlScott closed 5 years ago

RyanGlScott commented 5 years ago

While investigating #865, I noticed something fishy about the code that makeClassyPrisms generates for data types with exactly one constructor. Here is a GHCi session which illustrates the issue:

λ> newtype SDLException = SDLExceptionE String
λ> ; $(makeClassyPrisms ''SDLException)
<interactive>:12:5-35: Splicing declarations
    makeClassyPrisms ''SDLException
  ======>
    class AsSDLException r_a8H4 where
      _SDLException :: Control.Lens.Type.Prism' r_a8H4 SDLException
      _SDLExceptionE :: Control.Lens.Type.Prism' r_a8H4 String
      _SDLExceptionE = ((.) _SDLException) _SDLExceptionE
    instance AsSDLException SDLException where
      _SDLException = id
      _SDLExceptionE
        = (Control.Lens.Prism.prism (\ x1_a8H5 -> SDLExceptionE x1_a8H5))
            (\ x_a8H6
               -> case x_a8H6 of
                    SDLExceptionE y1_a8H7 -> Right y1_a8H7
                    _ -> Left x_a8H6)

<interactive>:12:5: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In a case alternative: _ -> ...

As GHC observes, the _ -> Left x_a8H6 case is completely unnecessary. It shouldn't be too hard to arrange the TH machinery not to generate this, at least in the simple case where a data type only has one constructor. (One could imagine dropping the catch-all case for certain GADTs as well, but that seems more difficult to achieve.)

ekmett commented 5 years ago

Resolved by 80eac5c

RyanGlScott commented 5 years ago

I'm assuming that https://github.com/ekmett/lens/issues/866#issuecomment-501182014 is your way of saying "I approve of #867" :)

Accordingly, I've just landed #867, so the aforementioned commit is now ced2f82d98863a6a5cf566808478c8035cfb97ac.