haskell / haskell-language-server

Official haskell ide support via language server (LSP). Successor of ghcide & haskell-ide-engine.
Apache License 2.0
2.71k stars 368 forks source link

Renaming multi component support #2193

Open jneira opened 3 years ago

jneira commented 3 years ago

//cc @pepeiborra @OliverMadine


After enabling the plugin we will track here the multi component support for renaming here

OliverMadine commented 3 years ago

It is just disabled due to the limited multi-cradle support.

I've had a temporary solution (just loading all components listed in the hie.yaml) working for a while now but we need #304 and #2009 to be merged first. Both of these PRs seem to be working and ready to merge.

michaelpj commented 3 years ago

I think if we enabled it with the caveat that it will only work reliably across one component that would be fine. Perhaps slap "(experimental)" after all the code action titles :)

OliverMadine commented 3 years ago

I think if we enabled it with the caveat that it will only work reliably across one component that would be fine. Perhaps slap "(experimental)" after all the code action titles :)

We don't use a code action since we get the new name directly from the client. So I don't think we have a great way of announcing this caveat to the user without them having to go through HLS logs etc.

It would be nice to have a vscode option (rather than a cabal build flag) so that we could enable the plugin for single-component use more easily, but I'm not really sure how to do this (especially without causing the plugin tests to fail). Also, hopefully we are close to some form of multi-component support so I'd rather just wait for that.

jneira commented 3 years ago

i am afraid #2009 needs https://github.com/haskell/hie-bios/pull/301 which in turn needs the show-build-info feature which will be avilable in Cabal-3.8 afaik

OliverMadine commented 3 years ago

i am afraid #2009 needs haskell/hie-bios#301 which in turn needs the show-build-info feature which will be avilable in Cabal-3.8 afaik

It was my understanding that this PR is just concerned with 'loading multiple components' and we were going to use show-build-info to 'get the components to load' later.

If not, this at least seems to be what it is doing currently so maybe we could merge what is done so far ('loading multiple components') then open a new PR to continue the work? #304 gets the necessary API changes in place for this.

fendor commented 3 years ago

Sorry, that PR is indeed more a POC for what is possible with show-build-info, not of the general multiple components loading mechanism.

However, it is true that the general issue of loading multiple components can be done independently of show-build-info, just the semantics of the proposed NonEmpty needs to be discussed.

jneira commented 3 years ago

It would be nice to have a vscode option (rather than a cabal build flag) so that we could enable the plugin for single-component use more easily, but I'm not really sure how to do this (especially without causing the plugin tests to fail). Also, hopefully we are close to some form of multi-component support so I'd rather just wait for that.

We could add a configuration option in vscode to enable/disable the plugin at runtime and make it disabled by default. In the config description we could note the caveats. Not sure if we could make the plugin disabled at runtime if no config is provided at all (but i think it should be possible)

//cc @berberman

berberman commented 3 years ago

Not sure if we could make the plugin disabled at runtime if no config is provided at all (but i think it should be possible)

In order to disable a plugin by default (no corresponding config section provided), we will need to change the behavior of parsing PluginConfig:https://github.com/haskell/haskell-language-server/blob/b6d1df2a362d8101dd3c03a2ea78a655ba85a685/hls-plugin-api/src/Ide/Plugin/Config.hs#L78-L95

Currently, the following "static" default values will be used if the client does not provide these options:

https://github.com/haskell/haskell-language-server/blob/b6d1df2a362d8101dd3c03a2ea78a655ba85a685/hls-plugin-api/src/Ide/Plugin/Config.hs#L132-L144

I think a easy way would be carrying the information that "the client does not give this value" in parsed config, rather than concealing it with a default value. So in plugin's handler, we could retrieve the config which says no option to enable this plugin is given by the client.

malteneuss commented 2 years ago

With the new rename plugin implementation in mind, is this plugin now suitable to be enabled? It would create a huge productivity boost, even if still has that one-component-only-caveat.

jneira commented 2 years ago

Yeah, i think the required changes in cabal will take more time and there is no a wip in stack so it is time to enable it, making clear its limitations in docs

pepeiborra commented 2 years ago

As a fallback, could we enable the rename plugin on single component projects?

michaelpj commented 2 years ago

Do we have a way of detecting whether we are in a single- or multi-component project? If so, we could simply make the rename handler fail informatively if it detects it is in a multi-component package.

fendor commented 2 years ago

Do we have a way of detecting whether we are in a single- or multi-component project?

No, we don't. In the current design of hie-bios, there is no way to enumerate all components / modules of a project. We definitely want something like it, but no one got around to implement it, yet.

JoeMShanahan commented 2 years ago

We definitely want something like it, but no one got around to implement it, yet.

Is there an issue somewhere that discusses this... perhaps a potential implementation or plan of attack?

fendor commented 2 years ago

Hi!

Yes, there is, and it requires multiple steps. First, we need to be able to actually get multiple components from a build tool. To achieve that for cabal, we have implemented https://github.com/haskell/cabal/issues/7489 and merged https://github.com/haskell/cabal/pull/7498 which means we are more than halfway there! The remaining PR for proper support by cabal is https://github.com/haskell/cabal/pull/7500. I'd be very happy if someone assists to get this merged. There is nothing difficult about it, currently, just a matter of not enough spare time on my end.

Afterwards, we would like to migrate hie-bios to use the new cabal API to see whether there are any design issues. The initial implementation for an old version of cabal show-build-info can be found here: https://github.com/haskell/hie-bios/pull/301 Before that, we can also try to get this merged: https://github.com/haskell/hie-bios/pull/304 which basically prepares the API to be able to emit multiple components at once.

After that, we can finally come to ghcide! We need to work on the ghcide session-loading logic which is very complicated and tricky to get right: https://github.com/haskell/haskell-language-server/blob/master/ghcide/session-loader/Development/IDE/Session.hs#L405 However, we can also look into the future since GHC multiple home unit support has been merged and will be released with 9.4, we should also look into utilising that.

I can be helpful on every step you want to tackle here, just ask for a couple clarifications and I can give you for cabal/hie-bios stuff step-by-step instructions.

santiweight commented 1 year ago

I am curious how this is going. As an HLS user, I certainly would love to see this come to fruition!

(Having to load in each component at my work codebase is still a sore spot!)

georgefst commented 1 year ago

Given that this doesn't appear to be happening any time soon, does anyone know how easy it might be to relax the explicit-export-list restriction for renaming local names? It's quite silly that I can't rename x here:

module M where
f x = ()
fendor commented 1 year ago

@georgefst it is likely to happen rather soon for ghc versoins > 9.4 once #3462 lands and the next major cabal release.

However, for your specific case, I think it should be easy to lift the restriction.

georgefst commented 1 year ago

Ah yes, I'd forgotten that this is yet another thing which will be solved by #3462 (although it is actually clear now that I've read the whole thread).

michaelpj commented 9 months ago

Multiple home unit support now exists, so I think this should now be possible. It would still need some work:

But that seems pretty doable!