From 9f548a02957314819aa0530d01f7c4a27dffdfc8 Mon Sep 17 00:00:00 2001 From: jDomantas Date: Fri, 14 Aug 2020 16:10:52 +0300 Subject: fixup whitespace when adding missing impl items --- .../src/handlers/add_missing_impl_members.rs | 63 ++++++++++++++++++++-- 1 file changed, 59 insertions(+), 4 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/handlers/add_missing_impl_members.rs b/crates/assists/src/handlers/add_missing_impl_members.rs index 81b61ebf8..83a2ada9a 100644 --- a/crates/assists/src/handlers/add_missing_impl_members.rs +++ b/crates/assists/src/handlers/add_missing_impl_members.rs @@ -48,7 +48,6 @@ enum AddMissingImplMembersMode { // fn foo(&self) -> u32 { // ${0:todo!()} // } -// // } // ``` pub(crate) fn add_missing_impl_members(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { @@ -89,8 +88,8 @@ pub(crate) fn add_missing_impl_members(acc: &mut Assists, ctx: &AssistContext) - // impl Trait for () { // Type X = (); // fn foo(&self) {} -// $0fn bar(&self) {} // +// $0fn bar(&self) {} // } // ``` pub(crate) fn add_missing_default_members(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { @@ -240,15 +239,18 @@ struct S; impl Foo for S { fn bar(&self) {} + $0type Output; + const CONST: usize = 42; + fn foo(&self) { todo!() } + fn baz(&self) { todo!() } - }"#, ); } @@ -281,10 +283,10 @@ struct S; impl Foo for S { fn bar(&self) {} + fn foo(&self) { ${0:todo!()} } - }"#, ); } @@ -599,6 +601,7 @@ trait Foo { struct S; impl Foo for S { $0type Output; + fn foo(&self) { todo!() } @@ -705,6 +708,58 @@ trait Tr { impl Tr for () { $0type Ty; +}"#, + ) + } + + #[test] + fn test_whitespace_fixup_preserves_bad_tokens() { + check_assist( + add_missing_impl_members, + r#" +trait Tr { + fn foo(); +} + +impl Tr for ()<|> { + +++ +}"#, + r#" +trait Tr { + fn foo(); +} + +impl Tr for () { + fn foo() { + ${0:todo!()} + } + +++ +}"#, + ) + } + + #[test] + fn test_whitespace_fixup_preserves_comments() { + check_assist( + add_missing_impl_members, + r#" +trait Tr { + fn foo(); +} + +impl Tr for ()<|> { + // very important +}"#, + r#" +trait Tr { + fn foo(); +} + +impl Tr for () { + fn foo() { + ${0:todo!()} + } + // very important }"#, ) } -- cgit v1.2.3 From 2052d33b9b0e2254f53848501a9113aa12ddf4da Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 15 Aug 2020 18:22:16 +0200 Subject: Remove deprecated Path::from_ast Long term, we probably should make hir::Path private to hir. --- crates/assists/src/ast_transform.rs | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/ast_transform.rs b/crates/assists/src/ast_transform.rs index 4c41c16d8..5216862ba 100644 --- a/crates/assists/src/ast_transform.rs +++ b/crates/assists/src/ast_transform.rs @@ -7,6 +7,17 @@ use syntax::{ ast::{self, AstNode}, }; +pub fn apply<'a, N: AstNode>(transformer: &dyn AstTransform<'a>, node: N) -> N { + SyntaxRewriter::from_fn(|element| match element { + syntax::SyntaxElement::Node(n) => { + let replacement = transformer.get_substitution(&n)?; + Some(replacement.into()) + } + _ => None, + }) + .rewrite_ast(&node) +} + pub trait AstTransform<'a> { fn get_substitution(&self, node: &syntax::SyntaxNode) -> Option; @@ -107,10 +118,7 @@ impl<'a> SubstituteTypeParams<'a> { ast::Type::PathType(path_type) => path_type.path()?, _ => return None, }; - // FIXME: use `hir::Path::from_src` instead. - #[allow(deprecated)] - let path = hir::Path::from_ast(path)?; - let resolution = self.source_scope.resolve_hir_path(&path)?; + let resolution = self.source_scope.speculative_resolve(&path)?; match resolution { hir::PathResolution::TypeParam(tp) => Some(self.substs.get(&tp)?.syntax().clone()), _ => None, @@ -146,10 +154,7 @@ impl<'a> QualifyPaths<'a> { // don't try to qualify `Fn(Foo) -> Bar` paths, they are in prelude anyway return None; } - // FIXME: use `hir::Path::from_src` instead. - #[allow(deprecated)] - let hir_path = hir::Path::from_ast(p.clone()); - let resolution = self.source_scope.resolve_hir_path(&hir_path?)?; + let resolution = self.source_scope.speculative_resolve(&p)?; match resolution { PathResolution::Def(def) => { let found_path = from.find_use_path(self.source_scope.db.upcast(), def)?; @@ -175,17 +180,6 @@ impl<'a> QualifyPaths<'a> { } } -pub fn apply<'a, N: AstNode>(transformer: &dyn AstTransform<'a>, node: N) -> N { - SyntaxRewriter::from_fn(|element| match element { - syntax::SyntaxElement::Node(n) => { - let replacement = transformer.get_substitution(&n)?; - Some(replacement.into()) - } - _ => None, - }) - .rewrite_ast(&node) -} - impl<'a> AstTransform<'a> for QualifyPaths<'a> { fn get_substitution(&self, node: &syntax::SyntaxNode) -> Option { self.get_substitution_inner(node).or_else(|| self.previous.get_substitution(node)) -- cgit v1.2.3 From 0ca1ba29e8e88c060dcf36946e4e02a6f015754b Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 15 Aug 2020 18:50:41 +0200 Subject: Don't expose hir::Path out of hir Conjecture: it's impossible to use hir::Path *correctly* from an IDE. I am not entirely sure about this, and we might need to add it back at some point, but I have to arguments that convince me that we probably won't: * `hir::Path` has to know about hygiene, which an IDE can't set up properly. * `hir::Path` lacks identity, but you actually have to know identity to resolve it correctly --- crates/assists/src/handlers/auto_import.rs | 2 +- crates/assists/src/handlers/expand_glob_import.rs | 20 +++------ .../handlers/extract_struct_from_enum_variant.rs | 7 ++- .../handlers/replace_qualified_name_with_use.rs | 50 +++++++++------------- crates/assists/src/utils/insert_use.rs | 5 +-- 5 files changed, 37 insertions(+), 47 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/handlers/auto_import.rs b/crates/assists/src/handlers/auto_import.rs index cce789972..b9ec3f10b 100644 --- a/crates/assists/src/handlers/auto_import.rs +++ b/crates/assists/src/handlers/auto_import.rs @@ -53,7 +53,7 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> |builder| { insert_use_statement( &auto_import_assets.syntax_under_caret, - &import, + &import.to_string(), ctx, builder.text_edit_builder(), ); diff --git a/crates/assists/src/handlers/expand_glob_import.rs b/crates/assists/src/handlers/expand_glob_import.rs index f690ec343..81d0af2f3 100644 --- a/crates/assists/src/handlers/expand_glob_import.rs +++ b/crates/assists/src/handlers/expand_glob_import.rs @@ -1,3 +1,4 @@ +use either::Either; use hir::{AssocItem, MacroDef, ModuleDef, Name, PathResolution, ScopeDef, SemanticsScope}; use ide_db::{ defs::{classify_name_ref, Definition, NameRefClass}, @@ -10,8 +11,6 @@ use crate::{ AssistId, AssistKind, }; -use either::Either; - // Assist: expand_glob_import // // Expands glob imports. @@ -40,11 +39,15 @@ use either::Either; pub(crate) fn expand_glob_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { let star = ctx.find_token_at_offset(T![*])?; let mod_path = find_mod_path(&star)?; + let module = match ctx.sema.resolve_path(&mod_path)? { + PathResolution::Def(ModuleDef::Module(it)) => it, + _ => return None, + }; let source_file = ctx.source_file(); let scope = ctx.sema.scope_at_offset(source_file.syntax(), ctx.offset()); - let defs_in_mod = find_defs_in_mod(ctx, scope, &mod_path)?; + let defs_in_mod = find_defs_in_mod(ctx, scope, module)?; let name_refs_in_source_file = source_file.syntax().descendants().filter_map(ast::NameRef::cast).collect(); let used_names = find_used_names(ctx, defs_in_mod, name_refs_in_source_file); @@ -82,17 +85,8 @@ impl Def { fn find_defs_in_mod( ctx: &AssistContext, from: SemanticsScope<'_>, - path: &ast::Path, + module: hir::Module, ) -> Option> { - let hir_path = ctx.sema.lower_path(&path)?; - let module = if let Some(PathResolution::Def(ModuleDef::Module(module))) = - from.resolve_hir_path_qualifier(&hir_path) - { - module - } else { - return None; - }; - let module_scope = module.scope(ctx.db(), from.module()); let mut defs = vec![]; diff --git a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs index 4bcdae7ba..d62e06b4a 100644 --- a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs @@ -106,7 +106,12 @@ fn insert_import( if let Some(mut mod_path) = mod_path { mod_path.segments.pop(); mod_path.segments.push(variant_hir_name.clone()); - insert_use_statement(path.syntax(), &mod_path, ctx, builder.text_edit_builder()); + insert_use_statement( + path.syntax(), + &mod_path.to_string(), + ctx, + builder.text_edit_builder(), + ); } Some(()) } 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 011bf1106..470e5f8ff 100644 --- a/crates/assists/src/handlers/replace_qualified_name_with_use.rs +++ b/crates/assists/src/handlers/replace_qualified_name_with_use.rs @@ -1,5 +1,5 @@ -use hir; -use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SmolStr, SyntaxNode}; +use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SyntaxNode, TextRange}; +use test_utils::mark; use crate::{ utils::{find_insert_use_container, insert_use_statement}, @@ -28,12 +28,19 @@ pub(crate) fn replace_qualified_name_with_use( if path.syntax().ancestors().find_map(ast::Use::cast).is_some() { return None; } - - let hir_path = ctx.sema.lower_path(&path)?; - let segments = collect_hir_path_segments(&hir_path)?; - if segments.len() < 2 { + if path.qualifier().is_none() { + mark::hit!(dont_import_trivial_paths); return None; } + let path_to_import = path.to_string().clone(); + let path_to_import = match path.segment()?.generic_arg_list() { + Some(generic_args) => { + let generic_args_start = + generic_args.syntax().text_range().start() - path.syntax().text_range().start(); + &path_to_import[TextRange::up_to(generic_args_start)] + } + None => path_to_import.as_str(), + }; let target = path.syntax().text_range(); acc.add( @@ -41,12 +48,16 @@ pub(crate) fn replace_qualified_name_with_use( "Replace qualified path 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()); + 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). @@ -58,26 +69,6 @@ pub(crate) fn replace_qualified_name_with_use( ) } -fn collect_hir_path_segments(path: &hir::Path) -> Option> { - let mut ps = Vec::::with_capacity(10); - match path.kind() { - hir::PathKind::Abs => ps.push("".into()), - hir::PathKind::Crate => ps.push("crate".into()), - hir::PathKind::Plain => {} - hir::PathKind::Super(0) => ps.push("self".into()), - hir::PathKind::Super(lvl) => { - let mut chain = "super".to_string(); - for _ in 0..*lvl { - chain += "::super"; - } - ps.push(chain.into()); - } - hir::PathKind::DollarCrate(_) => return None, - } - ps.extend(path.segments().iter().map(|it| it.name.to_string().into())); - 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() { @@ -467,7 +458,8 @@ impl Debug for Foo { } #[test] - fn test_replace_not_applicable_one_segment() { + fn dont_import_trivial_paths() { + mark::check!(dont_import_trivial_paths); check_assist_not_applicable( replace_qualified_name_with_use, r" diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 50a62ee82..49096a67c 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -5,7 +5,6 @@ use std::iter::successors; use either::Either; -use hir::{self, ModPath}; use syntax::{ ast::{self, NameOwner, VisibilityOwner}, AstNode, AstToken, Direction, SmolStr, @@ -35,11 +34,11 @@ pub(crate) fn find_insert_use_container( pub(crate) fn insert_use_statement( // Ideally the position of the cursor, used to position: &SyntaxNode, - path_to_import: &ModPath, + path_to_import: &str, ctx: &AssistContext, builder: &mut TextEditBuilder, ) { - let target = path_to_import.to_string().split("::").map(SmolStr::new).collect::>(); + let target = path_to_import.split("::").map(SmolStr::new).collect::>(); let container = find_insert_use_container(position, ctx); if let Some(container) = container { -- cgit v1.2.3 From 38e3088a563b634d593b7289281f344ddb22f97f Mon Sep 17 00:00:00 2001 From: jDomantas Date: Mon, 17 Aug 2020 10:47:13 +0300 Subject: update generated tests --- crates/assists/src/tests/generated.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/tests/generated.rs b/crates/assists/src/tests/generated.rs index d16e6fb0a..173567003 100644 --- a/crates/assists/src/tests/generated.rs +++ b/crates/assists/src/tests/generated.rs @@ -82,8 +82,8 @@ trait Trait { impl Trait for () { Type X = (); fn foo(&self) {} - $0fn bar(&self) {} + $0fn bar(&self) {} } "#####, ) @@ -115,7 +115,6 @@ impl Trait for () { fn foo(&self) -> u32 { ${0:todo!()} } - } "#####, ) -- cgit v1.2.3 From 6a4c9fc9fd6cb9ecf08bd5a22890eb19d22ad34e Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 17 Aug 2020 16:11:29 +0200 Subject: Don't make fields private unless you have to --- crates/assists/src/lib.rs | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/lib.rs b/crates/assists/src/lib.rs index ae90d68a3..c589b08dc 100644 --- a/crates/assists/src/lib.rs +++ b/crates/assists/src/lib.rs @@ -66,13 +66,13 @@ pub struct GroupLabel(pub String); #[derive(Debug, Clone)] pub struct Assist { - id: AssistId, + pub id: AssistId, /// Short description of the assist, as shown in the UI. label: String, - group: Option, + pub group: Option, /// Target ranges are used to sort assists: the smaller the target range, /// the more specific assist is, and so it should be sorted first. - target: TextRange, + pub target: TextRange, } #[derive(Debug, Clone)] @@ -82,6 +82,11 @@ pub struct ResolvedAssist { } impl Assist { + fn new(id: AssistId, label: String, group: Option, target: TextRange) -> Assist { + assert!(label.starts_with(char::is_uppercase)); + Assist { id, label, group, target } + } + /// Return all the assists applicable at the given position. /// /// Assists are returned in the "unresolved" state, that is only labels are @@ -114,30 +119,8 @@ impl Assist { acc.finish_resolved() } - pub(crate) fn new( - id: AssistId, - label: String, - group: Option, - target: TextRange, - ) -> Assist { - assert!(label.starts_with(|c: char| c.is_uppercase())); - Assist { id, label, group, target } - } - - pub fn id(&self) -> AssistId { - self.id - } - - pub fn label(&self) -> String { - self.label.clone() - } - - pub fn group(&self) -> Option { - self.group.clone() - } - - pub fn target(&self) -> TextRange { - self.target + pub fn label(&self) -> &str { + self.label.as_str() } } -- cgit v1.2.3