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

Remove a bunch of flexible instances #53

Merged
merged 1 commit into from
Nov 18, 2023
Merged

Conversation

treeowl
Copy link
Contributor

@treeowl treeowl commented Mar 13, 2018

I don't know for sure if this is a good idea. It should improve inference, but the documentation is pretty gross. a little less clear.

@treeowl
Copy link
Contributor Author

treeowl commented Mar 13, 2018

Another idea, not yet tested:

instance (m ~~ m', MonadIO m') => MonadIO (Codensity (m :: k -> TYPE rep)) where ...

It's a little more verbose, but should probably make for more readable documentation.

@treeowl
Copy link
Contributor Author

treeowl commented Mar 13, 2018

Yes, that's better.

@treeowl
Copy link
Contributor Author

treeowl commented Mar 13, 2018

@RyanGlScott, what's your opinion?

@RyanGlScott
Copy link
Collaborator

I'll be frank—I'm not sure if the cure is worse than the disease. This introduces a metric truckload of CPP, which will definitely make this trickier to maintain in the future.

Is the hit to type inference imposed by FlexibleInstances really that bad as to warrant this?

@treeowl
Copy link
Contributor Author

treeowl commented Mar 13, 2018

@RyanGlScott, I'm not sure. @ekmett expressed frustration about that effect of being polykinded, so I figured it might be worth finding a solution. I don't know if there are realistic situations where inference will suffer here. I'm not quite as concerned about the amount of CPP as you are. It's ugly as heck, for sure, but it's only for instance heads. How many of those are we ever likely to deal with?

I'm a little more concerned about the readability of the documentation. I imagine a fair number of people will be scared of it. We could lie a bit by using && !defined(__HADDOCK__) in the CPP, but that's also not so wonderful.

@RyanGlScott
Copy link
Collaborator

How many of those are we ever likely to deal with?

In my experience, more often than you'd expect. Superclasses get added. New instances get created. Each of these will require rediscovering the right balance of CPP to use. It might be possible for us to juggle everything in our heads, but we also the time it will take for other contributors to wade through the swamp.

I'm not so worried about how the Haddocks will look—I just want to ensure that we're willing to walk the path it takes to get to that point.

@treeowl
Copy link
Contributor Author

treeowl commented Mar 16, 2018

@Icelandjack, unfortunately, something about this approach causes GHC versions 8.0 and 8.2 to panic. Only 8.4 seems up to the job!

@Icelandjack
Copy link
Contributor

Can't GHC do this automatically instead of failing?

instance Applicative f => Monoid (Codensity (f :: k -> Type) a)
-- -- --> 
instance (f ~~ f', Applicative f') => Monoid (Codensity (f :: k -> Type) a) 

@ekmett
Copy link
Owner

ekmett commented Oct 28, 2023

Now that we're far enough into the future, we should probably broadly adopt something like this, even if it's just the standalone version @treeowl proposed with

instance (m ~~ m', MonadIO m') => MonadIO (Codensity (m :: k -> TYPE rep)) where ...

If our version floor is >= 8.2, no CPP need apply.

@RyanGlScott
Copy link
Collaborator

I forgot about #53 (comment) (which the CI just reminded me of), so we will need to use GHC >= 8.4 as the floor to avoid an old bug in GHC 8.2.

Co-authored-by: Ryan Scott <ryan.gl.scott@gmail.com>
@RyanGlScott RyanGlScott merged commit f354901 into ekmett:master Nov 18, 2023
8 checks passed
RyanGlScott added a commit that referenced this pull request Nov 18, 2023
Icelandjack pushed a commit to Icelandjack/kan-extensions that referenced this pull request Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants