Stream: t-compiler/rust-analyzer

Topic: PR #5935


Lukas Wirth (Sep 02 2020 at 17:53, on Zulip):

What would be the proper way to turn a hir::ModPath into an ast::Path? I need to do this for updating the handlers code

Florian Diebold (Sep 02 2020 at 17:54, on Zulip):

as far as I remember, turning it into a string and turning that into the AST using the make functions

Lukas Wirth (Sep 02 2020 at 20:26, on Zulip):

alright

Lukas Wirth (Sep 03 2020 at 11:01, on Zulip):

So I'm not sure what I'm supposed to do about the failing tests now, as said in https://github.com/rust-analyzer/rust-analyzer/pull/5935#issuecomment-685986572 the test suite itself panics due to the change of not using the text edit api i imagine?

matklad (Sep 03 2020 at 11:02, on Zulip):

I was going to just take a look at this now :)

matklad (Sep 03 2020 at 11:04, on Zulip):

So, the insert_use returns a new SyntaxNode, but there's nothing that actually replaces the old node with the new one

matklad (Sep 03 2020 at 11:04, on Zulip):

You need to do something like text_edit_builder.replace(old_node.text_range(), new_node.to_string()). Let me check if this works...

Lukas Wirth (Sep 03 2020 at 11:08, on Zulip):

Oh okay, didn't mean to rush :smile:

matklad (Sep 03 2020 at 11:21, on Zulip):

@veykril urgh, the API there are not great.

I've managed to fix repalce_qualified_name_with_use assist with this diff:

diff --git a/crates/assists/src/handlers/replace_qualified_name_with_use.rs b/crates/assists/src/handlers/replace_qualified_name_with_use.rs
index 470e5f8ff..bcf2f164f 100644
--- a/crates/assists/src/handlers/replace_qualified_name_with_use.rs
+++ b/crates/assists/src/handlers/replace_qualified_name_with_use.rs
@@ -2,9 +2,14 @@ use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SyntaxNode, TextRang
 use test_utils::mark;

 use crate::{
-    utils::{find_insert_use_container, insert_use_statement},
+    utils::{
+        find_insert_use_container,
+        insert_use::{insert_use, MergeBehaviour},
+        insert_use_statement,
+    },
     AssistContext, AssistId, AssistKind, Assists,
 };
+use ast::make;

 // Assist: replace_qualified_name_with_use
 //
@@ -43,28 +48,25 @@ pub(crate) fn replace_qualified_name_with_use(
     };

     let target = path.syntax().text_range();
+    let container = find_insert_use_container(path.syntax(), ctx)?;
+    let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone());
+
     acc.add(
         AssistId("replace_qualified_name_with_use", AssistKind::RefactorRewrite),
         "Replace qualified path with use",
         target,
         |builder| {
-            let container = match find_insert_use_container(path.syntax(), ctx) {
-                Some(c) => c,
-                None => return,
-            };
-            insert_use_statement(
-                path.syntax(),
-                &path_to_import.to_string(),
-                ctx,
-                builder.text_edit_builder(),
-            );
-
             // Now that we've brought the name into scope, re-qualify all paths that could be
             // affected (that is, all paths inside the node we added the `use` to).
             let mut rewriter = SyntaxRewriter::default();
-            let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone());
-            shorten_paths(&mut rewriter, syntax, path);
-            builder.rewrite(rewriter);
+            shorten_paths(&mut rewriter, syntax.clone(), path);
+            let new_syntax = rewriter.rewrite(&syntax);
+            let new_syntax = insert_use(
+                new_syntax.clone(),
+                make::path_from_text(path_to_import),
+                Some(MergeBehaviour::Full),
+            );
+            builder.replace(syntax.text_range(), new_syntax.to_string())
         },
     )
 }
matklad (Sep 03 2020 at 11:21, on Zulip):

Some tests still fail, but this seems to mostly be because of ordering differences ({foo, bar} vs {bar, foo}).

Lukas Wirth (Sep 03 2020 at 11:23, on Zulip):

yes the ordering differences I changed for my tests. I'll take a look at the other stuff then again now. This should be plenty helpful thanks

matklad (Sep 03 2020 at 11:25, on Zulip):

I am so exited to see this finally being worked on :-)

Auto-import which you need to fixup yourself is probably my No1 annoyance with rust-analyzer at the moment :0)

Lukas Wirth (Sep 03 2020 at 11:33, on Zulip):

good to hear, it was also one of my biggest annoyances to be honest :D. So just having it group properly will be quite nice, but also being able to configure it to not merge everything together is something I'm looking forward to cause I'm one of the people that don't like deeply nested imports :smiley:

matklad (Sep 03 2020 at 11:35, on Zulip):

/me takes mental note that making features more annoying improves contributions

Lukas Wirth (Sep 03 2020 at 11:57, on Zulip):

Okay I managed to fix up auto_import and all tests pass(except one where order changed again). replace_qualified_name_with_use partially broke with weird invalid rust output due to nested modules, will have to check that in a bit.

Lukas Wirth (Sep 03 2020 at 11:59, on Zulip):

This is the output from test_replace_add_use_no_anchor_in_mod_mod right now

mod foo {
    mod bar use std::fmt::Debug;

{
        Debug
    }
}
Lukas Wirth (Sep 03 2020 at 12:00, on Zulip):

I suppose the text range that is being replaced is not entirely correct

matklad (Sep 03 2020 at 12:21, on Zulip):

Yeah, I think this needs some debugging to determine the exact semantics of where_ / position.

In general, the problem of composing changes to syntax trees is not fully solved yet.

Lukas Wirth (Sep 03 2020 at 12:25, on Zulip):

I got it down to ~4 failing tests, one where it panics on an assertion in test_edit, two malformed outputs and one where it inserts imports in front of inner attributes

matklad (Sep 03 2020 at 12:28, on Zulip):

The assert in text_edit might be interesting

matklad (Sep 03 2020 at 12:28, on Zulip):

Basicallly, you build the edit by saying "replace this range with that text"

matklad (Sep 03 2020 at 12:28, on Zulip):

And the assert triggers if you have overlapping ranges.

matklad (Sep 03 2020 at 12:29, on Zulip):

Which is exactly the problem with composing changes.

Lukas Wirth (Sep 03 2020 at 12:29, on Zulip):

Oh I see, well that will be interesting to fix I guess :sweat_smile:

Lukas Wirth (Sep 03 2020 at 13:00, on Zulip):

Ah now I see the problem with the weird malformed inner module tests. The syntax node in those cases is an ItemList which contains the opening and closing braces, so since there are no imports in those modules I insert them with InsertPosition::First causing the imports to go before the braces

Lukas Wirth (Sep 03 2020 at 13:01, on Zulip):

Should I special case that by checking the first token is an opening brace? That seems kinda hacky so I hope there is a better option :sweat_smile:

matklad (Sep 03 2020 at 13:03, on Zulip):

I think some amount of special casing would be required

matklad (Sep 03 2020 at 13:03, on Zulip):

Someone has to figure out if we are inserting into an item list, a block (which might be supported one day) or a file.

matklad (Sep 03 2020 at 13:04, on Zulip):

It's not entirely clear where such special casing needs to happen though.

matklad (Sep 03 2020 at 13:04, on Zulip):

Maybe where_ should have a type like

enum ImportScope {
    File(ast::File),
    Motule(ast::ItemList),
}
Lukas Wirth (Sep 03 2020 at 13:07, on Zulip):

Ye, that might be a good idea, I mean that is already what find_insert_use_container returns anyways + that would indeed allow easier refactoring to potentially support blocks later on I imagine. I'll check where this approach leads to

Lukas Wirth (Sep 03 2020 at 14:02, on Zulip):

Okay I think I fixed that problem, except that the import is now incorrectly indented.

Lukas Wirth (Sep 03 2020 at 14:36, on Zulip):

Alright all that's left now is the failing assert, which I'm really not sure how to fix. So as you said the problem is that the same range is being edited multiple times in the same assist "run" right? The solution I imagine is to only partial patch the text, but this is probably gonna be difficult to do given insert_use returns the entire edited new node.

matklad (Sep 03 2020 at 14:41, on Zulip):

Uhu, but you might be able to fix around this elsewhere

matklad (Sep 03 2020 at 14:42, on Zulip):

I hit exactly the same problem in the dff in the stream above

matklad (Sep 03 2020 at 14:42, on Zulip):

Note how I've re-ordered rewrite and insert_use

Lukas Wirth (Sep 03 2020 at 14:52, on Zulip):

I dont think I can do that in this case, the problem is that the test inserts imports twice, where one of the text ranges of one import contains the text range of the other modified import node

matklad (Sep 03 2020 at 15:46, on Zulip):

Looking at that failure now

matklad (Sep 03 2020 at 15:46, on Zulip):

IIRC, this assist panics with the old impl as well sometimes...

matklad (Sep 03 2020 at 15:48, on Zulip):

(also, master branch has some better asserts in that area)

matklad (Sep 03 2020 at 15:55, on Zulip):

Urgh, I don't see any easy way to fix this.

matklad (Sep 03 2020 at 15:55, on Zulip):

I think for now I am fine with just commenting-out the insert_import bit in that assist

matklad (Sep 03 2020 at 15:55, on Zulip):

I think a proper fix would be changing other things to do an AST-based edit, instad of a text-based one

matklad (Sep 03 2020 at 15:56, on Zulip):

and it also interacts with that other issue about grouping "find-references" result by file

Lukas Wirth (Sep 03 2020 at 17:08, on Zulip):

Alright, I pushed the final commit then just now that comments out the use of that function for now

Lukas Wirth (Sep 03 2020 at 17:08, on Zulip):

Should be ready to be reviewed once more then

Last update: Jul 24 2021 at 21:30UTC