Initial support for GHC 9.4

[eddiejessup]

Sorry, hope this isn't treading on toes, but I was hoping to update doctest to work on GHC 9.4 and picked up the work in this PR.

I think I've fixed the tests; I implemented a different way of collecting the docstrings, where I capture all docstrings with one selector, then capture all the named-comments, then merge the two based on the locations.

I haven't tried to optimise the complexity of the merge, if this is likely to matter I can optimise it.

[sol]

I implemented a different way of collecting the docstrings, where I capture all docstrings with one selector, then capture all the named-comments, then merge the two based on the locations.

I haven't looked at the code in detail yet. But if you manage to simplify things that's of course a good thing. Historically, GHC used to store error thunks in the AST, which may be one reason why the existing code is more complicated than it needs to be. So again, any simplifications here are more than welcome.

[sol]

Overall this looks great 👍

• using doctest on itself fails on CI for 9.4.2, this is something we need to investigate
• I will confirm this later when I am back on the computer, but from a quick look I think e.g. fromLHsDecl is unused for 9.4.2. I think the issue is that we don't use -Werror on CI. This is something I would want to fix first, so that we don't burn cycles on keeping unused code working down the road.

[eddiejessup]

Overall this looks great 👍

• using doctest on itself fails on CI for 9.4.2, this is something we need to investigate
• I will confirm this later when I am back on the computer, but from a quick look I think e.g. fromLHsDecl is unused for 9.4.2. I think the issue is that we don't use -Werror on CI. This is something I would want to fix first, so that we don't burn cycles on keeping unused code working down the road.

For the doctest on itself (is this the error on the step after running the tests?), I've been investigating this, I'm not very familiar with exactly what is happening but seems like we get a stack overflow when waiting for ghci to terminate. I tested with ghc 9.2 and we don't get this stack overflow. But can investigate more further later on, I might be going down the wrong rabbit hole entirely.

On the second point, also will need to look later but I thought the lhsdecl was within the where clause of the previous version function, so it's unused in 9.4 but also not in scope there.

[sol]

On the second point, also will need to look later but I thought the lhsdecl was within the where clause of the previous version function, so it's unused in 9.4 but also not in scope there.

Ah, yes, I think you are right. I still opened #384 for -Werror on CI. We also weren't running the tests for ghci-wrapper (#385), but this needs some more work, so I'm deferring this for now.

[eddiejessup]

Okay I've had a play to try to get a minimal failing example for the stack-overflow.

I pushed a branch with a hacky but hopefully understandable repro. Explanation and instructions here.

It seems that if you just:

• Run a command with cabal 3.8
• do createProcess on GHC 9.4, with pipe as stdin and stdout
• Close stdin
• Wait for GHC process to end

You get a stack overflow, with the argument -this-unit-id doctest-0.20.0-inplace.

If you change the value to something else, eg -this-unit-id doctest-0.20.0-inplace2 (note the 2 at the end), you don't get an error, and things end okay.

I only tested this with cabal 3.8, I had a quick go with 3.6 and I got things looping in either case, but that might just be something in my repro code, I think maybe a red herring.

Would be good if @sol you could try to reproduce the above case; the arguments are baked in so I think it should be quite automatic to run by cloning the branch and following the instructions.

Seems like maybe there is some recursion going on, where invoking GHC within a package, with a dependency on that same package, somehow causes looping. Someone who understands GHC better might be able to get further insights, or I can keep digging to try to get to a clearer understanding, maybe get an even simpler repro case.

Let me know if anything is unclear!

[eddiejessup]

I added a really hacky fix for the stack-overflow error (here), just to check that CI passes. I think we can probably find a better fix.

[sol]

I think we can probably find a better fix.

I tried a couple of things yesterday. From what I understand, the issue is that we run doctest with cabal exec. I think there's no important reason to do so. I imagine we can do instead:

1. cabal install doctest
2. add Cabal's bin dir to the system PATH

[sol]

I can look into it tomorrow.

[eddiejessup]

Okay great thanks! Assuming that's fixed do you think this is good to merge? Anything else I can do? I guess we'll need a hackage release.

[sol]

@eddiejessup can you please rebase on main. I'll then try to do a final review later today. Sorry for the delay.

[sol]

Okay great thanks! Assuming that's fixed do you think this is good to merge? Anything else I can do? I guess we'll need a hackage release.

We will need a version bump in package.yaml. A hackage publish will then happen automatically.

[eddiejessup]

@eddiejessup can you please rebase on main. I'll then try to do a final review later today. Sorry for the delay.

Done. (Not a problem.)

[kazu-yamamoto]

Very nice!

[eddiejessup]

Cool! @sol will you be able to bump the package version sometime to get a new release?

[sol]

@eddiejessup I'm away from the computer right now. Can you open a PR for a new release (Version bump, change log entry)?

[kazu-yamamoto]

Bravo!

[eddiejessup]

@eddiejessup I'm away from the computer right now. Can you open a PR for a new release (Version bump, change log entry)?

@sol, opened PR #387

[sol]

Thanks @eddiejessup 🙏