love-haskell / coercible-utils

Utility functions for Coercible types
https://hackage.haskell.org/package/coercible-utils
BSD 3-Clause "New" or "Revised" License
9 stars 3 forks source link

0.1.0 fails to compile previously accepted programs #24

Open kozross opened 4 years ago

kozross commented 4 years ago

Consider the following program:

import CoercibleUtils (over2)
import Data.Finitary (Finitary(..))

newtype Finiteness a = Finiteness a

instance (Finitary a) => Eq (Finiteness a) where
  (==) = over2 Finiteness (equating toFinite)

equating :: (Eq b) => (a -> b) -> a -> a -> Bool
equating p x y = p x == p y

When using coercible-utils-0.0.0, this compiles. However, with coercible-utils-0.1.0, I get the following error:

    • There is no CoercibleUtils.Newtype.Newtype instance for
          Bool
      because it is not a newtype.
    • In the expression: over2 Finiteness (equating toFinite)
      In an equation for ‘==’:
          (==) = over2 Finiteness (equating toFinite)
      In the instance declaration for ‘Eq (Finiteness a)’
   |
 7 |   (==) = over2 Finiteness (equating toFinite)
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is this a regression? Was my original code somehow incorrect?

treeowl commented 4 years ago

It was a philosophy change. Type inference for the old version was pretty bad, so we're doing something different now. I didn't realize the old interface was going away altogether; I think it might make sense to bring it back in a different module for users who need its extreme flexibility.

treeowl commented 4 years ago

Anyway, you can fix your code by extracting the result from a Finiteness type. That error message is not the most helpful one here; I'm not sure how best to fix it, but I'll try.

kozross commented 4 years ago

I would certainly prefer if the old interface remained somewhere. Some of the locations where I use (for example) over2 cannot be rewritten as elegantly using the new interface - this one happens to be one that can, but it's quite trivial (for bug reporting purposes).

treeowl commented 4 years ago

I'm all in favor of offering both options, but would you mind showing an example or three that are less easily converted? That could point the way toward further development.

kozross commented 4 years ago

Let's consider the example above - based on what you told me, I would have to write something like

  (==) = op Finiteness . over2 Finiteness (equating toFinite)

Now, this is what it looks like without use of over2:

  (Finiteness x) == (Finiteness y) = equating toFinite x y

This is actually more concise than the version with the new interface. Another case I have (in the same package) is simplifying some boilerplate methods for MVector instances; for example:

import qualified Data.Vector.Generic.Mutable as VGM
import qualified Data.Vector.Unboxed as VU

-- some more stuff here

newtype PackInto a b = PackInto { unpackFrom :: b }

newtype instance VU.MVector s (PackInto a b) = MV_PackInto (VU.MVector s a)

-- some other stuff

  basicOverlaps = over2 MV_PackInto VGM.basicOverlaps

With the new interface, I have to do something like

  basicOverlaps = op ??? . over2 MV_PackInto VGM.basicOverlaps

Here, I'm not even sure what ??? should be, since MV_PackInto is a newtype wrapper around an MVector, not a Bool.

treeowl commented 4 years ago

For your == example, and others with that shape, I think there's a better way:

import Data.Function (on) -- Why isn't that in the Prelude?

instance Finitary a => Eq (Finiteness a) where
  (==) = (==) `on` (toFinite . op Finiteness)

I don't understand your second example well enough to have an opinion. I suspect you might want a function like this, among others, for some extreme performance hacking:

onco :: Coercible a b => (b -> b -> c) -> (a `arr` b) -> a -> a -> c
onco bbc _ = coerce bbc
treeowl commented 4 years ago

I definitely want to add the opposite of op:

upgrade :: Coercible a b => (a `arr` b) -> a -> b
upgrade _ = coerce
kozross commented 4 years ago

I didn't know about on - thanks! I agree that upgrade is useful and should exist. As far as onco is concerned, I can certainly do that, but this is very similar to what over2 used to do for me, and I wouldn't need onco (possibly in multiple places) if the old interface could continue to exist. I don't think having the old interface around does anyone any harm - you can keep the new interface as the default, and have the old interface remain for people who want to keep using it, in a separate module.

treeowl commented 4 years ago

I agree with you that the old interface should continue to exist. But since much of the old interface is awkward in practice (due to poor type inference), I would very much like to expand the new interface to accommodate unsatisfied real-world needs like yours!

