haskell / primitive

This package provides various primitive memory-related operations.
Other
114 stars 58 forks source link

Add convenience classes MonadPrim and MonadPrimBase #277

Closed andrewthad closed 4 years ago

lehins commented 4 years ago

@andrewthad Thank you for this PR, I've been wanting this for a while!!!!

andrewthad commented 4 years ago

@lehins Would you mind reviewing this? Mostly to check if the documentation makes sense.

andrewthad commented 4 years ago

@treeowl @ekmett @chessai @cartazio

I haven't thought super hard about this, and you had all thought about it some in https://github.com/haskell/primitive/issues/211. Above, @lehins suggests that

class (PrimBase m, s ~ PrimState m) => MonadPrimBase s m | m -> s where 

be changed to

class (PrimBase m, MonadPrim s m) => MonadPrimBase s m

Does anyone see any difference between these two approaches?

emilypi commented 4 years ago

Wonderful, thank you 👏

Does anyone see any difference between these two approaches?

The fundep should propagate in the exact same way. I don't see a meaningful difference. I prefer the second approach though for aesthetic reasons.

cartazio commented 4 years ago

I’m not sure if they propagate quite the same way. At least in older ghcs they work differently. I can’t recall at the moment what ghc version starts desugaring them the same way. Or if that change is just a paper by Richard. I feel like @RyanGlScott or @ekmett Or perhaps @hvr will have some clarity.

For more complicated fundep relations or older ghc theres definitely a difference.

emilypi commented 4 years ago

Maybe there is old lore for older GHC's I don't undersand about this, but testing locally seems to have to same results on GHC 8.x:

П> class A (m :: * -> *)
П> instance A []
П> type family B (m :: * -> *) :: *
П> type instance B [] = Int
П> class (A m, s ~ B m) => C s m | m -> s
П> instance C Int []
П> class D (m :: * -> *)
П> class (D m, C s m) => E s m
П> instance D []
П> instance E Char []

<interactive>:22:10: error:
    • Couldn't match type ‘Int’ with ‘Char’
        arising from a functional dependency between:
          constraint ‘C Char []’
            arising from the superclasses of an instance declaration
          instance ‘C Int []’ at <interactive>:17:10-17
    • In the instance declaration for ‘E Char []’
П> instance E Int []
П>

vs

П> class A (m :: * -> *)
П> instance A []
П> type family B (m :: * -> *) :: *
П> type instance B [] = Int
П> class C (m :: * -> *)
П> instance C []
П> class (C m, s ~ B m) => D s m | m -> s
П> instance D Int []
П> instance D Char []

<interactive>:14:10: error:
    • Couldn't match type ‘Int’ with ‘Char’
        arising from a functional dependency between:
          constraint ‘D Char []’
            arising from the superclasses of an instance declaration
          instance ‘D Int []’ at <interactive>:10:10-17
    • In the instance declaration for ‘E Char []’
П>
RyanGlScott commented 4 years ago

I can't think of any difference between these two versions of MonadPrimBase with respect to type inference. There is a slight difference in the underlying Core, however. This version:

class (PrimBase m, s ~ PrimState m) => MonadPrimBase s m

Will compile down to a dictionary with two fields of types PrimBase m and s ~ PrimState m, respectively. This version:

class (PrimBase m, MonadPrim s m) => MonadPrimBase s m

Will also compile down to a two-field dictionary, but this time, the s ~ PrimState m field is tucked underneath a MonadPrim s m dictionary. A consequence of this is that if you need to use MonadPrimBase to deduce that s ~ PrimState m, as in the following example:

f :: MonadPrimBase s m => proxy m -> s :~: PrimState m
f _ = Refl

Then with the first encoding of MonadPrimBase, the resulting Core needs to pattern-match on a single dictionary to extract out the evidence for s ~ PrimState m. With the second version, however, the resulting Core needs to pattern-match on two dictionaries.

Is this difference actually significant enough to matter in practice? I dunno. I'll let you be the judge of that :)

emilypi commented 4 years ago

@RyanGlScott thanks, that makes a lot of sense.

chessai commented 4 years ago

The only argument I can think of for why we should include the explicit fundep (i.e. not just letting it propagate) is for slightly better documentation (your eyes immediately see the fundep). But, this is so minour I'm not sure it matters at all. I think in practise that this will never cause UX issues?

chessai commented 4 years ago

A good argument for why it should be defined using propagation is uniformity, which I think is stronger than the minour detail of eye-catching fundeps. So I am +1 on defining it how @lehins mentioned.

lehins commented 4 years ago

I suggested an alternative definition because I though it might feel cleaner and avoid an extra fun dep, but that this is of course a pretty subjective suggestion. Whichever approach @andrewthad will choose will not matter to me I like this PR in either form ;)

treeowl commented 4 years ago

I don't see the point of the fundep. Franky, its redundancy made me do a double take. Can we get rid of that?

treeowl commented 4 years ago

@andrewthad, I'm sorry I'm late to this party, but I want to express my support for this class. I personally wish it were the fundamental one, but that ship has sailed. I'm not a big fan of the single-parameter-with-associated-type style, and I'm glad you've given us an alternative.

treeowl commented 4 years ago

Ahaha! Just noticed I was the one who opened the issue way back when. Forgot the whole thing.

chessai commented 4 years ago

see #288

cartazio commented 4 years ago

One option to put them on equal footing is have the default impl for the new and old classes be each other? I’ve not thought through the implications, but that’s one option to consider.

On Sat, Jun 20, 2020 at 11:00 PM chessai notifications@github.com wrote:

see #288 https://github.com/haskell/primitive/pull/288

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/haskell/primitive/pull/277#issuecomment-647071454, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABBQUPGPIFY4ACAOZVJT3RXVZWVANCNFSM4N4QLZRQ .