Add support for GHC 9.4.1 #152

christiaanb · 2022-07-30T12:51:59Z

This is work in progress

gelisam · 2022-07-30T14:20:57Z

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 · 2022-07-30T14:37:10Z

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 · 2022-07-30T14:39:20Z

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 · 2022-07-30T19:51:11Z

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 · 2022-07-30T21:20:10Z

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

martijnbastiaan · 2022-08-30T07:15:26Z

@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 · 2022-08-30T08:52:53Z

Right, the issue is that this context storing and restoring:

hint/src/Hint/Context.hs

Lines 126 to 134 in 42afe81

    
           mayFail (do -- GHC.load will remove all the modules from scope, so first 
        
                       -- we save the context... 
        
                       (old_top, old_imps) <- runGhc getContext 
        
                       -- 
        
                       runGhc $ GHC.addTarget t 
        
                       res <- runGhc $ GHC.load (GHC.LoadUpTo m) 
        
                       -- 
        
                       if isSucceeded res 
        
                         then do runGhc $ setContext old_top old_imps

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

This means that

hint/src/Hint/Context.hs

Lines 414 to 419 in 42afe81

    
           -- Call it when the support module is an active phantom module but has been 
        
           -- unloaded as a side effect by GHC (e.g. by calling GHC.loadTargets) 
        
           reinstallSupportModule :: MonadInterpreter m => m () 
        
           reinstallSupportModule = do pm <- fromState hintSupportModule 
        
                                       removePhantomModule pm 
        
                                       installSupportModule

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 · 2022-10-08T19:17:24Z

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 · 2022-11-02T13:43:11Z

Otherwise the package seems to compile without running these.

gelisam · 2022-11-02T14:03:45Z

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

danwdart · 2022-11-02T14:08:31Z

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 · 2023-03-12T13:08:15Z

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

gelisam · 2023-03-12T15:18:53Z

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 · 2023-05-06T12:24:16Z

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:

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

gelisam · 2023-05-07T03:15:56Z

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:

hint already expects GHC.load to unload all the modules, and even already has a workaround.
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 · 2023-05-07T06:54:11Z

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 · 2023-05-07T10:49:40Z

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 · 2023-05-07T19:15:18Z

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 · 2023-05-18T21:03:24Z

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 · 2023-05-19T07:55:42Z

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

martijnbastiaan · 2023-05-19T15:43:27Z

src/Hint/Context.hs

+#if MIN_VERSION_ghc(9,4,0)
+                   res <- runGhc $ GHC.load GHC.LoadAllTargets
+#else
                   res <- runGhc $ GHC.load (GHC.LoadUpTo m)
+#endif


Couldn't we use LoadAllTargets for all GHC versions now?

Perhaps... I guess I can try on the oldest supported version.

unit-tests/run-unit-tests.hs

src/Hint/Context.hs

Since this test is specifically about checking that hint can pass on package-db flags to GHC correctly, it makes sense to disable the environment file in this test using `-package-env -` so that the package-db is not used implicitly as a result of the environment file.

christiaanb · 2023-05-23T18:45:19Z

@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?

samuel-gelineau-at-well-dot · 2023-06-14T15:22:57Z

src/Hint/Context.hs

@@ -128,7 +125,7 @@ addPhantomModule mod_text =
                   (old_top, old_imps) <- runGhc getContext
                   --
                   runGhc $ GHC.addTarget t
-                   res <- runGhc $ GHC.load (GHC.LoadUpTo m)
+                   res <- runGhc $ GHC.loadPhantomModule m


~~I don't understand. With ghc-9.4, loadPhantomModule m is defined as GHC.load GHC.LoadAllTargets, which does not mention m. So where does m get loaded?~~

Also, this comment says that calling getContext and then setContext does not work anymore, because the saved context refers to the modules using some internal IDs which are no longer valid after calling LoadAllTargets. So why is the code still using getContext and setContext? Is the comment wrong, and setContext does work after all?

~~I now think that setContext does fail, but then the caller fixes it.~~

where does m get loaded?

I now understand that addTarget t followed by LoadAllTargets does load t 🙃

I tried removing the allegedly-broken getContext and setContext calls, but that broke the test_work_in_main test. I was thus wrong: the caller does not fix setContext, and in fact, setContext does work fine.

src/Hint/Context.hs

gelisam · 2023-06-14T16:15:23Z

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 · 2023-06-15T01:59:29Z

src/Hint/InterpreterT.hs

-mkGhcError :: (GHC.SDoc -> String) -> GHC.SrcSpan -> GHC.Message -> GhcError
-mkGhcError render src_span msg = GhcError{errMsg = niceErrMsg}
-    where niceErrMsg = render $ GHC.mkLocMessage GHC.SevError src_span msg
-


Thanks for moving all this stuff to the shims file!

gelisam · 2023-06-15T17:33:36Z

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 · 2023-06-15T17:54:32Z

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 · 2023-06-15T19:14:10Z

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 · 2023-06-15T20:11:22Z

@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.

christiaanb force-pushed the 941-support branch from 9268d9d to b2bc76a Compare July 30, 2022 13:20

gelisam mentioned this pull request Jul 30, 2022

GHC 9.4 compatibility #151

Closed

gelisam mentioned this pull request Jul 30, 2022

Test ghc-9.4 in CI #155

Closed

peterbecich mentioned this pull request Dec 7, 2022

GHC 9.4 compatible release haskell-beam/beam#649

Closed

christiaanb force-pushed the 941-support branch 2 times, most recently from b41a38b to ecc84b4 Compare May 18, 2023 20:52

christiaanb marked this pull request as ready for review May 18, 2023 20:53

martijnbastiaan reviewed May 19, 2023

View reviewed changes

gelisam reviewed May 22, 2023

View reviewed changes

src/Hint/Context.hs Outdated Show resolved Hide resolved

christiaanb force-pushed the 941-support branch from ecc84b4 to e6b1f1b Compare May 22, 2023 20:47

christiaanb added 2 commits May 23, 2023 20:40

Add support for GHC 9.4

bf20c6d

christiaanb force-pushed the 941-support branch from e6b1f1b to 05bfce9 Compare May 23, 2023 18:43

christiaanb requested a review from gelisam May 23, 2023 18:44

gelisam mentioned this pull request Jun 4, 2023

How to load external libraries #159

Closed

samuel-gelineau-at-well-dot reviewed Jun 14, 2023

View reviewed changes

src/Hint/Context.hs Show resolved Hide resolved

samuel-gelineau-at-well-dot reviewed Jun 14, 2023

View reviewed changes

src/Hint/Context.hs Show resolved Hide resolved

samuel-gelineau-at-well-dot reviewed Jun 14, 2023

View reviewed changes

src/Hint/Context.hs Show resolved Hide resolved

gelisam approved these changes Jun 14, 2023

View reviewed changes

gelisam reviewed Jun 15, 2023

View reviewed changes

gelisam merged commit af457d4 into main Jun 15, 2023
7 checks passed

gelisam deleted the 941-support branch June 15, 2023 02:20

gelisam mentioned this pull request Jun 15, 2023

hint-0.9.0.7 improvements #160

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for GHC 9.4.1 #152

Add support for GHC 9.4.1 #152

christiaanb commented Jul 30, 2022

gelisam commented Jul 30, 2022

christiaanb commented Jul 30, 2022

brandonchinn178 commented Jul 30, 2022

gelisam commented Jul 30, 2022

christiaanb commented Jul 30, 2022

martijnbastiaan commented Aug 30, 2022

christiaanb commented Aug 30, 2022

danwdart commented Oct 8, 2022

danwdart commented Nov 2, 2022

gelisam commented Nov 2, 2022 •

edited

danwdart commented Nov 2, 2022

yaxu commented Mar 12, 2023

gelisam commented Mar 12, 2023

martijnbastiaan commented May 6, 2023

gelisam commented May 7, 2023

martijnbastiaan commented May 7, 2023

gelisam commented May 7, 2023

martijnbastiaan commented May 7, 2023

christiaanb commented May 18, 2023

yaxu commented May 19, 2023 •

edited

martijnbastiaan May 19, 2023

christiaanb May 19, 2023

christiaanb commented May 23, 2023

samuel-gelineau-at-well-dot Jun 14, 2023 •

edited by gelisam

samuel-gelineau-at-well-dot Jun 14, 2023 •

edited by gelisam

gelisam Jun 14, 2023 •

edited

gelisam Jun 15, 2023

gelisam Jun 15, 2023

gelisam commented Jun 14, 2023 •

edited

gelisam Jun 15, 2023 •

edited

gelisam commented Jun 15, 2023

gelisam commented Jun 15, 2023

gelisam commented Jun 15, 2023

martijnbastiaan commented Jun 15, 2023

Add support for GHC 9.4.1 #152

Add support for GHC 9.4.1 #152

Conversation

christiaanb commented Jul 30, 2022

gelisam commented Jul 30, 2022

christiaanb commented Jul 30, 2022

brandonchinn178 commented Jul 30, 2022

gelisam commented Jul 30, 2022

christiaanb commented Jul 30, 2022

martijnbastiaan commented Aug 30, 2022

christiaanb commented Aug 30, 2022

danwdart commented Oct 8, 2022

danwdart commented Nov 2, 2022

gelisam commented Nov 2, 2022 • edited

danwdart commented Nov 2, 2022

yaxu commented Mar 12, 2023

gelisam commented Mar 12, 2023

martijnbastiaan commented May 6, 2023

gelisam commented May 7, 2023

martijnbastiaan commented May 7, 2023

gelisam commented May 7, 2023

martijnbastiaan commented May 7, 2023

christiaanb commented May 18, 2023

yaxu commented May 19, 2023 • edited

martijnbastiaan May 19, 2023

Choose a reason for hiding this comment

christiaanb May 19, 2023

Choose a reason for hiding this comment

christiaanb commented May 23, 2023

samuel-gelineau-at-well-dot Jun 14, 2023 • edited by gelisam

Choose a reason for hiding this comment

samuel-gelineau-at-well-dot Jun 14, 2023 • edited by gelisam

Choose a reason for hiding this comment

gelisam Jun 14, 2023 • edited

Choose a reason for hiding this comment

gelisam Jun 15, 2023

Choose a reason for hiding this comment

gelisam Jun 15, 2023

Choose a reason for hiding this comment

gelisam commented Jun 14, 2023 • edited

gelisam Jun 15, 2023 • edited

Choose a reason for hiding this comment

gelisam commented Jun 15, 2023

gelisam commented Jun 15, 2023

gelisam commented Jun 15, 2023

martijnbastiaan commented Jun 15, 2023

gelisam commented Nov 2, 2022 •

edited

yaxu commented May 19, 2023 •

edited

samuel-gelineau-at-well-dot Jun 14, 2023 •

edited by gelisam

samuel-gelineau-at-well-dot Jun 14, 2023 •

edited by gelisam

gelisam Jun 14, 2023 •

edited

gelisam commented Jun 14, 2023 •

edited

gelisam Jun 15, 2023 •

edited