From c039810b16c15c0d94eb57022130db4d0142918f Mon Sep 17 00:00:00 2001 From: Jonas Bushart Date: Thu, 31 Mar 2022 18:15:01 +0000 Subject: [PATCH 1/3] 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 --- crates/ide_db/src/imports/insert_use.rs | 114 +++++++++--------- 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"", @@ -103,6 +103,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)]", From 0cc079f3e96968cdd88cb016ab34c22ed5f93dc6 Mon Sep 17 00:00:00 2001 From: Jonas Bushart Date: Sun, 3 Apr 2022 12:01:11 +0000 Subject: [PATCH 2/3] Merge test functions using the same coverage marks to avoid parallelism --- crates/ide_db/src/imports/insert_use/tests.rs | 162 +++++++++--------- 1 file changed, 84 insertions(+), 78 deletions(-) diff --git a/crates/ide_db/src/imports/insert_use/tests.rs b/crates/ide_db/src/imports/insert_use/tests.rs index 39d2b22ff0..2048b7baed 100644 --- a/crates/ide_db/src/imports/insert_use/tests.rs +++ b/crates/ide_db/src/imports/insert_use/tests.rs @@ -84,63 +84,6 @@ use external_crate2::bar::A;", ); } -#[test] -fn insert_not_group_empty() { - cov_mark::check!(insert_empty_file); - check_with_config( - "use external_crate2::bar::A", - r"", - r"use external_crate2::bar::A; - -", - &InsertUseConfig { - granularity: ImportGranularity::Item, - enforce_granularity: true, - prefix_kind: PrefixKind::Plain, - group: false, - skip_glob_imports: true, - }, - ); -} - -#[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;") @@ -359,45 +302,108 @@ fn main() {}", #[test] fn insert_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( - "foo::bar", - "", - r"use foo::bar; + { + // Default configuration + 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( + "foo::bar", + "", + r"use foo::bar; ", - ) + ); + } + { + // "not group" configuration + cov_mark::check!(insert_empty_file); + check_with_config( + "use external_crate2::bar::A", + r"", + r"use external_crate2::bar::A; + +", + &InsertUseConfig { + granularity: ImportGranularity::Item, + enforce_granularity: true, + prefix_kind: PrefixKind::Plain, + group: false, + skip_glob_imports: true, + }, + ); + } } #[test] fn insert_empty_module() { - cov_mark::check!(insert_empty_module); - check( - "foo::bar", - r" + { + // Default configuration + cov_mark::check!(insert_empty_module); + check( + "foo::bar", + r" mod x {$0} ", - r" + r" mod x { use foo::bar; } ", - ImportGranularity::Item, - ) + ImportGranularity::Item, + ); + } + { + // "not group" configuration + 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_after_inner_attr() { - cov_mark::check!(insert_empty_inner_attr); - check_crate( - "foo::bar", - r"#![allow(unused_imports)]", - r"#![allow(unused_imports)] + { + // Default configuration + cov_mark::check!(insert_empty_inner_attr); + check_crate( + "foo::bar", + r"#![allow(unused_imports)]", + r"#![allow(unused_imports)] use foo::bar;", - ) + ); + } + { + // "not group" configuration + 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] From 156f9074e19346d79d8d7bc21ff643f50ba6a954 Mon Sep 17 00:00:00 2001 From: Jonas Bushart Date: Sun, 3 Apr 2022 18:34:06 +0000 Subject: [PATCH 3/3] Use check_count! instead of multiple check! in separate scopes Changes are based on the review feedback by Veykril. --- crates/ide_db/src/imports/insert_use/tests.rs | 147 ++++++++---------- 1 file changed, 69 insertions(+), 78 deletions(-) diff --git a/crates/ide_db/src/imports/insert_use/tests.rs b/crates/ide_db/src/imports/insert_use/tests.rs index 2048b7baed..70f28b9241 100644 --- a/crates/ide_db/src/imports/insert_use/tests.rs +++ b/crates/ide_db/src/imports/insert_use/tests.rs @@ -302,108 +302,99 @@ fn main() {}", #[test] fn insert_empty_file() { - { - // Default configuration - 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( - "foo::bar", - "", - r"use foo::bar; + cov_mark::check_count!(insert_empty_file, 2); + + // Default configuration + // empty files will get two trailing newlines + // this is due to the test case insert_no_imports above + check_crate( + "foo::bar", + "", + r"use foo::bar; ", - ); - } - { - // "not group" configuration - cov_mark::check!(insert_empty_file); - check_with_config( - "use external_crate2::bar::A", - r"", - r"use external_crate2::bar::A; + ); + + // "not group" configuration + check_with_config( + "use external_crate2::bar::A", + r"", + r"use external_crate2::bar::A; ", - &InsertUseConfig { - granularity: ImportGranularity::Item, - enforce_granularity: true, - prefix_kind: PrefixKind::Plain, - group: false, - skip_glob_imports: true, - }, - ); - } + &InsertUseConfig { + granularity: ImportGranularity::Item, + enforce_granularity: true, + prefix_kind: PrefixKind::Plain, + group: false, + skip_glob_imports: true, + }, + ); } #[test] fn insert_empty_module() { - { - // Default configuration - cov_mark::check!(insert_empty_module); - check( - "foo::bar", - r" + cov_mark::check_count!(insert_empty_module, 2); + + // Default configuration + check( + "foo::bar", + r" mod x {$0} ", - r" + r" mod x { use foo::bar; } ", - ImportGranularity::Item, - ); - } - { - // "not group" configuration - cov_mark::check!(insert_empty_module); - check_with_config( - "foo::bar", - r"mod x {$0}", - r"mod x { + ImportGranularity::Item, + ); + + // "not group" configuration + 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, - }, - ); - } + &InsertUseConfig { + granularity: ImportGranularity::Item, + enforce_granularity: true, + prefix_kind: PrefixKind::Plain, + group: false, + skip_glob_imports: true, + }, + ); } #[test] fn insert_after_inner_attr() { - { - // Default configuration - cov_mark::check!(insert_empty_inner_attr); - check_crate( - "foo::bar", - r"#![allow(unused_imports)]", - r"#![allow(unused_imports)] + cov_mark::check_count!(insert_empty_inner_attr, 2); + + // Default configuration + check_crate( + "foo::bar", + r"#![allow(unused_imports)]", + r"#![allow(unused_imports)] use foo::bar;", - ); - } - { - // "not group" configuration - cov_mark::check!(insert_empty_inner_attr); - check_with_config( - "foo::bar", - r"#![allow(unused_imports)]", - r"#![allow(unused_imports)] + ); + + // "not group" configuration + 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, - }, - ); - } + &InsertUseConfig { + granularity: ImportGranularity::Item, + enforce_granularity: true, + prefix_kind: PrefixKind::Plain, + group: false, + skip_glob_imports: true, + }, + ); } #[test]