mgsloan / store

Fast binary serialization in Haskell
MIT License
109 stars 35 forks source link

Build fails on new vector version #148

Closed identicalsnowflake closed 4 years ago

identicalsnowflake commented 4 years ago

vector-0.12.1.2 was recently released and apparently fixes a number of longstanding bugs. However, store fails to build (possibly due to breaking upstream changes?) with this version with the following error:

src/Data/Store/Internal.hs:779:3: error:
    • Expected kind ‘* -> *’, but ‘f’ has kind ‘*’
    • In the first argument of ‘Alt’, namely ‘f’
      In the first argument of ‘Data.Vector.Unboxed.Base.Vector’, namely
        ‘(Alt f a)’
      In the first argument of ‘Store’, namely
        ‘(Data.Vector.Unboxed.Base.Vector (Alt f a))’
    |
779 | $(deriveManyStoreUnboxVector)
    |   ^^^^^^^^^^^^^^^^^^^^^^^^^^
sjakobi commented 4 years ago

@cartazio Any idea which change in vector might have triggered this?

cartazio commented 4 years ago

Eep. There shouldn’t be any such change. What is the th doing here? I’m afk atm and headed to day job. But I’ll try to carve out some time to look.

Things that’ll help me poke at this quickly 1) what’s the definition of this th code? 2) what’s the equivalent code that was generated for 0.12.0 series vector (the src that’s generated and gets type checked ?) 3) what’s the equivalent code that’s generated now that fails to type check?

One possibility is that the new instances in unbox that vector now provides conflicts with previously orphaned instances ? There were a bunch of monoid ish newtypes in base that didn’t have instances for unboxed vectors previously. Could the issue lie with thst ?

cartazio commented 4 years ago

https://hackage.haskell.org/package/vector-0.12.1.2/docs/Data-Vector-Unboxed.html#t:Vector

Unboxed Vector now has an instance for Alt.

Is your unboxed instance different ? Or just an Orphan?

cartazio commented 4 years ago

Naively : orphan instances ?

Shimuuar commented 4 years ago

No TH can't work with type parameters kind other than *. store uses TH to generate Store instances for all types which are also has Unboxed instance. Here is instance head that is generated for Alt f a:

instance ( Store (U.Vector f)
         , Store (U.Vector a)
         ) => Store (U.Vector (Alt f a)) where
cartazio commented 4 years ago

So it’s a bug in how the TH here is recursively lifting vector instances into store instances. Previously vector didn’t have this sort of functor decomposition so that case wasn’t needed before ?

cartazio commented 4 years ago

It should be generating a “Store vector (f a) => store vector (Alt f a)” Right ?

Shimuuar commented 4 years ago

Yep. Previously all instances for vector only had type parameter of kind *. Main problem is it's difficult to work with type parameters other than * in TH. If you have data type Foo (f :: * → *) (a :: *) you may need all sort of constraints in head: Store (f Int), Store (f a), Store a or any combination of them.

I'm not sure what is right fix here.

cartazio commented 4 years ago

Fuzzily one approach would be to write logic specific to some known stuff.

Eg for newtypes you could take the body type of the newtype as the head constraint?

On Thu, Feb 6, 2020 at 12:03 PM Aleksey Khudyakov notifications@github.com wrote:

Yep. Previously all instances for vector only had type parameter of kind . Main problem is it's difficult to work with type parameters other than in TH. If you have data type Foo (f :: ) (a :: *) you may need all sort of constraints in head: Store (f Int), Store (f a), Store a or any combination of them.

I'm not sure what is right fix here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mgsloan/store/issues/148?email_source=notifications&email_token=AAABBQR5SGJF55IWYMD4YHLRBQ7FZA5CNFSM4KPBA6IKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK77RDY#issuecomment-583006351, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABBQUMYOZLWUBDNDWP6E3RBQ7FZANCNFSM4KPBA6IA .

mgsloan commented 4 years ago

Hey all, thanks for digging into this!

The problem is that deriveManyStoreUnboxVector was just assuming all type variables needed Store (Vector a) instances. I resolved this by reifying the instances of Unbox and using that to inform the correct constraints. So now we get

instance Store (Vector (f (g a))) => Store (Vector (Compose f g a))

instead of the silly wrong thing

instance (Store (Vector f), Store (Vector g), Store (Vector a)) => Store (Vector (Compose f g a))

because the instance head is now based on substituting Unbox ... with Store (Vector ...) based on the Unbox instance

instance Unbox (f (g a)) => Unbox (Compose f g a)
mgsloan commented 4 years ago

Fixed in version 0.7.2