Make splice plugin compatible with GHC 9.2 #2816
Conversation
eddiejessup
force-pushed
the
elliotm-92-splice
branch
from
6b00690
to
8b11b4b
Compare
We have a compatibility layer in |
|
@eddiejessup are you able to finish this PR? |
pepeiborra
added
the
status: unfinished
|
Hey @pepeiborra, thanks for checking in on this and sorry for not updating the issue. I got this compiling but had some test failures. I spent a bunch of time trying to work them out, I think it's essentially compatibility problems with the new exact printing stuff, but I had a lot of headaches nailing things down. Maybe what I can do is push what I have, show the failing tests and my understanding so far about what's going wrong? Then I can try to find some time to pick it up again, or if someone else wants to pick it up they can. |
eddiejessup
force-pushed
the
elliotm-92-splice
branch
from
71d8cf2
to
9a76346
Compare
|
Hi, very sorry for the long delay on this, but I've found some time to dig into this more, I have all the tests passing on GHC 9.2, though I confess one change is quite ad hoc to get the tests passing. Though it's to do with spacing, so given the staleness of this plugin, maybe even if we get some problems with extra/missing spaces, at this point maybe that's better than nothing. Anyway I'm now working on making the changes back-compatible with < 9.2. |
eddiejessup
force-pushed
the
elliotm-92-splice
branch
2 times, most recently
from
e2becdc
to
50f2ee3
Compare
eddiejessup
requested review from
Anton-Latukha,
Ailrun,
michaelpj and
konn
as code owners
|
Some unrelated plugin tests seem to fail in CI, but they pass for me locally. I'm not sure if this is flakiness or something, would appreciate any input on this. I think apart from that, this work is in a reviewable state. I've added some comments inline to try to explain in more detail. |
| #endif | ||
|
|
||
| #if MIN_VERSION_ghc(9,2,0) | ||
| setPrecedingLines :: Default t => LocatedAn t a -> Int -> Int -> LocatedAn t a |
There was a problem hiding this comment.
This was removed from exactprint, but we still use it for sorting out spacing, so reimplement here in later versions
| @@ -374,7 +382,7 @@ graftWithM dst trans = Graft $ \dflags a -> do | |||
| #if MIN_VERSION_ghc(9,2,0) | |||
| val'' <- | |||
| hoistTransform (either Fail.fail pure) $ | |||
| annotate dflags True $ maybeParensAST val' | |||
| annotate dflags False $ maybeParensAST val' | |||
There was a problem hiding this comment.
I made this change because there was a failing test which was expanding a type signature to:
f :: Int
instead of,
f :: Int
(note the extra space before `Int.)
I'm not sure what changed to make this differ from the pre-9.2 change, and it might not be the right fix to be honest, but since I think it just messes up formatting, and I find it very difficult to debug this exactprint annotation stuff, I think it's either this or nothing, unless someone else wants to dig in!
| @@ -533,7 +552,7 @@ annotate dflags needs_space ast = do | |||
| let rendered = render dflags ast | |||
| #if MIN_VERSION_ghc(9,2,0) | |||
| expr' <- lift $ mapLeft show $ parseAST dflags uniq rendered | |||
| pure expr' | |||
| pure $ setPrecedingLines expr' 0 (bool 0 1 needs_space) | |||
There was a problem hiding this comment.
Change to match pre-9.2
| @@ -542,6 +561,7 @@ annotate dflags needs_space ast = do | |||
|
|
|||
| -- | Given an 'LHsDecl', compute its exactprint annotations. | |||
| annotateDecl :: DynFlags -> LHsDecl GhcPs -> TransformT (Either String) (LHsDecl GhcPs) | |||
| #if !MIN_VERSION_ghc(9,2,0) | |||
| -- The 'parseDecl' function fails to parse 'FunBind' 'ValD's which contain | |||
There was a problem hiding this comment.
I'm leaning quite heavily on the tests here, but seems like this splitting then merging causes problems for on >= 9.2
|
Thank you for the tough work! |
|
Do I understand right that the rerun checks are passing? Also I see I need to rebase, I can do that |
eddiejessup
force-pushed
the
elliotm-92-splice
branch
from
0267fb5
to
9aa3760
Compare
|
@konn thanks! Do the current reviewers include one or more Exactprint people? Or we should add someone? |
|
I'm not sure who is, but I think given the tests all pass this seems like you've probably done a good job. Perhaps @wz1000 or @pepeiborra might want to take a look, but I think we can merge for now and if they have any comments we can address them afterwards. Thanks for fixing this! |
I don't really know what I'm doing, but I did the minimal amount to make the splice plugin compatible with GHC 9.2.
I import from
GHC.Types.Errorto extract the warnings and errors, I'm not sure if that's okay, or if the idea is to avoid plugins depending directly onGHCmodules?Good chance I've done something wrong here, feedback welcome.