Reintroduce ghc-lib flag for hlint plugin

[RaoulHC]

The ghc-lib flag was removed in #3015, but it's still useful to be able to compile hls-hlint-plugin using the GHC API if you've done so for hlint and ghc-lib-parser-ex, rather than using ghc-lib-parser.

A lot of the HLINT_ON_GHC_LIB gated code which has probably been bitrotting since this flag was removed has also been removed, and is probably from when hlint used to work on haskell-src-exts. As ghc-lib-parser has the same API as GHC itself, there's no need for code to be cpp gated.

I've also updated the configuration-ghc94.nix file to show how it can be built using the GHC libraries instead of ghc-lib-parser.

[RaoulHC]

I guess I'm not clear about the motivation, can you say more? Do you want to do this for all the other packages too? Where is this going? I don't mind this too much but I would like to have a clear policy (our policy so far has been moving towards using ghc-lib-parser unconditionally as much as possible).

I have been trying to get hls in the horizon package set working better, and in that case where you have full control over the toolchain it seems it would make more sense to be able to use the GHC library rather than having to recompile it all in ghc-lib-parser. hlint, ghc-lib-parser-ex and even hie-compat have similar flags so I thought it made sense to do something similar here. I think the only other plugin that only uses ghc-lib-parser is fourmolu, but that change would be outside of hls anyway.

@fendor pointed out on IRC that using the GHC library should allow the hlint plugin to not have to reparse the file which would be a nice benefit, it looks like the old gated code was trying to do that so I'm trying to get it working again now, though I'm running into issues with apply-refact working on already parsed modules.

[michaelpj]

it seems it would make more sense to be able to use the GHC library rather than having to recompile it all in ghc-lib-parser

So the main motivation is reduced compile time? I'm not sure that's worth the added complexity on our side, tbh.

That said, I'm not sure I totally understand why we are using the ghc-lib packages at all here, given that we need an exact GHC match anyway :thinking:

[michaelpj]

Ah right, I think I remember. The issue is that version X of hlint may only work with particular versions of the GHC API, while we support many versions of GHC. However, a particular version of ghc-lib-parser can be build on many versions of GHC, so that reduces the problem a lot.

[RaoulHC]

So the main motivation is reduced compile time? I'm not sure that's worth the added complexity on our side, tbh.

That said, I'm not sure I totally understand why we are using the ghc-lib packages at all here, given that we need an exact GHC match anyway 🤔

Does this not reduce complexity as is? The package imports weren't needed and the unused cpp gated code has been removed. The flag swaps out dependencies with the only difference in the plugin being how it checks the GHC API version which was there already.

That said, I'll spend a bit more time today seeing if I can get applyRefactoring' to work with the already compiled module, avoiding recompiling modules for hlint seems like it be worth the complexity on that side, and it looks like it works slightly better CPP, the CPP via language pragma test works with the compile module. But I seem to get a bunch of off by one issues in the refactoring and it's not obvious to me why it would differ with this approach.

[michaelpj]

Does this not reduce complexity as is? The package imports weren't needed and the unused cpp gated code has been removed.

That change is great and I love it :)

The flag swaps out dependencies with the only difference in the plugin being how it checks the GHC API version which was there already.

This gives us two ways to do things. The second way (using the GHC API) will not currently be tested (and I'd rather not add yet another dimension to our test matrix), so we won't know if it works. Having multiple ways to do things that aren't all tested seems like more complexity to me, and quite probably a source of bugs in the future.

[RaoulHC]

This gives us two ways to do things. The second way (using the GHC API) will not currently be tested (and I'd rather not add yet another dimension to our test matrix), so we won't know if it works. Having multiple ways to do things that aren't all tested seems like more complexity to me, and quite probably a source of bugs in the future.

Is this not sufficiently tested anyway? ghc-lib is essentially bootstrapping to make a newer GHC API available, and I would not expect any difference in behaviour, it is compiling the same code after all. As it stands it will at least be built with ghc-9.4 on Nix in CI, where GHC would otherwise be bootstrapping itself for ghc-lib-parser-9.4.5.

For what its worth I managed to get the hlint plugin to work directly on the already parsed modules, though it requires a fix to apply-refact to be released as it seems applyRefactoring' had probably been similarly bitrotting. You can see it on this branch here. Your concerns are probably more acute for that change, but not having to reparse modules for hlint could be quite beneficial for any slow to parse modules.

[michaelpj]

Okay, I'm still unsure this is necessarily a good idea, but if you want it and will be testing it then I'm not sure it causes much harm, so let's bring it in.

There is a conflict.

[RaoulHC]

Updated.

I notice that #3804 removes configuration-ghc-94.nix but I've left the change to that in while it's still in draft.

[Vekhir]

@michaelpj The Arch package of hls-hlint-plugin will use this patch starting with version 1.1.2.0-36 to keep the dependencies simple. We are currently in the works of updating GHC to get up to speed again with the latest releases. The Arch packages are curated as to always depend on a single version of GHC, so there cannot be any conflicts. In addition, the testing process ensures package stability. Given this development, would you consider merging this PR once the issues have been fixed?

@RaoulHC Some of the checks are failing with conflicts detected. Can you take a look to bring the PR back into a mergeable state?

-- Vekhir

[RaoulHC]

@Vekhir I've updated it to be mergable.

It's worth noting that to avoid ghc-lib-parser you'd have to avoid the ormolu/fourmolu plugins because they depend on ghc-lib-parser and it looks like they've made the decision to not to have an option to depend on GHC: https://github.com/tweag/ormolu/pull/948. I think there's less value to this change with that in mind, but maybe you can try and convince them.

[Vekhir]

@RaoulHC Since this PR does two things (cleanup of dead code and other general improvements; introduce flag for GHC instead of ghc-lib-parser), would you mind splitting the first part off as a separate PR? Since it's only cleanup/refactoring, it's more likely to get merged, and it would already help me a lot. I will still support the second part, and I think the separation also makes it clearer what we want to achieve.

W.r.t. ormolu/fourmolu, that's not an issue right now. Medium-term it could be a goal, and then it would help to have hlint as a showcase that it can be done and is worth it.

[michaelpj]

Actually given our discussion, I'm fine just merging this, but thanks for taking the trouble to try splitting it up, that's appreciated.

[RaoulHC]

Updated so at least the individual commits work by themselves.

[RaoulHC]

Don't think the test failure is related to any of my changes, is that a known flaky test?

[jhrcek]

Yes, this is one of the flaky tests. I restarted the failed job.