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 😄 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 <flodiebold@gmail.com>
Co-authored-by: Florian Diebold <florian.diebold@freiheit.com>

Cargo.lock

@@ -872,6 +872,7 @@ dependencies = [
  "ra_hir 0.1.0",
  "ra_syntax 0.1.0",
  "ra_text_edit 0.1.0",
+ "rustc-hash 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "test_utils 0.1.0",
 ]
 

crates/ra_assists/Cargo.toml

@@ -10,6 +10,7 @@ doctest = false
 [dependencies]
 format-buf = "1.0.0"
 join_to_string = "0.1.3"
+rustc-hash = "1.0"
 itertools = "0.8.0"
 
 ra_syntax = { path = "../ra_syntax" }

crates/ra_assists/src/assists/add_missing_impl_members.rs

@@ -1,12 +1,13 @@
-use std::collections::HashMap;
-
-use hir::{db::HirDatabase, HasSource};
+use hir::{db::HirDatabase, HasSource, InFile};
 use ra_syntax::{
     ast::{self, edit, make, AstNode, NameOwner},
     SmolStr,
 };
 
-use crate::{Assist, AssistCtx, AssistId};
+use crate::{
+    ast_transform::{self, AstTransform, QualifyPaths, SubstituteTypeParams},
+    Assist, AssistCtx, AssistId,
+};
 
 #[derive(PartialEq)]
 enum AddMissingImplMembersMode {
@@ -134,25 +135,23 @@ fn add_missing_impl_members_inner(
         return None;
     }
 
-    let file_id = ctx.frange.file_id;
     let db = ctx.db;
+    let file_id = ctx.frange.file_id;
+    let trait_file_id = trait_.source(db).file_id;
 
     ctx.add_assist(AssistId(assist_id), label, |edit| {
         let n_existing_items = impl_item_list.impl_items().count();
-        let substs = get_syntactic_substs(impl_node).unwrap_or_default();
-        let generic_def: hir::GenericDef = trait_.into();
-        let substs_by_param: HashMap<_, _> = generic_def
-            .params(db)
-            .into_iter()
-            // this is a trait impl, so we need to skip the first type parameter -- this is a bit hacky
-            .skip(1)
-            .zip(substs.into_iter())
-            .collect();
+        let module = hir::SourceAnalyzer::new(
+            db,
+            hir::InFile::new(file_id.into(), impl_node.syntax()),
+            None,
+        )
+        .module();
+        let ast_transform = QualifyPaths::new(db, module)
+            .or(SubstituteTypeParams::for_trait_impl(db, trait_, impl_node));
         let items = missing_items
             .into_iter()
-            .map(|it| {
-                substitute_type_params(db, hir::InFile::new(file_id.into(), it), &substs_by_param)
-            })
+            .map(|it| ast_transform::apply(&*ast_transform, InFile::new(trait_file_id, it)))
             .map(|it| match it {
                 ast::ImplItem::FnDef(def) => ast::ImplItem::FnDef(add_body(def)),
                 _ => it,
@@ -177,56 +176,6 @@ fn add_body(fn_def: ast::FnDef) -> ast::FnDef {
     }
 }
 
-// FIXME: It would probably be nicer if we could get this via HIR (i.e. get the
-// trait ref, and then go from the types in the substs back to the syntax)
-// FIXME: This should be a general utility (not even just for assists)
-fn get_syntactic_substs(impl_block: ast::ImplBlock) -> Option<Vec<ast::TypeRef>> {
-    let target_trait = impl_block.target_trait()?;
-    let path_type = match target_trait {
-        ast::TypeRef::PathType(path) => path,
-        _ => return None,
-    };
-    let type_arg_list = path_type.path()?.segment()?.type_arg_list()?;
-    let mut result = Vec::new();
-    for type_arg in type_arg_list.type_args() {
-        let type_arg: ast::TypeArg = type_arg;
-        result.push(type_arg.type_ref()?);
-    }
-    Some(result)
-}
-
-// FIXME: This should be a general utility (not even just for assists)
-fn substitute_type_params<N: AstNode>(
-    db: &impl HirDatabase,
-    node: hir::InFile<N>,
-    substs: &HashMap<hir::TypeParam, ast::TypeRef>,
-) -> N {
-    let type_param_replacements = node
-        .value
-        .syntax()
-        .descendants()
-        .filter_map(ast::TypeRef::cast)
-        .filter_map(|n| {
-            let path = match &n {
-                ast::TypeRef::PathType(path_type) => path_type.path()?,
-                _ => return None,
-            };
-            let analyzer = hir::SourceAnalyzer::new(db, node.with_value(n.syntax()), None);
-            let resolution = analyzer.resolve_path(db, &path)?;
-            match resolution {
-                hir::PathResolution::TypeParam(tp) => Some((n, substs.get(&tp)?.clone())),
-                _ => None,
-            }
-        })
-        .collect::<Vec<_>>();
-
-    if type_param_replacements.is_empty() {
-        node.value
-    } else {
-        edit::replace_descendants(&node.value, type_param_replacements.into_iter())
-    }
-}
-
 /// Given an `ast::ImplBlock`, resolves the target trait (the one being
 /// implemented) to a `ast::TraitDef`.
 fn resolve_target_trait_def(
@@ -400,6 +349,174 @@ impl Foo for S {
         )
     }
 
+    #[test]
+    fn test_qualify_path_1() {
+        check_assist(
+            add_missing_impl_members,
+            "
+mod foo {
+    pub struct Bar;
+    trait Foo { fn foo(&self, bar: Bar); }
+}
+struct S;
+impl foo::Foo for S { <|> }",
+            "
+mod foo {
+    pub struct Bar;
+    trait Foo { fn foo(&self, bar: Bar); }
+}
+struct S;
+impl foo::Foo for S {
+    <|>fn foo(&self, bar: foo::Bar) { unimplemented!() }
+}",
+        );
+    }
+
+    #[test]
+    fn test_qualify_path_generic() {
+        check_assist(
+            add_missing_impl_members,
+            "
+mod foo {
+    pub struct Bar<T>;
+    trait Foo { fn foo(&self, bar: Bar<u32>); }
+}
+struct S;
+impl foo::Foo for S { <|> }",
+            "
+mod foo {
+    pub struct Bar<T>;
+    trait Foo { fn foo(&self, bar: Bar<u32>); }
+}
+struct S;
+impl foo::Foo for S {
+    <|>fn foo(&self, bar: foo::Bar<u32>) { unimplemented!() }
+}",
+        );
+    }
+
+    #[test]
+    fn test_qualify_path_and_substitute_param() {
+        check_assist(
+            add_missing_impl_members,
+            "
+mod foo {
+    pub struct Bar<T>;
+    trait Foo<T> { fn foo(&self, bar: Bar<T>); }
+}
+struct S;
+impl foo::Foo<u32> for S { <|> }",
+            "
+mod foo {
+    pub struct Bar<T>;
+    trait Foo<T> { fn foo(&self, bar: Bar<T>); }
+}
+struct S;
+impl foo::Foo<u32> for S {
+    <|>fn foo(&self, bar: foo::Bar<u32>) { unimplemented!() }
+}",
+        );
+    }
+
+    #[test]
+    fn test_substitute_param_no_qualify() {
+        // when substituting params, the substituted param should not be qualified!
+        check_assist(
+            add_missing_impl_members,
+            "
+mod foo {
+    trait Foo<T> { fn foo(&self, bar: T); }
+    pub struct Param;
+}
+struct Param;
+struct S;
+impl foo::Foo<Param> for S { <|> }",
+            "
+mod foo {
+    trait Foo<T> { fn foo(&self, bar: T); }
+    pub struct Param;
+}
+struct Param;
+struct S;
+impl foo::Foo<Param> for S {
+    <|>fn foo(&self, bar: Param) { unimplemented!() }
+}",
+        );
+    }
+
+    #[test]
+    fn test_qualify_path_associated_item() {
+        check_assist(
+            add_missing_impl_members,
+            "
+mod foo {
+    pub struct Bar<T>;
+    impl Bar<T> { type Assoc = u32; }
+    trait Foo { fn foo(&self, bar: Bar<u32>::Assoc); }
+}
+struct S;
+impl foo::Foo for S { <|> }",
+            "
+mod foo {
+    pub struct Bar<T>;
+    impl Bar<T> { type Assoc = u32; }
+    trait Foo { fn foo(&self, bar: Bar<u32>::Assoc); }
+}
+struct S;
+impl foo::Foo for S {
+    <|>fn foo(&self, bar: foo::Bar<u32>::Assoc) { unimplemented!() }
+}",
+        );
+    }
+
+    #[test]
+    fn test_qualify_path_nested() {
+        check_assist(
+            add_missing_impl_members,
+            "
+mod foo {
+    pub struct Bar<T>;
+    pub struct Baz;
+    trait Foo { fn foo(&self, bar: Bar<Baz>); }
+}
+struct S;
+impl foo::Foo for S { <|> }",
+            "
+mod foo {
+    pub struct Bar<T>;
+    pub struct Baz;
+    trait Foo { fn foo(&self, bar: Bar<Baz>); }
+}
+struct S;
+impl foo::Foo for S {
+    <|>fn foo(&self, bar: foo::Bar<foo::Baz>) { unimplemented!() }
+}",
+        );
+    }
+
+    #[test]
+    fn test_qualify_path_fn_trait_notation() {
+        check_assist(
+            add_missing_impl_members,
+            "
+mod foo {
+    pub trait Fn<Args> { type Output; }
+    trait Foo { fn foo(&self, bar: dyn Fn(u32) -> i32); }
+}
+struct S;
+impl foo::Foo for S { <|> }",
+            "
+mod foo {
+    pub trait Fn<Args> { type Output; }
+    trait Foo { fn foo(&self, bar: dyn Fn(u32) -> i32); }
+}
+struct S;
+impl foo::Foo for S {
+    <|>fn foo(&self, bar: dyn Fn(u32) -> i32) { unimplemented!() }
+}",
+        );
+    }
+
     #[test]
     fn test_empty_trait() {
         check_assist_not_applicable(

crates/ra_assists/src/ast_transform.rs

@@ -0,0 +1,179 @@
+//! `AstTransformer`s are functions that replace nodes in an AST and can be easily combined.
+use rustc_hash::FxHashMap;
+
+use hir::{db::HirDatabase, InFile, PathResolution};
+use ra_syntax::ast::{self, make, AstNode};
+
+pub trait AstTransform<'a> {
+    fn get_substitution(
+        &self,
+        node: InFile<&ra_syntax::SyntaxNode>,
+    ) -> Option<ra_syntax::SyntaxNode>;
+
+    fn chain_before(self, other: Box<dyn AstTransform<'a> + 'a>) -> Box<dyn AstTransform<'a> + 'a>;
+    fn or<T: AstTransform<'a> + 'a>(self, other: T) -> Box<dyn AstTransform<'a> + 'a>
+    where
+        Self: Sized + 'a,
+    {
+        self.chain_before(Box::new(other))
+    }
+}
+
+struct NullTransformer;
+
+impl<'a> AstTransform<'a> for NullTransformer {
+    fn get_substitution(
+        &self,
+        _node: InFile<&ra_syntax::SyntaxNode>,
+    ) -> Option<ra_syntax::SyntaxNode> {
+        None
+    }
+    fn chain_before(self, other: Box<dyn AstTransform<'a> + 'a>) -> Box<dyn AstTransform<'a> + 'a> {
+        other
+    }
+}
+
+pub struct SubstituteTypeParams<'a, DB: HirDatabase> {
+    db: &'a DB,
+    substs: FxHashMap<hir::TypeParam, ast::TypeRef>,
+    previous: Box<dyn AstTransform<'a> + 'a>,
+}
+
+impl<'a, DB: HirDatabase> SubstituteTypeParams<'a, DB> {
+    pub fn for_trait_impl(
+        db: &'a DB,
+        trait_: hir::Trait,
+        impl_block: ast::ImplBlock,
+    ) -> SubstituteTypeParams<'a, DB> {
+        let substs = get_syntactic_substs(impl_block).unwrap_or_default();
+        let generic_def: hir::GenericDef = trait_.into();
+        let substs_by_param: FxHashMap<_, _> = generic_def
+            .params(db)
+            .into_iter()
+            // this is a trait impl, so we need to skip the first type parameter -- this is a bit hacky
+            .skip(1)
+            .zip(substs.into_iter())
+            .collect();
+        return SubstituteTypeParams {
+            db,
+            substs: substs_by_param,
+            previous: Box::new(NullTransformer),
+        };
+
+        // FIXME: It would probably be nicer if we could get this via HIR (i.e. get the
+        // trait ref, and then go from the types in the substs back to the syntax)
+        fn get_syntactic_substs(impl_block: ast::ImplBlock) -> Option<Vec<ast::TypeRef>> {
+            let target_trait = impl_block.target_trait()?;
+            let path_type = match target_trait {
+                ast::TypeRef::PathType(path) => path,
+                _ => return None,
+            };
+            let type_arg_list = path_type.path()?.segment()?.type_arg_list()?;
+            let mut result = Vec::new();
+            for type_arg in type_arg_list.type_args() {
+                let type_arg: ast::TypeArg = type_arg;
+                result.push(type_arg.type_ref()?);
+            }
+            Some(result)
+        }
+    }
+    fn get_substitution_inner(
+        &self,
+        node: InFile<&ra_syntax::SyntaxNode>,
+    ) -> Option<ra_syntax::SyntaxNode> {
+        let type_ref = ast::TypeRef::cast(node.value.clone())?;
+        let path = match &type_ref {
+            ast::TypeRef::PathType(path_type) => path_type.path()?,
+            _ => return None,
+        };
+        let analyzer = hir::SourceAnalyzer::new(self.db, node, None);
+        let resolution = analyzer.resolve_path(self.db, &path)?;
+        match resolution {
+            hir::PathResolution::TypeParam(tp) => Some(self.substs.get(&tp)?.syntax().clone()),
+            _ => None,
+        }
+    }
+}
+
+impl<'a, DB: HirDatabase> AstTransform<'a> for SubstituteTypeParams<'a, DB> {
+    fn get_substitution(
+        &self,
+        node: InFile<&ra_syntax::SyntaxNode>,
+    ) -> Option<ra_syntax::SyntaxNode> {
+        self.get_substitution_inner(node).or_else(|| self.previous.get_substitution(node))
+    }
+    fn chain_before(self, other: Box<dyn AstTransform<'a> + 'a>) -> Box<dyn AstTransform<'a> + 'a> {
+        Box::new(SubstituteTypeParams { previous: other, ..self })
+    }
+}
+
+pub struct QualifyPaths<'a, DB: HirDatabase> {
+    db: &'a DB,
+    from: Option<hir::Module>,
+    previous: Box<dyn AstTransform<'a> + 'a>,
+}
+
+impl<'a, DB: HirDatabase> QualifyPaths<'a, DB> {
+    pub fn new(db: &'a DB, from: Option<hir::Module>) -> Self {
+        Self { db, from, previous: Box::new(NullTransformer) }
+    }
+
+    fn get_substitution_inner(
+        &self,
+        node: InFile<&ra_syntax::SyntaxNode>,
+    ) -> Option<ra_syntax::SyntaxNode> {
+        // FIXME handle value ns?
+        let from = self.from?;
+        let p = ast::Path::cast(node.value.clone())?;
+        if p.segment().and_then(|s| s.param_list()).is_some() {
+            // don't try to qualify `Fn(Foo) -> Bar` paths, they are in prelude anyway
+            return None;
+        }
+        let analyzer = hir::SourceAnalyzer::new(self.db, node, None);
+        let resolution = analyzer.resolve_path(self.db, &p)?;
+        match resolution {
+            PathResolution::Def(def) => {
+                let found_path = from.find_use_path(self.db, def)?;
+                let args = p
+                    .segment()
+                    .and_then(|s| s.type_arg_list())
+                    .map(|arg_list| apply(self, node.with_value(arg_list)));
+                Some(make::path_with_type_arg_list(path_to_ast(found_path), args).syntax().clone())
+            }
+            PathResolution::Local(_)
+            | PathResolution::TypeParam(_)
+            | PathResolution::SelfType(_) => None,
+            PathResolution::Macro(_) => None,
+            PathResolution::AssocItem(_) => None,
+        }
+    }
+}
+
+pub fn apply<'a, N: AstNode>(transformer: &dyn AstTransform<'a>, node: InFile<N>) -> N {
+    let syntax = node.value.syntax();
+    let result = ra_syntax::algo::replace_descendants(syntax, &|element| match element {
+        ra_syntax::SyntaxElement::Node(n) => {
+            let replacement = transformer.get_substitution(node.with_value(&n))?;
+            Some(replacement.into())
+        }
+        _ => None,
+    });
+    N::cast(result).unwrap()
+}
+
+impl<'a, DB: HirDatabase> AstTransform<'a> for QualifyPaths<'a, DB> {
+    fn get_substitution(
+        &self,
+        node: InFile<&ra_syntax::SyntaxNode>,
+    ) -> Option<ra_syntax::SyntaxNode> {
+        self.get_substitution_inner(node).or_else(|| self.previous.get_substitution(node))
+    }
+    fn chain_before(self, other: Box<dyn AstTransform<'a> + 'a>) -> Box<dyn AstTransform<'a> + 'a> {
+        Box::new(QualifyPaths { previous: other, ..self })
+    }
+}
+
+fn path_to_ast(path: hir::ModPath) -> ast::Path {
+    let parse = ast::SourceFile::parse(&path.to_string());
+    parse.tree().syntax().descendants().find_map(ast::Path::cast).unwrap()
+}

