Merge #10439

10439: fix: fix insert_use incorrectly merging glob imports r=Veykril a=Veykril

Fixes https://github.com/rust-analyzer/rust-analyzer/issues/6800
bors r+

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

crates/ide_db/src/helpers/insert_use.rs

@@ -1,4 +1,4 @@
-//! Handle syntactic aspects of inserting a new `use`.
+//! Handle syntactic aspects of inserting a new `use` item.
 #[cfg(test)]
 mod tests;
 
@@ -103,81 +103,6 @@ pub fn clone_for_update(&self) -> Self {
             ImportScope::Block(block) => ImportScope::Block(block.clone_for_update()),
         }
     }
-
-    fn guess_granularity_from_scope(&self) -> ImportGranularityGuess {
-        // The idea is simple, just check each import as well as the import and its precedent together for
-        // whether they fulfill a granularity criteria.
-        let use_stmt = |item| match item {
-            ast::Item::Use(use_) => {
-                let use_tree = use_.use_tree()?;
-                Some((use_tree, use_.visibility(), use_.attrs()))
-            }
-            _ => None,
-        };
-        let mut use_stmts = match self {
-            ImportScope::File(f) => f.items(),
-            ImportScope::Module(m) => m.items(),
-            ImportScope::Block(b) => b.items(),
-        }
-        .filter_map(use_stmt);
-        let mut res = ImportGranularityGuess::Unknown;
-        let (mut prev, mut prev_vis, mut prev_attrs) = match use_stmts.next() {
-            Some(it) => it,
-            None => return res,
-        };
-        loop {
-            if let Some(use_tree_list) = prev.use_tree_list() {
-                if use_tree_list.use_trees().any(|tree| tree.use_tree_list().is_some()) {
-                    // Nested tree lists can only occur in crate style, or with no proper style being enforced in the file.
-                    break ImportGranularityGuess::Crate;
-                } else {
-                    // Could still be crate-style so continue looking.
-                    res = ImportGranularityGuess::CrateOrModule;
-                }
-            }
-
-            let (curr, curr_vis, curr_attrs) = match use_stmts.next() {
-                Some(it) => it,
-                None => break res,
-            };
-            if eq_visibility(prev_vis, curr_vis.clone()) && eq_attrs(prev_attrs, curr_attrs.clone())
-            {
-                if let Some((prev_path, curr_path)) = prev.path().zip(curr.path()) {
-                    if let Some((prev_prefix, _)) = common_prefix(&prev_path, &curr_path) {
-                        if prev.use_tree_list().is_none() && curr.use_tree_list().is_none() {
-                            let prefix_c = prev_prefix.qualifiers().count();
-                            let curr_c = curr_path.qualifiers().count() - prefix_c;
-                            let prev_c = prev_path.qualifiers().count() - prefix_c;
-                            if curr_c == 1 && prev_c == 1 {
-                                // Same prefix, only differing in the last segment and no use tree lists so this has to be of item style.
-                                break ImportGranularityGuess::Item;
-                            } else {
-                                // Same prefix and no use tree list but differs in more than one segment at the end. This might be module style still.
-                                res = ImportGranularityGuess::ModuleOrItem;
-                            }
-                        } else {
-                            // Same prefix with item tree lists, has to be module style as it
-                            // can't be crate style since the trees wouldn't share a prefix then.
-                            break ImportGranularityGuess::Module;
-                        }
-                    }
-                }
-            }
-            prev = curr;
-            prev_vis = curr_vis;
-            prev_attrs = curr_attrs;
-        }
-    }
-}
-
-#[derive(PartialEq, PartialOrd, Debug, Clone, Copy)]
-enum ImportGranularityGuess {
-    Unknown,
-    Item,
-    Module,
-    ModuleOrItem,
-    Crate,
-    CrateOrModule,
 }
 
 /// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur.
@@ -189,7 +114,7 @@ pub fn insert_use(scope: &ImportScope, path: ast::Path, cfg: &InsertUseConfig) {
         ImportGranularity::Item | ImportGranularity::Preserve => None,
     };
     if !cfg.enforce_granularity {
-        let file_granularity = scope.guess_granularity_from_scope();
+        let file_granularity = guess_granularity_from_scope(scope);
         mb = match file_granularity {
             ImportGranularityGuess::Unknown => mb,
             ImportGranularityGuess::Item => None,
@@ -271,6 +196,80 @@ fn new(path: &ast::Path) -> ImportGroup {
     }
 }
 
+#[derive(PartialEq, PartialOrd, Debug, Clone, Copy)]
+enum ImportGranularityGuess {
+    Unknown,
+    Item,
+    Module,
+    ModuleOrItem,
+    Crate,
+    CrateOrModule,
+}
+
+fn guess_granularity_from_scope(scope: &ImportScope) -> ImportGranularityGuess {
+    // The idea is simple, just check each import as well as the import and its precedent together for
+    // whether they fulfill a granularity criteria.
+    let use_stmt = |item| match item {
+        ast::Item::Use(use_) => {
+            let use_tree = use_.use_tree()?;
+            Some((use_tree, use_.visibility(), use_.attrs()))
+        }
+        _ => None,
+    };
+    let mut use_stmts = match scope {
+        ImportScope::File(f) => f.items(),
+        ImportScope::Module(m) => m.items(),
+        ImportScope::Block(b) => b.items(),
+    }
+    .filter_map(use_stmt);
+    let mut res = ImportGranularityGuess::Unknown;
+    let (mut prev, mut prev_vis, mut prev_attrs) = match use_stmts.next() {
+        Some(it) => it,
+        None => return res,
+    };
+    loop {
+        if let Some(use_tree_list) = prev.use_tree_list() {
+            if use_tree_list.use_trees().any(|tree| tree.use_tree_list().is_some()) {
+                // Nested tree lists can only occur in crate style, or with no proper style being enforced in the file.
+                break ImportGranularityGuess::Crate;
+            } else {
+                // Could still be crate-style so continue looking.
+                res = ImportGranularityGuess::CrateOrModule;
+            }
+        }
+
+        let (curr, curr_vis, curr_attrs) = match use_stmts.next() {
+            Some(it) => it,
+            None => break res,
+        };
+        if eq_visibility(prev_vis, curr_vis.clone()) && eq_attrs(prev_attrs, curr_attrs.clone()) {
+            if let Some((prev_path, curr_path)) = prev.path().zip(curr.path()) {
+                if let Some((prev_prefix, _)) = common_prefix(&prev_path, &curr_path) {
+                    if prev.use_tree_list().is_none() && curr.use_tree_list().is_none() {
+                        let prefix_c = prev_prefix.qualifiers().count();
+                        let curr_c = curr_path.qualifiers().count() - prefix_c;
+                        let prev_c = prev_path.qualifiers().count() - prefix_c;
+                        if curr_c == 1 && prev_c == 1 {
+                            // Same prefix, only differing in the last segment and no use tree lists so this has to be of item style.
+                            break ImportGranularityGuess::Item;
+                        } else {
+                            // Same prefix and no use tree list but differs in more than one segment at the end. This might be module style still.
+                            res = ImportGranularityGuess::ModuleOrItem;
+                        }
+                    } else {
+                        // Same prefix with item tree lists, has to be module style as it
+                        // can't be crate style since the trees wouldn't share a prefix then.
+                        break ImportGranularityGuess::Module;
+                    }
+                }
+            }
+        }
+        prev = curr;
+        prev_vis = curr_vis;
+        prev_attrs = curr_attrs;
+    }
+}
+
 fn insert_use_(
     scope: &ImportScope,
     insert_path: &ast::Path,

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

@@ -612,6 +612,7 @@ fn merge_mod_into_glob() {
 
 #[test]
 fn merge_self_glob() {
+    cov_mark::check!(merge_self_glob);
     check_with_config(
         "self",
         r"use self::*;",
@@ -627,6 +628,17 @@ fn merge_self_glob() {
     // FIXME: have it emit `use {self, *}`?
 }
 
+#[test]
+fn merge_glob() {
+    check_crate(
+        "syntax::SyntaxKind",
+        r"
+use syntax::{SyntaxKind::*};",
+        r"
+use syntax::{SyntaxKind::{*, self}};",
+    )
+}
+
 #[test]
 fn merge_glob_nested() {
     check_crate(
@@ -931,5 +943,5 @@ fn check_merge_only_fail(ra_fixture0: &str, ra_fixture1: &str, mb: MergeBehavior
 fn check_guess(ra_fixture: &str, expected: ImportGranularityGuess) {
     let syntax = ast::SourceFile::parse(ra_fixture).tree().syntax().clone();
     let file = super::ImportScope::from(syntax).unwrap();
-    assert_eq!(file.guess_granularity_from_scope(), expected);
+    assert_eq!(super::guess_granularity_from_scope(&file), expected);
 }

crates/ide_db/src/helpers/merge_imports.rs

@@ -19,7 +19,6 @@ pub enum MergeBehavior {
 }
 
 impl MergeBehavior {
-    #[inline]
     fn is_tree_allowed(&self, tree: &ast::UseTree) -> bool {
         match self {
             MergeBehavior::Crate => true,
@@ -114,7 +113,7 @@ fn recursive_merge(
                 let rhs_path = rhs_path?;
                 let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?;
                 if lhs_prefix == lhs_path && rhs_prefix == rhs_path {
-                    let tree_is_self = |tree: ast::UseTree| {
+                    let tree_is_self = |tree: &ast::UseTree| {
                         tree.path().as_ref().map(path_is_self).unwrap_or(false)
                     };
                     // Check if only one of the two trees has a tree list, and
@@ -123,7 +122,7 @@ fn recursive_merge(
                     // the list is already included in the other one via `self`.
                     let tree_contains_self = |tree: &ast::UseTree| {
                         tree.use_tree_list()
-                            .map(|tree_list| tree_list.use_trees().any(tree_is_self))
+                            .map(|tree_list| tree_list.use_trees().any(|it| tree_is_self(&it)))
                             .unwrap_or(false)
                     };
                     match (tree_contains_self(lhs_t), tree_contains_self(&rhs_t)) {
@@ -141,17 +140,18 @@ fn recursive_merge(
                     // glob import of said module see the `merge_self_glob` or
                     // `merge_mod_into_glob` tests.
                     if lhs_t.star_token().is_some() || rhs_t.star_token().is_some() {
-                        *lhs_t = make::use_tree(
+                        if tree_is_self(lhs_t) || tree_is_self(&rhs_t) {
-                            make::path_unqualified(make::path_segment_self()),
+                            cov_mark::hit!(merge_self_glob);
-                            None,
+                            *lhs_t = make::use_tree(
-                            None,
+                                make::path_unqualified(make::path_segment_self()),
-                            false,
+                                None,
-                        );
+                                None,
-                        use_trees.insert(idx, make::use_tree_glob());
+                                false,
-                        continue;
+                            );
-                    }
+                            use_trees.insert(idx, make::use_tree_glob());
-
+                            continue;
-                    if lhs_t.use_tree_list().is_none() && rhs_t.use_tree_list().is_none() {
+                        }
+                    } else if lhs_t.use_tree_list().is_none() && rhs_t.use_tree_list().is_none() {
                         continue;
                     }
                 }