haskell-language-server icon indicating copy to clipboard operation
haskell-language-server copied to clipboard

Remove the ghcide executable

Open michaelpj opened this issue 3 years ago • 10 comments

The HLS executable can already be built without all the plugins, there are flags for that. So it's unclear what purpose the ghcide executable serves any more. Removing it would let us simplify some code, and perhaps move some of the glue code out of ghcide (is that desirable?).

Anyone have any strong reasons to keep it?

michaelpj avatar Jun 21 '22 10:06 michaelpj

+1

Previous discussions:

https://github.com/haskell/haskell-language-server/issues/813 https://github.com/haskell/ghcide/pull/939

pepeiborra avatar Jun 21 '22 10:06 pepeiborra

I did take a look, and it is currently non-trivial because the ghcide-testsuite depends on the ghcide executable and instructs it to emit certain LSP messages for the tests.

To remove the ghcide executable, we need to make the testsuite depend on HLS which requires a bit more untangling of the hls-plugin-api/ghcide/HLS dependency graph.

fendor avatar Oct 05 '22 11:10 fendor

+1

soulomoon avatar Mar 21 '24 16:03 soulomoon

Do you guys think this might be a good approach? we start by adding a core plugin, then move most of the ghcide functionality to the core plugin bit by bit((switching the test to use hls test in the proccess). This should solve the dependency problem. If you guys can agree with me on this approach, I'll add a core plugin and move a bit of ghcide functionality and test over. In time we can get rid of the dependency for the ghcide binary.

soulomoon avatar Mar 21 '24 17:03 soulomoon

To be perfectly clear, ghcide would not expose a plugin any more, consequentially there would be barely any tests in ghcide? I am not sure that's enough, I think we should first focus on the dependency graph and look at what dependency edges we wish to sever. I created this:

hls-deps drawio(1)

I hope there are no mistakes, but it can help identifying why it is non-trivial to refactor :upside_down_face:

fendor avatar Mar 22 '24 09:03 fendor

I find the ghcide executable very useful for quickly testing changes without waiting for all of HLS and its dependencies to build, and without manually disabling all the plugin flags.

wz1000 avatar Mar 22 '24 09:03 wz1000

I am not sure that's enough, I think we should first focus on the dependency graph and look at what dependency edges we wish to sever

In the long run I'm not sure even that is enough. If we were starting again today, I doubt we would have a separate package like ghcide at all. I imagine we might have:

  • hls-core: the rule system, the handlers and rules that deal with file system changes, position mapping etc.
  • hls-haskell-plugin: a "normal" plugin that provides handlers for Haskell stuff, the Typecheck rules etc.

I think what @soulomoon proposes is a way we could get there: create hls-haskell-plugin, gradually move the Haskell handlers over there. Most of the tests would go too. Then we'd be left with some random stuff in ghcide that could become hls-core or something.

I find the ghcide executable very useful for quickly testing changes without waiting for all of HLS and its dependencies to build, and without manually disabling all the plugin flags.

I don't think it would be that hard to define a minimal HLS executable that only includes the core Haskell plugins? The objectionable bit about the ghcide executable is that adds a lot of very similar code to the HLS one, and leads to some confusing attempts to abstract the bits that they share.

michaelpj avatar Mar 22 '24 09:03 michaelpj

The objectionable bit about the ghcide executable is that adds a lot of very similar code to the HLS one, and leads to some confusing attempts to abstract the bits that they share.

it is already quite abstracted? All the ghcide executable does is set up its configuration and call Development.IDE.Main.defaultMain, which is shared between ghcide and HLS.

wz1000 avatar Mar 22 '24 09:03 wz1000

Sure, I'm just saying that it's confusing. And e.g. all the logger setup is duplicated (and not consistent). And they have different "commands" that we map between. It's just a bunch of unnecessary (IMO) stuff.

michaelpj avatar Mar 22 '24 10:03 michaelpj

Nice graph @fendor, it is even messier if we consider the hls-bench and ghcide-bench. We have duplicated "everything" hls versus ghcide, result in a clumsy set of duplicated code and concepts which is rather confusing as @michaelpj point out.

  • hls-bench versus ghcide-bench.
  • exe:ghcide versus exe:hls.
  • ghcide-test-utils versus hls-test-utils.

From a higher perspective, ghcide package is pretty self contained and having full set of functionalities. It is like a old kingdown on its own. But we would want Depriving a king of power, slowly killing off ghcide as in @michaelpj's vision , making things more modularized. As for @wz1000 's concerns, I agree on the minimal HLS executable.

Since the refactor is non-trivial as @fendor point out in the graph. It is not realistic do everything in one go. But we can start attacking it by weakening the ghcide package, moving things from ghcide Package down to HLS Package or hls-test-util pcakge or up to hls-plugin-api package.


Here is the part I think we can do first.

  1. Plugin that take away PluginMethodHandler and its test from ghcide, as what is already suggested. To prove the concept, something like this https://github.com/soulomoon/haskell-language-server/pull/3
  2. The dependency "Some plugins -> ghcide-test-utils" can shifit to Some plugins -> hls-test-utils.

soulomoon avatar Mar 23 '24 12:03 soulomoon