ekmett / bifunctors

Haskell 98 bifunctors, bifoldables and bitraversables
Other
57 stars 42 forks source link

deriveBifoldable generates an unused-variable #89

Closed nfrisby closed 3 years ago

nfrisby commented 3 years ago

With

newtype XY a b= XY { getResp :: Either X (Y a b) }
  deriving (Show, Functor, Foldable)

we get

    TH.deriveBifoldable ''XY
  ======>
    instance Bifoldable XY where
      bifoldr
        = \ f_asLl g_asLm z_asLn value_asLo
            -> ((((bifunctors-5.5.9:Data.Bifunctor.TH.Internal.bifoldrConst
                     (case value_asLo of {
                        XY _arg1_asMt
                          -> ((\ n1_asMn n2_asMo
                                 -> (((bifoldr (\ n1_asMp n2_asMq -> n2_asMq))
                                        (\ n1_asMr n2_asMs
                                           -> (((bifoldr f_asLl) g_asLm) n2_asMs) n1_asMr))
                                       n2_asMo)
                                      n1_asMn)
                                _arg1_asMt)
                               z_asLn }))
                    f_asLl)
                   g_asLm)
                  z_asLn)
                 value_asLo
      bifoldMap
        = \ f_asMv g_asMw value_asMy
            -> (((bifunctors-5.5.9:Data.Bifunctor.TH.Internal.bifoldMapConst
                    (case value_asMy of {
                       XY _arg1_asNy
                         -> ((bifoldMap (\ n_asNx -> mempty)) ((bifoldMap f_asMv) g_asMw))
                              _arg1_asNy }))
                   f_asMv)
                  g_asMw)
                 value_asMy

and two warnings:

warning: [-Wunused-matches]
    Defined but not used: ‘n1’
    |
304 | TH.deriveBifoldable    ''XY
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: [-Wunused-matches]
    Defined but not used: ‘n’
    |
304 | TH.deriveBifoldable    ''XY
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The offenses are (\ n1_asMp n2_asMq -> n2_asMq) and (\ n_asNx -> mempty), I think.

We're unfortunately handling this by disabling that warning in the module, for now.

Thanks!

RyanGlScott commented 3 years ago

Just to be clear, what are the definitions of X and Y that you are using? The code that deriveBifoldable will generate depends slightly on this, so I want to make sure that I can reproduce exactly what you're seeing.

nfrisby commented 3 years ago

Ah! I figured I was being helpful by reducing noise. I'm surprised it's inspecting occurrences of other types --- maybe it's looking through synonyms?

But "the offenses" I called out at the bottom of my Issue do seem to indicate that the problem is quite localized.

HTH. Thanks!

XY is newtype Resp fp h = Resp { getResp :: Either FsError (Success fp h) }

Y is

data Success fp h =
    WHandle    fp h
  | RHandle    h
  | Unit       ()
  | Path       fp ()
  | Word64     Word64
  | ByteString ByteString
  | Strings    (Set String)
  | Bool       Bool

X is FsErrorPath.

newtype MountPoint = MountPoint FilePath
data FsErrorPath = FsErrorPath (Maybe MountPoint) FsPath
RyanGlScott commented 3 years ago

Ah! I figured I was being helpful by reducing noise.

To be clear, I definitely appreciate a minimal example. The reason I ask about what X and Y is that deriveBifoldable has a special case for data types where the last two type parameters have phantom roles, as it can generate code which does less work in that case. Indeed, my first attempt at guessing what Y was data Y a b, but since its type parameters have phantom roles, deriveBifoldable generated code that bypassed the bug altogether. At that point, I realized that I should stop guessing your intent and just ask you directly :)

In any case, https://github.com/ekmett/bifunctors/issues/89#issuecomment-762989059 gives me enough information to go on. I've submitted #90 with a fix. Can you see if that resolves your issue?

nfrisby commented 3 years ago

Thanks Ryan! To be honest, I haven't master the dependency management enough to easily slot it in and try. I've put it on my TODO list, but I don't know when it will float near the top.

For now, I've inspected your PR's diff, and it seems very likely that that'll fix our specific use-case. 👍

RyanGlScott commented 3 years ago

OK. I definitely did fix a bug in #90, so I decided to be bold and just make a new Hackage release in bifunctors-5.5.10.