From 946b0ba02c2ab126b1b1d29027e60f21915d631e Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Tue, 8 Jan 2019 14:40:41 +0100 Subject: Fix name resolution across source roots It was using the wrong name in that case. --- crates/ra_hir/src/mock.rs | 47 +++++++++++++----- crates/ra_hir/src/nameres.rs | 48 ++++++++++++++++--- crates/ra_hir/src/nameres/tests.rs | 97 +++++++++++++++++++++++++++++++++++++- 3 files changed, 173 insertions(+), 19 deletions(-) diff --git a/crates/ra_hir/src/mock.rs b/crates/ra_hir/src/mock.rs index 8d176662c..c9af38009 100644 --- a/crates/ra_hir/src/mock.rs +++ b/crates/ra_hir/src/mock.rs @@ -15,6 +15,7 @@ pub(crate) struct MockDatabase { events: Mutex>>>, runtime: salsa::Runtime, id_maps: Arc, + 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) { 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) { 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 = - Some(db.add_file_with_position(&mut source_root, &entry.meta, &entry.text)); + position = Some(self.add_file_with_position( + 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) - .set(WORKSPACE, Arc::new(source_root.clone())); - (db, source_root, position) + self.query_mut(ra_db::SourceRootQuery) + .set(source_root_id, Arc::new(source_root.clone())); + (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, }) } } diff --git a/crates/ra_hir/src/nameres.rs b/crates/ra_hir/src/nameres.rs index 4181bf4b8..20adc9ec4 100644 --- a/crates/ra_hir/src/nameres.rs +++ b/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 { + 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, - // TODO: error - None => return Ok(true), // this can't suddenly resolve if we just resolve some other imports + None => { + // TODO: error + 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); diff --git a/crates/ra_hir/src/nameres/tests.rs b/crates/ra_hir/src/nameres/tests.rs index 4e3659ad0..c511c40b2 100644 --- a/crates/ra_hir/src/nameres/tests.rs +++ b/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( -- cgit v1.2.3