Unnamed repository; edit this file 'description' to name the repository.
Fix: Select correct insert position for disabled group import
The logic for importing with and without `group_imports` differed
significantly when no previous group existed. This lead to the problem
of using the wrong position when importing inside a module (#11585) or
when inner attributes are involved.
The existing code for grouped imports is better and takes these things
into account.
This PR changes the flow to use the pre-existing code for adding a new
import group even for the non-grouped import settings.
Some coverage markers are updated and the `group` is removed, since they
are now invoked in both cases (grouping and no grouping).
Tests are updated and two tests (empty module and inner attribute) are
added.
Fixes #11585
| -rw-r--r-- | crates/ide_db/src/imports/insert_use.rs | 114 | ||||
| -rw-r--r-- | crates/ide_db/src/imports/insert_use/tests.rs | 46 |
2 files changed, 98 insertions, 62 deletions
diff --git a/crates/ide_db/src/imports/insert_use.rs b/crates/ide_db/src/imports/insert_use.rs index 9e39c26b45..a19969ecc4 100644 --- a/crates/ide_db/src/imports/insert_use.rs +++ b/crates/ide_db/src/imports/insert_use.rs @@ -337,67 +337,65 @@ fn insert_use_( Some((path, has_tl, node)) }); - if !group_imports { + if group_imports { + // Iterator that discards anything thats not in the required grouping + // This implementation allows the user to rearrange their import groups as this only takes the first group that fits + let group_iter = path_node_iter + .clone() + .skip_while(|(path, ..)| ImportGroup::new(path) != group) + .take_while(|(path, ..)| ImportGroup::new(path) == group); + + // 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: 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 + }); + + 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()); + } + + // 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 { + 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()); + } + return; + } + // 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; + } + } else { + // There exists a group, so append to the end of it 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()); + return; } - return; - } - - // Iterator that discards anything thats not in the required grouping - // This implementation allows the user to rearrange their import groups as this only takes the first group that fits - let group_iter = path_node_iter - .clone() - .skip_while(|(path, ..)| ImportGroup::new(path) != group) - .take_while(|(path, ..)| ImportGroup::new(path) == group); - - // 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: 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 - }); - - 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()); } - // 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 { - 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()); - } - return; - } - // 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; - } // there are no imports in this file at all if let Some(last_inner_element) = scope_syntax .children_with_tokens() @@ -407,14 +405,14 @@ fn insert_use_( }) .last() { - cov_mark::hit!(insert_group_empty_inner_attr); + cov_mark::hit!(insert_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; } let l_curly = match scope { ImportScope::File(_) => { - cov_mark::hit!(insert_group_empty_file); + cov_mark::hit!(insert_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()); return; @@ -426,7 +424,7 @@ fn insert_use_( }; match l_curly { Some(b) => { - cov_mark::hit!(insert_group_empty_module); + cov_mark::hit!(insert_empty_module); ted::insert(ted::Position::after(&b), make::tokens::single_newline()); ted::insert(ted::Position::after(&b), use_item.syntax()); } diff --git a/crates/ide_db/src/imports/insert_use/tests.rs b/crates/ide_db/src/imports/insert_use/tests.rs index 4219358a07..39d2b22ff0 100644 --- a/crates/ide_db/src/imports/insert_use/tests.rs +++ b/crates/ide_db/src/imports/insert_use/tests.rs @@ -86,7 +86,7 @@ use external_crate2::bar::A;", #[test] fn insert_not_group_empty() { - cov_mark::check!(insert_no_grouping_last2); + cov_mark::check!(insert_empty_file); check_with_config( "use external_crate2::bar::A", r"", @@ -104,6 +104,44 @@ fn insert_not_group_empty() { } #[test] +fn insert_not_group_empty_module() { + cov_mark::check!(insert_empty_module); + check_with_config( + "foo::bar", + r"mod x {$0}", + r"mod x { + use foo::bar; +}", + &InsertUseConfig { + granularity: ImportGranularity::Item, + enforce_granularity: true, + prefix_kind: PrefixKind::Plain, + group: false, + skip_glob_imports: true, + }, + ); +} + +#[test] +fn insert_no_group_after_inner_attr() { + cov_mark::check!(insert_empty_inner_attr); + check_with_config( + "foo::bar", + r"#![allow(unused_imports)]", + r"#![allow(unused_imports)] + +use foo::bar;", + &InsertUseConfig { + granularity: ImportGranularity::Item, + enforce_granularity: true, + prefix_kind: PrefixKind::Plain, + group: false, + skip_glob_imports: true, + }, + ) +} + +#[test] fn insert_existing() { check_crate("std::fs", "use std::fs;", "use std::fs;") } @@ -321,7 +359,7 @@ fn main() {}", #[test] fn insert_empty_file() { - cov_mark::check!(insert_group_empty_file); + cov_mark::check!(insert_empty_file); // empty files will get two trailing newlines // this is due to the test case insert_no_imports above check_crate( @@ -335,7 +373,7 @@ fn insert_empty_file() { #[test] fn insert_empty_module() { - cov_mark::check!(insert_group_empty_module); + cov_mark::check!(insert_empty_module); check( "foo::bar", r" @@ -352,7 +390,7 @@ mod x { #[test] fn insert_after_inner_attr() { - cov_mark::check!(insert_group_empty_inner_attr); + cov_mark::check!(insert_empty_inner_attr); check_crate( "foo::bar", r"#![allow(unused_imports)]", |