Add support for GHC 9.4.1

[christiaanb]

This is work in progress

[gelisam]

I really liked how @brandon-leapyear did it last time: instead of adding ifdefs all over the codebase, he put them all inside shims. Each shim is a wrapper around a particular GHC function, with a type signature which is carefully picked so that it can be implemented with every supported GHC version, sometimes as a no-op or by ignoring some of its arguments. This way, the rest of the library implementation can be written against the shim's API, without having to worry about the GHC version.

That being said, it is much more important to get the library to work against GHC-9.4.1 than to make the code look nice, so perhaps you can focus on making it work and then I can worry about refactoring the code to the shims style :)

[christiaanb]

Yeah, the plan is to make it work first.

Currently everything compiles, but none of the test pass because it doesn’t load the modules. Once that’s sorted I’ll refactor to use shims.

[brandonchinn178]

FYI I found this other technique to minimize the number of #if macros even more, if you're interested in it: https://github.com/brandonchinn178/tasty-autocollect/blob/main/src/Test/Tasty/AutoCollect/GHC/Shim.hs

https://www.reddit.com/r/haskell/comments/2uehlo/stop_abusing_cpp_in_haskell_sources/?utm_medium=android_app&utm_source=share

[gelisam]

I had to add the following cabal.project file in order to get cabal test to run. I assume you have something similar locally as well. Should we commit this to the repo, in order to help others compile with ghc-9.4.1? Maybe under the name cabal.project-ghc-9.4.1, so that these version bound overrides don't get used in the common case in which users are using a released version of ghc?

packages: .
tests: True
constraints:
  async >= 2.0.0.0
allow-newer:
  async:base,
  hashable:base,
  hashable:ghc-bignum

[christiaanb]

I don’t know, once those packages are upgraded the cabal.project file would be redundant.

[martijnbastiaan]

@christiaanb I doubt you have time to address this, but do you have any sense of what's wrong / a way to fix it you could share with us?

[christiaanb]

Right, the issue is that this context storing and restoring: https://github.com/haskell-hint/hint/blob/42afe8197e7cc8b68d0ba005b42efdc027c95b57/src/Hint/Context.hs#L126-L134

is no longer sufficient. And I haven't figure out how to implement it in the GHC 9.4 API.

This means that https://github.com/haskell-hint/hint/blob/42afe8197e7cc8b68d0ba005b42efdc027c95b57/src/Hint/Context.hs#L414-L419 causes the non-Phantom modules to be unloaded. Which in turn means that basically we only have the Phantom modules in scope, which are rather useless by themselves.

[danwdart]

At the moment with this, I got unit test failures on ghc 9.4.2:

running tests
Running 1 test suites...
Test suite unit-tests: RUNNING...
^MCases: 16  Tried: 0  Errors: 0  Failures: 0^M                                           ^M### Failure in: 0:reload_modified
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nTEST_ReloadModified\n"
^MCases: 16  Tried: 1  Errors: 0  Failures: 1^MCases: 16  Tried: 2  Errors: 0  Failures: 1^M                                           ^M### Failure in: 2:work_in_main
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nMain\n"
^MCases: 16  Tried: 3  Errors: 0  Failures: 2^MCases: 16  Tried: 4  Errors: 0  Failures: 2^M                                           ^M### Failure in: 4:qual_import
unit-tests/run-unit-tests.hs:471
GhcException "Cannot add module M68944334256838729282258 to context: not a home module"
^MCases: 16  Tried: 5  Errors: 0  Failures: 3^M                                           ^M### Failure in: 5:full_import
unit-tests/run-unit-tests.hs:471
GhcException "Cannot add module M77412322302094290192258 to context: not a home module"
^MCases: 16  Tried: 6  Errors: 0  Failures: 4^MCases: 16  Tried: 7  Errors: 0  Failures: 4^MCases: 16  Tried: 8  Errors: 0  Failures: 4^MCases: 16  Tried: 9  Errors: 0  Failures: 4^MCases>
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nT\n"
^MCases: 16  Tried: 11  Errors: 0  Failures: 5^MCases: 16  Tried: 12  Errors: 0  Failures: 5^MCases: 16  Tried: 13  Errors: 0  Failures: 5^MCases: 16  Tried: 14  Errors: 0  Failures: 5^M >
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nMain\n"
^MCases: 16  Tried: 15  Errors: 0  Failures: 6^M                                            ^M### Failure in: 15:normalize_type
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nT\n"
Cases: 16  Tried: 16  Errors: 0  Failures: 7
^MCases: 2  Tried: 0  Errors: 0  Failures: 0^MCases: 2  Tried: 1  Errors: 0  Failures: 0^M                                          ^M### Error in:   1:package_db
cabal: startProcess: exec: does not exist (No such file or directory)
Cases: 2  Tried: 2  Errors: 1  Failures: 0
^MCases: 16  Tried: 0  Errors: 0  Failures: 0^M                                           ^M### Failure in: 0:reload_modified
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nTEST_ReloadModified\n"
^MCases: 16  Tried: 1  Errors: 0  Failures: 1^MCases: 16  Tried: 2  Errors: 0  Failures: 1^M                                           ^M### Failure in: 2:work_in_main
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nMain\n"
^MCases: 16  Tried: 3  Errors: 0  Failures: 2^MCases: 16  Tried: 4  Errors: 0  Failures: 2^M                                           ^M### Failure in: 4:qual_import
unit-tests/run-unit-tests.hs:471
GhcException "Cannot add module M66940689969892995022258 to context: not a home module"
^MCases: 16  Tried: 5  Errors: 0  Failures: 3^M                                           ^M### Failure in: 5:full_import
unit-tests/run-unit-tests.hs:471
GhcException "Cannot add module M58670291326063233382258 to context: not a home module"
^MCases: 16  Tried: 6  Errors: 0  Failures: 4^MCases: 16  Tried: 7  Errors: 0  Failures: 4^MCases: 16  Tried: 8  Errors: 0  Failures: 4^MCases: 16  Tried: 9  Errors: 0  Failures: 4^MCases>
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nT\n"
^MCases: 16  Tried: 11  Errors: 0  Failures: 5^MCases: 16  Tried: 12  Errors: 0  Failures: 5^MCases: 16  Tried: 13  Errors: 0  Failures: 5^MCases: 16  Tried: 14  Errors: 0  Failures: 5^M >
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nMain\n"
^MCases: 16  Tried: 15  Errors: 0  Failures: 6^M                                            ^M### Failure in: 15:normalize_type
unit-tests/run-unit-tests.hs:471
NotAllowed "These modules have not been loaded:\nT\n"
Cases: 16  Tried: 16  Errors: 0  Failures: 7
^MCases: 2  Tried: 0  Errors: 0  Failures: 0^MCases: 2  Tried: 1  Errors: 0  Failures: 0^M                                          ^M### Error in:   1:package_db
cabal: startProcess: exec: does not exist (No such file or directory)
Cases: 2  Tried: 2  Errors: 1  Failures: 0
Test suite unit-tests: FAIL

