Merge #4878

4878: Make "Replace qualified name with use" replace *all* mentions of the path r=matklad a=jonas-schievink

Fixes https://github.com/rust-analyzer/rust-analyzer/issues/4836

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>

crates/ra_assists/src/assist_context.rs

@@ -252,7 +252,7 @@ impl AssistBuilder {
     pub(crate) fn rewrite(&mut self, rewriter: SyntaxRewriter) {
         let node = rewriter.rewrite_root().unwrap();
         let new = rewriter.rewrite(&node);
-        algo::diff(&node, &new).into_text_edit(&mut self.edit)
+        algo::diff(&node, &new).into_text_edit(&mut self.edit);
     }
 
     // FIXME: kill this API

crates/ra_assists/src/handlers/replace_qualified_name_with_use.rs

@@ -1,7 +1,10 @@
 use hir;
-use ra_syntax::{ast, AstNode, SmolStr, TextRange};
+use ra_syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SmolStr, SyntaxNode};
 
-use crate::{utils::insert_use_statement, AssistContext, AssistId, Assists};
+use crate::{
+    utils::{find_insert_use_container, insert_use_statement},
+    AssistContext, AssistId, Assists,
+};
 
 // Assist: replace_qualified_name_with_use
 //
@@ -39,16 +42,18 @@ pub(crate) fn replace_qualified_name_with_use(
         target,
         |builder| {
             let path_to_import = hir_path.mod_path().clone();
+            let container = match find_insert_use_container(path.syntax(), ctx) {
+                Some(c) => c,
+                None => return,
+            };
             insert_use_statement(path.syntax(), &path_to_import, ctx, builder.text_edit_builder());
 
-            if let Some(last) = path.segment() {
-                // Here we are assuming the assist will provide a correct use statement
-                // so we can delete the path qualifier
-                builder.delete(TextRange::new(
-                    path.syntax().text_range().start(),
-                    last.syntax().text_range().start(),
-                ));
-            }
+            // 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);
         },
     )
 }
@@ -73,6 +78,69 @@ fn collect_hir_path_segments(path: &hir::Path) -> Option<Vec<SmolStr>> {
     Some(ps)
 }
 
+/// Adds replacements to `re` that shorten `path` in all descendants of `node`.
+fn shorten_paths(rewriter: &mut SyntaxRewriter<'static>, node: SyntaxNode, path: ast::Path) {
+    for child in node.children() {
+        match_ast! {
+            match child {
+                // Don't modify `use` items, as this can break the `use` item when injecting a new
+                // import into the use tree.
+                ast::UseItem(_it) => continue,
+                // Don't descend into submodules, they don't have the same `use` items in scope.
+                ast::Module(_it) => continue,
+
+                ast::Path(p) => {
+                    match maybe_replace_path(rewriter, p.clone(), path.clone()) {
+                        Some(()) => {},
+                        None => shorten_paths(rewriter, p.syntax().clone(), path.clone()),
+                    }
+                },
+                _ => shorten_paths(rewriter, child, path.clone()),
+            }
+        }
+    }
+}
+
+fn maybe_replace_path(
+    rewriter: &mut SyntaxRewriter<'static>,
+    path: ast::Path,
+    target: ast::Path,
+) -> Option<()> {
+    if !path_eq(path.clone(), target.clone()) {
+        return None;
+    }
+
+    // Shorten `path`, leaving only its last segment.
+    if let Some(parent) = path.qualifier() {
+        rewriter.delete(parent.syntax());
+    }
+    if let Some(double_colon) = path.coloncolon_token() {
+        rewriter.delete(&double_colon);
+    }
+
+    Some(())
+}
+
+fn path_eq(lhs: ast::Path, rhs: ast::Path) -> bool {
+    let mut lhs_curr = lhs;
+    let mut rhs_curr = rhs;
+    loop {
+        match (lhs_curr.segment(), rhs_curr.segment()) {
+            (Some(lhs), Some(rhs)) if lhs.syntax().text() == rhs.syntax().text() => (),
+            _ => return false,
+        }
+
+        match (lhs_curr.qualifier(), rhs_curr.qualifier()) {
+            (Some(lhs), Some(rhs)) => {
+                lhs_curr = lhs;
+                rhs_curr = rhs;
+            }
+            (None, None) => return true,
+            _ => return false,
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::tests::{check_assist, check_assist_not_applicable};
@@ -83,10 +151,10 @@ mod tests {
     fn test_replace_add_use_no_anchor() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 std::fmt::Debug<|>
     ",
-            "
+            r"
 use std::fmt::Debug;
 
 Debug
@@ -97,13 +165,13 @@ Debug
     fn test_replace_add_use_no_anchor_with_item_below() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 std::fmt::Debug<|>
 
 fn main() {
 }
     ",
-            "
+            r"
 use std::fmt::Debug;
 
 Debug
@@ -118,13 +186,13 @@ fn main() {
     fn test_replace_add_use_no_anchor_with_item_above() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 fn main() {
 }
 
 std::fmt::Debug<|>
     ",
-            "
+            r"
 use std::fmt::Debug;
 
 fn main() {
@@ -139,10 +207,10 @@ Debug
     fn test_replace_add_use_no_anchor_2seg() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 std::fmt<|>::Debug
     ",
-            "
+            r"
 use std::fmt;
 
 fmt::Debug
@@ -154,13 +222,13 @@ fmt::Debug
     fn test_replace_add_use() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use stdx;
 
 impl std::fmt::Debug<|> for Foo {
 }
     ",
-            "
+            r"
 use stdx;
 use std::fmt::Debug;
 
@@ -174,11 +242,11 @@ impl Debug for Foo {
     fn test_replace_file_use_other_anchor() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 impl std::fmt::Debug<|> for Foo {
 }
     ",
-            "
+            r"
 use std::fmt::Debug;
 
 impl Debug for Foo {
@@ -191,11 +259,11 @@ impl Debug for Foo {
     fn test_replace_add_use_other_anchor_indent() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
     impl std::fmt::Debug<|> for Foo {
     }
     ",
-            "
+            r"
     use std::fmt::Debug;
 
     impl Debug for Foo {
@@ -208,13 +276,13 @@ impl Debug for Foo {
     fn test_replace_split_different() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt;
 
 impl std::io<|> for Foo {
 }
     ",
-            "
+            r"
 use std::{io, fmt};
 
 impl io for Foo {
@@ -227,13 +295,13 @@ impl io for Foo {
     fn test_replace_split_self_for_use() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt;
 
 impl std::fmt::Debug<|> for Foo {
 }
     ",
-            "
+            r"
 use std::fmt::{self, Debug, };
 
 impl Debug for Foo {
@@ -246,13 +314,13 @@ impl Debug for Foo {
     fn test_replace_split_self_for_target() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt::Debug;
 
 impl std::fmt<|> for Foo {
 }
     ",
-            "
+            r"
 use std::fmt::{self, Debug};
 
 impl fmt for Foo {
@@ -265,13 +333,13 @@ impl fmt for Foo {
     fn test_replace_add_to_nested_self_nested() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt::{Debug, nested::{Display}};
 
 impl std::fmt::nested<|> for Foo {
 }
 ",
-            "
+            r"
 use std::fmt::{Debug, nested::{Display, self}};
 
 impl nested for Foo {
@@ -284,13 +352,13 @@ impl nested for Foo {
     fn test_replace_add_to_nested_self_already_included() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt::{Debug, nested::{self, Display}};
 
 impl std::fmt::nested<|> for Foo {
 }
 ",
-            "
+            r"
 use std::fmt::{Debug, nested::{self, Display}};
 
 impl nested for Foo {
@@ -303,13 +371,13 @@ impl nested for Foo {
     fn test_replace_add_to_nested_nested() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt::{Debug, nested::{Display}};
 
 impl std::fmt::nested::Debug<|> for Foo {
 }
 ",
-            "
+            r"
 use std::fmt::{Debug, nested::{Display, Debug}};
 
 impl Debug for Foo {
@@ -322,13 +390,13 @@ impl Debug for Foo {
     fn test_replace_split_common_target_longer() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt::Debug;
 
 impl std::fmt::nested::Display<|> for Foo {
 }
 ",
-            "
+            r"
 use std::fmt::{nested::Display, Debug};
 
 impl Display for Foo {
@@ -341,13 +409,13 @@ impl Display for Foo {
     fn test_replace_split_common_use_longer() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt::nested::Debug;
 
 impl std::fmt::Display<|> for Foo {
 }
 ",
-            "
+            r"
 use std::fmt::{Display, nested::Debug};
 
 impl Display for Foo {
@@ -360,7 +428,7 @@ impl Display for Foo {
     fn test_replace_use_nested_import() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use crate::{
     ty::{Substs, Ty},
     AssocItem,
@@ -368,7 +436,7 @@ use crate::{
 
 fn foo() { crate::ty::lower<|>::trait_env() }
 ",
-            "
+            r"
 use crate::{
     ty::{Substs, Ty, lower},
     AssocItem,
@@ -383,13 +451,13 @@ fn foo() { lower::trait_env() }
     fn test_replace_alias() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt as foo;
 
 impl foo::Debug<|> for Foo {
 }
 ",
-            "
+            r"
 use std::fmt as foo;
 
 impl Debug for Foo {
@@ -402,7 +470,7 @@ impl Debug for Foo {
     fn test_replace_not_applicable_one_segment() {
         check_assist_not_applicable(
             replace_qualified_name_with_use,
-            "
+            r"
 impl foo<|> for Foo {
 }
 ",
@@ -413,7 +481,7 @@ impl foo<|> for Foo {
     fn test_replace_not_applicable_in_use() {
         check_assist_not_applicable(
             replace_qualified_name_with_use,
-            "
+            r"
 use std::fmt<|>;
 ",
         );
@@ -423,14 +491,14 @@ use std::fmt<|>;
     fn test_replace_add_use_no_anchor_in_mod_mod() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 mod foo {
     mod bar {
         std::fmt::Debug<|>
     }
 }
     ",
-            "
+            r"
 mod foo {
     mod bar {
         use std::fmt::Debug;
@@ -446,19 +514,131 @@ mod foo {
     fn inserts_imports_after_inner_attributes() {
         check_assist(
             replace_qualified_name_with_use,
-            "
+            r"
 #![allow(dead_code)]
 
 fn main() {
     std::fmt::Debug<|>
 }
     ",
-            "
+            r"
 #![allow(dead_code)]
 use std::fmt::Debug;
 
 fn main() {
     Debug
+}
+    ",
+        );
+    }
+
+    #[test]
+    fn replaces_all_affected_paths() {
+        check_assist(
+            replace_qualified_name_with_use,
+            r"
+fn main() {
+    std::fmt::Debug<|>;
+    let x: std::fmt::Debug = std::fmt::Debug;
+}
+    ",
+            r"
+use std::fmt::Debug;
+
+fn main() {
+    Debug;
+    let x: Debug = Debug;
+}
+    ",
+        );
+    }
+
+    #[test]
+    fn replaces_all_affected_paths_mod() {
+        check_assist(
+            replace_qualified_name_with_use,
+            r"
+mod m {
+    fn f() {
+        std::fmt::Debug<|>;
+        let x: std::fmt::Debug = std::fmt::Debug;
+    }
+    fn g() {
+        std::fmt::Debug;
+    }
+}
+
+fn f() {
+    std::fmt::Debug;
+}
+    ",
+            r"
+mod m {
+    use std::fmt::Debug;
+
+    fn f() {
+        Debug;
+        let x: Debug = Debug;
+    }
+    fn g() {
+        Debug;
+    }
+}
+
+fn f() {
+    std::fmt::Debug;
+}
+    ",
+        );
+    }
+
+    #[test]
+    fn does_not_replace_in_submodules() {
+        check_assist(
+            replace_qualified_name_with_use,
+            r"
+fn main() {
+    std::fmt::Debug<|>;
+}
+
+mod sub {
+    fn f() {
+        std::fmt::Debug;
+    }
+}
+    ",
+            r"
+use std::fmt::Debug;
+
+fn main() {
+    Debug;
+}
+
+mod sub {
+    fn f() {
+        std::fmt::Debug;
+    }
+}
+    ",
+        );
+    }
+
+    #[test]
+    fn does_not_replace_in_use() {
+        check_assist(
+            replace_qualified_name_with_use,
+            r"
+use std::fmt::Display;
+
+fn main() {
+    std::fmt<|>;
+}
+    ",
+            r"
+use std::fmt::{self, Display};
+
+fn main() {
+    fmt;
 }
     ",
         );

crates/ra_assists/src/utils.rs

@@ -13,7 +13,7 @@ use rustc_hash::FxHashSet;
 
 use crate::assist_config::SnippetCap;
 
-pub(crate) use insert_use::insert_use_statement;
+pub(crate) use insert_use::{find_insert_use_container, insert_use_statement};
 
 #[derive(Clone, Copy, Debug)]
 pub(crate) enum Cursor<'a> {

crates/ra_assists/src/utils/insert_use.rs

@@ -12,6 +12,20 @@ use ra_syntax::{
 use ra_text_edit::TextEditBuilder;
 
 use crate::assist_context::AssistContext;
+use either::Either;
+
+/// Determines the containing syntax node in which to insert a `use` statement affecting `position`.
+pub(crate) fn find_insert_use_container(
+    position: &SyntaxNode,
+    ctx: &AssistContext,
+) -> Option<Either<ast::ItemList, ast::SourceFile>> {
+    ctx.sema.ancestors_with_macros(position.clone()).find_map(|n| {
+        if let Some(module) = ast::Module::cast(n.clone()) {
+            return module.item_list().map(|it| Either::Left(it));
+        }
+        Some(Either::Right(ast::SourceFile::cast(n)?))
+    })
+}
 
 /// Creates and inserts a use statement for the given path to import.
 /// The use statement is inserted in the scope most appropriate to the
@@ -24,15 +38,11 @@ pub(crate) fn insert_use_statement(
     builder: &mut TextEditBuilder,
 ) {
     let target = path_to_import.to_string().split("::").map(SmolStr::new).collect::<Vec<_>>();
-    let container = ctx.sema.ancestors_with_macros(position.clone()).find_map(|n| {
-        if let Some(module) = ast::Module::cast(n.clone()) {
-            return module.item_list().map(|it| it.syntax().clone());
-        }
-        ast::SourceFile::cast(n).map(|it| it.syntax().clone())
-    });
+    let container = find_insert_use_container(position, ctx);
 
     if let Some(container) = container {
-        let action = best_action_for_target(container, position.clone(), &target);
+        let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone());
+        let action = best_action_for_target(syntax, position.clone(), &target);
         make_assist(&action, &target, builder);
     }
 }

crates/ra_syntax/src/algo.rs

@@ -290,6 +290,11 @@ impl<'a> SyntaxRewriter<'a> {
         N::cast(self.rewrite(node.syntax())).unwrap()
     }
 
+    /// Returns a node that encompasses all replacements to be done by this rewriter.
+    ///
+    /// Passing the returned node to `rewrite` will apply all replacements queued up in `self`.
+    ///
+    /// Returns `None` when there are no replacements.
     pub fn rewrite_root(&self) -> Option<SyntaxNode> {
         assert!(self.f.is_none());
         self.replacements
@@ -298,6 +303,9 @@ impl<'a> SyntaxRewriter<'a> {
                 SyntaxElement::Node(it) => it.clone(),
                 SyntaxElement::Token(it) => it.parent(),
             })
+            // If we only have one replacement, we must return its parent node, since `rewrite` does
+            // not replace the node passed to it.
+            .map(|it| it.parent().unwrap_or(it))
             .fold1(|a, b| least_common_ancestor(&a, &b).unwrap())
     }