Skip to content

Don't reject renames in modules that have implicit exports #3130

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

Closed
lf- opened this issue Sep 1, 2022 · 7 comments
Closed

Don't reject renames in modules that have implicit exports #3130

lf- opened this issue Sep 1, 2022 · 7 comments
Assignees

Comments

@lf-
Copy link
Contributor

lf- commented Sep 1, 2022

Is your enhancement request related to a problem? Please describe.

I can't use the rename plugin because I often have modules with implicit export lists, and it rejects those, printing this to the server log (this manifests as silently appearing to not do anything as this exclusively ends up in server logs):

[Error  - 7:17:54 PM] Request textDocument/rename failed.
  Message: Explicit export list required for renaming
  Code: -32603 

Describe the solution you'd like

It would be much more helpful if the rename plugin acted as a more reliable find-and-replace, even if it didn't properly rename references in other modules in the case of implicit exports. A warning would be helpful as well.

Describe alternatives you've considered

Additional context

@michaelpj
Copy link
Collaborator

Regarding the error, I suspect this is a client issue. That log shows that we've returned an error from the code action request, which is really the main way we're supposed to indicate failure. If the client doesn't make that obvious or show the message then there's not much we can do.

@OliverMadine
Copy link
Collaborator

OliverMadine commented Sep 10, 2022

Hello, Sorry for the late response on this.

Best effort cross-module renaming can be enabled with "haskell.plugin.rename.config.crossModule": true.
The functionality is hidden behind this config option because of limited multi-component support (#2193).

@tri97nguyen
Copy link

can anyone tell me how to set haskell.plugin.rename.config.crossModule in neovim please?

@Memorytaco
Copy link

can anyone tell me how to set haskell.plugin.rename.config.crossModule in neovim please?

lspconfig.hls.setup{
  settings = {
    haskell = {
      plugin = {
        rename = { config = { crossModule = true }}
      }
    }
  }
}

@tri97nguyen you can try adding a "haskell.plugin.rename.config.crossModule" lsp setting in "settings" when you are configuring neovim lsp.

more settings of hls are available here at haskell-language-server.readthedocs.io , but not all of them works.

@isomorpheme
Copy link

Best effort cross-module renaming can be enabled with "haskell.plugin.rename.config.crossModule": true.
The functionality is hidden behind this config option because of limited multi-component support (#2193).

Having run into this error a lot (and without the message indicating that this config options exists), I'd really prefer if that option were just on by default. Waiting on cross-module renaming to be able to handle all cases seems like letting the perfect be the enemy of the good. Right now, HLS is the odd one out among language servers (which also have weird edge cases where they rename improperly, I'm sure) in that I can try to do a rename action that ends up failing after all, even for e.g. let bindings that can't be exported anyway. It's pretty bad UX.

@michaelpj
Copy link
Collaborator

The rename plugin is lacking an active maintainer at the moment, so really we need someone keen to step up and work on improving it!

@fendor
Copy link
Collaborator

fendor commented Nov 26, 2024

The hls-rename-plugin is roughly 200 LOC. Most of the relevant code lives in hiedb, afaik. So, a new maintainer wouldn't have a lot of code to read :)

The reason this config isn't enabled by default is that with lazy component loading, we cannot be sure that the refactoring will be valid.
However, I think it would be fine to enable the config by default. Users can now eagerly load all components with some tricks, and I think it isn't too bad to break code in some situations, in particular when an identifier is used across multiple components. It is reasonably explainable, at least in the issue tracker, and I always have the config enabled. Very rarely it breaks code.

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

No branches or pull requests

8 participants