[danwdart]

Otherwise the package seems to compile without running these.

[gelisam]

Compiling is not sufficient; the failing tests indicate that hint cannot load modules, which is a key functionality we can't ship without.

[danwdart]

Yes of course, I didn't need that feature for my particular project so I'm not suggesting it goes in, only that I have an override set.

[yaxu]

Hi, is ghc 9.4 support on the horizon for hint?

[gelisam]

I have not had much time for open source projects recently, but thanks for reminding me about this, it helps me to prioritize which issues to focus on when I do find the time!

[martijnbastiaan]

This seems to come down to behavioral changes in load, I've reported an issue over here: https://gitlab.haskell.org/ghc/ghc/-/issues/23353. AFAICT one of these options need to happen:

1. Hint has to change to not rely on this behavior
2. GHC needs to revert to old behavior
3. GHC needs to add an additional load option LoadAllTargetsUpTo (see issue report)

[gelisam]

Thanks for tracking this down! However, there is still something I don't understand. Immediately above the code where hint calls LoadUpTo, there is a comment which says:

GHC.load will remove all the modules from scope, so first we save the context

Then there are calls to getContext and setContext around GHC.load. This seems to imply that:

1. hint already expects GHC.load to unload all the modules, and even already has a workaround.
2. The workaround doesn't work anymore.

That doesn't make much sense though, since your test case demonstrates that ghc's behavior has changed. Maybe an even older version of GHC was behaving like GHC 9.4 in that regard? Or maybe the comment isn't saying that the modules get unloaded, instead it is saying that they are still loaded but that they need to be imported again?

[martijnbastiaan]

Or maybe the comment isn't saying that the modules get unloaded, instead it is saying that they are still loaded but that they need to be imported again?

I'm pretty sure this is it. There's still a chance I'm chasing a red herring though, so I'll implement a patch for GHC and see if it fixes Hint's issues.

[gelisam]

I'll implement a patch for GHC and see if it fixes Hint's issues.

That sounds like a lot of work! Personally, I would start with approach 1, trying to change Hint to not rely on this behavior. After all, we know exactly what modules should be in scope, so it shouldn't be too hard to list them all if that's what the new GHC API requires.

[martijnbastiaan]

That sounds like a lot of work!

It's quite alright, I found the process relatively straightforward. I implemented two patches; one fixing a straight up bug in load, another for the desired new constructor: https://gitlab.haskell.org/martijnbastiaan/ghc/-/commits/add-load-all-targets-upto-9.4.5/.

Unfortunately, the trick:

(old_top, old_imps) <- getContext
-- do stuff
setContext old_top old_imps

doesn't work anymore for newer GHCs, presumably because references aren't valid after calling load again. So it looks like Hint will have to change its behavior.

[christiaanb]

All tests pass now, and I've added testing on GHC 9.4 to CI.

All that remains is some cleaning up and we should be able to make a new release.

[yaxu]

Excellent news thanks so much @christiaanb @martijnbastiaan + co!

[christiaanb]

@gelisam I've cleaned up the PR, could you give it another review, and if everything looks good to you merge the PR into main?

[gelisam]

Sorry it took me so long to find the time to review this!

@christiaanb since you obviously care about the library and you clearly have more time to dedicate to it than I do, would you like to take over as maintainer?

[gelisam]

Unfortunately, the trick:

(old_top, old_imps) <- getContext
-- do stuff
setContext old_top old_imps

doesn't work anymore for newer GHCs, presumably because references aren't valid after calling load again. So it looks like Hint will have to change its behavior.

@martijnbastiaan what made you conclude that this trick doesn't work anymore? It seems on the contrary that the trick works and is essential for the test_work_in_main test to pass.

[gelisam]

I now think that the main change in ghc-9.4 isn't to getContext and setContext, but rather to LoadUpTo m. Previously, it kept all currently-loaded targets and also loaded m and its dependencies. Now, it unloads all currently-loaded targets and only loads m and its dependencies. Thus, we now need to call LoadAllTargets instead of LoadUpTo, which is what this PR does.

[gelisam]

All that remains is some cleaning up and we should be able to make a new release.

released as hint-0.9.0.7

[martijnbastiaan]

@martijnbastiaan what made you conclude that this trick doesn't work anymore? It seems on the contrary that the trick works and is essential for the test_work_in_main test to pass.

I believe it - I probably got confused at some point while hacking on Hint.