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

[LSP] Support symbol rename #1776

Closed
suimong opened this issue Jan 24, 2024 · 4 comments · Fixed by #1811
Closed

[LSP] Support symbol rename #1776

suimong opened this issue Jan 24, 2024 · 4 comments · Fixed by #1811

Comments

@suimong
Copy link
Contributor

suimong commented Jan 24, 2024

Using Nickel for a while, it occurs to me that the defining features of Nickel, i.e. recursion, laziness, merge system etc. makes writing program based on "single source of truth" a breeze, meaning that you define static data somewhere once, and everywhere else you can refer to that data with nickel symbol. Which brings the topic of this issue: supporting symbol rename in the LSP.

Right now with VSCode, the LSP does nothing when placing the cursor on a symbol and pressing "F2", but imagine if it does, the experience of refactoring nickel code with VSCode or any other LSP-supporting editor, would be comparable to, even better than that of Java with Intellij or Python with PyCharm.

Describe the solution you'd like

In VSCode at least, pressing 'F2' on a symbol shows the rename dialog box, user enter a new name, VSCode shows preview of all usages of that symbol within the workspace, and do the job.

Describe alternatives you've considered

ast-grep + Nickel's tree-sitter grammar is a potentially viable alternative, though I haven't tried it yet.

@yannham
Copy link
Member

yannham commented Jan 24, 2024

My current understanding is that, because we don't have a proper Concrete Syntax Tree (the LSP uses the same parser as the main Nickel executable), some information is lost during parsing, which makes non-trivial refactoring task hard to implement.

That being said, renaming is probably one that is doable, as we keep all the identifier positions around, and we are already computing usage sites. Maybe @jneem will know better.

@suimong
Copy link
Contributor Author

suimong commented Jan 24, 2024

Ah, that makes sense. This should be lower priority feature request, especially now that ADT is on the way 🤩

@jneem
Copy link
Member

jneem commented Jan 24, 2024

Symbol rename should certainly be doable.

One caveat is that some of the usage tracking is "best effort" right now, particularly when merges and contract application is involved. For example, the lsp currently recognizes that the two "foo"s in { foo = 1 } | { foo | Number } refer to one another. But if you put a function in between, like (std.function.id { foo = 1 }) | { foo | Number } it doesn't recognize that they refer to one another. It's possible that symbol rename shouldn't try to resolve merges at all, and just work with static scopes...

@yannham
Copy link
Member

yannham commented Jan 24, 2024

The {foo = 1} | {foo | Number} case is interesting. I don't know for sure if there's an obviously right solution (only rename one of them, or rename both) - at least none of the solutions sounds entirely unreasonable to me. However, it might be indeed more conservative to start by only renaming based on static scopes.

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

Successfully merging a pull request may close this issue.

3 participants