From e8744ed9bb3e139e5d427db1f4f219f1fdeee13e Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 19 Apr 2021 20:29:14 +0200 Subject: [PATCH 1/6] Replace SyntaxRewriter usage with ted in reorder_impl assist --- .../ide_assists/src/handlers/reorder_impl.rs | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/ide_assists/src/handlers/reorder_impl.rs b/crates/ide_assists/src/handlers/reorder_impl.rs index f976e73adc..fd2897c4cc 100644 --- a/crates/ide_assists/src/handlers/reorder_impl.rs +++ b/crates/ide_assists/src/handlers/reorder_impl.rs @@ -4,9 +4,8 @@ use rustc_hash::FxHashMap; use hir::{PathResolution, Semantics}; use ide_db::RootDatabase; use syntax::{ - algo, ast::{self, NameOwner}, - AstNode, + ted, AstNode, }; use crate::{AssistContext, AssistId, AssistKind, Assists}; @@ -75,13 +74,18 @@ pub(crate) fn reorder_impl(acc: &mut Assists, ctx: &AssistContext) -> Option<()> } let target = items.syntax().text_range(); - acc.add(AssistId("reorder_impl", AssistKind::RefactorRewrite), "Sort methods", target, |edit| { - let mut rewriter = algo::SyntaxRewriter::default(); - for (old, new) in methods.iter().zip(&sorted) { - rewriter.replace(old.syntax(), new.syntax()); - } - edit.rewrite(rewriter); - }) + acc.add( + AssistId("reorder_impl", AssistKind::RefactorRewrite), + "Sort methods", + target, + |builder| { + for (old, new) in + methods.into_iter().zip(sorted).filter(|(field, sorted)| field != sorted) + { + ted::replace(builder.make_ast_mut(old).syntax(), new.clone_for_update().syntax()); + } + }, + ) } fn compute_method_ranks(path: &ast::Path, ctx: &AssistContext) -> Option> { From fa20a5064be85349d2d05abcd66f5662d3aecb0c Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 20 Apr 2021 02:05:22 +0200 Subject: [PATCH 2/6] Remove SyntaxRewriter usage in insert_use in favor of ted --- .../ide_assists/src/handlers/auto_import.rs | 8 +- .../extract_struct_from_enum_variant.rs | 85 +++--- .../replace_qualified_name_with_use.rs | 28 +- crates/ide_completion/src/item.rs | 8 +- crates/ide_db/src/helpers/insert_use.rs | 262 +++++++----------- crates/ide_db/src/helpers/insert_use/tests.rs | 19 +- crates/syntax/src/ast/make.rs | 1 + crates/syntax/src/ted.rs | 17 +- 8 files changed, 185 insertions(+), 243 deletions(-) diff --git a/crates/ide_assists/src/handlers/auto_import.rs b/crates/ide_assists/src/handlers/auto_import.rs index 49aa70f74e..6db2d2edd6 100644 --- a/crates/ide_assists/src/handlers/auto_import.rs +++ b/crates/ide_assists/src/handlers/auto_import.rs @@ -101,9 +101,11 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> format!("Import `{}`", import.import_path), range, |builder| { - let rewriter = - insert_use(&scope, mod_path_to_ast(&import.import_path), ctx.config.insert_use); - builder.rewrite(rewriter); + let scope = match scope.clone() { + ImportScope::File(it) => ImportScope::File(builder.make_ast_mut(it)), + ImportScope::Module(it) => ImportScope::Module(builder.make_ast_mut(it)), + }; + insert_use(&scope, mod_path_to_ast(&import.import_path), ctx.config.insert_use); }, ); } diff --git a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs index a8d6355bdd..26e1c66ab4 100644 --- a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs @@ -13,9 +13,9 @@ use ide_db::{ }; use rustc_hash::FxHashSet; use syntax::{ - algo::{find_node_at_offset, SyntaxRewriter}, - ast::{self, edit::IndentLevel, make, AstNode, NameOwner, VisibilityOwner}, - SourceFile, SyntaxElement, SyntaxNode, T, + algo::find_node_at_offset, + ast::{self, make, AstNode, NameOwner, VisibilityOwner}, + ted, SourceFile, SyntaxElement, SyntaxNode, T, }; use crate::{AssistContext, AssistId, AssistKind, Assists}; @@ -62,14 +62,17 @@ pub(crate) fn extract_struct_from_enum_variant( let mut visited_modules_set = FxHashSet::default(); let current_module = enum_hir.module(ctx.db()); visited_modules_set.insert(current_module); - let mut def_rewriter = None; + let mut def_file_references = None; for (file_id, references) in usages { - let mut rewriter = SyntaxRewriter::default(); - let source_file = ctx.sema.parse(file_id); + if file_id == ctx.frange.file_id { + def_file_references = Some(references); + continue; + } + builder.edit_file(file_id); + let source_file = builder.make_ast_mut(ctx.sema.parse(file_id)); for reference in references { update_reference( ctx, - &mut rewriter, reference, &source_file, &enum_module_def, @@ -77,25 +80,27 @@ pub(crate) fn extract_struct_from_enum_variant( &mut visited_modules_set, ); } - if file_id == ctx.frange.file_id { - def_rewriter = Some(rewriter); - continue; - } - builder.edit_file(file_id); - builder.rewrite(rewriter); } - let mut rewriter = def_rewriter.unwrap_or_default(); - update_variant(&mut rewriter, &variant); + builder.edit_file(ctx.frange.file_id); + let variant = builder.make_ast_mut(variant.clone()); + let source_file = builder.make_ast_mut(ctx.sema.parse(ctx.frange.file_id)); + for reference in def_file_references.into_iter().flatten() { + update_reference( + ctx, + reference, + &source_file, + &enum_module_def, + &variant_hir_name, + &mut visited_modules_set, + ); + } extract_struct_def( - &mut rewriter, - &enum_ast, variant_name.clone(), &field_list, &variant.parent_enum().syntax().clone().into(), enum_ast.visibility(), ); - builder.edit_file(ctx.frange.file_id); - builder.rewrite(rewriter); + update_variant(&variant); }, ) } @@ -138,7 +143,6 @@ fn existing_definition(db: &RootDatabase, variant_name: &ast::Name, variant: &Va fn insert_import( ctx: &AssistContext, - rewriter: &mut SyntaxRewriter, scope_node: &SyntaxNode, module: &Module, enum_module_def: &ModuleDef, @@ -151,14 +155,12 @@ fn insert_import( mod_path.pop_segment(); mod_path.push_segment(variant_hir_name.clone()); let scope = ImportScope::find_insert_use_container(scope_node, &ctx.sema)?; - *rewriter += insert_use(&scope, mod_path_to_ast(&mod_path), ctx.config.insert_use); + insert_use(&scope, mod_path_to_ast(&mod_path), ctx.config.insert_use); } Some(()) } fn extract_struct_def( - rewriter: &mut SyntaxRewriter, - enum_: &ast::Enum, variant_name: ast::Name, field_list: &Either, start_offset: &SyntaxElement, @@ -180,33 +182,34 @@ fn extract_struct_def( .into(), }; - rewriter.insert_before( - start_offset, - make::struct_(visibility, variant_name, None, field_list).syntax(), + ted::insert_raw( + ted::Position::before(start_offset), + make::struct_(visibility, variant_name, None, field_list).clone_for_update().syntax(), ); - rewriter.insert_before(start_offset, &make::tokens::blank_line()); + ted::insert_raw(ted::Position::before(start_offset), &make::tokens::blank_line()); - if let indent_level @ 1..=usize::MAX = IndentLevel::from_node(enum_.syntax()).0 as usize { - rewriter - .insert_before(start_offset, &make::tokens::whitespace(&" ".repeat(4 * indent_level))); - } + // if let indent_level @ 1..=usize::MAX = IndentLevel::from_node(enum_.syntax()).0 as usize { + // ted::insert(ted::Position::before(start_offset), &make::tokens::blank_line()); + // rewriter + // .insert_before(start_offset, &make::tokens::whitespace(&" ".repeat(4 * indent_level))); + // } Some(()) } -fn update_variant(rewriter: &mut SyntaxRewriter, variant: &ast::Variant) -> Option<()> { +fn update_variant(variant: &ast::Variant) -> Option<()> { let name = variant.name()?; let tuple_field = make::tuple_field(None, make::ty(&name.text())); let replacement = make::variant( name, Some(ast::FieldList::TupleFieldList(make::tuple_field_list(iter::once(tuple_field)))), - ); - rewriter.replace(variant.syntax(), replacement.syntax()); + ) + .clone_for_update(); + ted::replace(variant.syntax(), replacement.syntax()); Some(()) } fn update_reference( ctx: &AssistContext, - rewriter: &mut SyntaxRewriter, reference: FileReference, source_file: &SourceFile, enum_module_def: &ModuleDef, @@ -230,14 +233,16 @@ fn update_reference( let module = ctx.sema.scope(&expr).module()?; if !visited_modules_set.contains(&module) { - if insert_import(ctx, rewriter, &expr, &module, enum_module_def, variant_hir_name).is_some() - { + if insert_import(ctx, &expr, &module, enum_module_def, variant_hir_name).is_some() { visited_modules_set.insert(module); } } - rewriter.insert_after(segment.syntax(), &make::token(T!['('])); - rewriter.insert_after(segment.syntax(), segment.syntax()); - rewriter.insert_after(&expr, &make::token(T![')'])); + ted::insert_raw( + ted::Position::before(segment.syntax()), + make::path_from_text(&format!("{}", segment)).clone_for_update().syntax(), + ); + ted::insert_raw(ted::Position::before(segment.syntax()), make::token(T!['('])); + ted::insert_raw(ted::Position::after(&expr), make::token(T![')'])); Some(()) } diff --git a/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs b/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs index 36d2e0331e..2f2306fcc2 100644 --- a/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs +++ b/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs @@ -1,5 +1,5 @@ use ide_db::helpers::insert_use::{insert_use, ImportScope}; -use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SyntaxNode}; +use syntax::{ast, match_ast, ted, AstNode, SyntaxNode}; use crate::{AssistContext, AssistId, AssistKind, Assists}; @@ -40,18 +40,17 @@ pub(crate) fn replace_qualified_name_with_use( |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(); - shorten_paths(&mut rewriter, syntax.clone(), &path); + let syntax = builder.make_mut(syntax.clone()); if let Some(ref import_scope) = ImportScope::from(syntax.clone()) { - rewriter += insert_use(import_scope, path, ctx.config.insert_use); - builder.rewrite(rewriter); + insert_use(import_scope, path.clone(), ctx.config.insert_use); } + shorten_paths(syntax.clone(), &path.clone_for_update()); }, ) } /// Adds replacements to `re` that shorten `path` in all descendants of `node`. -fn shorten_paths(rewriter: &mut SyntaxRewriter<'static>, node: SyntaxNode, path: &ast::Path) { +fn shorten_paths(node: SyntaxNode, path: &ast::Path) { for child in node.children() { match_ast! { match child { @@ -62,32 +61,28 @@ fn shorten_paths(rewriter: &mut SyntaxRewriter<'static>, node: SyntaxNode, path: ast::Module(_it) => continue, ast::Path(p) => { - match maybe_replace_path(rewriter, p.clone(), path.clone()) { + match maybe_replace_path(p.clone(), path.clone()) { Some(()) => {}, - None => shorten_paths(rewriter, p.syntax().clone(), path), + None => shorten_paths(p.syntax().clone(), path), } }, - _ => shorten_paths(rewriter, child, path), + _ => shorten_paths(child, path), } } } } -fn maybe_replace_path( - rewriter: &mut SyntaxRewriter<'static>, - path: ast::Path, - target: ast::Path, -) -> Option<()> { +fn maybe_replace_path(path: ast::Path, target: ast::Path) -> Option<()> { if !path_eq(path.clone(), target) { return None; } // Shorten `path`, leaving only its last segment. if let Some(parent) = path.qualifier() { - rewriter.delete(parent.syntax()); + ted::remove(parent.syntax()); } if let Some(double_colon) = path.coloncolon_token() { - rewriter.delete(&double_colon); + ted::remove(&double_colon); } Some(()) @@ -150,6 +145,7 @@ Debug ", ); } + #[test] fn test_replace_add_use_no_anchor_with_item_below() { check_assist( diff --git a/crates/ide_completion/src/item.rs b/crates/ide_completion/src/item.rs index 16991b6880..99edb94992 100644 --- a/crates/ide_completion/src/item.rs +++ b/crates/ide_completion/src/item.rs @@ -377,11 +377,11 @@ impl ImportEdit { pub fn to_text_edit(&self, cfg: InsertUseConfig) -> Option { let _p = profile::span("ImportEdit::to_text_edit"); - let rewriter = - insert_use::insert_use(&self.scope, mod_path_to_ast(&self.import.import_path), cfg); - let old_ast = rewriter.rewrite_root()?; + let new_ast = self.scope.clone_for_update(); + insert_use::insert_use(&new_ast, mod_path_to_ast(&self.import.import_path), cfg); let mut import_insert = TextEdit::builder(); - algo::diff(&old_ast, &rewriter.rewrite(&old_ast)).into_text_edit(&mut import_insert); + algo::diff(self.scope.as_syntax_node(), new_ast.as_syntax_node()) + .into_text_edit(&mut import_insert); Some(import_insert.finish()) } diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index be3a22725a..498d76f722 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -4,13 +4,9 @@ use std::{cmp::Ordering, iter::successors}; use hir::Semantics; use itertools::{EitherOrBoth, Itertools}; use syntax::{ - algo::SyntaxRewriter, - ast::{ - self, - edit::{AstNodeEdit, IndentLevel}, - make, AstNode, AttrsOwner, PathSegmentKind, VisibilityOwner, - }, - AstToken, InsertPosition, NodeOrToken, SyntaxElement, SyntaxNode, SyntaxToken, + algo, + ast::{self, edit::AstNodeEdit, make, AstNode, AttrsOwner, PathSegmentKind, VisibilityOwner}, + ted, AstToken, Direction, NodeOrToken, SyntaxNode, SyntaxToken, }; use crate::RootDatabase; @@ -56,127 +52,32 @@ impl ImportScope { } } - fn indent_level(&self) -> IndentLevel { + pub fn clone_for_update(&self) -> Self { match self { - ImportScope::File(file) => file.indent_level(), - ImportScope::Module(item_list) => item_list.indent_level() + 1, + ImportScope::File(file) => ImportScope::File(file.clone_for_update()), + ImportScope::Module(item_list) => ImportScope::Module(item_list.clone_for_update()), } } - - fn first_insert_pos(&self) -> (InsertPosition, AddBlankLine) { - match self { - ImportScope::File(_) => (InsertPosition::First, AddBlankLine::AfterTwice), - // don't insert the imports before the item list's opening curly brace - ImportScope::Module(item_list) => item_list - .l_curly_token() - .map(|b| (InsertPosition::After(b.into()), AddBlankLine::Around)) - .unwrap_or((InsertPosition::First, AddBlankLine::AfterTwice)), - } - } - - fn insert_pos_after_last_inner_element(&self) -> (InsertPosition, AddBlankLine) { - self.as_syntax_node() - .children_with_tokens() - .filter(|child| match child { - NodeOrToken::Node(node) => is_inner_attribute(node.clone()), - NodeOrToken::Token(token) => is_inner_comment(token.clone()), - }) - .last() - .map(|last_inner_element| { - (InsertPosition::After(last_inner_element), AddBlankLine::BeforeTwice) - }) - .unwrap_or_else(|| self.first_insert_pos()) - } -} - -fn is_inner_attribute(node: SyntaxNode) -> bool { - ast::Attr::cast(node).map(|attr| attr.kind()) == Some(ast::AttrKind::Inner) -} - -fn is_inner_comment(token: SyntaxToken) -> bool { - ast::Comment::cast(token).and_then(|comment| comment.kind().doc) - == Some(ast::CommentPlacement::Inner) } /// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur. -pub fn insert_use<'a>( - scope: &ImportScope, - path: ast::Path, - cfg: InsertUseConfig, -) -> SyntaxRewriter<'a> { +pub fn insert_use<'a>(scope: &ImportScope, path: ast::Path, cfg: InsertUseConfig) { let _p = profile::span("insert_use"); - let mut rewriter = SyntaxRewriter::default(); - let use_item = make::use_(None, make::use_tree(path.clone(), None, None, false)); + let use_item = + make::use_(None, make::use_tree(path.clone(), None, None, false)).clone_for_update(); // merge into existing imports if possible if let Some(mb) = cfg.merge { for existing_use in scope.as_syntax_node().children().filter_map(ast::Use::cast) { if let Some(merged) = try_merge_imports(&existing_use, &use_item, mb) { - rewriter.replace(existing_use.syntax(), merged.syntax()); - return rewriter; + ted::replace(existing_use.syntax(), merged.syntax()); + return; } } } // either we weren't allowed to merge or there is no import that fits the merge conditions // so look for the place we have to insert to - let (insert_position, add_blank) = find_insert_position(scope, path, cfg.group); - - let indent = if let ident_level @ 1..=usize::MAX = scope.indent_level().0 as usize { - Some(make::tokens::whitespace(&" ".repeat(4 * ident_level)).into()) - } else { - None - }; - - let to_insert: Vec = { - let mut buf = Vec::new(); - - match add_blank { - AddBlankLine::Before | AddBlankLine::Around => { - buf.push(make::tokens::single_newline().into()) - } - AddBlankLine::BeforeTwice => buf.push(make::tokens::blank_line().into()), - _ => (), - } - - if add_blank.has_before() { - if let Some(indent) = indent.clone() { - cov_mark::hit!(insert_use_indent_before); - buf.push(indent); - } - } - - buf.push(use_item.syntax().clone().into()); - - match add_blank { - AddBlankLine::After | AddBlankLine::Around => { - buf.push(make::tokens::single_newline().into()) - } - AddBlankLine::AfterTwice => buf.push(make::tokens::blank_line().into()), - _ => (), - } - - // only add indentation *after* our stuff if there's another node directly after it - if add_blank.has_after() && matches!(insert_position, InsertPosition::Before(_)) { - if let Some(indent) = indent { - cov_mark::hit!(insert_use_indent_after); - buf.push(indent); - } - } else if add_blank.has_after() && matches!(insert_position, InsertPosition::After(_)) { - cov_mark::hit!(insert_use_no_indent_after); - } - - buf - }; - - match insert_position { - InsertPosition::First => { - rewriter.insert_many_as_first_children(scope.as_syntax_node(), to_insert) - } - InsertPosition::Last => return rewriter, // actually unreachable - InsertPosition::Before(anchor) => rewriter.insert_many_before(&anchor, to_insert), - InsertPosition::After(anchor) => rewriter.insert_many_after(&anchor, to_insert), - } - rewriter + insert_use_(scope, path, cfg.group, use_item); } fn eq_visibility(vis0: Option, vis1: Option) -> bool { @@ -235,7 +136,7 @@ pub fn try_merge_trees( } else { (lhs.split_prefix(&lhs_prefix), rhs.split_prefix(&rhs_prefix)) }; - recursive_merge(&lhs, &rhs, merge).map(|it| it.clone_for_update()) + recursive_merge(&lhs, &rhs, merge) } /// Recursively "zips" together lhs and rhs. @@ -334,7 +235,12 @@ fn recursive_merge( } } } - Some(lhs.with_use_tree_list(make::use_tree_list(use_trees))) + + Some(if let Some(old) = lhs.use_tree_list() { + lhs.replace_descendant(old, make::use_tree_list(use_trees)).clone_for_update() + } else { + lhs.clone() + }) } /// Traverses both paths until they differ, returning the common prefix of both. @@ -520,32 +426,15 @@ impl ImportGroup { } } -#[derive(PartialEq, Eq)] -enum AddBlankLine { - Before, - BeforeTwice, - Around, - After, - AfterTwice, -} - -impl AddBlankLine { - fn has_before(&self) -> bool { - matches!(self, AddBlankLine::Before | AddBlankLine::BeforeTwice | AddBlankLine::Around) - } - fn has_after(&self) -> bool { - matches!(self, AddBlankLine::After | AddBlankLine::AfterTwice | AddBlankLine::Around) - } -} - -fn find_insert_position( +fn insert_use_( scope: &ImportScope, insert_path: ast::Path, group_imports: bool, -) -> (InsertPosition, AddBlankLine) { + use_item: ast::Use, +) { + let scope_syntax = scope.as_syntax_node(); let group = ImportGroup::new(&insert_path); - let path_node_iter = scope - .as_syntax_node() + let path_node_iter = scope_syntax .children() .filter_map(|node| ast::Use::cast(node.clone()).zip(Some(node))) .flat_map(|(use_, node)| { @@ -557,9 +446,12 @@ fn find_insert_position( if !group_imports { if let Some((_, _, node)) = path_node_iter.last() { - return (InsertPosition::After(node.into()), AddBlankLine::Before); + ted::insert(ted::Position::after(node), use_item.syntax()); + } else { + ted::insert(ted::Position::first_child_of(scope_syntax), make::tokens::blank_line()); + ted::insert(ted::Position::first_child_of(scope_syntax), use_item.syntax()); } - return (InsertPosition::First, AddBlankLine::AfterTwice); + return; } // Iterator that discards anything thats not in the required grouping @@ -572,43 +464,83 @@ fn find_insert_position( // track the last element we iterated over, if this is still None after the iteration then that means we never iterated in the first place let mut last = None; // find the element that would come directly after our new import - let post_insert = group_iter.inspect(|(.., node)| last = Some(node.clone())).find( - |&(ref path, has_tl, _)| { + let post_insert: Option<(_, _, SyntaxNode)> = group_iter + .inspect(|(.., node)| last = Some(node.clone())) + .find(|&(ref path, has_tl, _)| { use_tree_path_cmp(&insert_path, false, path, has_tl) != Ordering::Greater - }, - ); + }); - match post_insert { + if let Some((.., node)) = post_insert { // insert our import before that element - Some((.., node)) => (InsertPosition::Before(node.into()), AddBlankLine::After), + return ted::insert(ted::Position::before(node), use_item.syntax()); + } + if let Some(node) = last { // there is no element after our new import, so append it to the end of the group - None => match last { - Some(node) => (InsertPosition::After(node.into()), AddBlankLine::Before), - // the group we were looking for actually doesnt exist, so insert + return ted::insert(ted::Position::after(node), use_item.syntax()); + } + + // the group we were looking for actually doesn't exist, so insert + + let mut last = None; + // find the group that comes after where we want to insert + let post_group = path_node_iter + .inspect(|(.., node)| last = Some(node.clone())) + .find(|(p, ..)| ImportGroup::new(p) > group); + if let Some((.., node)) = post_group { + ted::insert(ted::Position::before(&node), use_item.syntax()); + if let Some(node) = algo::non_trivia_sibling(node.into(), Direction::Prev) { + ted::insert(ted::Position::after(node), make::tokens::single_newline()); + } + return; + } + // there is no such group, so append after the last one + if let Some(node) = last { + ted::insert(ted::Position::after(&node), use_item.syntax()); + ted::insert(ted::Position::after(node), make::tokens::single_newline()); + return; + } + // there are no imports in this file at all + if let Some(last_inner_element) = scope_syntax + .children_with_tokens() + .filter(|child| match child { + NodeOrToken::Node(node) => is_inner_attribute(node.clone()), + NodeOrToken::Token(token) => is_inner_comment(token.clone()), + }) + .last() + { + ted::insert(ted::Position::after(&last_inner_element), use_item.syntax()); + ted::insert(ted::Position::after(last_inner_element), make::tokens::single_newline()); + return; + } + match scope { + ImportScope::File(_) => { + ted::insert(ted::Position::first_child_of(scope_syntax), make::tokens::blank_line()); + ted::insert(ted::Position::first_child_of(scope_syntax), use_item.syntax()) + } + // don't insert the imports before the item list's opening curly brace + ImportScope::Module(item_list) => match item_list.l_curly_token() { + Some(b) => { + ted::insert(ted::Position::after(&b), make::tokens::single_newline()); + ted::insert(ted::Position::after(&b), use_item.syntax()); + } None => { - // similar concept here to the `last` from above - let mut last = None; - // find the group that comes after where we want to insert - let post_group = path_node_iter - .inspect(|(.., node)| last = Some(node.clone())) - .find(|(p, ..)| ImportGroup::new(p) > group); - match post_group { - Some((.., node)) => { - (InsertPosition::Before(node.into()), AddBlankLine::AfterTwice) - } - // there is no such group, so append after the last one - None => match last { - Some(node) => { - (InsertPosition::After(node.into()), AddBlankLine::BeforeTwice) - } - // there are no imports in this file at all - None => scope.insert_pos_after_last_inner_element(), - }, - } + ted::insert( + ted::Position::first_child_of(scope_syntax), + make::tokens::blank_line(), + ); + ted::insert(ted::Position::first_child_of(scope_syntax), use_item.syntax()); } }, } } +fn is_inner_attribute(node: SyntaxNode) -> bool { + ast::Attr::cast(node).map(|attr| attr.kind()) == Some(ast::AttrKind::Inner) +} + +fn is_inner_comment(token: SyntaxToken) -> bool { + ast::Comment::cast(token).and_then(|comment| comment.kind().doc) + == Some(ast::CommentPlacement::Inner) +} #[cfg(test)] mod tests; diff --git a/crates/ide_db/src/helpers/insert_use/tests.rs b/crates/ide_db/src/helpers/insert_use/tests.rs index 3d151e629d..a3464d606b 100644 --- a/crates/ide_db/src/helpers/insert_use/tests.rs +++ b/crates/ide_db/src/helpers/insert_use/tests.rs @@ -51,17 +51,16 @@ use std::bar::G;", #[test] fn insert_start_indent() { - cov_mark::check!(insert_use_indent_after); check_none( "std::bar::AA", r" use std::bar::B; - use std::bar::D;", + use std::bar::C;", r" use std::bar::AA; use std::bar::B; - use std::bar::D;", - ) + use std::bar::C;", + ); } #[test] @@ -120,7 +119,6 @@ use std::bar::ZZ;", #[test] fn insert_end_indent() { - cov_mark::check!(insert_use_indent_before); check_none( "std::bar::ZZ", r" @@ -255,7 +253,6 @@ fn insert_empty_file() { #[test] fn insert_empty_module() { - cov_mark::check!(insert_use_no_indent_after); check( "foo::bar", "mod x {}", @@ -615,7 +612,7 @@ fn check( if module { syntax = syntax.descendants().find_map(ast::Module::cast).unwrap().syntax().clone(); } - let file = super::ImportScope::from(syntax).unwrap(); + let file = super::ImportScope::from(syntax.clone_for_update()).unwrap(); let path = ast::SourceFile::parse(&format!("use {};", path)) .tree() .syntax() @@ -623,12 +620,8 @@ fn check( .find_map(ast::Path::cast) .unwrap(); - let rewriter = insert_use( - &file, - path, - InsertUseConfig { merge: mb, prefix_kind: PrefixKind::Plain, group }, - ); - let result = rewriter.rewrite(file.as_syntax_node()).to_string(); + insert_use(&file, path, InsertUseConfig { merge: mb, prefix_kind: PrefixKind::Plain, group }); + let result = file.as_syntax_node().to_string(); assert_eq_text!(ra_fixture_after, &result); } diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index 94d4f2cf0b..882e9fa090 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -598,6 +598,7 @@ pub mod tokens { SOURCE_FILE .tree() .syntax() + .clone_for_update() .descendants_with_tokens() .filter_map(|it| it.into_token()) .find(|it| it.kind() == WHITESPACE && it.text() == "\n\n") diff --git a/crates/syntax/src/ted.rs b/crates/syntax/src/ted.rs index 450f2e447a..91a06101f5 100644 --- a/crates/syntax/src/ted.rs +++ b/crates/syntax/src/ted.rs @@ -7,7 +7,7 @@ use std::{mem, ops::RangeInclusive}; use parser::T; use crate::{ - ast::{edit::IndentLevel, make}, + ast::{self, edit::IndentLevel, make, AstNode}, SyntaxElement, SyntaxKind, SyntaxNode, SyntaxToken, }; @@ -147,6 +147,16 @@ pub fn append_child_raw(node: &(impl Into + Clone), child: impl Elem fn ws_before(position: &Position, new: &SyntaxElement) -> Option { let prev = match &position.repr { PositionRepr::FirstChild(_) => return None, + PositionRepr::After(it) if it.kind() == SyntaxKind::L_CURLY => { + if new.kind() == SyntaxKind::USE { + if let Some(item_list) = it.parent().and_then(ast::ItemList::cast) { + let mut indent = IndentLevel::from_element(&item_list.syntax().clone().into()); + indent.0 += 1; + return Some(make::tokens::whitespace(&format!("\n{}", indent))); + } + } + it + } PositionRepr::After(it) => it, }; ws_between(prev, new) @@ -173,7 +183,10 @@ fn ws_between(left: &SyntaxElement, right: &SyntaxElement) -> Option Date: Tue, 20 Apr 2021 17:36:36 +0200 Subject: [PATCH 3/6] Rewrite extract_struct_from_enum_variant assist --- .../extract_struct_from_enum_variant.rs | 190 +++++++++--------- 1 file changed, 98 insertions(+), 92 deletions(-) diff --git a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs index 26e1c66ab4..1f800f82b3 100644 --- a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs @@ -15,7 +15,7 @@ use rustc_hash::FxHashSet; use syntax::{ algo::find_node_at_offset, ast::{self, make, AstNode, NameOwner, VisibilityOwner}, - ted, SourceFile, SyntaxElement, SyntaxNode, T, + ted, SyntaxNode, T, }; use crate::{AssistContext, AssistId, AssistKind, Assists}; @@ -62,6 +62,7 @@ pub(crate) fn extract_struct_from_enum_variant( let mut visited_modules_set = FxHashSet::default(); let current_module = enum_hir.module(ctx.db()); visited_modules_set.insert(current_module); + // record file references of the file the def resides in, we only want to swap to the edited file in the builder once let mut def_file_references = None; for (file_id, references) in usages { if file_id == ctx.frange.file_id { @@ -70,36 +71,57 @@ pub(crate) fn extract_struct_from_enum_variant( } builder.edit_file(file_id); let source_file = builder.make_ast_mut(ctx.sema.parse(file_id)); - for reference in references { - update_reference( - ctx, - reference, - &source_file, - &enum_module_def, - &variant_hir_name, - &mut visited_modules_set, - ); - } - } - builder.edit_file(ctx.frange.file_id); - let variant = builder.make_ast_mut(variant.clone()); - let source_file = builder.make_ast_mut(ctx.sema.parse(ctx.frange.file_id)); - for reference in def_file_references.into_iter().flatten() { - update_reference( + let processed = process_references( ctx, - reference, - &source_file, + &mut visited_modules_set, + source_file.syntax(), &enum_module_def, &variant_hir_name, - &mut visited_modules_set, + references, ); + processed.into_iter().for_each(|(segment, node, import)| { + if let Some((scope, path)) = import { + insert_use(&scope, mod_path_to_ast(&path), ctx.config.insert_use); + } + ted::insert_raw( + ted::Position::before(segment.syntax()), + make::path_from_text(&format!("{}", segment)).clone_for_update().syntax(), + ); + ted::insert_raw(ted::Position::before(segment.syntax()), make::token(T!['('])); + ted::insert_raw(ted::Position::after(&node), make::token(T![')'])); + }); } - extract_struct_def( - variant_name.clone(), - &field_list, - &variant.parent_enum().syntax().clone().into(), - enum_ast.visibility(), - ); + builder.edit_file(ctx.frange.file_id); + let source_file = builder.make_ast_mut(ctx.sema.parse(ctx.frange.file_id)); + let variant = builder.make_ast_mut(variant.clone()); + if let Some(references) = def_file_references { + let processed = process_references( + ctx, + &mut visited_modules_set, + source_file.syntax(), + &enum_module_def, + &variant_hir_name, + references, + ); + processed.into_iter().for_each(|(segment, node, import)| { + if let Some((scope, path)) = import { + insert_use(&scope, mod_path_to_ast(&path), ctx.config.insert_use); + } + ted::insert_raw( + ted::Position::before(segment.syntax()), + make::path_from_text(&format!("{}", segment)).clone_for_update().syntax(), + ); + ted::insert_raw(ted::Position::before(segment.syntax()), make::token(T!['('])); + ted::insert_raw(ted::Position::after(&node), make::token(T![')'])); + }); + } + + let def = create_struct_def(variant_name.clone(), &field_list, enum_ast.visibility()) + .unwrap(); + let start_offset = &variant.parent_enum().syntax().clone(); + ted::insert_raw(ted::Position::before(start_offset), def.syntax()); + ted::insert_raw(ted::Position::before(start_offset), &make::tokens::blank_line()); + update_variant(&variant); }, ) @@ -141,31 +163,11 @@ fn existing_definition(db: &RootDatabase, variant_name: &ast::Name, variant: &Va .any(|(name, _)| name.to_string() == variant_name.to_string()) } -fn insert_import( - ctx: &AssistContext, - scope_node: &SyntaxNode, - module: &Module, - enum_module_def: &ModuleDef, - variant_hir_name: &Name, -) -> Option<()> { - let db = ctx.db(); - let mod_path = - module.find_use_path_prefixed(db, *enum_module_def, ctx.config.insert_use.prefix_kind); - if let Some(mut mod_path) = mod_path { - mod_path.pop_segment(); - mod_path.push_segment(variant_hir_name.clone()); - let scope = ImportScope::find_insert_use_container(scope_node, &ctx.sema)?; - insert_use(&scope, mod_path_to_ast(&mod_path), ctx.config.insert_use); - } - Some(()) -} - -fn extract_struct_def( +fn create_struct_def( variant_name: ast::Name, field_list: &Either, - start_offset: &SyntaxElement, visibility: Option, -) -> Option<()> { +) -> Option { let pub_vis = Some(make::visibility_pub()); let field_list = match field_list { Either::Left(field_list) => { @@ -182,18 +184,7 @@ fn extract_struct_def( .into(), }; - ted::insert_raw( - ted::Position::before(start_offset), - make::struct_(visibility, variant_name, None, field_list).clone_for_update().syntax(), - ); - ted::insert_raw(ted::Position::before(start_offset), &make::tokens::blank_line()); - - // if let indent_level @ 1..=usize::MAX = IndentLevel::from_node(enum_.syntax()).0 as usize { - // ted::insert(ted::Position::before(start_offset), &make::tokens::blank_line()); - // rewriter - // .insert_before(start_offset, &make::tokens::whitespace(&" ".repeat(4 * indent_level))); - // } - Some(()) + Some(make::struct_(visibility, variant_name, None, field_list).clone_for_update()) } fn update_variant(variant: &ast::Variant) -> Option<()> { @@ -208,42 +199,57 @@ fn update_variant(variant: &ast::Variant) -> Option<()> { Some(()) } -fn update_reference( +fn process_references( ctx: &AssistContext, - reference: FileReference, - source_file: &SourceFile, + visited_modules: &mut FxHashSet, + source_file: &SyntaxNode, enum_module_def: &ModuleDef, variant_hir_name: &Name, - visited_modules_set: &mut FxHashSet, -) -> Option<()> { - let offset = reference.range.start(); - let (segment, expr) = if let Some(path_expr) = - find_node_at_offset::(source_file.syntax(), offset) - { - // tuple variant - (path_expr.path()?.segment()?, path_expr.syntax().parent()?) - } else if let Some(record_expr) = - find_node_at_offset::(source_file.syntax(), offset) - { - // record variant - (record_expr.path()?.segment()?, record_expr.syntax().clone()) - } else { - return None; - }; + refs: Vec, +) -> Vec<(ast::PathSegment, SyntaxNode, Option<(ImportScope, hir::ModPath)>)> { + refs.into_iter() + .flat_map(|reference| { + let (segment, scope_node, module) = + reference_to_node(&ctx.sema, source_file, reference)?; + if !visited_modules.contains(&module) { + let mod_path = module.find_use_path_prefixed( + ctx.sema.db, + *enum_module_def, + ctx.config.insert_use.prefix_kind, + ); + if let Some(mut mod_path) = mod_path { + mod_path.pop_segment(); + mod_path.push_segment(variant_hir_name.clone()); + // uuuh this wont properly work, find_insert_use_container ascends macros so we might a get new syntax node??? + let scope = ImportScope::find_insert_use_container(&scope_node, &ctx.sema)?; + visited_modules.insert(module); + return Some((segment, scope_node, Some((scope, mod_path)))); + } + } + Some((segment, scope_node, None)) + }) + .collect() +} - let module = ctx.sema.scope(&expr).module()?; - if !visited_modules_set.contains(&module) { - if insert_import(ctx, &expr, &module, enum_module_def, variant_hir_name).is_some() { - visited_modules_set.insert(module); - } +fn reference_to_node( + sema: &hir::Semantics, + source_file: &SyntaxNode, + reference: FileReference, +) -> Option<(ast::PathSegment, SyntaxNode, hir::Module)> { + let offset = reference.range.start(); + if let Some(path_expr) = find_node_at_offset::(source_file, offset) { + // tuple variant + Some((path_expr.path()?.segment()?, path_expr.syntax().parent()?)) + } else if let Some(record_expr) = find_node_at_offset::(source_file, offset) { + // record variant + Some((record_expr.path()?.segment()?, record_expr.syntax().clone())) + } else { + None } - ted::insert_raw( - ted::Position::before(segment.syntax()), - make::path_from_text(&format!("{}", segment)).clone_for_update().syntax(), - ); - ted::insert_raw(ted::Position::before(segment.syntax()), make::token(T!['('])); - ted::insert_raw(ted::Position::after(&expr), make::token(T![')'])); - Some(()) + .and_then(|(segment, expr)| { + let module = sema.scope(&expr).module()?; + Some((segment, expr, module)) + }) } #[cfg(test)] @@ -350,7 +356,7 @@ mod my_mod { pub struct MyField(pub u8, pub u8); - pub enum MyEnum { +pub enum MyEnum { MyField(MyField), } } From b290cd578260f307e872a95f971e5a7c656a80ed Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 20 Apr 2021 19:28:18 +0200 Subject: [PATCH 4/6] Add cov_marks to insert_use tests --- .../ide_assists/src/handlers/auto_import.rs | 2 +- .../extract_struct_from_enum_variant.rs | 55 ++++++++++--------- .../replace_qualified_name_with_use.rs | 18 +++--- .../src/completions/flyimport.rs | 2 +- crates/ide_completion/src/lib.rs | 2 +- crates/ide_db/src/helpers/insert_use.rs | 17 +++++- crates/ide_db/src/helpers/insert_use/tests.rs | 23 ++++++++ 7 files changed, 77 insertions(+), 42 deletions(-) diff --git a/crates/ide_assists/src/handlers/auto_import.rs b/crates/ide_assists/src/handlers/auto_import.rs index 6db2d2edd6..a454a2af32 100644 --- a/crates/ide_assists/src/handlers/auto_import.rs +++ b/crates/ide_assists/src/handlers/auto_import.rs @@ -93,7 +93,7 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> let range = ctx.sema.original_range(&syntax_under_caret).range; let group_label = group_label(import_assets.import_candidate()); - let scope = ImportScope::find_insert_use_container(&syntax_under_caret, &ctx.sema)?; + let scope = ImportScope::find_insert_use_container_with_macros(&syntax_under_caret, &ctx.sema)?; for import in proposed_imports { acc.add_group( &group_label, diff --git a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs index 1f800f82b3..66f274fa78 100644 --- a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs @@ -5,7 +5,7 @@ use hir::{Module, ModuleDef, Name, Variant}; use ide_db::{ defs::Definition, helpers::{ - insert_use::{insert_use, ImportScope}, + insert_use::{insert_use, ImportScope, InsertUseConfig}, mod_path_to_ast, }, search::FileReference, @@ -79,16 +79,8 @@ pub(crate) fn extract_struct_from_enum_variant( &variant_hir_name, references, ); - processed.into_iter().for_each(|(segment, node, import)| { - if let Some((scope, path)) = import { - insert_use(&scope, mod_path_to_ast(&path), ctx.config.insert_use); - } - ted::insert_raw( - ted::Position::before(segment.syntax()), - make::path_from_text(&format!("{}", segment)).clone_for_update().syntax(), - ); - ted::insert_raw(ted::Position::before(segment.syntax()), make::token(T!['('])); - ted::insert_raw(ted::Position::after(&node), make::token(T![')'])); + processed.into_iter().for_each(|(path, node, import)| { + apply_references(ctx.config.insert_use, path, node, import) }); } builder.edit_file(ctx.frange.file_id); @@ -103,21 +95,12 @@ pub(crate) fn extract_struct_from_enum_variant( &variant_hir_name, references, ); - processed.into_iter().for_each(|(segment, node, import)| { - if let Some((scope, path)) = import { - insert_use(&scope, mod_path_to_ast(&path), ctx.config.insert_use); - } - ted::insert_raw( - ted::Position::before(segment.syntax()), - make::path_from_text(&format!("{}", segment)).clone_for_update().syntax(), - ); - ted::insert_raw(ted::Position::before(segment.syntax()), make::token(T!['('])); - ted::insert_raw(ted::Position::after(&node), make::token(T![')'])); + processed.into_iter().for_each(|(path, node, import)| { + apply_references(ctx.config.insert_use, path, node, import) }); } - let def = create_struct_def(variant_name.clone(), &field_list, enum_ast.visibility()) - .unwrap(); + let def = create_struct_def(variant_name.clone(), &field_list, enum_ast.visibility()); let start_offset = &variant.parent_enum().syntax().clone(); ted::insert_raw(ted::Position::before(start_offset), def.syntax()); ted::insert_raw(ted::Position::before(start_offset), &make::tokens::blank_line()); @@ -167,7 +150,7 @@ fn create_struct_def( variant_name: ast::Name, field_list: &Either, visibility: Option, -) -> Option { +) -> ast::Struct { let pub_vis = Some(make::visibility_pub()); let field_list = match field_list { Either::Left(field_list) => { @@ -184,7 +167,7 @@ fn create_struct_def( .into(), }; - Some(make::struct_(visibility, variant_name, None, field_list).clone_for_update()) + make::struct_(visibility, variant_name, None, field_list).clone_for_update() } fn update_variant(variant: &ast::Variant) -> Option<()> { @@ -199,6 +182,23 @@ fn update_variant(variant: &ast::Variant) -> Option<()> { Some(()) } +fn apply_references( + insert_use_cfg: InsertUseConfig, + segment: ast::PathSegment, + node: SyntaxNode, + import: Option<(ImportScope, hir::ModPath)>, +) { + if let Some((scope, path)) = import { + insert_use(&scope, mod_path_to_ast(&path), insert_use_cfg); + } + ted::insert_raw( + ted::Position::before(segment.syntax()), + make::path_from_text(&format!("{}", segment)).clone_for_update().syntax(), + ); + ted::insert_raw(ted::Position::before(segment.syntax()), make::token(T!['('])); + ted::insert_raw(ted::Position::after(&node), make::token(T![')'])); +} + fn process_references( ctx: &AssistContext, visited_modules: &mut FxHashSet, @@ -207,6 +207,8 @@ fn process_references( variant_hir_name: &Name, refs: Vec, ) -> Vec<(ast::PathSegment, SyntaxNode, Option<(ImportScope, hir::ModPath)>)> { + // we have to recollect here eagerly as we are about to edit the tree we need to calculate the changes + // and corresponding nodes up front refs.into_iter() .flat_map(|reference| { let (segment, scope_node, module) = @@ -220,8 +222,7 @@ fn process_references( if let Some(mut mod_path) = mod_path { mod_path.pop_segment(); mod_path.push_segment(variant_hir_name.clone()); - // uuuh this wont properly work, find_insert_use_container ascends macros so we might a get new syntax node??? - let scope = ImportScope::find_insert_use_container(&scope_node, &ctx.sema)?; + let scope = ImportScope::find_insert_use_container(&scope_node)?; visited_modules.insert(module); return Some((segment, scope_node, Some((scope, mod_path)))); } diff --git a/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs b/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs index 2f2306fcc2..99ba798606 100644 --- a/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs +++ b/crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs @@ -31,7 +31,7 @@ pub(crate) fn replace_qualified_name_with_use( } let target = path.syntax().text_range(); - let scope = ImportScope::find_insert_use_container(path.syntax(), &ctx.sema)?; + let scope = ImportScope::find_insert_use_container_with_macros(path.syntax(), &ctx.sema)?; let syntax = scope.as_syntax_node(); acc.add( AssistId("replace_qualified_name_with_use", AssistKind::RefactorRewrite), @@ -42,15 +42,15 @@ pub(crate) fn replace_qualified_name_with_use( // affected (that is, all paths inside the node we added the `use` to). let syntax = builder.make_mut(syntax.clone()); if let Some(ref import_scope) = ImportScope::from(syntax.clone()) { - insert_use(import_scope, path.clone(), ctx.config.insert_use); + shorten_paths(&syntax, &path.clone_for_update()); + insert_use(import_scope, path, ctx.config.insert_use); } - shorten_paths(syntax.clone(), &path.clone_for_update()); }, ) } /// Adds replacements to `re` that shorten `path` in all descendants of `node`. -fn shorten_paths(node: SyntaxNode, path: &ast::Path) { +fn shorten_paths(node: &SyntaxNode, path: &ast::Path) { for child in node.children() { match_ast! { match child { @@ -59,14 +59,10 @@ fn shorten_paths(node: SyntaxNode, path: &ast::Path) { ast::Use(_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(p.clone(), path.clone()) { - Some(()) => {}, - None => shorten_paths(p.syntax().clone(), path), - } + ast::Path(p) => if maybe_replace_path(p.clone(), path.clone()).is_none() { + shorten_paths(p.syntax(), path); }, - _ => shorten_paths(child, path), + _ => shorten_paths(&child, path), } } } diff --git a/crates/ide_completion/src/completions/flyimport.rs b/crates/ide_completion/src/completions/flyimport.rs index 8e211ae1ed..9d5b61562a 100644 --- a/crates/ide_completion/src/completions/flyimport.rs +++ b/crates/ide_completion/src/completions/flyimport.rs @@ -132,7 +132,7 @@ pub(crate) fn import_on_the_fly(acc: &mut Completions, ctx: &CompletionContext) let user_input_lowercased = potential_import_name.to_lowercase(); let import_assets = import_assets(ctx, potential_import_name)?; - let import_scope = ImportScope::find_insert_use_container( + let import_scope = ImportScope::find_insert_use_container_with_macros( position_for_import(ctx, Some(import_assets.import_candidate()))?, &ctx.sema, )?; diff --git a/crates/ide_completion/src/lib.rs b/crates/ide_completion/src/lib.rs index 6f3d5c5c57..e326335654 100644 --- a/crates/ide_completion/src/lib.rs +++ b/crates/ide_completion/src/lib.rs @@ -179,7 +179,7 @@ pub fn resolve_completion_edits( ) -> Option> { let ctx = CompletionContext::new(db, position, config)?; let position_for_import = position_for_import(&ctx, None)?; - let scope = ImportScope::find_insert_use_container(position_for_import, &ctx.sema)?; + let scope = ImportScope::find_insert_use_container_with_macros(position_for_import, &ctx.sema)?; let current_module = ctx.sema.scope(position_for_import).module()?; let current_crate = current_module.krate(); diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index 498d76f722..a43504a275 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -38,13 +38,18 @@ impl ImportScope { } /// Determines the containing syntax node in which to insert a `use` statement affecting `position`. - pub fn find_insert_use_container( + pub fn find_insert_use_container_with_macros( position: &SyntaxNode, sema: &Semantics<'_, RootDatabase>, ) -> Option { sema.ancestors_with_macros(position.clone()).find_map(Self::from) } + /// Determines the containing syntax node in which to insert a `use` statement affecting `position`. + pub fn find_insert_use_container(position: &SyntaxNode) -> Option { + std::iter::successors(Some(position.clone()), SyntaxNode::parent).find_map(Self::from) + } + pub fn as_syntax_node(&self) -> &SyntaxNode { match self { ImportScope::File(file) => file.syntax(), @@ -446,8 +451,10 @@ fn insert_use_( if !group_imports { if let Some((_, _, node)) = path_node_iter.last() { + cov_mark::hit!(insert_no_grouping_last); ted::insert(ted::Position::after(node), use_item.syntax()); } else { + cov_mark::hit!(insert_no_grouping_last2); ted::insert(ted::Position::first_child_of(scope_syntax), make::tokens::blank_line()); ted::insert(ted::Position::first_child_of(scope_syntax), use_item.syntax()); } @@ -471,10 +478,12 @@ fn insert_use_( }); if let Some((.., node)) = post_insert { + cov_mark::hit!(insert_group); // insert our import before that element return ted::insert(ted::Position::before(node), use_item.syntax()); } if let Some(node) = last { + cov_mark::hit!(insert_group_last); // there is no element after our new import, so append it to the end of the group return ted::insert(ted::Position::after(node), use_item.syntax()); } @@ -487,6 +496,7 @@ fn insert_use_( .inspect(|(.., node)| last = Some(node.clone())) .find(|(p, ..)| ImportGroup::new(p) > group); if let Some((.., node)) = post_group { + cov_mark::hit!(insert_group_new_group); ted::insert(ted::Position::before(&node), use_item.syntax()); if let Some(node) = algo::non_trivia_sibling(node.into(), Direction::Prev) { ted::insert(ted::Position::after(node), make::tokens::single_newline()); @@ -495,6 +505,7 @@ fn insert_use_( } // there is no such group, so append after the last one if let Some(node) = last { + cov_mark::hit!(insert_group_no_group); ted::insert(ted::Position::after(&node), use_item.syntax()); ted::insert(ted::Position::after(node), make::tokens::single_newline()); return; @@ -508,22 +519,26 @@ fn insert_use_( }) .last() { + cov_mark::hit!(insert_group_empty_inner_attr); ted::insert(ted::Position::after(&last_inner_element), use_item.syntax()); ted::insert(ted::Position::after(last_inner_element), make::tokens::single_newline()); return; } match scope { ImportScope::File(_) => { + cov_mark::hit!(insert_group_empty_file); ted::insert(ted::Position::first_child_of(scope_syntax), make::tokens::blank_line()); ted::insert(ted::Position::first_child_of(scope_syntax), use_item.syntax()) } // don't insert the imports before the item list's opening curly brace ImportScope::Module(item_list) => match item_list.l_curly_token() { Some(b) => { + cov_mark::hit!(insert_group_empty_module); ted::insert(ted::Position::after(&b), make::tokens::single_newline()); ted::insert(ted::Position::after(&b), use_item.syntax()); } None => { + // This should never happens, broken module syntax node ted::insert( ted::Position::first_child_of(scope_syntax), make::tokens::blank_line(), diff --git a/crates/ide_db/src/helpers/insert_use/tests.rs b/crates/ide_db/src/helpers/insert_use/tests.rs index a3464d606b..048c213e22 100644 --- a/crates/ide_db/src/helpers/insert_use/tests.rs +++ b/crates/ide_db/src/helpers/insert_use/tests.rs @@ -5,6 +5,7 @@ use test_utils::assert_eq_text; #[test] fn insert_not_group() { + cov_mark::check!(insert_no_grouping_last); check( "use external_crate2::bar::A", r" @@ -26,6 +27,21 @@ use external_crate2::bar::A;", ); } +#[test] +fn insert_not_group_empty() { + cov_mark::check!(insert_no_grouping_last2); + check( + "use external_crate2::bar::A", + r"", + r"use external_crate2::bar::A; + +", + None, + false, + false, + ); +} + #[test] fn insert_existing() { check_full("std::fs", "use std::fs;", "use std::fs;") @@ -65,6 +81,7 @@ fn insert_start_indent() { #[test] fn insert_middle() { + cov_mark::check!(insert_group); check_none( "std::bar::EE", r" @@ -101,6 +118,7 @@ fn insert_middle_indent() { #[test] fn insert_end() { + cov_mark::check!(insert_group_last); check_none( "std::bar::ZZ", r" @@ -199,6 +217,7 @@ fn insert_first_matching_group() { #[test] fn insert_missing_group_std() { + cov_mark::check!(insert_group_new_group); check_none( "std::fmt", r" @@ -214,6 +233,7 @@ fn insert_missing_group_std() { #[test] fn insert_missing_group_self() { + cov_mark::check!(insert_group_no_group); check_none( "self::fmt", r" @@ -240,6 +260,7 @@ fn main() {}", #[test] fn insert_empty_file() { + cov_mark::check!(insert_group_empty_file); // empty files will get two trailing newlines // this is due to the test case insert_no_imports above check_full( @@ -253,6 +274,7 @@ fn insert_empty_file() { #[test] fn insert_empty_module() { + cov_mark::check!(insert_group_empty_module); check( "foo::bar", "mod x {}", @@ -267,6 +289,7 @@ fn insert_empty_module() { #[test] fn insert_after_inner_attr() { + cov_mark::check!(insert_group_empty_inner_attr); check_full( "foo::bar", r"#![allow(unused_imports)]", From d5c9de65c555235d7a27dc4617858b7a24be4935 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 22 Apr 2021 00:54:31 +0200 Subject: [PATCH 5/6] Don't filter equal nodes in reorder assists --- crates/ide_assists/src/handlers/reorder_fields.rs | 8 +++----- crates/ide_assists/src/handlers/reorder_impl.rs | 8 +++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/crates/ide_assists/src/handlers/reorder_fields.rs b/crates/ide_assists/src/handlers/reorder_fields.rs index 1a95135ca1..e90bbdbcf2 100644 --- a/crates/ide_assists/src/handlers/reorder_fields.rs +++ b/crates/ide_assists/src/handlers/reorder_fields.rs @@ -83,11 +83,9 @@ fn replace( fields: impl Iterator, sorted_fields: impl IntoIterator, ) { - fields.zip(sorted_fields).filter(|(field, sorted)| field != sorted).for_each( - |(field, sorted_field)| { - ted::replace(field.syntax(), sorted_field.syntax().clone_for_update()); - }, - ); + fields.zip(sorted_fields).for_each(|(field, sorted_field)| { + ted::replace(field.syntax(), sorted_field.syntax().clone_for_update()) + }); } fn compute_fields_ranks(path: &ast::Path, ctx: &AssistContext) -> Option> { diff --git a/crates/ide_assists/src/handlers/reorder_impl.rs b/crates/ide_assists/src/handlers/reorder_impl.rs index fd2897c4cc..72d8892481 100644 --- a/crates/ide_assists/src/handlers/reorder_impl.rs +++ b/crates/ide_assists/src/handlers/reorder_impl.rs @@ -79,11 +79,9 @@ pub(crate) fn reorder_impl(acc: &mut Assists, ctx: &AssistContext) -> Option<()> "Sort methods", target, |builder| { - for (old, new) in - methods.into_iter().zip(sorted).filter(|(field, sorted)| field != sorted) - { - ted::replace(builder.make_ast_mut(old).syntax(), new.clone_for_update().syntax()); - } + methods.into_iter().zip(sorted).for_each(|(old, new)| { + ted::replace(builder.make_ast_mut(old).syntax(), new.clone_for_update().syntax()) + }); }, ) } From e6e4417bbbcc7e135d1b372e4e278cd3efa2d4b8 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 23 Apr 2021 18:36:43 +0200 Subject: [PATCH 6/6] Remove SyntaxRewriter::from_fn --- crates/ide_assists/src/ast_transform.rs | 33 ++++++++++++++----------- crates/ide_assists/src/utils.rs | 3 ++- crates/syntax/src/algo.rs | 19 +++----------- 3 files changed, 24 insertions(+), 31 deletions(-) diff --git a/crates/ide_assists/src/ast_transform.rs b/crates/ide_assists/src/ast_transform.rs index 4a3ed7783e..e5ae718c9f 100644 --- a/crates/ide_assists/src/ast_transform.rs +++ b/crates/ide_assists/src/ast_transform.rs @@ -3,20 +3,27 @@ use hir::{HirDisplay, PathResolution, SemanticsScope}; use ide_db::helpers::mod_path_to_ast; use rustc_hash::FxHashMap; use syntax::{ - algo::SyntaxRewriter, ast::{self, AstNode}, - SyntaxNode, + ted, SyntaxNode, }; -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, transformer)?; - Some(replacement.into()) +pub fn apply<'a, N: AstNode>(transformer: &dyn AstTransform<'a>, node: &N) { + let mut skip_to = None; + for event in node.syntax().preorder() { + match event { + syntax::WalkEvent::Enter(node) if skip_to.is_none() => { + skip_to = transformer.get_substitution(&node, transformer).zip(Some(node)); + } + syntax::WalkEvent::Enter(_) => (), + syntax::WalkEvent::Leave(node) => match &skip_to { + Some((replacement, skip_target)) if *skip_target == node => { + ted::replace(node, replacement.clone_for_update()); + skip_to.take(); + } + _ => (), + }, } - _ => None, - }) - .rewrite_ast(&node) + } } /// `AstTransform` helps with applying bulk transformations to syntax nodes. @@ -191,11 +198,9 @@ impl<'a> AstTransform<'a> for QualifyPaths<'a> { let found_path = from.find_use_path(self.source_scope.db.upcast(), def)?; let mut path = mod_path_to_ast(&found_path); - let type_args = p - .segment() - .and_then(|s| s.generic_arg_list()) - .map(|arg_list| apply(recur, arg_list)); + let type_args = p.segment().and_then(|s| s.generic_arg_list()); if let Some(type_args) = type_args { + apply(recur, &type_args); let last_segment = path.segment().unwrap(); path = path.with_segment(last_segment.with_generic_args(type_args)) } diff --git a/crates/ide_assists/src/utils.rs b/crates/ide_assists/src/utils.rs index d67524937e..5a90ad715b 100644 --- a/crates/ide_assists/src/utils.rs +++ b/crates/ide_assists/src/utils.rs @@ -140,7 +140,8 @@ pub fn add_trait_assoc_items_to_impl( let items = items .into_iter() - .map(|it| ast_transform::apply(&*ast_transform, it)) + .map(|it| it.clone_for_update()) + .inspect(|it| ast_transform::apply(&*ast_transform, it)) .map(|it| match it { ast::AssocItem::Fn(def) => ast::AssocItem::Fn(add_body(def)), ast::AssocItem::TypeAlias(def) => ast::AssocItem::TypeAlias(def.remove_bounds()), diff --git a/crates/syntax/src/algo.rs b/crates/syntax/src/algo.rs index a153a9e1c3..c9229c4e07 100644 --- a/crates/syntax/src/algo.rs +++ b/crates/syntax/src/algo.rs @@ -342,10 +342,10 @@ enum InsertPos { #[derive(Default)] pub struct SyntaxRewriter<'a> { - f: Option Option + 'a>>, //FIXME: add debug_assertions that all elements are in fact from the same file. replacements: FxHashMap, insertions: IndexMap>, + _pd: std::marker::PhantomData<&'a ()>, } impl fmt::Debug for SyntaxRewriter<'_> { @@ -357,14 +357,7 @@ impl fmt::Debug for SyntaxRewriter<'_> { } } -impl<'a> SyntaxRewriter<'a> { - pub fn from_fn(f: impl Fn(&SyntaxElement) -> Option + 'a) -> SyntaxRewriter<'a> { - SyntaxRewriter { - f: Some(Box::new(f)), - replacements: FxHashMap::default(), - insertions: IndexMap::default(), - } - } +impl SyntaxRewriter<'_> { pub fn delete>(&mut self, what: &T) { let what = what.clone().into(); let replacement = Replacement::Delete; @@ -470,7 +463,7 @@ impl<'a> SyntaxRewriter<'a> { pub fn rewrite(&self, node: &SyntaxNode) -> SyntaxNode { let _p = profile::span("rewrite"); - if self.f.is_none() && self.replacements.is_empty() && self.insertions.is_empty() { + if self.replacements.is_empty() && self.insertions.is_empty() { return node.clone(); } let green = self.rewrite_children(node); @@ -495,7 +488,6 @@ impl<'a> SyntaxRewriter<'a> { } } - assert!(self.f.is_none()); self.replacements .keys() .filter_map(element_to_node_or_parent) @@ -510,10 +502,6 @@ impl<'a> SyntaxRewriter<'a> { } fn replacement(&self, element: &SyntaxElement) -> Option { - if let Some(f) = &self.f { - assert!(self.replacements.is_empty()); - return f(element).map(Replacement::Single); - } self.replacements.get(element).cloned() } @@ -574,7 +562,6 @@ fn element_to_green(element: SyntaxElement) -> NodeOrToken { fn add_assign(&mut self, rhs: SyntaxRewriter) { - assert!(rhs.f.is_none()); self.replacements.extend(rhs.replacements); for (pos, insertions) in rhs.insertions.into_iter() { match self.insertions.entry(pos) {