Closed chshersh closed 3 years ago
@ChShersh :D this is my old example)
class (forall x. Functor (f x)) => Bifunctor f where
bimap :: (a -> b) -> (c -> d) -> f a c -> f b d
lmap :: (a -> b) -> f a c -> f b c
class (forall x. Functor (f x)) => Profunctor f where
dimap :: (a -> b) -> (c -> d) -> f b c -> f a d
lcmap :: (a -> b) -> f b c -> f a c
GHC 8.10 was released yesterday. The relevant feature in base-4.14
is that it brings needed instances for Kleisli
, so they no longer required since GHC-8.10. I've implemented this change and rebased on the latest master
.
Naive question:
Is it possible to define rmap = fmap
such that we have {-# MINIMAL dimap | lmap #-}
instead of {-# MINIMAL dimap | (lmap, rmap) #-}
?
@Topsii It should be possible.
Hi, I put together a tool yesterday that I have been meaning to for a while. Essentially it constructs a build matrix using a package and its reverse dependencies, which can be useful for estimating breakage in situations like this. I have already tested it on semirings
, but haven't tested profunctors
yet. Note that the tool requires you have GHC on $PATH.
Here is how you can run the build, if you are so inclined:
# clone the repo
git clone git@github.com:chessai/pump
# build the package
cabal v2-build
# copy the binary somewhere or put it on your $PATH
...do whatever you want here...
# Generate the package index (dumps the package index as binary to file `index`)
pump download -o index
# Make sure you have the following `overrides.json`:
$ cat overrides.json
[
{
"tag": "FetchFromGitHub",
"owner": "chshersh",
"subPath": null,
"package": "profunctors",
"repo": "profunctors",
"rev": "7bb87f3466b06052fe136cb56fde72671d1338fb"
}
]
# Generate the build matrix for `profunctors`
pump matrix -i index -p profunctors -o matrix.json --prettify --overrides overrides.json
# Build all packages against the new profunctors and generate a report (will take a long time)
pump realise -m matrix.json -o report.json --prettify
Actually, I am going to lunch now. I will just let it run. But, this should be useful going forward.
I put the above instructions up at https://github.com/chessai/pump/tree/master/examples/profunctors
Here is the build report: https://gist.github.com/chessai/a9695e7f89352d607ac65120223e25dc
Looks like this PR breaks at least 17 packages, probably more.
38 packages built, 117 failed to build. Not clear how many didn't build beforehand (just because I haven't looked).
Thanks @chessai this is super useful. Given that list, I've run through and made all the necessary PRs.
package | PR | Merge Status |
---|---|---|
extensible-skeleton |
https://github.com/fumieval/extensible/pull/32 | ✅ |
extensible |
depends on extensible-skeleton |
n/a |
generic-lens |
https://github.com/kcsongor/generic-lens/pull/124 | ❌ |
zuramaru |
depends on extensible |
n/a |
thyme |
seems abandoned | n/a ? |
row-types |
depends on generic-lens |
n/a |
box |
depends on generic-lens |
n/a |
unbound-generics |
https://github.com/lambdageek/unbound-generics/pull/40 | ✅ |
asif |
depends on generic-lens |
n/a |
profunctor-arrows |
https://github.com/cmk/profunctor-arrows/pull/1 | ❌ |
squares |
https://github.com/sjoerdvisscher/squares/pull/2 | ❌ |
@RyanGlScott Thanks for your review and comments!
the CLC is moving forward with some sort of plan here
Unofficially, at this point. I would like to write up the proposal as soon as the work here goes in, so Chessai and I are kicking up the dirt to see where things are at. We've done nothing else to push this work forward aside from an agreement that having profunctors
in base is a good idea, and we should begin work to make that as easy as possisble if (or when) any proposal is accepted. As far as this PR is concerned, this seems to be the one everyone points to as a necessary prerequisite.
Is there a concrete proposal somewhere that describes all of the planned changes?
No, I'll write one up with Chessai in the next week or two (I'm on vacation finally!).
I can't directly comment (i'm not authorized as a collaborator or maintainer of Profunctors), so I'll respond here:
After all, Cayley is isomorphic to Tannen from the bifunctors package, and Tannen already has a Functor instance
This seems a bit of a Star/Kleisli
situation. I'm fairly sure Cayley
here refers to the Cayley functor from Pastro-Street, which happens to have the same shape as Tannen
. The difference being intent, where Cayley
witnesses the fact that Day convolution of monoidal V-actions is isomorphic to the composition of endomodules of a profunctor, in contrast with Tannen
, which spiritually considers only bifunctors that are covariant in both parameters. Ideally, a Bifunctor
constraint would be nowhere near Cayley
because it violates that intent.
As I noted in my review, there are a ton of extraneous and actually just plain unnecessary bifunctor constraints added to many signatures. I think it's worth a review from @chshersh to see if that was perhaps the result of automated tooling or something else.
Ideally, a
Bifunctor
constraint would be nowhere nearCayley
because it violates that intent.
Fair enough. But what should be the instance context for the Functor (Cayley f p a)
instance? I currently see three options:
instance (Functor f, Functor (p a)) => Functor (Cayley f p a)
instance (Functor f, Profunctor p) => Functor (Cayley f p a)
instance (Functor f, forall x. Functor (p x)) => Functor (Cayley f p a)
It's not clear to me which is the best approach.
As I noted in my review, there are a ton of extraneous and actually just plain unnecessary bifunctor constraints added to many signatures.
Perhaps I'm missing something, but I still maintain that every Bifunctor
constraint in this PR is forced by the Functor
instances for Biff
and Tannen
, both of which use Bifunctor
in their instance contexts. (I didn't bother commenting as such under every use of Bifunctor
, but that was the intent.) The new Functor
superclass of Profunctor
affects each of these instances, so I was unable to remove the Bifunctor
constraint on any instance without first changing the upstream Functor
instance to avoid Bifunctor
.
Perhaps I'm missing something
Or maybe I'm missing something 😅 - I didn't compile before I made that statement. If it's due to Biff
and Tannen
, then yeah, I agree with you 100%. I may have gotten ahead of myself!
But what should be the instance context for the
Functor (Cayley f p a)
instance?
I'm leaning towards 2), but I would like to get @ekmett's thoughts since he's thought most about this implementation. 3) probably isn't out of the question, considering the implementation of Functor
for Kleisli
.
As mentioned in the ghc thread, I'm pro adding the quantified superclass constraint here on GHC versions that support QuantifiedConstraints
.
The main caveat is it basically takes the concept of Profunctor
and makes it a little bit harder for a newbie coming to Haskell to understand what is going on, and takes base further away from the Haskell report. They already have to get their head around the contravariance thing, but it resolves a bunch of little manufactured dilemmas like Ryan's point/question:
instance (Functor f, Functor (p a)) => Functor (Cayley f p a)
now becomes clearly better than the other two. Both of the others require unnecessary extras, assuming the forall x. Functor (p x) constraint is a superclass of BOTH Profunctor and Bifunctor, and this provides a proper lower bound. (Lower than the version with the quantifier even, so it is a lower bound for all three).
Regardless, If the CLC pulls parts of profunctors
into base I'm more than happy to let them decide how to change up the API, but I'd encourage this change to both Profunctor
and Bifunctor
at a minimum.
We do need them in several places in base anyways (I'm looking at you MonadTrans
!)
Heck I'd be willing to cut off support for GHCs old enough to lack QuantifiedConstraints
for that matter. It HAS been a while.
That last comment seems out of character for Ed. Has your cat started writing Haskell and taken over your GitHub account?
On Sat, Jul 11, 2020, 3:30 PM Edward Kmett notifications@github.com wrote:
Heck I'd be willing to cut off support for GHCs old enough to lack QuantifiedConstraints for that matter. It HAS been a while.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ekmett/profunctors/pull/70#issuecomment-657140075, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEOIX27WXEECIB3YH36WOMLR3DRZ7ANCNFSM4HADFWWQ .
@chessai I usually try to ensure I can maintain backwards compatibility for 3+ major releases when it doesn't come at a cost. Here I think it may well come at a cost and make determining which superclass you have non-trivial.
FWIW,
We do need them in several places in base anyways (I'm looking at you MonadTrans!)
MonadTrans
is in transformers
, so it's Ross' territory.
And in fact, I'd like first to see MonadTrans
acquiring QuantifiedConstraint
before anything else.
A quick note: users have been requesting some of the Functor
instances that this PR introduces, such as the instance for Coyoneda
(see #92). These Functor
instances can be introduced independently of the larger work to restructure Profunctor
's superclasses, so I have opted to do so in #93.
Thanks for doing that @RyanGlScott. I'll have bandwidth for this in a week or so.
Another question: Would it at some point in the future be desirable to have
class (foreach a. Functor (p a)) => Profunctor p where
instead of
class (forall a. Functor (p a)) => Profunctor p where
?
Trying to come up with an example I considered this to be vaguely similar to vectors whose size n
is tracked in their type. There we have instance forall n. Functor (Vector n)
but instance foreach n. Applicative (Vector n)
because the size n
is relevant for pure
.
Hence my guess is that forall
suffices for Profunctor
, but for ProductProfunctor
we might sometimes actually need
class (foreach a. Applicative (p a), Profunctor p) => ProductProfunctor p where
instead of
class (forall a. Applicative (p a), Profunctor p) => ProductProfunctor p where
(I did not know about the ProductProfunctor
class before asking myself this question. Hopefully this is correct.)
Perhaps that means we should refrain from defining ProductProfunctor
via QuantifiedConstraints
like we plan to do for Profunctor
to admit both forall
and foreach
.
Closing this PR due to lack of motivation.
Bad timing. I literally just wound up implementing this. ;)
This PR is just proof-of-concept of using
-XQuantifiedConstraints
in theprofunctors
library. The change is motivated by the discussion under corresponding GHC issue:The implementation might not be perfect. Possible improvements I can see:
{-# INLINE #-}
pragmas where appropriate.rmap
asfmap
where possible.-XFlexibleContexts
extension.But I decided to gather feedback first before putting too much effort into this. So feel free to share any thoughts!