Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require Functor over Bifunctor where possible #125

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

expipiplus1
Copy link

Fixes #124

Note that this does add a Functor requirement where none was required
before

@expipiplus1
Copy link
Author

expipiplus1 commented May 21, 2023

tests/T124Spec.hs Show resolved Hide resolved
tests/T124Spec.hs Outdated Show resolved Hide resolved
tests/T124Spec.hs Outdated Show resolved Hide resolved
tests/BifunctorSpec.hs Outdated Show resolved Hide resolved
src/Data/Bifunctor/TH.hs Outdated Show resolved Hide resolved
src/Data/Bifunctor/TH.hs Outdated Show resolved Hide resolved
src/Data/Bifunctor/TH.hs Show resolved Hide resolved
@expipiplus1
Copy link
Author

expipiplus1 commented May 22, 2023

Thank you for the patient review, @RyanGlScott!

Still to fix is the instance context generation, as it still wants to generate Bifunctor instances for any type variable f : * -> * -> *. This should now depend on its usage.

For example this, although f and g both have kind * -> * -> *, f requires a Bifunctor instance and g a Functor instance:

data Baz f g a b where
  Baz1 :: f a b -> Baz f g a b
  Baz2 :: g Int b -> Baz f g a b

deriveBifunctor ''Baz

An alternative would be to not change this context generation heuristic and simply use the old behavior of requiring a Bifunctor instance when dealing with variables with two type variables (regardless of if they're ours) i.e. only use this new behavior when it's a concrete type constructor. (Nicest yet would be just put a wildcard for the instance context, and allow GHC to fill it with the inferred context (https://gitlab.haskell.org/ghc/ghc/-/issues/13324))

For now I've fallen back to requiring Bifunctor for type variables, it's still an improvement over what was before I think though.

@expipiplus1
Copy link
Author

Another alternative would be to allow configuring this behavior via Options, something like preferFmap, that way people who can tolerate writing their own instance head can use that and allow type variables with arity > 2 to still use Functor where possible.

@RyanGlScott
Copy link
Collaborator

Ugh, I completely forgot about instance context generation. I'll be the first person to admit that the way bifunctors infers instance contexts is imperfect, and that's largely to be expected. Doing It Right™ would require something akin to full-blown type inference, and that is so difficult to do with Template Haskell that I don't think it is worth bothering to try.

This is all to say: I'm not terribly bothered by the fact that deriveBifunctor ''Baz would attempt to generate instance (Bifunctor f, Bifunctor g) => Bifunctor (Baz f g). Examples like Baz2 :: g Int b -> Baz f g a b are not the common case, so I'm fine with making users manually write the instance context out for such instances (in conjunction with makeBimap). Moreover, this generated instance will actually typecheck with GHC 9.6 due to Bifunctor having a quantified Functor superclass, so this bothers me even less.

What does bother me a bit is the fact that these two instances:

data Scooby1 f a b = Scooby1 (f Int a)
data Scooby2 a b = Scooby2 (Either Int a)

Will interact quite differently with deriveBifunctor. The former would use bimap in its generated instance, while the latter will use fmap. I'm not a fan of this because I generally like things to be invariant under substitution, and Scooby2 is basically the same thing as Scooby1, where f is instantiated to Either. Ideally, calling bimap on Scooby1 Either would behave exactly the same as calling bimap on Scooby2, but that isn't the case with the way the patch is set up currently.

To summarize, I'd prefer using fmap uniformly for things that look like F Int a or f Int a, even if that doesn't always work with bifunctors' limited type inference capabilities. You could also guard this functionality behind Options if you'd like—I don't have a strong opinion on that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kind mismatch in derived instances
2 participants