feat(rome_lsp): renaming symbols #2910

xunilrj · 2022-07-20T21:50:20Z

Summary

This PR is the first step to implementing #2903.
It introduces an rename extension method to the BatchMutation.

I still need to understand what to do when the cursor is not at a symbol.

Test Plan

> cargo test -p rome_js_analyze -- rename

or directly using the lsp.

cloudflare-workers-and-pages · 2022-07-20T21:53:26Z

Deploying with Cloudflare Pages

Latest commit:	`847b802`
Status:	✅ Deploy successful!
Preview URL:	https://553abfca.tools-8rn.pages.dev
Branch Preview URL:	https://feature-rename-batch-mutatio.tools-8rn.pages.dev

View logs

github-actions · 2022-07-20T21:54:00Z

Playground for commit 12c6d21

github-actions · 2022-07-20T21:57:02Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	5946	5946	0
Passed	395	395	0
Failed	5551	5551	0
Panics	0	0	0
Coverage	6.64%	6.64%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12393	12393	0
Failed	3864	3864	0
Panics	0	0	0
Coverage	76.23%	76.23%	0.00%

IWANABETHATGUY · 2022-07-21T01:17:05Z

Could our LSP rename handle the case that reference across the file?

xunilrj · 2022-07-21T04:56:15Z

Could our LSP rename handle the case that reference across the file?

Not yet, but it will.

MichaReiser · 2022-07-21T06:38:28Z

crates/rome_js_analyze/src/utils/rename.rs

+            if scope
+                .ancestors()
+                .filter_map(|scope| scope.get_binding(new_name))
+                .next()
+                .is_some()
+            {


Unrelated to the PR itself but I was surprised to learn that scope.get_binding(new_name) only checks the current scope but not the parent scopes as well to retrieve any binding with that name.

I think, ideally, scope.get_binding(new_name) should check the whole ancestor chain to see if there's any binding with the given name but scope can have a scope.get_own_binding(new_name) method that only checks the current scope. That would most closely match my expectations of get_binding.

I have no problem with this approach, but as soon as we start adding more methods, we will need to keep consistency: "all_reads", "all_writes", "all_references" etc... will also navigate upwards. Which means we will end up with two sets of methods.

And in the end .ancestors().filter_map(...) solves the problem.

MichaReiser · 2022-07-21T06:42:27Z

crates/rome_js_analyze/src/utils/rename.rs

+                    .node()
+                    .clone()
+                    .cast::<JsIdentifierAssignment>()


This last clone should not be necessary if you use all_references.into_iter()

I still need it because Reference::node(...) returns a borrow.
But I indeed can consume the all_references.

MichaReiser · 2022-07-21T06:42:58Z

crates/rome_js_analyze/src/utils/rename.rs

+            }
+        }
+
+        // Now it is safe to pish changes to the batch mutation


pish -> push?

ops... and "pish" is not a nice word! 😄

MichaReiser · 2022-07-21T06:43:58Z

crates/rome_js_analyze/src/utils.rs

+#[cfg(test)]
+pub mod tests;
+
+#[macro_export]


Is it possible to limit these test helpers to test builds only or is this something Rust doesn't support?

This was annoying me also. I think I managed to improve it.

MichaReiser · 2022-07-21T06:45:06Z

crates/rome_js_semantic/src/semantic_model.rs

@@ -56,6 +56,18 @@ struct SemanticModelData {
 }

 impl SemanticModelData {
+    pub fn scope(&self, range: &TextRange) -> usize {


Nit: No pub as this is an internal type?

MichaReiser · 2022-07-21T06:46:03Z

crates/rome_rowan/src/ast/batch.rs

@@ -122,6 +122,24 @@ where
        });
    }

+    pub fn replace_token(&mut self, prev_token: SyntaxToken<L>, next_token: SyntaxToken<L>) {


Can we add some documentation to these new public methods?

MichaReiser · 2022-07-21T06:47:22Z

crates/rome_service/src/file_handlers/javascript.rs

+        match node.try_into() {
+            Ok(node) => {
+                let mut batch = root.begin();
+                batch.rename_with_any_can_be_renamed(&model, node, &new_name);


Should we return an error if the renaming fails in case new_name is already used?

I found that vscode will not emit an error when the new name is already used.

Yes, we should. Done.

IWANABETHATGUY · 2022-07-21T07:46:53Z

I think we should add an option like "rome.analysis.enableRenameSymbol": true. Because in vscode javascript-language-service is built in which have more stable functionality on SymbolRename, I am not sure if there would be some side-effects when there are two language server providing SymbolRename service.

xunilrj · 2022-07-21T08:23:05Z

I think we should add an option like "rome.analysis.enableRenameSymbol": true. Because in vscode javascript-language-service is built in which have more stable functionality on SymbolRename, I am not sure if there would be some side-effects when there are two language server providing SymbolRename service.

I had to disable the built-in to test ours. I will research later if there is another option.

crates/rome_js_analyze/src/utils/rename.rs

crates/rome_service/src/lib.rs

crates/rome_js_analyze/src/utils/rename.rs

crates/rome_js_semantic/src/semantic_model.rs

crates/rome_rowan/src/syntax/trivia.rs

crates/rome_service/src/file_handlers/javascript.rs

xunilrj · 2022-07-22T09:06:22Z

Given that this changed the LSP, I would include it in the change log of #2835

xunilrj added 2 commits July 20, 2022 19:11

rename as batchmutation extension method

e2a4abf

rename in lsp

34415ee

xunilrj temporarily deployed to aws July 20, 2022 21:50 Inactive

fmt and clippy issues

b3736ec

xunilrj temporarily deployed to aws July 21, 2022 05:06 Inactive

better renaming error

6c1c996

xunilrj temporarily deployed to aws July 21, 2022 05:38 Inactive

xunilrj marked this pull request as ready for review July 21, 2022 06:01

xunilrj requested review from ematipico and leops as code owners July 21, 2022 06:01

xunilrj requested a review from a team July 21, 2022 06:01

MichaReiser reviewed Jul 21, 2022

View reviewed changes

fixing pr issues

d38992b

xunilrj temporarily deployed to aws July 21, 2022 07:33 Inactive

ematipico reviewed Jul 21, 2022

View reviewed changes

fixing PR issues

282f63b

xunilrj temporarily deployed to aws July 21, 2022 21:26 Inactive

renaming some functions

847b802

xunilrj temporarily deployed to aws July 22, 2022 04:53 Inactive

ematipico approved these changes Jul 22, 2022

View reviewed changes

xunilrj merged commit e6471f8 into main Jul 22, 2022

xunilrj deleted the feature/rename-batch-mutation-and-lsp branch July 22, 2022 07:57

feat(rome_lsp): renaming symbols #2910

feat(rome_lsp): renaming symbols #2910

Uh oh!

Conversation

xunilrj commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

cloudflare-workers-and-pages bot commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

github-actions bot commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 20, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

Uh oh!

IWANABETHATGUY commented Jul 21, 2022

Uh oh!

xunilrj commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IWANABETHATGUY commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xunilrj commented Jul 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xunilrj commented Jul 22, 2022

Uh oh!

Uh oh!

xunilrj commented Jul 20, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 20, 2022 •

edited

Loading

github-actions bot commented Jul 20, 2022 •

edited

Loading

xunilrj commented Jul 21, 2022 •

edited

Loading

IWANABETHATGUY commented Jul 21, 2022 •

edited

Loading