diff --git a/crates/ra_assists/src/assist_context.rs b/crates/ra_assists/src/assist_context.rs index edd8255f44a1..ee614de7260b 100644 --- a/crates/ra_assists/src/assist_context.rs +++ b/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 diff --git a/crates/ra_assists/src/handlers/replace_qualified_name_with_use.rs b/crates/ra_assists/src/handlers/replace_qualified_name_with_use.rs index 0197a8cf0678..b4784c333114 100644 --- a/crates/ra_assists/src/handlers/replace_qualified_name_with_use.rs +++ b/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> { 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; } ", ); diff --git a/crates/ra_assists/src/utils.rs b/crates/ra_assists/src/utils.rs index 0038a9764b15..c1ff0de7b0b8 100644 --- a/crates/ra_assists/src/utils.rs +++ b/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> { diff --git a/crates/ra_assists/src/utils/insert_use.rs b/crates/ra_assists/src/utils/insert_use.rs index 0ee43482f798..8c4f33e59ac9 100644 --- a/crates/ra_assists/src/utils/insert_use.rs +++ b/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> { + 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::>(); - 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); } } diff --git a/crates/ra_syntax/src/algo.rs b/crates/ra_syntax/src/algo.rs index 664894d1f839..f7a885eb3eb1 100644 --- a/crates/ra_syntax/src/algo.rs +++ b/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 { 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()) }