commercialhaskell / path

Typed filepath
Other
122 stars 45 forks source link

support GHC 9.0 #170

Closed amesgen closed 3 years ago

amesgen commented 3 years ago

EDIT: This description is now outdated, see below.


In template-haskell-2.17.0.0 in GHC 9.0.1, the signature of lift changed (see here) from

lift :: t -> Q Exp

to

lift :: Quote m => t -> m Exp 

which means that the current instance of Lift (Path a b) does not work anymore (as lookupTypeName :: String -> Q (Maybe Name)).

As a solution, I simply removed the instance, and moved it into a top-level function liftPath, but maybe there are other ways.

Also, this means that this is technically a breaking change, as the Lift instance is no longer present.


Tested with

cabal test -w ghc-9.0.1 --allow-newer='hashable:base,time-compat:base,these:base,assoc:base'
amesgen commented 3 years ago

We also can add a Lift (Path a b) instance with DeriveLift, but we (still) can't use it instead of liftPath due to #159.

NorfairKing commented 3 years ago

@amesgen This seems like a pretty big breakage on ghc's end. Is there anywhere I can read up on this?

amesgen commented 3 years ago

@NorfairKing https://gitlab.haskell.org/ghc/ghc/-/wikis/migration/9.0#overloaded-quotation-brackets

NorfairKing commented 3 years ago

@amesgen That link suggests a few possible resolutions. I'd prefer to make a non-breaking change to path; to preserve the Lift instance. Do you have a suggestion for how to do that?

amesgen commented 3 years ago

No, as the current implementation uses a function with the signature

lookupTypeName :: String -> Q (Maybe Name)

which can't be used in lift, as I wrote above. But maybe there are ways to rewrite it to not use lookupTypeName, but I am not aware of any.

Also, see https://github.com/commercialhaskell/path/pull/170#issuecomment-774513056

amesgen commented 3 years ago

To expand on why I am not sure if the derived Lift instance should be included: This (in the vein of #159) can happen:

 Λ :set -XStandaloneDeriving -XDeriveLift -XTemplateHaskell
 Λ import Language.Haskell.TH.Syntax
 Λ import Path.Internal
 Λ deriving instance Lift (Path a b)
 Λ $(lift (Path "/test" :: Path Abs File)) :: Path Rel Dir
"/test"

which can be very confusing.

NorfairKing commented 3 years ago

@amesgen the docs mention th-compat. Could that be used?

amesgen commented 3 years ago

I think not, as th-compat is about backporting Quote/Code, and not about "forward"porting functions like lookupTypeName to work with a Quote constraint.

NorfairKing commented 3 years ago

@amesgen I brought it up in https://github.com/ghc-proposals/ghc-proposals/pull/246#issuecomment-775211649. Let's see what they say :)

yav commented 3 years ago

Hi, I think something like this should work:

{-# Language TemplateHaskell #-}
import Language.Haskell.TH.Syntax
import Data.Typeable

newtype T a = T String

getTCName :: Typeable a => proxy a -> Name
getTCName a = Name occ flav
  where
  tc   = typeRepTyCon (typeRep a)
  occ  = OccName (tyConName tc)
  flav = NameG TcClsName (PkgName (tyConPackage tc)) (ModName (tyConModule tc))

liftExample :: (Typeable a, Quote m) => T a -> m Exp
liftExample x@(T s)  = [| T s :: T $(pure (ConT (getTCName x))) |]

This assumes that the type argument to T is a simple constructor, which seems to be the case for path. If not, one could put more work into rebuilding the type using the information from Data.Typeable.TypeRep. I guess it would be handy to add a function to the template-haskell package that rebuilds a Typeable type as a TH type.

amesgen commented 3 years ago

@yav Excellent, thank you!


We could save one CPP if branch with liftTypedFromUntypedSplice, which would imply a new dependency on th-compat.

NorfairKing commented 3 years ago

So f I understand correctly, this PR now adds support for GHC 9.0 without any downsides, and CI passes. Is that right?

I'm happy to merge then. What do you think @mrkkrp ?

NorfairKing commented 3 years ago

@amesgen would you mind rebasing on top of master? I'll be happy to merge by myself :)

amesgen commented 3 years ago

@NorfairKing squashed and rebased!