ndmitchell / hlint

Haskell source code suggestions
Other
1.45k stars 194 forks source link

updates for compatibility with ghc-9.10 #1594

Open shayne-fletcher opened 1 month ago

shayne-fletcher commented 1 month ago

upgrade from the ghc-9.8 to the ghc-9.10 api. fixes https://github.com/ndmitchell/hlint/issues/1593

shayne-fletcher commented 1 month ago

@alanz @zliu41 apply-refact updates (ghc-exactprint > 1.8.0.0) will be needed

alanz commented 1 month ago

I will bump the version number on my https://github.com/alanz/ghc-exactprint/tree/ghc-9.10 branch (tomorrow) if that will help. If I have a way of installing the new cabal for github CI, I can make a release

alanz commented 1 month ago

I will bump the version number

Done

shayne-fletcher commented 1 month ago

I will bump the version number

Done

thanks alan, that is indeed a help (although of course, we will need a release in due course). @zliu41 with this cabal.project file

allow-newer: all
source-repository-package
  type: git
  location: https://github.com/alanz/ghc-exactprint.git
  tag: f334b0c2da5ca9193297e77a9d1fc620f859ffde
packages: apply-refact.cabal

cabal install all produces the errors in this pastebin https://pastebin.com/HyvFvR8R indicating a few fixes to apply-refact source itself will be needed in addition to this.

alanz commented 1 month ago

https://hackage.haskell.org/package/ghc-exactprint-1.9.0.0

zliu41 commented 1 month ago

cabal install all produces the errors in this pastebin https://pastebin.com/HyvFvR8R indicating a few fixes to apply-refact source itself will be needed in addition to this.

@shayne-fletcher I've fixed these errors but there are many more. I'll continue fixing them over the next week. The working branch is https://github.com/mpickering/apply-refact/tree/ghc-9.10

alanz commented 1 month ago

Note: you need to use a Cabal-3.12-based pre-release of cabal-install to be able to compile ghc-exactprint.

See https://discourse.haskell.org/t/ann-cabal-3-12-0-0-released/9504#how-to-get-the-cabal-install-pre-release-3

shayne-fletcher commented 1 month ago

cabal install all produces the errors in this pastebin https://pastebin.com/HyvFvR8R indicating a few fixes to apply-refact source itself will be needed in addition to this.

@shayne-fletcher I've fixed these errors but there are many more. I'll continue fixing them over the next week. The working branch is https://github.com/mpickering/apply-refact/tree/ghc-9.10

i hope this saves you some time @zliu41:

on the ghc-9.10.1-fixes branch on my fork https://github.com/shayne-fletcher/apply-refact.git, after making this change, of the 449 tests, the failure count is reduced from 249 to 79.

alanz commented 1 month ago

@zliu41 given the magnitude of the GHC 9.10.1 upgrade, it might make sense to strip the CPP, and adjust the bounds so the next version only works with GHC 9.10.1 (as originally happened with apply-refact, IIRC).

And my notes on the changes are at https://gist.github.com/alanz/e127e7561ddf1cfeb07fbdee9a966794

zliu41 commented 1 month ago

given the magnitude of the GHC 9.10.1 upgrade, it might make sense to strip the CPP

@alanz Good point! Let me see how bad it is. And thanks for the notes!

alanz commented 1 month ago

Heads up all, I am in the process of making the transform API pure, should be making a new ghc-exactprint release soon. See https://github.com/alanz/ghc-exactprint/commit/5e7e8abcbaa05075570a1703e95009b19c468d79

alanz commented 1 month ago

https://hackage.haskell.org/package/ghc-exactprint-1.10.0.0

zliu41 commented 1 month ago

Update: I've made it build but there are many test failures. Will continue investigating. I don't have an ETA at the moment, but I should have some time to work on it during ZuriHac (if I haven't finished it by then).

alanz commented 1 month ago

I should have some time to work on it during ZuriHac

I can possibly lend a hand. But tied up from now until then

shayne-fletcher commented 1 month ago

Update: I've made it build but there are many test failures. Will continue investigating. I don't have an ETA at the moment, but I should have some time to work on it during ZuriHac (if I haven't finished it by then).

as i mentioned above @zliu41 , removing the call to makeDeltaAst at line 739 of Internal.hs Right ast -> pure $ Right (makeDeltaAst ast)(on your branch) halves the failure count to 135 of 449 tests.

zliu41 commented 1 month ago

@shayne-fletcher Sorry I completely missed your message above :sweat_smile: Thanks for helping out!

zliu41 commented 4 weeks ago

Worked on it for about 2 hours today with the help of @alanz, and reduced the number of test failures from 175 to 172 😅 Will continue later.. looks like a lot more setEntryDPs needed.

zliu41 commented 1 week ago

Sorry for the lack of follow up - I've been on PTO since after ZuriHac. Will try to get back to this as soon as I can.

zliu41 commented 1 week ago

The problem I have: before GHC 9.10, EpAnn has both RealSrcSpan and delta (the latter is from AnchorOperation). Both are useful for apply-refact: the RealSrcSpan is used to locate the subexpression to be replaced, and the delta is convenient for carrying out the replacement - the old and new subexpressions often have different lengths, so using delta obviates the need to adjust a lot of RealSrcSpans.

However, in 9.10, there's only either RealSrcSpan or delta (from EpaLocation', depending on whether I makeDeltaAst or not), which appears to make the job a lot harder.

@alanz any suggestions? cc @jhrcek

zliu41 commented 5 days ago

A concrete example:

--input
yes = if x then (f y) else z
--expected
yes = if x then f y else z
--actual, GHC 9.10, extra space before `else`
yes = if x then f y  else z

Prior to 9.10, all apply-refact needs to do is replacing (f y) with f y. It just works, presumably because the deltas are available from MovedAnchor, and else has EpaDelta (SameLine 1), so ghc-exactprint is able to do the right thing.

In 9.10, however, the fact that the deltas aren't available may necessitate updating the EpaSpan of else to make it work, but that would be infeasible to do in general.

By the way, I don't know why there's only one extra space in the output before else; I'd expect to see two, because the EpaSpan of else is unchanged, which means the else in the output should be in the same position as the input. So there seems to be something I'm missing. @alanz Do you know why that is? And how about a version of makeDeltaAst that keeps SrcSpans?

alanz commented 5 days ago

either RealSrcSpan or delta (from EpaLocation'

I have realised that this is a serious mistake.

The intended flow is you work on the original, and just have to use deltas where things change. But this does not work, when there are comments sprinkled around

This is what has me stuck in the retrie update.

I suspect we need to go back to a version with a span it it always for reference, and optional delta.

alanz commented 5 days ago

I suspect the best way forward is more long term

But that is likely to leave GHC 9.10.x out in the cold. Or at the least 9.10.1

alanz commented 4 days ago

See https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12993, a first cut at restoring it. I think it will be pretty non-invasive

zliu41 commented 4 days ago

See https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12993, a first cut at restoring it. I think it will be pretty non-invasive

Thanks, looks good. Hopefully this gets into 9.10.x; if not, I can make another attempt in apply-refact to make things work by processing the original modu and makeDeltaAst modu simultaneously. That would be hacky and ugly, and tricky to get right but it could potentially work in most cases.

alanz commented 4 days ago

That would be hacky and ugly, and tricky to get right

Exactly my experience for retrie. I think declaring 9.10.1 a bust and fixing it for 9.10.2+ is the only practical way forward