kozross commented 4 years ago

I think what I might do is try to learn the new interface properly. It might show me that my issues aren't actually issues - just misunderstandings. After that, I might be able to offer more concrete examples and better feedback.

sjakobi commented 4 years ago

If we bring back the old interface, I would prefer it not to live in the same package as the new one: I believe that would simply be confusing. Since IMO the name coercible-utils fits the old interface better, the new interface would have to find a new home.

kozross commented 4 years ago

OK, having done a bit of reading, and thinking, I think I'm ready to give a proper, thought-out and explained example, as well as some suggestions.

Background

This example is based on my finitary library. Without going into it too deeply, if we have an instance of Finitary for a type a, we get the following things:

One useful thing this gets us is the ability to 'pack' a type with a Finitary instance into another type with a Finitary instance, since fromFinite must be a bijection (by the laws. Therefore, we can munge the types into each other, without worrying about overlaps or anything like that. This is particularly useful for Unbox instances - as long as we can munge into a type with an existing Unbox instance, and said type's cardinality isn't smaller than ours, we now essentially gain such an instance 'for free'.

To do this, we need something analogous to this:

{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE TypeInType #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE TypeFamilies #-}

import GHC.TypeNats
import Data.Finite
import Data.Kind

import qualified Data.Vector.Unboxed as VU
import qualified Data.Vector.Generic as VG
import qualified Data.Vector.Generic.Mutable as VGM

newtype PackInto (a :: Type) (b :: Type) = PackInto b

This is essentially Flip Const, mostly for convenience with DerivingVia: basically, a corresponds to the type we 'munge into', while b is the actual thing we want to represent. The desired goal is being able to write something like deriving Unbox via PackInto Word8 or such.

Now, to write an Unbox instance, there is a fairly hefty procedure. In essence, we need to write a 'wrapper' around an existing Unbox instance, and then provide all the necessary machinery for the vector library to handle it. First, we define a newtype instance of the relevant type:

newtype instance VU.MVector s (PackInto a b) = MV_PackInto (VU.MVector s a)

Essentially, this says 'a mutable unboxed vector of PackInto a bs is really just a newtype wrapper (MV_PackInto) around a mutable unboxed vector of a, with the same state token type'. Having done that, we now have to instantiate the VGM.MVector type class - in our case, the constraints look like so:

instance (Finitary a, Finitary b, Cardinality b <= Cardinality a, VU.Unbox a) => VGM.MVector VU.MVector (PackInto a b) where

A bunch of methods are required here: the one I'll use as an example (with all constraints noted) is:

basicOverlaps :: (Finitary a, Finitary b, Cardinality b <= Cardinality a, VU.Unbox a, VGM.MVector v (PackInto a b)) => v s (PackInto a b) -> v s (PackInto a b) -> Bool

Now, the implementation here is rather trivial - if the underlying vectors overlap, so do the wrappers. Therefore, the 'basic' way of doing this would be to write something like this

basicOverlaps (MV_PackInto v1) (MV_PackInto v2) = VGM.basicOverlaps v1 v2

This gets tedious quickly. Also, there are quite a few such methods where all we do is delegate to the underlying types.

The problem

coercible-utils-0.0.0 to the rescue! This is basically an ideal use of over2:

basicOverlaps = over2 MV_PackInto VGM.basicOverlaps

This is concise and fairly clear in its effects. The key thing that makes this work is the resulting type, which is (class constraints omitted for brevity):

over2 MV_PackInto VGM.basicOverlaps :: (Coercible (VU.MVector s a) (MV_PackInto s (PackInto a b), Coercible Bool b') => MV_PackInto s (PackInto a b) -> MV_PackInto s (PackInto a b) -> b' 

This works like a charm, because Coercible is reflexive, which means that b' will happily unify with Bool, and everyone goes home happy.

However, this is not possible with the new API, as the type of over2 is now

over2 :: (Newtype n o, Newtype n' o', Similar n n') => (o `to` n) -> (o -> o -> o') -> n -> n -> n' 

After 'concretifying' to the types in question, we get this:

over2 MV_PackInto VGM.basicOverlaps :: (Newtype (MV_PackInto s (PackInto a b)) (VU.MVector s a), Newtype n' Bool, Similar (MV_PackInto s (PackInto a b)) n') => MV_PackInto s (PackInto a b) -> MV_PackInto s (PackInto a b) -> n'

The problem is that this doesn't work - n' will not unify with Bool. Indeed, it cannot even unify with anything that meaningfully could store a Bool for us, due to the definition of Similar:

Two types are Similar if they are built from the same type constructor and the same kind arguments.

Given that MV_PackInto is just a newtype around VU.MVector, there is nothing Similar to it that I could use here. The kind of MV_PackInto is Type -> Type -> Type -> Type, and the 'underlying thing' is an entire mutable vector - both are not suitable in any way for storing a single Bool.

Now, I assume the Similar constraint has a reason to be there, but it's ultimately what's creating this issue. Looking through the other similar functions (under, under2, ala, etc), it seems that a similar (sorry, bad pun) Similar constraint is present there also.

Proposed solutions

These are based on my understanding, which is likely limited. However, the way I see it, two things need to happen here to restore this functionality, while keeping (some of) the advantages of the new interface.

Remove the Similar constraint on over2 (and possibly other similar cases)

I'm not entirely clear on why this constraint is required - it is a severe restriction relative the old version, and potentially breaks a lot of prior uses. The kindedness enforcement is particularly harsh here - I don't even see a reasonable solution under that kind of (again with these puns, argh) constraint.

Weaken the second Newtype to Coercible

Given the compiler error, it would seem that Newtype lacks the reflexivity properties of Coercible. However, it is this very reflexivity that allows uses similar to the one I described. I believe stronger direction vis-a-vis the 'packer' (as well as the Newtype constraint used there) is warranted, but the return type could be considerably freer.

Proposed over2 signature

over2 :: (Newtype n o, Coercible a b) => (o `to` n) -> (o -> o -> a) -> n -> n -> b 

Conclusion

Hopefully this makes my use case, situation, and issues, clear. I certainly think that I'd be far from alone in having such issues, and in my view, the new interface significantly weakens the power of the old. At the same time, as someone who has (painfully) experienced the difficulties of using the old interface, I welcome any improvement to type inference and usability. I believe a middle ground is possible, and could be pursued.

If this is not possible, I agree with @sjakobi - the naming could cause confusion. Since I am not a maintainer of this package, though, the decision does not rest with me.

sjakobi commented 4 years ago

Fascinating stuff! First time I've seen "newtype instance" admittedly and I definitely don't comprehend all of this.

I hope that @treeowl can explain why exactly the new over2 doesn't work for your basicOverlaps example.

Regarding the type you propose for over2:

over2 :: (Newtype n o, Coercible a b) => (o `to` n) -> (o -> o -> a) -> n -> n -> b

This just wouldn't fit into the new API since there's no way to infer the result type from the arguments to over2.

What I think you can do, is define

over2' pack op a b = coerce (over2 pack op a b)

Does that work for you?

treeowl commented 4 years ago

@sjakobi I don't have a terribly strong opinion about where the Newtype API should live. Should I open a discussion on the libraries list?

sjakobi commented 4 years ago

@treeowl Actually if you, @kozross, or @chessai, or anybody else wants to take over the package, the new maintainer can do as they please. For me, this package was mostly about experimenting with Coercible. Your new API is super interesting too! But I don't really have any use for this package…

kozross commented 4 years ago

@sjakobi No, I don't think this would work, for precisely the non-unification reason given before. Ultimately, coerce requires that whatever I get out of over2 is Coercible to Bool; however, over2 in the new interface requires Similar to hold for the result, which basically makes this impossible to satisfy.

I'm happy to take on maintenance of this package, but at the same time, @treeowl is definitely a major contributor and thus has priority. I don't wanna unilaterally wipe out or modify someone else's (considerable) work, and so if @treeowl wants this, I'd rather they take it instead of me.

treeowl commented 4 years ago

I don't need to maintain this package. I do want the Newtype version (as it is now, or maybe even with stronger inference somehow) to survive somewhere. I don't care where. I do think we should reach a satisfactory answer to that question before we proceed though.

kozross commented 4 years ago

@treeowl If it were up to me, I would basically have your interface be the default, then bring back the old interface exactly as-was in a separate module, which people could import if they choose (qualified if they need both). I believe for a lot of use cases, your interface is superior: mine just happens not to be one of them.

treeowl commented 4 years ago

That strikes me as totally reasonable. @sjakobi is concerned it will be confusing, but I disagree.

On Fri, Sep 13, 2019, 5:49 PM Koz Ross notifications@github.com wrote:

@treeowl https://github.com/treeowl If it were up to me, I would basically have your interface be the default, then bring back the old interface exactly as-was in a separate module, which people could import if they choose (qualified if they need both). I believe for a lot of use cases, your interface is superior: mine just happens not to be one of them.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sjakobi/coercible-utils/issues/24?email_source=notifications&email_token=AAOOF7LZ5CF5TXQONTBHHLLQJQDFZA5CNFSM4IVL3NYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6WIZFI#issuecomment-531401877, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOOF7LYBKWQK5RRI2XIZYLQJQDFZANCNFSM4IVL3NYA .

kozross commented 4 years ago

The only source of confusion I could foresee would be for people who upgrade from 0.0.0 to whatever version above that, then suddenly being hit with code not compiling as it used to. However, I believe that the PVP permits this, and as long as this is properly documented (preferably in several places), it shouldn't be an issue. Otherwise, I don't believe that confusion is possible.

sjakobi commented 4 years ago

The confusion I was concerned about is when you use QuickJump in the haddocks, find two versions of e.g. over2, and then have to figure out which one is right for you. Good documentation can probably address this.

I would be happy to add you as a maintainer on Hackage @kozross, and to transfer this repo to you. Having a maintainer who uses this package himself will be much better for this package.

@chessai Any objections?

sjakobi commented 4 years ago

@kozross I've added you as a maintainer on Hackage.

To transfer this repo to you, it seems like you'd have to remove your fork. Alternatively I can rename the repo. What should I do? :)

chessai commented 4 years ago

No objections here.

treeowl commented 4 years ago

It seems generally much nicer to create GitHub "organizations" to own packages. Then they don't have to move around as maintainers change.

On Mon, Sep 16, 2019, 3:04 PM chessai notifications@github.com wrote:

No objections here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sjakobi/coercible-utils/issues/24?email_source=notifications&email_token=AAOOF7NSDNCSBTBWWFOX3DLQJ7KENA5CNFSM4IVL3NYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD62FZ2A#issuecomment-531913960, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOOF7IJ6KPULXZMZMWPMFLQJ7KENANCNFSM4IVL3NYA .

kozross commented 4 years ago

@sjakobi Thanks! I've now removed my fork, so you can transfer it to me.

chessai commented 4 years ago

It seems generally much nicer to create GitHub "organizations" to own packages. Then they don't have to move around as maintainers change. On Mon, Sep 16, 2019, 3:04 PM chessai @.***> wrote: No objections here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24?email_source=notifications&email_token=AAOOF7NSDNCSBTBWWFOX3DLQJ7KENA5CNFSM4IVL3NYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD62FZ2A#issuecomment-531913960>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOOF7IJ6KPULXZMZMWPMFLQJ7KENANCNFSM4IVL3NYA .

I agree with this. I've done this with haskell-primitive and haskell-snmp, and there's also haskell-streaming. this is probably the best course of action.

sjakobi commented 4 years ago

It seems generally much nicer to create GitHub "organizations" to own packages. Then they don't have to move around as maintainers change.

I'll let @kozross decide this.

kozross commented 4 years ago

I have no problem with this idea.

sjakobi commented 4 years ago

I have no problem with this idea.

Sounds great! :) Can you implement it then?

kozross commented 4 years ago

@sjakobi I'll probably work on this closer to the weekend. If I find time during the week, I will definitely do so.

kozross commented 4 years ago

I have created the organization, hopefully correctly. I've invited @treeowl - who else should be added? I'll transfer the repository to it shortly.

treeowl commented 4 years ago

That's a surprising name... Others use names like haskell-streaming, for example. But it's up to you ;-)

On Tue, Sep 17, 2019 at 4:42 PM Koz Ross notifications@github.com wrote:

I have created the organization https://github.com/love-haskell, hopefully correctly. I've invited @treeowl https://github.com/treeowl - who else should be added? I'll transfer the repository to it shortly.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kozross/coercible-utils/issues/24?email_source=notifications&email_token=AAOOF7MDNA63SUUP2DWRDJDQKE6KNA5CNFSM4IVL3NYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD65234I#issuecomment-532393457, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOOF7KQQUYYWPEFFR6F5WDQKE6KNANCNFSM4IVL3NYA .

kozross commented 4 years ago

@treeowl I dunno - guess I'm in that kind of mood today.