Fix name resolution across source roots

It was using the wrong name in that case.

crates/ra_hir/src/mock.rs

@@ -15,6 +15,7 @@ pub(crate) struct MockDatabase {
     events: Mutex<Option<Vec<salsa::Event<MockDatabase>>>>,
     runtime: salsa::Runtime<MockDatabase>,
     id_maps: Arc<IdMaps>,
+    file_counter: u32,
 }
 
 impl MockDatabase {
@@ -27,7 +28,7 @@ impl MockDatabase {
     pub(crate) fn with_single_file(text: &str) -> (MockDatabase, SourceRoot, FileId) {
         let mut db = MockDatabase::default();
         let mut source_root = SourceRoot::default();
-        let file_id = db.add_file(&mut source_root, "/main.rs", text);
+        let file_id = db.add_file(WORKSPACE, &mut source_root, "/main.rs", text);
         db.query_mut(ra_db::SourceRootQuery)
             .set(WORKSPACE, Arc::new(source_root.clone()));
 
@@ -51,6 +52,16 @@ impl MockDatabase {
     fn from_fixture(fixture: &str) -> (MockDatabase, SourceRoot, Option<FilePosition>) {
         let mut db = MockDatabase::default();
 
+        let (source_root, pos) = db.add_fixture(WORKSPACE, fixture);
+
+        (db, source_root, pos)
+    }
+
+    pub fn add_fixture(
+        &mut self,
+        source_root_id: SourceRootId,
+        fixture: &str,
+    ) -> (SourceRoot, Option<FilePosition>) {
         let mut position = None;
         let mut source_root = SourceRoot::default();
         for entry in parse_fixture(fixture) {
@@ -59,39 +70,51 @@ impl MockDatabase {
                     position.is_none(),
                     "only one marker (<|>) per fixture is allowed"
                 );
-                position =
+                position = Some(self.add_file_with_position(
-                    Some(db.add_file_with_position(&mut source_root, &entry.meta, &entry.text));
+                    source_root_id,
+                    &mut source_root,
+                    &entry.meta,
+                    &entry.text,
+                ));
             } else {
-                db.add_file(&mut source_root, &entry.meta, &entry.text);
+                self.add_file(source_root_id, &mut source_root, &entry.meta, &entry.text);
             }
         }
-        db.query_mut(ra_db::SourceRootQuery)
+        self.query_mut(ra_db::SourceRootQuery)
-            .set(WORKSPACE, Arc::new(source_root.clone()));
+            .set(source_root_id, Arc::new(source_root.clone()));
-        (db, source_root, position)
+        (source_root, position)
     }
 
-    fn add_file(&mut self, source_root: &mut SourceRoot, path: &str, text: &str) -> FileId {
+    fn add_file(
+        &mut self,
+        source_root_id: SourceRootId,
+        source_root: &mut SourceRoot,
+        path: &str,
+        text: &str,
+    ) -> FileId {
         assert!(path.starts_with('/'));
         let path = RelativePathBuf::from_path(&path[1..]).unwrap();
-        let file_id = FileId(source_root.files.len() as u32);
+        let file_id = FileId(self.file_counter);
+        self.file_counter += 1;
         let text = Arc::new(text.to_string());
         self.query_mut(ra_db::FileTextQuery).set(file_id, text);
         self.query_mut(ra_db::FileRelativePathQuery)
             .set(file_id, path.clone());
         self.query_mut(ra_db::FileSourceRootQuery)
-            .set(file_id, WORKSPACE);
+            .set(file_id, source_root_id);
         source_root.files.insert(path, file_id);
         file_id
     }
 
     fn add_file_with_position(
         &mut self,
+        source_root_id: SourceRootId,
         source_root: &mut SourceRoot,
         path: &str,
         text: &str,
     ) -> FilePosition {
         let (offset, text) = extract_offset(text);
-        let file_id = self.add_file(source_root, path, &text);
+        let file_id = self.add_file(source_root_id, source_root, path, &text);
         FilePosition { file_id, offset }
     }
 }
@@ -121,6 +144,7 @@ impl Default for MockDatabase {
             events: Default::default(),
             runtime: salsa::Runtime::default(),
             id_maps: Default::default(),
+            file_counter: 0,
         };
         db.query_mut(ra_db::CrateGraphQuery)
             .set((), Default::default());
@@ -138,6 +162,7 @@ impl salsa::ParallelDatabase for MockDatabase {
             events: Default::default(),
             runtime: self.runtime.snapshot(self),
             id_maps: self.id_maps.clone(),
+            file_counter: self.file_counter,
         })
     }
 }

crates/ra_hir/src/nameres.rs

@@ -433,6 +433,7 @@ where
                 continue;
             }
             if self.resolve_import(module_id, import)? {
+                log::debug!("import {:?} resolved (or definite error)", import);
                 self.processed_imports.insert((module_id, i));
             }
         }
