Skip to content
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

updates for compatibility with ghc-9.10 #1594

Merged
merged 1 commit into from
Aug 26, 2024
Merged

updates for compatibility with ghc-9.10 #1594

merged 1 commit into from
Aug 26, 2024

Conversation

shayne-fletcher
Copy link
Collaborator

@shayne-fletcher shayne-fletcher commented May 12, 2024

upgrade from the ghc-9.8 to the ghc-9.10 api. fixes #1593

@shayne-fletcher
Copy link
Collaborator Author

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

@alanz
Copy link
Contributor

alanz commented May 12, 2024

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
Copy link
Contributor

alanz commented May 13, 2024

I will bump the version number

Done

@shayne-fletcher
Copy link
Collaborator Author

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
Copy link
Contributor

alanz commented May 16, 2024

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

@zliu41
Copy link
Collaborator

zliu41 commented May 19, 2024

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
Copy link
Contributor

alanz commented May 19, 2024

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
Copy link
Collaborator Author

shayne-fletcher commented May 20, 2024

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
Copy link
Contributor

alanz commented May 20, 2024

@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
Copy link
Collaborator

zliu41 commented May 20, 2024

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
Copy link
Contributor

alanz commented May 21, 2024

Heads up all, I am in the process of making the transform API pure, should be making a new ghc-exactprint release soon.
See alanz/ghc-exactprint@5e7e8ab

@alanz
Copy link
Contributor

alanz commented May 21, 2024

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

@zliu41
Copy link
Collaborator

zliu41 commented May 25, 2024

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
Copy link
Contributor

alanz commented May 25, 2024

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
Copy link
Collaborator Author

shayne-fletcher commented May 25, 2024

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
Copy link
Collaborator

zliu41 commented May 25, 2024

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

@zliu41
Copy link
Collaborator

zliu41 commented Jun 9, 2024

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.

@shayne-fletcher shayne-fletcher force-pushed the ghc-9.10.1 branch 2 times, most recently from 78ffbc9 to 9f0624d Compare June 22, 2024 18:09
@zliu41
Copy link
Collaborator

zliu41 commented Jun 29, 2024

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
Copy link
Collaborator

zliu41 commented Jul 1, 2024

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
Copy link
Collaborator

zliu41 commented Jul 4, 2024

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
Copy link
Contributor

alanz commented Jul 4, 2024

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
Copy link
Contributor

alanz commented Jul 4, 2024

I suspect the best way forward is more long term

  • I properly document what we have, and why
  • We mutually agree what needs to change
  • We do it

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

@alanz
Copy link
Contributor

alanz commented Jul 4, 2024

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
Copy link
Collaborator

zliu41 commented Jul 5, 2024

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
Copy link
Contributor

alanz commented Jul 5, 2024

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

@shayne-fletcher shayne-fletcher force-pushed the ghc-9.10.1 branch 2 times, most recently from 6801faa to cf788e2 Compare July 17, 2024 21:01
dhess added a commit to hackworthltd/primer that referenced this pull request Jul 18, 2024
Note that Protolude upstream doesn't support GHC 9.10, so we use
a (hopefully temporary) fork.

Temporary workarounds required:

* Disable `hlint`: ndmitchell/hlint#1594
* Disable `cabal-fmt`: input-output-hk/haskell.nix#2205

Signed-off-by: Drew Hess <[email protected]>
dhess added a commit to hackworthltd/primer that referenced this pull request Jul 18, 2024
Note that Protolude upstream doesn't support GHC 9.10, so we use
a (hopefully temporary) fork.

Temporary workarounds required:

* Disable `hlint`: ndmitchell/hlint#1594
* Disable `cabal-fmt`: input-output-hk/haskell.nix#2205

Signed-off-by: Drew Hess <[email protected]>
@shayne-fletcher shayne-fletcher force-pushed the ghc-9.10.1 branch 2 times, most recently from 798499b to 49f4728 Compare August 3, 2024 05:02
@ndmitchell ndmitchell merged commit 05257f6 into master Aug 26, 2024
3 of 8 checks passed
@ndmitchell ndmitchell deleted the ghc-9.10.1 branch August 26, 2024 15:45
ndmitchell added a commit that referenced this pull request Aug 26, 2024
Summary:

Test Plan:
@ndmitchell
Copy link
Owner

What's the update with Retrie? What should we be doing here?

@alanz
Copy link
Contributor

alanz commented Sep 9, 2024

What's the update with Retrie? What should we be doing here?

I broke ghc-exactprint for GHC 9.10, so retrie cannot be updated. The fix will be in GHC 9.12, but given hlint builds effectively against GHC master you can try the version at https://github.com/alanz/ghc-exactprint/tree/ghc-head which compiles and passes all tests, using head.hackage, and an update to the 'extra' package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cabal install hlint fails with ghc-9.10.1
4 participants