crates/ra_assists/src/lib.rs

@@ -11,6 +11,7 @@ mod marks;
 mod doc_tests;
 #[cfg(test)]
 mod test_db;
+pub mod ast_transform;
 
 use hir::db::HirDatabase;
 use ra_db::FileRange;

crates/ra_hir/src/code_model.rs

@@ -227,6 +227,21 @@ impl Module {
     pub(crate) fn with_module_id(self, module_id: LocalModuleId) -> Module {
         Module::new(self.krate(), module_id)
     }
+
+    /// Finds a path that can be used to refer to the given item from within
+    /// this module, if possible.
+    pub fn find_use_path(
+        self,
+        db: &impl DefDatabase,
+        item: ModuleDef,
+    ) -> Option<hir_def::path::ModPath> {
+        // FIXME expose namespace choice
+        hir_def::find_path::find_path(
+            db,
+            hir_def::item_scope::ItemInNs::Types(item.into()),
+            self.into(),
+        )
+    }
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]

crates/ra_hir/src/from_id.rs

@@ -91,6 +91,22 @@ impl From<ModuleDefId> for ModuleDef {
     }
 }
 
+impl From<ModuleDef> for ModuleDefId {
+    fn from(id: ModuleDef) -> Self {
+        match id {
+            ModuleDef::Module(it) => ModuleDefId::ModuleId(it.into()),
+            ModuleDef::Function(it) => ModuleDefId::FunctionId(it.into()),
+            ModuleDef::Adt(it) => ModuleDefId::AdtId(it.into()),
+            ModuleDef::EnumVariant(it) => ModuleDefId::EnumVariantId(it.into()),
+            ModuleDef::Const(it) => ModuleDefId::ConstId(it.into()),
+            ModuleDef::Static(it) => ModuleDefId::StaticId(it.into()),
+            ModuleDef::Trait(it) => ModuleDefId::TraitId(it.into()),
+            ModuleDef::TypeAlias(it) => ModuleDefId::TypeAliasId(it.into()),
+            ModuleDef::BuiltinType(it) => ModuleDefId::BuiltinType(it),
+        }
+    }
+}
+
 impl From<DefWithBody> for DefWithBodyId {
     fn from(def: DefWithBody) -> Self {
         match def {

crates/ra_hir/src/source_binder.rs

@@ -205,6 +205,10 @@ impl SourceAnalyzer {
         }
     }
 
+    pub fn module(&self) -> Option<crate::code_model::Module> {
+        Some(crate::code_model::Module { id: self.resolver.module()? })
+    }
+
     fn expr_id(&self, expr: &ast::Expr) -> Option<ExprId> {
         let src = InFile { file_id: self.file_id, value: expr };
         self.body_source_map.as_ref()?.node_expr(src)

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");
+    }
+}

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,
+        }
+    }
+}

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;

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]

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>(

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();

crates/ra_hir_expand/src/lib.rs

@@ -322,3 +322,13 @@ impl InFile<SyntaxNode> {
         })
     }
 }
+
+impl<N: AstNode> InFile<N> {
+    pub fn descendants<T: AstNode>(self) -> impl Iterator<Item = InFile<T>> {
+        self.value.syntax().descendants().filter_map(T::cast).map(move |n| self.with_value(n))
+    }
+
+    pub fn syntax(&self) -> InFile<&SyntaxNode> {
+        self.with_value(self.value.syntax())
+    }
+}

crates/ra_ide/src/expand_macro.rs

@@ -7,8 +7,7 @@ use rustc_hash::FxHashMap;
 
 use ra_syntax::{
     algo::{find_node_at_offset, replace_descendants},
-    ast::{self},
-    AstNode, NodeOrToken, SyntaxKind, SyntaxNode, WalkEvent, T,
+    ast, AstNode, NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode, WalkEvent, T,
 };
 
 pub struct ExpandedMacro {
@@ -43,7 +42,7 @@ fn expand_macro_recur(
     let mut expanded: SyntaxNode = db.parse_or_expand(macro_file_id)?;
 
     let children = expanded.descendants().filter_map(ast::MacroCall::cast);
-    let mut replaces = FxHashMap::default();
+    let mut replaces: FxHashMap<SyntaxElement, SyntaxElement> = FxHashMap::default();
 
     for child in children.into_iter() {
         let node = hir::InFile::new(macro_file_id, &child);
@@ -59,7 +58,7 @@ fn expand_macro_recur(
         }
     }
 
-    Some(replace_descendants(&expanded, &replaces))
+    Some(replace_descendants(&expanded, &|n| replaces.get(n).cloned()))
 }
 
 // FIXME: It would also be cool to share logic here and in the mbe tests,

crates/ra_syntax/src/algo.rs

@@ -184,17 +184,17 @@ pub fn replace_children(
 /// to create a type-safe abstraction on top of it instead.
 pub fn replace_descendants(
     parent: &SyntaxNode,
-    map: &FxHashMap<SyntaxElement, SyntaxElement>,
+    map: &impl Fn(&SyntaxElement) -> Option<SyntaxElement>,
 ) -> SyntaxNode {
     //  FIXME: this could be made much faster.
     let new_children = parent.children_with_tokens().map(|it| go(map, it)).collect::<Vec<_>>();
     return with_children(parent, new_children);
 
     fn go(
-        map: &FxHashMap<SyntaxElement, SyntaxElement>,
+        map: &impl Fn(&SyntaxElement) -> Option<SyntaxElement>,
         element: SyntaxElement,
     ) -> NodeOrToken<rowan::GreenNode, rowan::GreenToken> {
-        if let Some(replacement) = map.get(&element) {
+        if let Some(replacement) = map(&element) {
             return match replacement {
                 NodeOrToken::Node(it) => NodeOrToken::Node(it.green().clone()),
                 NodeOrToken::Token(it) => NodeOrToken::Token(it.green().clone()),

crates/ra_syntax/src/ast/edit.rs

@@ -236,8 +236,8 @@ pub fn replace_descendants<N: AstNode, D: AstNode>(
 ) -> N {
     let map = replacement_map
         .map(|(from, to)| (from.syntax().clone().into(), to.syntax().clone().into()))
-        .collect::<FxHashMap<_, _>>();
-    let new_syntax = algo::replace_descendants(parent.syntax(), &map);
+        .collect::<FxHashMap<SyntaxElement, _>>();
+    let new_syntax = algo::replace_descendants(parent.syntax(), &|n| map.get(n).cloned());
     N::cast(new_syntax).unwrap()
 }
 
@@ -292,7 +292,7 @@ impl IndentLevel {
                 )
             })
             .collect();
-        algo::replace_descendants(&node, &replacements)
+        algo::replace_descendants(&node, &|n| replacements.get(n).cloned())
     }
 
     pub fn decrease_indent<N: AstNode>(self, node: N) -> N {
@@ -320,7 +320,7 @@ impl IndentLevel {
                 )
             })
             .collect();
-        algo::replace_descendants(&node, &replacements)
+        algo::replace_descendants(&node, &|n| replacements.get(n).cloned())
     }
 }
 

crates/ra_syntax/src/ast/make.rs

@@ -2,7 +2,7 @@
 //! of smaller pieces.
 use itertools::Itertools;
 
-use crate::{ast, AstNode, SourceFile};
+use crate::{algo, ast, AstNode, SourceFile};
 
 pub fn name(text: &str) -> ast::Name {
     ast_from_text(&format!("mod {};", text))
@@ -21,6 +21,20 @@ pub fn path_qualified(qual: ast::Path, name_ref: ast::NameRef) -> ast::Path {
 fn path_from_text(text: &str) -> ast::Path {
     ast_from_text(text)
 }
+pub fn path_with_type_arg_list(path: ast::Path, args: Option<ast::TypeArgList>) -> ast::Path {
+    if let Some(args) = args {
+        let syntax = path.syntax();
+        // FIXME: remove existing type args
+        let new_syntax = algo::insert_children(
+            syntax,
+            crate::algo::InsertPosition::Last,
+            &mut Some(args).into_iter().map(|n| n.syntax().clone().into()),
+        );
+        ast::Path::cast(new_syntax).unwrap()
+    } else {
+        path
+    }
+}
 
 pub fn record_field(name: ast::NameRef, expr: Option<ast::Expr>) -> ast::RecordField {
     return match expr {