From b290cd578260f307e872a95f971e5a7c656a80ed Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 20 Apr 2021 19:28:18 +0200 Subject: Add cov_marks to insert_use tests --- crates/ide_assists/src/handlers/auto_import.rs | 2 +- .../handlers/extract_struct_from_enum_variant.rs | 55 +++++++++++----------- .../handlers/replace_qualified_name_with_use.rs | 18 +++---- crates/ide_completion/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 6db2d2edd..a454a2af3 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 1f800f82b..66f274fa7 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 2f2306fcc..99ba79860 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 8e211ae1e..9d5b61562 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 6f3d5c5c5..e32633565 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 498d76f72..a43504a27 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 a3464d606..048c213e2 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)]", -- cgit v1.2.3