Merge #6073

6073: Dont unnecessarily unnest imports r=matklad a=Veykril

Fixes #6071

This has the side effect that paths that refer to items inside of the current module get prefixed with `self`. Changing this behavior is unfortunately not straightforward should it be unwanted, though I don't see a problem with this as prefixing imports like this with `self` is what I do personally anyways 😅. You can see what I mean with this in one of the tests which had to be changed in `crates/ssr/src/tests.rs`.

There is one test that i still have to look at though, ~~which I by accident pushed with `#[ignore]` on it~~, which is `different_crate_renamed`, for some reason this now doesn't use the crate alias. This also makes me believe that aliases in general will break with this. So maybe this is not as straight forwards as I'd hoped for, but I don't really know how aliases work here.

Edit: The failing test should work now

Co-authored-by: Lukas Wirth <[email protected]>

5 files changed, 203 insertions, 55 deletions

diff --git a/crates/assists/src/handlers/add_missing_impl_members.rs b/crates/assists/src/handlers/add_missing_impl_members.rs
index 8df1d786b..1ac5fefd6 100644
--- a/crates/assists/src/handlers/add_missing_impl_members.rs
+++ b/crates/assists/src/handlers/add_missing_impl_members.rs
@@ -415,6 +415,41 @@ impl foo::Foo for S {
     }
 
     #[test]
+    fn test_qualify_path_2() {
+        check_assist(
+            add_missing_impl_members,
+            r#"
+mod foo {
+    pub mod bar {
+        pub struct Bar;
+        pub trait Foo { fn foo(&self, bar: Bar); }
+    }
+}
+
+use foo::bar;
+
+struct S;
+impl bar::Foo for S { <|> }"#,
+            r#"
+mod foo {
+    pub mod bar {
+        pub struct Bar;
+        pub trait Foo { fn foo(&self, bar: Bar); }
+    }
+}
+
+use foo::bar;
+
+struct S;
+impl bar::Foo for S {
+    fn foo(&self, bar: bar::Bar) {
+        ${0:todo!()}
+    }
+}"#,
+        );
+    }
+
+    #[test]
     fn test_qualify_path_generic() {
         check_assist(
             add_missing_impl_members,
diff --git a/crates/assists/src/handlers/auto_import.rs b/crates/assists/src/handlers/auto_import.rs
index b5eb2c722..ee7277c04 100644
--- a/crates/assists/src/handlers/auto_import.rs
+++ b/crates/assists/src/handlers/auto_import.rs
@@ -196,10 +196,10 @@ impl AutoImportAssets {
             })
             .filter_map(|candidate| match candidate {
                 Either::Left(module_def) => {
-                    self.module_with_name_to_import.find_use_path(db, module_def)
+                    self.module_with_name_to_import.find_use_path_prefixed(db, module_def)
                 }
                 Either::Right(macro_def) => {
-                    self.module_with_name_to_import.find_use_path(db, macro_def)
+                    self.module_with_name_to_import.find_use_path_prefixed(db, macro_def)
                 }
             })
             .filter(|use_path| !use_path.segments.is_empty())
@@ -291,6 +291,35 @@ mod tests {
     use crate::tests::{check_assist, check_assist_not_applicable, check_assist_target};
 
     #[test]
+    fn applicable_when_found_an_import_partial() {
+        check_assist(
+            auto_import,
+            r"
+            mod std {
+                pub mod fmt {
+                    pub struct Formatter;
+                }
+            }
+
+            use std::fmt;
+
+            <|>Formatter
+            ",
+            r"
+            mod std {
+                pub mod fmt {
+                    pub struct Formatter;
+                }
+            }
+
+            use std::fmt::{self, Formatter};
+
+            Formatter
+            ",
+        );
+    }
+
+    #[test]
     fn applicable_when_found_an_import() {
         check_assist(
             auto_import,
diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs
index 09f4a2224..5719b06af 100644
--- a/crates/assists/src/utils/insert_use.rs
+++ b/crates/assists/src/utils/insert_use.rs
@@ -810,16 +810,6 @@ use std::io;",
     }
 
     #[test]
-    #[ignore] // FIXME: Support this
-    fn merge_partial_path() {
-        check_full(
-            "ast::Foo",
-            r"use syntax::{ast, algo};",
-            r"use syntax::{ast::{self, Foo}, algo};",
-        )
-    }
-
-    #[test]
     fn merge_glob_nested() {
         check_full(
             "foo::bar::quux::Fez",
diff --git a/crates/hir/src/code_model.rs b/crates/hir/src/code_model.rs
index a2a166e0a..567fd91af 100644
--- a/crates/hir/src/code_model.rs
+++ b/crates/hir/src/code_model.rs
@@ -383,6 +383,16 @@ impl Module {
     pub fn find_use_path(self, db: &dyn DefDatabase, item: impl Into<ItemInNs>) -> Option<ModPath> {
         hir_def::find_path::find_path(db, item.into(), self.into())
     }
+
+    /// Finds a path that can be used to refer to the given item from within
+    /// this module, if possible. This is used for returning import paths for use-statements.
+    pub fn find_use_path_prefixed(
+        self,
+        db: &dyn DefDatabase,
+        item: impl Into<ItemInNs>,
+    ) -> Option<ModPath> {
+        hir_def::find_path::find_path_prefixed(db, item.into(), self.into())
+    }
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
diff --git a/crates/hir_def/src/find_path.rs b/crates/hir_def/src/find_path.rs
index ac2c54ac5..baf374144 100644
--- a/crates/hir_def/src/find_path.rs
+++ b/crates/hir_def/src/find_path.rs
@@ -4,6 +4,7 @@ use hir_expand::name::{known, AsName, Name};
 use rustc_hash::FxHashSet;
 use test_utils::mark;
 
+use crate::nameres::CrateDefMap;
 use crate::{
     db::DefDatabase,
     item_scope::ItemInNs,
@@ -18,7 +19,12 @@ use crate::{
 /// *from where* you're referring to the item, hence the `from` parameter.
 pub fn find_path(db: &dyn DefDatabase, item: ItemInNs, from: ModuleId) -> Option<ModPath> {
     let _p = profile::span("find_path");
-    find_path_inner(db, item, from, MAX_PATH_LEN)
+    find_path_inner(db, item, from, MAX_PATH_LEN, Prefixed::Not)
+}
+
+pub fn find_path_prefixed(db: &dyn DefDatabase, item: ItemInNs, from: ModuleId) -> Option<ModPath> {
+    let _p = profile::span("find_path_absolute");
+    find_path_inner(db, item, from, MAX_PATH_LEN, Prefixed::Plain)
 }
 
 const MAX_PATH_LEN: usize = 15;
@@ -36,11 +42,67 @@ impl ModPath {
     }
 }
 
+fn check_crate_self_super(
+    def_map: &CrateDefMap,
+    item: ItemInNs,
+    from: ModuleId,
+) -> Option<ModPath> {
+    // - if the item is the crate root, return `crate`
+    if item
+        == ItemInNs::Types(ModuleDefId::ModuleId(ModuleId {
+            krate: from.krate,
+            local_id: def_map.root,
+        }))
+    {
+        Some(ModPath::from_segments(PathKind::Crate, Vec::new()))
+    } else if item == ItemInNs::Types(from.into()) {
+        // - if the item is the module we're in, use `self`
+        Some(ModPath::from_segments(PathKind::Super(0), Vec::new()))
+    } else {
+        if let Some(parent_id) = def_map.modules[from.local_id].parent {
+            // - if the item is the parent module, use `super` (this is not used recursively, since `super::super` is ugly)
+            if item
+                == ItemInNs::Types(ModuleDefId::ModuleId(ModuleId {
+                    krate: from.krate,
+                    local_id: parent_id,
+                }))
+            {
+                return Some(ModPath::from_segments(PathKind::Super(1), Vec::new()));
+            }
+        }
+        None
+    }
+}
+
+#[derive(Copy, Clone, PartialEq, Eq)]
+pub enum Prefixed {
+    Not,
+    BySelf,
+    Plain,
+}
+
+impl Prefixed {
+    #[inline]
+    fn prefix(self) -> Option<PathKind> {
+        match self {
+            Prefixed::Not => None,
+            Prefixed::BySelf => Some(PathKind::Super(0)),
+            Prefixed::Plain => Some(PathKind::Plain),
+        }
+    }
+
+    #[inline]
+    fn prefixed(self) -> bool {
+        self != Prefixed::Not
+    }
+}
+
 fn find_path_inner(
     db: &dyn DefDatabase,
     item: ItemInNs,
     from: ModuleId,
     max_len: usize,
+    prefixed: Prefixed,
 ) -> Option<ModPath> {
     if max_len == 0 {
         return None;
@@ -51,41 +113,22 @@ fn find_path_inner(
     // - if the item is already in scope, return the name under which it is
     let def_map = db.crate_def_map(from.krate);
     let from_scope: &crate::item_scope::ItemScope = &def_map.modules[from.local_id].scope;
-    if let Some((name, _)) = from_scope.name_of(item) {
-        return Some(ModPath::from_segments(PathKind::Plain, vec![name.clone()]));
+    let scope_name =
+        if let Some((name, _)) = from_scope.name_of(item) { Some(name.clone()) } else { None };
+    if !prefixed.prefixed() && scope_name.is_some() {
+        return scope_name
+            .map(|scope_name| ModPath::from_segments(PathKind::Plain, vec![scope_name]));
     }
 
-    // - if the item is the crate root, return `crate`
-    if item
-        == ItemInNs::Types(ModuleDefId::ModuleId(ModuleId {
-            krate: from.krate,
-            local_id: def_map.root,
-        }))
-    {
-        return Some(ModPath::from_segments(PathKind::Crate, Vec::new()));
-    }
-
-    // - if the item is the module we're in, use `self`
-    if item == ItemInNs::Types(from.into()) {
-        return Some(ModPath::from_segments(PathKind::Super(0), Vec::new()));
-    }
-
-    // - if the item is the parent module, use `super` (this is not used recursively, since `super::super` is ugly)
-    if let Some(parent_id) = def_map.modules[from.local_id].parent {
-        if item
-            == ItemInNs::Types(ModuleDefId::ModuleId(ModuleId {
-                krate: from.krate,
-                local_id: parent_id,
-            }))
-        {
-            return Some(ModPath::from_segments(PathKind::Super(1), Vec::new()));
-        }
+    if let modpath @ Some(_) = check_crate_self_super(&def_map, item, from) {
+        return modpath;
     }
 
     // - if the item is the crate root of a dependency crate, return the name from the extern prelude
     for (name, def_id) in &def_map.extern_prelude {
         if item == ItemInNs::Types(*def_id) {
-            return Some(ModPath::from_segments(PathKind::Plain, vec![name.clone()]));
+            let name = scope_name.unwrap_or_else(|| name.clone());
+            return Some(ModPath::from_segments(PathKind::Plain, vec![name]));
         }
     }
 
@@ -138,6 +181,7 @@ fn find_path_inner(
                 ItemInNs::Types(ModuleDefId::ModuleId(module_id)),
                 from,
                 best_path_len - 1,
+                prefixed,
             ) {
                 path.segments.push(name);
 
@@ -165,6 +209,7 @@ fn find_path_inner(
                     ItemInNs::Types(ModuleDefId::ModuleId(info.container)),
                     from,
                     best_path_len - 1,
+                    prefixed,
                 )?;
                 path.segments.push(info.path.segments.last().unwrap().clone());
                 Some(path)
@@ -181,7 +226,13 @@ fn find_path_inner(
         }
     }
 
-    best_path
+    if let Some(prefix) = prefixed.prefix() {
+        best_path.or_else(|| {
+            scope_name.map(|scope_name| ModPath::from_segments(prefix, vec![scope_name]))
+        })
+    } else {
+        best_path
+    }
 }
 
 fn select_best_path(old_path: ModPath, new_path: ModPath, prefer_no_std: bool) -> ModPath {
@@ -304,7 +355,7 @@ mod tests {
     /// `code` needs to contain a cursor marker; checks that `find_path` for the
     /// item the `path` refers to returns that same path when called from the
     /// module the cursor is in.
-    fn check_found_path(ra_fixture: &str, path: &str) {
+    fn check_found_path_(ra_fixture: &str, path: &str, absolute: bool) {
         let (db, pos) = TestDB::with_position(ra_fixture);
         let module = db.module_for_file(pos.file_id);
         let parsed_path_file = syntax::SourceFile::parse(&format!("use {};", path));
@@ -324,9 +375,20 @@ mod tests {
             .take_types()
             .unwrap();
 
-        let found_path = find_path(&db, ItemInNs::Types(resolved), module);
+        let found_path = if absolute { find_path_prefixed } else { find_path }(
+            &db,
+            ItemInNs::Types(resolved),
+            module,
+        );
+        assert_eq!(found_path, Some(mod_path), "absolute {}", absolute);
+    }
+
+    fn check_found_path(ra_fixture: &str, path: &str) {
+        check_found_path_(ra_fixture, path, false);
+    }
 
-        assert_eq!(found_path, Some(mod_path));
+    fn check_found_path_abs(ra_fixture: &str, path: &str) {
+        check_found_path_(ra_fixture, path, true);
     }
 
     #[test]
@@ -337,6 +399,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "S");
+        check_found_path_abs(code, "S");
     }
 
     #[test]
@@ -347,6 +410,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "E::A");
+        check_found_path_abs(code, "E::A");
     }
 
     #[test]
@@ -359,6 +423,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "foo::S");
+        check_found_path_abs(code, "foo::S");
     }
 
     #[test]
@@ -373,6 +438,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "super::S");
+        check_found_path_abs(code, "super::S");
     }
 
     #[test]
@@ -384,6 +450,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "self");
+        check_found_path_abs(code, "self");
     }
 
     #[test]
@@ -395,6 +462,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "crate");
+        check_found_path_abs(code, "crate");
     }
 
     #[test]
@@ -407,6 +475,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "crate::S");
+        check_found_path_abs(code, "crate::S");
     }
 
     #[test]
@@ -418,6 +487,7 @@ mod tests {
             pub struct S;
         "#;
         check_found_path(code, "std::S");
+        check_found_path_abs(code, "std::S");
     }
 
     #[test]
@@ -430,14 +500,14 @@ mod tests {
             pub struct S;
         "#;
         check_found_path(code, "std_renamed::S");
+        check_found_path_abs(code, "std_renamed::S");
     }
 
     #[test]
     fn partially_imported() {
         // Tests that short paths are used even for external items, when parts of the path are
         // already in scope.
-        check_found_path(
-            r#"
+        let code = r#"
             //- /main.rs crate:main deps:syntax
 
             use syntax::ast;
@@ -449,12 +519,11 @@ mod tests {
                     A, B, C,
                 }
             }
-        "#,
-            "ast::ModuleItem",
-        );
+        "#;
+        check_found_path(code, "ast::ModuleItem");
+        check_found_path_abs(code, "syntax::ast::ModuleItem");
 
-        check_found_path(
-            r#"
+        let code = r#"
             //- /main.rs crate:main deps:syntax
 
             <|>
@@ -465,9 +534,9 @@ mod tests {
                     A, B, C,
                 }
             }
-        "#,
-            "syntax::ast::ModuleItem",
-        );
+        "#;
+        check_found_path(code, "syntax::ast::ModuleItem");
+        check_found_path_abs(code, "syntax::ast::ModuleItem");
     }
 
     #[test]
@@ -481,6 +550,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "bar::S");
+        check_found_path_abs(code, "bar::S");
     }
 
     #[test]
@@ -494,6 +564,7 @@ mod tests {
             <|>
         "#;
         check_found_path(code, "bar::U");
+        check_found_path_abs(code, "bar::U");
     }
 
     #[test]
@@ -507,6 +578,7 @@ mod tests {
             pub struct S;
         "#;
         check_found_path(code, "std::S");
+        check_found_path_abs(code, "std::S");
     }
 
     #[test]
@@ -520,6 +592,7 @@ mod tests {
             pub use prelude::*;
         "#;
         check_found_path(code, "S");
+        check_found_path_abs(code, "S");
     }
 
     #[test]
@@ -537,6 +610,8 @@ mod tests {
         "#;
         check_found_path(code, "None");
         check_found_path(code, "Some");
+        check_found_path_abs(code, "None");
+        check_found_path_abs(code, "Some");
     }
 
     #[test]
@@ -553,6 +628,7 @@ mod tests {
             pub use crate::foo::bar::S;
         "#;
         check_found_path(code, "baz::S");
+        check_found_path_abs(code, "baz::S");
     }
 
     #[test]
@@ -567,6 +643,7 @@ mod tests {
         "#;
         // crate::S would be shorter, but using private imports seems wrong
         check_found_path(code, "crate::bar::S");
+        check_found_path_abs(code, "crate::bar::S");
     }
 
     #[test]
@@ -585,6 +662,7 @@ mod tests {
             pub use super::foo;
         "#;
         check_found_path(code, "crate::foo::S");
+        check_found_path_abs(code, "crate::foo::S");
     }
 
     #[test]
@@ -605,6 +683,7 @@ mod tests {
         }
         "#;
         check_found_path(code, "std::sync::Arc");
+        check_found_path_abs(code, "std::sync::Arc");
     }
 
     #[test]
@@ -629,6 +708,7 @@ mod tests {
         }
         "#;
         check_found_path(code, "core::fmt::Error");
+        check_found_path_abs(code, "core::fmt::Error");
     }
 
     #[test]
@@ -652,6 +732,7 @@ mod tests {
         }
         "#;
         check_found_path(code, "alloc::sync::Arc");
+        check_found_path_abs(code, "alloc::sync::Arc");
     }
 
     #[test]
@@ -669,6 +750,7 @@ mod tests {
         pub struct Arc;
         "#;
         check_found_path(code, "megaalloc::Arc");
+        check_found_path_abs(code, "megaalloc::Arc");
     }
 
     #[test]
@@ -683,5 +765,7 @@ mod tests {
         "#;
         check_found_path(code, "u8");
         check_found_path(code, "u16");
+        check_found_path_abs(code, "u8");
+        check_found_path_abs(code, "u16");
     }
 }