Merge #2727

2727: Qualify paths in 'add impl members' r=flodiebold a=flodiebold

This makes the 'add impl members' assist qualify paths, so that they should resolve to the same thing as in the definition. To do that, it adds an algorithm that finds a path to refer to any item from any module (if possible), which is actually probably the more important part of this PR :smile: It handles visibility, reexports, renamed crates, prelude etc.; I think the only thing that's missing is support for local items. I'm not sure about the performance, since it takes into account every location where the target item has been `pub use`d, and then recursively goes up the module tree; there's probably potential for optimization by memoizing more, but I think the general shape of the algorithm is necessary to handle every case in Rust's module system.

~The 'find path' part is actually pretty complete, I think; I'm still working on the assist (hence the failing tests).~

Fixes #1943.

Co-authored-by: Florian Diebold <[email protected]>
Co-authored-by: Florian Diebold <[email protected]>

6 files changed, 559 insertions, 6 deletions

diff --git a/crates/ra_hir_def/src/find_path.rs b/crates/ra_hir_def/src/find_path.rs
new file mode 100644
index 000000000..f7dc8acb7
--- /dev/null
+++ b/crates/ra_hir_def/src/find_path.rs
@@ -0,0 +1,455 @@
+//! An algorithm to find a path to refer to a certain item.
+
+use crate::{
+    db::DefDatabase,
+    item_scope::ItemInNs,
+    path::{ModPath, PathKind},
+    visibility::Visibility,
+    CrateId, ModuleDefId, ModuleId,
+};
+use hir_expand::name::Name;
+
+const MAX_PATH_LEN: usize = 15;
+
+// FIXME: handle local items
+
+/// Find a path that can be used to refer to a certain item. This can depend on
+/// *from where* you're referring to the item, hence the `from` parameter.
+pub fn find_path(db: &impl DefDatabase, item: ItemInNs, from: ModuleId) -> Option<ModPath> {
+    find_path_inner(db, item, from, MAX_PATH_LEN)
+}
+
+fn find_path_inner(
+    db: &impl DefDatabase,
+    item: ItemInNs,
+    from: ModuleId,
+    max_len: usize,
+) -> Option<ModPath> {
+    if max_len == 0 {
+        return None;
+    }
+
+    // Base cases:
+
+    // - 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_simple_segments(PathKind::Plain, vec![name.clone()]));
+    }
+
+    // - 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_simple_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_simple_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_simple_segments(PathKind::Super(1), Vec::new()));
+        }
+    }
+
+    // - 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_simple_segments(PathKind::Plain, vec![name.clone()]));
+        }
+    }
+
+    // - if the item is in the prelude, return the name from there
+    if let Some(prelude_module) = def_map.prelude {
+        let prelude_def_map = db.crate_def_map(prelude_module.krate);
+        let prelude_scope: &crate::item_scope::ItemScope =
+            &prelude_def_map.modules[prelude_module.local_id].scope;
+        if let Some((name, vis)) = prelude_scope.name_of(item) {
+            if vis.is_visible_from(db, from) {
+                return Some(ModPath::from_simple_segments(PathKind::Plain, vec![name.clone()]));
+            }
+        }
+    }
+
+    // Recursive case:
+    // - if the item is an enum variant, refer to it via the enum
+    if let Some(ModuleDefId::EnumVariantId(variant)) = item.as_module_def_id() {
+        if let Some(mut path) = find_path(db, ItemInNs::Types(variant.parent.into()), from) {
+            let data = db.enum_data(variant.parent);
+            path.segments.push(data.variants[variant.local_id].name.clone());
+            return Some(path);
+        }
+        // If this doesn't work, it seems we have no way of referring to the
+        // enum; that's very weird, but there might still be a reexport of the
+        // variant somewhere
+    }
+
+    // - otherwise, look for modules containing (reexporting) it and import it from one of those
+    let importable_locations = find_importable_locations(db, item, from);
+    let mut best_path = None;
+    let mut best_path_len = max_len;
+    for (module_id, name) in importable_locations {
+        let mut path = match find_path_inner(
+            db,
+            ItemInNs::Types(ModuleDefId::ModuleId(module_id)),
+            from,
+            best_path_len - 1,
+        ) {
+            None => continue,
+            Some(path) => path,
+        };
+        path.segments.push(name);
+        if path_len(&path) < best_path_len {
+            best_path_len = path_len(&path);
+            best_path = Some(path);
+        }
+    }
+    best_path
+}
+
+fn path_len(path: &ModPath) -> usize {
+    path.segments.len()
+        + match path.kind {
+            PathKind::Plain => 0,
+            PathKind::Super(i) => i as usize,
+            PathKind::Crate => 1,
+            PathKind::Abs => 0,
+            PathKind::DollarCrate(_) => 1,
+        }
+}
+
+fn find_importable_locations(
+    db: &impl DefDatabase,
+    item: ItemInNs,
+    from: ModuleId,
+) -> Vec<(ModuleId, Name)> {
+    let crate_graph = db.crate_graph();
+    let mut result = Vec::new();
+    // We only look in the crate from which we are importing, and the direct
+    // dependencies. We cannot refer to names from transitive dependencies
+    // directly (only through reexports in direct dependencies).
+    for krate in Some(from.krate)
+        .into_iter()
+        .chain(crate_graph.dependencies(from.krate).map(|dep| dep.crate_id))
+    {
+        result.extend(
+            importable_locations_in_crate(db, item, krate)
+                .iter()
+                .filter(|(_, _, vis)| vis.is_visible_from(db, from))
+                .map(|(m, n, _)| (*m, n.clone())),
+        );
+    }
+    result
+}
+
+/// Collects all locations from which we might import the item in a particular
+/// crate. These include the original definition of the item, and any
+/// non-private `use`s.
+///
+/// Note that the crate doesn't need to be the one in which the item is defined;
+/// it might be re-exported in other crates.
+fn importable_locations_in_crate(
+    db: &impl DefDatabase,
+    item: ItemInNs,
+    krate: CrateId,
+) -> Vec<(ModuleId, Name, Visibility)> {
+    let def_map = db.crate_def_map(krate);
+    let mut result = Vec::new();
+    for (local_id, data) in def_map.modules.iter() {
+        if let Some((name, vis)) = data.scope.name_of(item) {
+            let is_private = if let Visibility::Module(private_to) = vis {
+                private_to.local_id == local_id
+            } else {
+                false
+            };
+            let is_original_def = if let Some(module_def_id) = item.as_module_def_id() {
+                data.scope.declarations().any(|it| it == module_def_id)
+            } else {
+                false
+            };
+            if is_private && !is_original_def {
+                // Ignore private imports. these could be used if we are
+                // in a submodule of this module, but that's usually not
+                // what the user wants; and if this module can import
+                // the item and we're a submodule of it, so can we.
+                // Also this keeps the cached data smaller.
+                continue;
+            }
+            result.push((ModuleId { krate, local_id }, name.clone(), vis));
+        }
+    }
+    result
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::test_db::TestDB;
+    use hir_expand::hygiene::Hygiene;
+    use ra_db::fixture::WithFixture;
+    use ra_syntax::ast::AstNode;
+
+    /// `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(code: &str, path: &str) {
+        let (db, pos) = TestDB::with_position(code);
+        let module = db.module_for_file(pos.file_id);
+        let parsed_path_file = ra_syntax::SourceFile::parse(&format!("use {};", path));
+        let ast_path = parsed_path_file
+            .syntax_node()
+            .descendants()
+            .find_map(ra_syntax::ast::Path::cast)
+            .unwrap();
+        let mod_path = ModPath::from_src(ast_path, &Hygiene::new_unhygienic()).unwrap();
+
+        let crate_def_map = db.crate_def_map(module.krate);
+        let resolved = crate_def_map
+            .resolve_path(
+                &db,
+                module.local_id,
+                &mod_path,
+                crate::item_scope::BuiltinShadowMode::Module,
+            )
+            .0
+            .take_types()
+            .unwrap();
+
+        let found_path = find_path(&db, ItemInNs::Types(resolved), module);
+
+        assert_eq!(found_path, Some(mod_path));
+    }
+
+    #[test]
+    fn same_module() {
+        let code = r#"
+            //- /main.rs
+            struct S;
+            <|>
+        "#;
+        check_found_path(code, "S");
+    }
+
+    #[test]
+    fn enum_variant() {
+        let code = r#"
+            //- /main.rs
+            enum E { A }
+            <|>
+        "#;
+        check_found_path(code, "E::A");
+    }
+
+    #[test]
+    fn sub_module() {
+        let code = r#"
+            //- /main.rs
+            mod foo {
+                pub struct S;
+            }
+            <|>
+        "#;
+        check_found_path(code, "foo::S");
+    }
+
+    #[test]
+    fn super_module() {
+        let code = r#"
+            //- /main.rs
+            mod foo;
+            //- /foo.rs
+            mod bar;
+            struct S;
+            //- /foo/bar.rs
+            <|>
+        "#;
+        check_found_path(code, "super::S");
+    }
+
+    #[test]
+    fn self_module() {
+        let code = r#"
+            //- /main.rs
+            mod foo;
+            //- /foo.rs
+            <|>
+        "#;
+        check_found_path(code, "self");
+    }
+
+    #[test]
+    fn crate_root() {
+        let code = r#"
+            //- /main.rs
+            mod foo;
+            //- /foo.rs
+            <|>
+        "#;
+        check_found_path(code, "crate");
+    }
+
+    #[test]
+    fn same_crate() {
+        let code = r#"
+            //- /main.rs
+            mod foo;
+            struct S;
+            //- /foo.rs
+            <|>
+        "#;
+        check_found_path(code, "crate::S");
+    }
+
+    #[test]
+    fn different_crate() {
+        let code = r#"
+            //- /main.rs crate:main deps:std
+            <|>
+            //- /std.rs crate:std
+            pub struct S;
+        "#;
+        check_found_path(code, "std::S");
+    }
+
+    #[test]
+    fn different_crate_renamed() {
+        let code = r#"
+            //- /main.rs crate:main deps:std
+            extern crate std as std_renamed;
+            <|>
+            //- /std.rs crate:std
+            pub struct S;
+        "#;
+        check_found_path(code, "std_renamed::S");
+    }
+
+    #[test]
+    fn same_crate_reexport() {
+        let code = r#"
+            //- /main.rs
+            mod bar {
+                mod foo { pub(super) struct S; }
+                pub(crate) use foo::*;
+            }
+            <|>
+        "#;
+        check_found_path(code, "bar::S");
+    }
+
+    #[test]
+    fn same_crate_reexport_rename() {
+        let code = r#"
+            //- /main.rs
+            mod bar {
+                mod foo { pub(super) struct S; }
+                pub(crate) use foo::S as U;
+            }
+            <|>
+        "#;
+        check_found_path(code, "bar::U");
+    }
+
+    #[test]
+    fn different_crate_reexport() {
+        let code = r#"
+            //- /main.rs crate:main deps:std
+            <|>
+            //- /std.rs crate:std deps:core
+            pub use core::S;
+            //- /core.rs crate:core
+            pub struct S;
+        "#;
+        check_found_path(code, "std::S");
+    }
+
+    #[test]
+    fn prelude() {
+        let code = r#"
+            //- /main.rs crate:main deps:std
+            <|>
+            //- /std.rs crate:std
+            pub mod prelude { pub struct S; }
+            #[prelude_import]
+            pub use prelude::*;
+        "#;
+        check_found_path(code, "S");
+    }
+
+    #[test]
+    fn enum_variant_from_prelude() {
+        let code = r#"
+            //- /main.rs crate:main deps:std
+            <|>
+            //- /std.rs crate:std
+            pub mod prelude {
+                pub enum Option<T> { Some(T), None }
+                pub use Option::*;
+            }
+            #[prelude_import]
+            pub use prelude::*;
+        "#;
+        check_found_path(code, "None");
+        check_found_path(code, "Some");
+    }
+
+    #[test]
+    fn shortest_path() {
+        let code = r#"
+            //- /main.rs
+            pub mod foo;
+            pub mod baz;
+            struct S;
+            <|>
+            //- /foo.rs
+            pub mod bar { pub struct S; }
+            //- /baz.rs
+            pub use crate::foo::bar::S;
+        "#;
+        check_found_path(code, "baz::S");
+    }
+
+    #[test]
+    fn discount_private_imports() {
+        let code = r#"
+            //- /main.rs
+            mod foo;
+            pub mod bar { pub struct S; }
+            use bar::S;
+            //- /foo.rs
+            <|>
+        "#;
+        // crate::S would be shorter, but using private imports seems wrong
+        check_found_path(code, "crate::bar::S");
+    }
+
+    #[test]
+    fn import_cycle() {
+        let code = r#"
+            //- /main.rs
+            pub mod foo;
+            pub mod bar;
+            pub mod baz;
+            //- /bar.rs
+            <|>
+            //- /foo.rs
+            pub use super::baz;
+            pub struct S;
+            //- /baz.rs
+            pub use super::foo;
+        "#;
+        check_found_path(code, "crate::foo::S");
+    }
+}
diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs
index fe7bb9779..d74a1cef2 100644
--- a/crates/ra_hir_def/src/item_scope.rs
+++ b/crates/ra_hir_def/src/item_scope.rs
@@ -104,6 +104,15 @@ impl ItemScope {
         }
     }
 
+    pub(crate) fn name_of(&self, item: ItemInNs) -> Option<(&Name, Visibility)> {
+        for (name, per_ns) in &self.visible {
+            if let Some(vis) = item.match_with(*per_ns) {
+                return Some((name, vis));
+            }
+        }
+        None
+    }
+
     pub(crate) fn traits<'a>(&'a self) -> impl Iterator<Item = TraitId> + 'a {
         self.visible.values().filter_map(|def| match def.take_types() {
             Some(ModuleDefId::TraitId(t)) => Some(t),
@@ -173,3 +182,33 @@ impl PerNs {
         }
     }
 }
+
+#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
+pub enum ItemInNs {
+    Types(ModuleDefId),
+    Values(ModuleDefId),
+    Macros(MacroDefId),
+}
+
+impl ItemInNs {
+    fn match_with(self, per_ns: PerNs) -> Option<Visibility> {
+        match self {
+            ItemInNs::Types(def) => {
+                per_ns.types.filter(|(other_def, _)| *other_def == def).map(|(_, vis)| vis)
+            }
+            ItemInNs::Values(def) => {
+                per_ns.values.filter(|(other_def, _)| *other_def == def).map(|(_, vis)| vis)
+            }
+            ItemInNs::Macros(def) => {
+                per_ns.macros.filter(|(other_def, _)| *other_def == def).map(|(_, vis)| vis)
+            }
+        }
+    }
+
+    pub fn as_module_def_id(self) -> Option<ModuleDefId> {
+        match self {
+            ItemInNs::Types(id) | ItemInNs::Values(id) => Some(id),
+            ItemInNs::Macros(_) => None,
+        }
+    }
+}
diff --git a/crates/ra_hir_def/src/lib.rs b/crates/ra_hir_def/src/lib.rs
index 61f044ecf..ebc12e891 100644
--- a/crates/ra_hir_def/src/lib.rs
+++ b/crates/ra_hir_def/src/lib.rs
@@ -37,6 +37,7 @@ pub mod src;
 pub mod child_by_source;
 
 pub mod visibility;
+pub mod find_path;
 
 #[cfg(test)]
 mod test_db;
diff --git a/crates/ra_hir_def/src/path.rs b/crates/ra_hir_def/src/path.rs
index 82cfa67a9..9f93a5424 100644
--- a/crates/ra_hir_def/src/path.rs
+++ b/crates/ra_hir_def/src/path.rs
@@ -1,7 +1,11 @@
 //! A desugared representation of paths like `crate::foo` or `<Type as Trait>::bar`.
 mod lower;
 
-use std::{iter, sync::Arc};
+use std::{
+    fmt::{self, Display},
+    iter,
+    sync::Arc,
+};
 
 use hir_expand::{
     hygiene::Hygiene,
@@ -248,6 +252,42 @@ impl From<Name> for ModPath {
     }
 }
 
+impl Display for ModPath {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut first_segment = true;
+        let mut add_segment = |s| {
+            if !first_segment {
+                f.write_str("::")?;
+            }
+            first_segment = false;
+            f.write_str(s)?;
+            Ok(())
+        };
+        match self.kind {
+            PathKind::Plain => {}
+            PathKind::Super(n) => {
+                if n == 0 {
+                    add_segment("self")?;
+                }
+                for _ in 0..n {
+                    add_segment("super")?;
+                }
+            }
+            PathKind::Crate => add_segment("crate")?,
+            PathKind::Abs => add_segment("")?,
+            PathKind::DollarCrate(_) => add_segment("$crate")?,
+        }
+        for segment in &self.segments {
+            if !first_segment {
+                f.write_str("::")?;
+            }
+            first_segment = false;
+            write!(f, "{}", segment)?;
+        }
+        Ok(())
+    }
+}
+
 pub use hir_expand::name as __name;
 
 #[macro_export]
diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs
index 5d16dd087..f7bac5801 100644
--- a/crates/ra_hir_def/src/resolver.rs
+++ b/crates/ra_hir_def/src/resolver.rs
@@ -128,7 +128,7 @@ impl Resolver {
         path: &ModPath,
         shadow: BuiltinShadowMode,
     ) -> PerNs {
-        let (item_map, module) = match self.module() {
+        let (item_map, module) = match self.module_scope() {
             Some(it) => it,
             None => return PerNs::none(),
         };
@@ -239,7 +239,7 @@ impl Resolver {
     ) -> Option<Visibility> {
         match visibility {
             RawVisibility::Module(_) => {
-                let (item_map, module) = match self.module() {
+                let (item_map, module) = match self.module_scope() {
                     Some(it) => it,
                     None => return None,
                 };
@@ -379,7 +379,7 @@ impl Resolver {
         db: &impl DefDatabase,
         path: &ModPath,
     ) -> Option<MacroDefId> {
-        let (item_map, module) = self.module()?;
+        let (item_map, module) = self.module_scope()?;
         item_map.resolve_path(db, module, &path, BuiltinShadowMode::Other).0.take_macros()
     }
 
@@ -403,7 +403,7 @@ impl Resolver {
         traits
     }
 
-    fn module(&self) -> Option<(&CrateDefMap, LocalModuleId)> {
+    fn module_scope(&self) -> Option<(&CrateDefMap, LocalModuleId)> {
         self.scopes.iter().rev().find_map(|scope| match scope {
             Scope::ModuleScope(m) => Some((&*m.crate_def_map, m.module_id)),
 
@@ -411,8 +411,13 @@ impl Resolver {
         })
     }
 
+    pub fn module(&self) -> Option<ModuleId> {
+        let (def_map, local_id) = self.module_scope()?;
+        Some(ModuleId { krate: def_map.krate, local_id })
+    }
+
     pub fn krate(&self) -> Option<CrateId> {
-        self.module().map(|t| t.0.krate)
+        self.module_scope().map(|t| t.0.krate)
     }
 
     pub fn where_predicates_in_scope<'a>(
diff --git a/crates/ra_hir_def/src/test_db.rs b/crates/ra_hir_def/src/test_db.rs
index 54e3a84bd..1568820e9 100644
--- a/crates/ra_hir_def/src/test_db.rs
+++ b/crates/ra_hir_def/src/test_db.rs
@@ -5,6 +5,7 @@ use std::{
     sync::{Arc, Mutex},
 };
 
+use crate::db::DefDatabase;
 use ra_db::{salsa, CrateId, FileId, FileLoader, FileLoaderDelegate, RelativePath};
 
 #[salsa::database(
@@ -54,6 +55,18 @@ impl FileLoader for TestDB {
 }
 
 impl TestDB {
+    pub fn module_for_file(&self, file_id: FileId) -> crate::ModuleId {
+        for &krate in self.relevant_crates(file_id).iter() {
+            let crate_def_map = self.crate_def_map(krate);
+            for (local_id, data) in crate_def_map.modules.iter() {
+                if data.origin.file_id() == Some(file_id) {
+                    return crate::ModuleId { krate, local_id };
+                }
+            }
+        }
+        panic!("Can't find module for file")
+    }
+
     pub fn log(&self, f: impl FnOnce()) -> Vec<salsa::Event<TestDB>> {
         *self.events.lock().unwrap() = Some(Vec::new());
         f();