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

Do not ignore kinded type variables - fix for issue 52 #56

Merged
merged 1 commit into from Oct 26, 2015

Conversation

Projects
None yet
3 participants
@ddssff
Collaborator

ddssff commented Jun 23, 2015

No description provided.

@stepcut

This comment has been minimized.

Show comment
Hide comment
@stepcut

stepcut Oct 26, 2015

Member

Testing if this comment will trigger a travis-ci build. Probably not.

Member

stepcut commented Oct 26, 2015

Testing if this comment will trigger a travis-ci build. Probably not.

stepcut added a commit that referenced this pull request Oct 26, 2015

Merge pull request #56 from seereason/master
Do not ignore kinded type variables - fix for issue 52

@stepcut stepcut merged commit f37c73c into acid-state:master Oct 26, 2015

@oxij

This comment has been minimized.

Show comment
Hide comment
@oxij

oxij Nov 20, 2015

Just wondering here, why do you use allTyVarBndrNames in some places, and plainTyVarBndrNames in others?

I have a piece of code like this

{-# LANGUAGE UnicodeSyntax, OverloadedStrings
  , MultiParamTypeClasses, FlexibleContexts
  , DataKinds, KindSignatures, GADTs, TypeFamilies
  , DeriveDataTypeable, TemplateHaskell, QuasiQuotes
  , RecordWildCards, ScopedTypeVariables
  #-}
-- I think it breaks because of either DataKinds or KindSignatures

data Server q = Server [q]
                        deriving (Show, Typeable)

serverRequest :: Eq q => [q] → Update (Server q) ()
serverRequest = undefined

$(makeAcidic ''Server [ 'serverRequest ])

which compiled before ghc 7.10 and all the updates to acid-state that followed, but it doesn't compile now.

With this PR merged it generates splices that don't have (SafeCopy q, Typeable q, ...) => in IsAcidic instance, which can be fixed on case-by-case basis by adding those constraints to event handlers (e.g. serverRequest), but there is a TH code that seems to be designed to automate this, and it doesn't work with the example above because mkCxtFromTyVars uses plainTyVarBndrNames and q is not plain.

The following patch fixes it and all my code (and I have a lot) that worked before 7.10 starts working again, but then I wonder why these plain* functions were introduces in the first place?

diff --git a/src/Data/Acid/TemplateHaskell.hs b/src/Data/Acid/TemplateHaskell.hs
index 2454c7a..4e5e82c 100644
--- a/src/Data/Acid/TemplateHaskell.hs
+++ b/src/Data/Acid/TemplateHaskell.hs
@@ -249,7 +249,7 @@ makeSafeCopyInstance eventName eventType
           structName (x:xs) = toUpper x : xs

 mkCxtFromTyVars preds tyvars extraContext
-    = cxt $ [ classP classPred [varT tyvar] | tyvar <- plainTyVarBndrNames tyvars, classPred <- preds ] ++
+    = cxt $ [ classP classPred [varT tyvar] | tyvar <- allTyVarBndrNames tyvars, classPred <- preds ] ++
             map return extraContext

 {-
@@ -262,7 +262,7 @@ makeMethodInstance eventName eventType
     = do let preds = [ ''SafeCopy, ''Typeable ]
              ty = AppT (ConT ''Method) (foldl AppT (ConT eventStructName) (map VarT (allTyVarBndrNames tyvars)))
              structType = foldl appT (conT eventStructName) (map varT (allTyVarBndrNames tyvars))
-         instanceD (cxt $ [ classP classPred [varT tyvar] | tyvar <- plainTyVarBndrNames tyvars, classPred <- preds ] ++ map return context)
+         instanceD (cxt $ [ classP classPred [varT tyvar] | tyvar <- allTyVarBndrNames tyvars, classPred <- preds ] ++ map return context)
                    (return ty)
 #if __GLASGOW_HASKELL__ >= 707
                    [ tySynInstD ''MethodResult (tySynEqn [structType] (return resultType))
@@ -283,7 +283,7 @@ makeEventInstance eventName eventType
     = do let preds = [ ''SafeCopy, ''Typeable ]
              eventClass = if isUpdate then ''UpdateEvent else ''QueryEvent
              ty = AppT (ConT eventClass) (foldl AppT (ConT eventStructName) (map VarT (allTyVarBndrNames tyvars)))
-         instanceD (cxt $ [ classP classPred [varT tyvar] | tyvar <- plainTyVarBndrNames tyvars, classPred <- preds ] ++ map return context)
+         instanceD (cxt $ [ classP classPred [varT tyvar] | tyvar <- allTyVarBndrNames tyvars, classPred <- preds ] ++ map return context)
                    (return ty)
                    []
     where (tyvars, context, _args, _stateType, _resultType, isUpdate) = analyseType eventName eventType
@@ -329,12 +329,5 @@ tyVarBndrName :: TyVarBndr -> Name
 tyVarBndrName (PlainTV n)    = n
 tyVarBndrName (KindedTV n _) = n

-plainTyVarBndrName :: TyVarBndr -> Maybe Name
-plainTyVarBndrName (PlainTV name) = Just name
-plainTyVarBndrName _ = Nothing
-
-plainTyVarBndrNames :: [TyVarBndr] -> [Name]
-plainTyVarBndrNames tyvars = mapMaybe plainTyVarBndrName tyvars
-
 allTyVarBndrNames :: [TyVarBndr] -> [Name]
 allTyVarBndrNames tyvars = map tyVarBndrName tyvars

oxij commented Nov 20, 2015

Just wondering here, why do you use allTyVarBndrNames in some places, and plainTyVarBndrNames in others?

I have a piece of code like this

{-# LANGUAGE UnicodeSyntax, OverloadedStrings
  , MultiParamTypeClasses, FlexibleContexts
  , DataKinds, KindSignatures, GADTs, TypeFamilies
  , DeriveDataTypeable, TemplateHaskell, QuasiQuotes
  , RecordWildCards, ScopedTypeVariables
  #-}
-- I think it breaks because of either DataKinds or KindSignatures

data Server q = Server [q]
                        deriving (Show, Typeable)

serverRequest :: Eq q => [q] → Update (Server q) ()
serverRequest = undefined

$(makeAcidic ''Server [ 'serverRequest ])

which compiled before ghc 7.10 and all the updates to acid-state that followed, but it doesn't compile now.

With this PR merged it generates splices that don't have (SafeCopy q, Typeable q, ...) => in IsAcidic instance, which can be fixed on case-by-case basis by adding those constraints to event handlers (e.g. serverRequest), but there is a TH code that seems to be designed to automate this, and it doesn't work with the example above because mkCxtFromTyVars uses plainTyVarBndrNames and q is not plain.

The following patch fixes it and all my code (and I have a lot) that worked before 7.10 starts working again, but then I wonder why these plain* functions were introduces in the first place?

diff --git a/src/Data/Acid/TemplateHaskell.hs b/src/Data/Acid/TemplateHaskell.hs
index 2454c7a..4e5e82c 100644
--- a/src/Data/Acid/TemplateHaskell.hs
+++ b/src/Data/Acid/TemplateHaskell.hs
@@ -249,7 +249,7 @@ makeSafeCopyInstance eventName eventType
           structName (x:xs) = toUpper x : xs

 mkCxtFromTyVars preds tyvars extraContext
-    = cxt $ [ classP classPred [varT tyvar] | tyvar <- plainTyVarBndrNames tyvars, classPred <- preds ] ++
+    = cxt $ [ classP classPred [varT tyvar] | tyvar <- allTyVarBndrNames tyvars, classPred <- preds ] ++
             map return extraContext

 {-
@@ -262,7 +262,7 @@ makeMethodInstance eventName eventType
     = do let preds = [ ''SafeCopy, ''Typeable ]
              ty = AppT (ConT ''Method) (foldl AppT (ConT eventStructName) (map VarT (allTyVarBndrNames tyvars)))
              structType = foldl appT (conT eventStructName) (map varT (allTyVarBndrNames tyvars))
-         instanceD (cxt $ [ classP classPred [varT tyvar] | tyvar <- plainTyVarBndrNames tyvars, classPred <- preds ] ++ map return context)
+         instanceD (cxt $ [ classP classPred [varT tyvar] | tyvar <- allTyVarBndrNames tyvars, classPred <- preds ] ++ map return context)
                    (return ty)
 #if __GLASGOW_HASKELL__ >= 707
                    [ tySynInstD ''MethodResult (tySynEqn [structType] (return resultType))
@@ -283,7 +283,7 @@ makeEventInstance eventName eventType
     = do let preds = [ ''SafeCopy, ''Typeable ]
              eventClass = if isUpdate then ''UpdateEvent else ''QueryEvent
              ty = AppT (ConT eventClass) (foldl AppT (ConT eventStructName) (map VarT (allTyVarBndrNames tyvars)))
-         instanceD (cxt $ [ classP classPred [varT tyvar] | tyvar <- plainTyVarBndrNames tyvars, classPred <- preds ] ++ map return context)
+         instanceD (cxt $ [ classP classPred [varT tyvar] | tyvar <- allTyVarBndrNames tyvars, classPred <- preds ] ++ map return context)
                    (return ty)
                    []
     where (tyvars, context, _args, _stateType, _resultType, isUpdate) = analyseType eventName eventType
@@ -329,12 +329,5 @@ tyVarBndrName :: TyVarBndr -> Name
 tyVarBndrName (PlainTV n)    = n
 tyVarBndrName (KindedTV n _) = n

-plainTyVarBndrName :: TyVarBndr -> Maybe Name
-plainTyVarBndrName (PlainTV name) = Just name
-plainTyVarBndrName _ = Nothing
-
-plainTyVarBndrNames :: [TyVarBndr] -> [Name]
-plainTyVarBndrNames tyvars = mapMaybe plainTyVarBndrName tyvars
-
 allTyVarBndrNames :: [TyVarBndr] -> [Name]
 allTyVarBndrNames tyvars = map tyVarBndrName tyvars
@ddssff

This comment has been minimized.

Show comment
Hide comment
@ddssff

ddssff Nov 20, 2015

Collaborator

I'm testing this change now.

Collaborator

ddssff commented Nov 20, 2015

I'm testing this change now.

@ddssff

This comment has been minimized.

Show comment
Hide comment
@ddssff

ddssff Nov 23, 2015

Collaborator

This looks good to me.

Collaborator

ddssff commented Nov 23, 2015

This looks good to me.

@oxij

This comment has been minimized.

Show comment
Hide comment
@oxij

oxij Nov 23, 2015

oxij commented Nov 23, 2015

@ddssff

This comment has been minimized.

Show comment
Hide comment
@ddssff

ddssff Nov 23, 2015

Collaborator

I did it

On Mon, Nov 23, 2015 at 7:30 AM, Jan Malakhovski notifications@github.com
wrote:

Should I make it into a PR?


Reply to this email directly or view it on GitHub
#56 (comment).

Collaborator

ddssff commented Nov 23, 2015

I did it

On Mon, Nov 23, 2015 at 7:30 AM, Jan Malakhovski notifications@github.com
wrote:

Should I make it into a PR?


Reply to this email directly or view it on GitHub
#56 (comment).

@ddssff

This comment has been minimized.

Show comment
Hide comment
@ddssff
Collaborator

ddssff commented Nov 23, 2015

@oxij

This comment has been minimized.

Show comment
Hide comment
@oxij

oxij Nov 23, 2015

oxij commented Nov 23, 2015

@stepcut

This comment has been minimized.

Show comment
Hide comment
@stepcut

stepcut Nov 23, 2015

Member

Travis is more happy now,

https://travis-ci.org/acid-state/acid-state/builds/92732101

Works fine on the builds that use cereal 0.4. Should work fine when we get the cereal 0.5 patch in. Any objections to pulling this now?

Member

stepcut commented Nov 23, 2015

Travis is more happy now,

https://travis-ci.org/acid-state/acid-state/builds/92732101

Works fine on the builds that use cereal 0.4. Should work fine when we get the cereal 0.5 patch in. Any objections to pulling this now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment