Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the ghcide executable #2979

Open
michaelpj opened this issue Jun 21, 2022 · 10 comments
Open

Remove the ghcide executable #2979

michaelpj opened this issue Jun 21, 2022 · 10 comments
Labels

Comments

@michaelpj
Copy link
Collaborator

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?

@pepeiborra
Copy link
Collaborator

pepeiborra commented Jun 21, 2022

+1

Previous discussions:

#813
haskell/ghcide#939

@fendor
Copy link
Collaborator

fendor commented Oct 5, 2022

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.

@soulomoon
Copy link
Collaborator

+1

@soulomoon
Copy link
Collaborator

soulomoon commented Mar 21, 2024

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.

@fendor
Copy link
Collaborator

fendor commented Mar 22, 2024

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 🙃

@wz1000
Copy link
Collaborator

wz1000 commented Mar 22, 2024

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.

@michaelpj
Copy link
Collaborator Author

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.

@wz1000
Copy link
Collaborator

wz1000 commented Mar 22, 2024

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.

@michaelpj
Copy link
Collaborator Author

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.

@soulomoon
Copy link
Collaborator

soulomoon commented Mar 23, 2024

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 add core plugin soulomoon/haskell-language-server#3
  2. The dependency "Some plugins -> ghcide-test-utils" can shifit to Some plugins -> hls-test-utils.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants