Merge #11865

11865: Fix: Select correct insert position for disabled group import r=jonasbb a=jonasbb

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

Co-authored-by: Jonas Bushart <jonas@bushart.org>

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 {
+            return;
-            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;
     }
 
-    // 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());
         }

crates/ide_db/src/imports/insert_use/tests.rs

@@ -84,25 +84,6 @@ use external_crate2::bar::A;",
     );
 }
 
-#[test]
-fn insert_not_group_empty() {
-    cov_mark::check!(insert_no_grouping_last2);
-    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_existing() {
     check_crate("std::fs", "use std::fs;", "use std::fs;")
@@ -321,7 +302,9 @@ fn main() {}",
 
 #[test]
 fn insert_empty_file() {
-    cov_mark::check!(insert_group_empty_file);
+    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(
@@ -330,12 +313,30 @@ fn insert_empty_file() {
         r"use foo::bar;
 
 ",
-    )
+    );
+
+    // "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,
+        },
+    );
 }
 
 #[test]
 fn insert_empty_module() {
-    cov_mark::check!(insert_group_empty_module);
+    cov_mark::check_count!(insert_empty_module, 2);
+
+    // Default configuration
     check(
         "foo::bar",
         r"
@@ -347,19 +348,53 @@ 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,
+        },
+    );
 }
 
 #[test]
 fn insert_after_inner_attr() {
-    cov_mark::check!(insert_group_empty_inner_attr);
+    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
+    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]