@@ -440,6 +441,7 @@ where
     }
 
     fn resolve_import(&mut self, module_id: ModuleId, import: &Import) -> Cancelable<bool> {
+        log::debug!("resolving import: {:?}", import);
         let ptr = match import.kind {
             ImportKind::Glob => return Ok(false),
             ImportKind::Named(ptr) => ptr,
@@ -450,8 +452,11 @@ where
             PathKind::Super => {
                 match module_id.parent(&self.module_tree) {
                     Some(it) => it,
+                    None => {
                         // TODO: error
-                    None => return Ok(true), // this can't suddenly resolve if we just resolve some other imports
+                        log::debug!("super path in root module");
+                        return Ok(true); // this can't suddenly resolve if we just resolve some other imports
+                    }
                 }
             }
             PathKind::Crate => module_id.crate_root(&self.module_tree),
@@ -462,13 +467,20 @@ where
 
             let def_id = match self.result.per_module[&curr].items.get(name) {
                 Some(res) if !res.def_id.is_none() => res.def_id,
-                _ => return Ok(false),
+                _ => {
+                    log::debug!("path segment {:?} not found", name);
+                    return Ok(false);
+                }
             };
 
             if !is_last {
                 let type_def_id = if let Some(d) = def_id.take(Namespace::Types) {
                     d
                 } else {
+                    log::debug!(
+                        "path segment {:?} resolved to value only, but is not last",
+                        name
+                    );
                     return Ok(false);
                 };
                 curr = match type_def_id.loc(self.db) {
@@ -486,27 +498,49 @@ where
                                 segments: import.path.segments[i + 1..].iter().cloned().collect(),
                                 kind: PathKind::Crate,
                             };
+                            log::debug!("resolving {:?} in other source root", path);
                             let def_id = module.resolve_path(self.db, &path)?;
                             if !def_id.is_none() {
+                                let name = path.segments.last().unwrap();
                                 self.update(module_id, |items| {
                                     let res = Resolution {
-                                        def_id: def_id,
+                                        def_id,
                                         import: Some(ptr),
                                     };
                                     items.items.insert(name.clone(), res);
                                 });
+                                log::debug!(
+                                    "resolved import {:?} ({:?}) cross-source root to {:?}",
+                                    name,
+                                    import,
+                                    def_id.map(|did| did.loc(self.db))
+                                );
                                 return Ok(true);
                             } else {
-                                return Ok(false);
+                                log::debug!("rest of path did not resolve in other source root");
+                                return Ok(true);
                             }
                         }
                     }
-                    _ => return Ok(true), // this resolved to a non-module, so the path won't ever resolve
+                    _ => {
+                        log::debug!(
+                            "path segment {:?} resolved to non-module {:?}, but is not last",
+                            name,
+                            type_def_id.loc(self.db)
+                        );
+                        return Ok(true); // this resolved to a non-module, so the path won't ever resolve
+                    }
                 }
             } else {
+                log::debug!(
+                    "resolved import {:?} ({:?}) within source root to {:?}",
+                    name,
+                    import,
+                    def_id.map(|did| did.loc(self.db))
+                );
                 self.update(module_id, |items| {
                     let res = Resolution {
-                        def_id: def_id,
+                        def_id,
                         import: Some(ptr),
                     };
                     items.items.insert(name.clone(), res);

crates/ra_hir/src/nameres/tests.rs

@@ -1,7 +1,7 @@
 use std::sync::Arc;
 
 use salsa::Database;
-use ra_db::{FilesDatabase, CrateGraph};
+use ra_db::{FilesDatabase, CrateGraph, SourceRootId};
 use relative_path::RelativePath;
 use test_utils::assert_eq_text;
 
@@ -227,6 +227,101 @@ fn item_map_across_crates() {
     );
 }
 
+#[test]
+fn import_across_source_roots() {
+    let (mut db, sr) = MockDatabase::with_files(
+        "
+        //- /lib.rs
+        pub mod a {
+            pub mod b {
+                pub struct C;
+            }
+        }
+    ",
+    );
+    let lib_id = sr.files[RelativePath::new("/lib.rs")];
+
+    let source_root = SourceRootId(1);
+
+    let (sr2, pos) = db.add_fixture(
+        source_root,
+        "
+        //- /main.rs
+        use test_crate::a::b::C;
+    ",
+    );
+    assert!(pos.is_none());
+
+    let main_id = sr2.files[RelativePath::new("/main.rs")];
+
+    eprintln!("lib = {:?}, main = {:?}", lib_id, main_id);
+
+    let mut crate_graph = CrateGraph::default();
+    let main_crate = crate_graph.add_crate_root(main_id);
+    let lib_crate = crate_graph.add_crate_root(lib_id);
+    crate_graph.add_dep(main_crate, "test_crate".into(), lib_crate);
+
+    db.set_crate_graph(crate_graph);
+
+    let module = crate::source_binder::module_from_file_id(&db, main_id)
+        .unwrap()
+        .unwrap();
+    let module_id = module.def_id.loc(&db).module_id;
+    let item_map = db.item_map(source_root).unwrap();
+
+    check_module_item_map(
+        &item_map,
+        module_id,
+        "
+            C: t v
+            test_crate: t
+        ",
+    );
+}
+
+#[test]
+fn reexport_across_crates() {
+    let (mut db, sr) = MockDatabase::with_files(
+        "
+        //- /main.rs
+        use test_crate::Baz;
+
+        //- /lib.rs
+        pub use foo::Baz;
+
+        mod foo;
+
+        //- /foo.rs
+        pub struct Baz;
+    ",
+    );
+    let main_id = sr.files[RelativePath::new("/main.rs")];
+    let lib_id = sr.files[RelativePath::new("/lib.rs")];
+
+    let mut crate_graph = CrateGraph::default();
+    let main_crate = crate_graph.add_crate_root(main_id);
+    let lib_crate = crate_graph.add_crate_root(lib_id);
+    crate_graph.add_dep(main_crate, "test_crate".into(), lib_crate);
+
+    db.set_crate_graph(crate_graph);
+
+    let source_root = db.file_source_root(main_id);
+    let module = crate::source_binder::module_from_file_id(&db, main_id)
+        .unwrap()
+        .unwrap();
+    let module_id = module.def_id.loc(&db).module_id;
+    let item_map = db.item_map(source_root).unwrap();
+
+    check_module_item_map(
+        &item_map,
+        module_id,
+        "
+            Baz: t v
+            test_crate: t
+        ",
+    );
+}
+
 #[test]
 fn typing_inside_a_function_should_not_invalidate_item_map() {
     let (mut db, pos) = MockDatabase::with_position(