Skip to content

Commit 2f07976

Browse files
bors[bot]flodiebold
bors[bot]
and
flodiebold
committed
460: Name resolution fixes r=flodiebold a=flodiebold Found two problems: - use tree desugaring lost the prefix if the path had just one segment (e.g. in `use foo::{bar, baz}`) - when resolving imports across source roots, it actually used the name of the segment from the other source root... so e.g. in `use ra_syntax::foo` it'd map `ra_syntax` to the import instead of `foo` 😄 Both of these are one-line fixes, most of this is making it possible to write tests with multiple source roots. I also left in debug logs for the name resolution, in case it turns out there's still more to fix ;) Co-authored-by: Florian Diebold <[email protected]>
2 parents 562b448 + 946b0ba commit 2f07976

File tree

4 files changed

+203
-20
lines changed

4 files changed

+203
-20
lines changed

‎crates/ra_hir/src/mock.rs

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ pub(crate) struct MockDatabase {
1515
events: Mutex<Option<Vec<salsa::Event<MockDatabase>>>>,
1616
runtime: salsa::Runtime<MockDatabase>,
1717
id_maps: Arc<IdMaps>,
18+
file_counter: u32,
1819
}
1920

2021
impl MockDatabase {
@@ -27,7 +28,7 @@ impl MockDatabase {
2728
pub(crate) fn with_single_file(text: &str) -> (MockDatabase, SourceRoot, FileId) {
2829
let mut db = MockDatabase::default();
2930
let mut source_root = SourceRoot::default();
30-
let file_id = db.add_file(&mut source_root, "/main.rs", text);
31+
let file_id = db.add_file(WORKSPACE, &mut source_root, "/main.rs", text);
3132
db.query_mut(ra_db::SourceRootQuery)
3233
.set(WORKSPACE, Arc::new(source_root.clone()));
3334

@@ -51,6 +52,16 @@ impl MockDatabase {
5152
fn from_fixture(fixture: &str) -> (MockDatabase, SourceRoot, Option<FilePosition>) {
5253
let mut db = MockDatabase::default();
5354

55+
let (source_root, pos) = db.add_fixture(WORKSPACE, fixture);
56+
57+
(db, source_root, pos)
58+
}
59+
60+
pub fn add_fixture(
61+
&mut self,
62+
source_root_id: SourceRootId,
63+
fixture: &str,
64+
) -> (SourceRoot, Option<FilePosition>) {
5465
let mut position = None;
5566
let mut source_root = SourceRoot::default();
5667
for entry in parse_fixture(fixture) {
@@ -59,39 +70,51 @@ impl MockDatabase {
5970
position.is_none(),
6071
"only one marker (<|>) per fixture is allowed"
6172
);
62-
position =
63-
Some(db.add_file_with_position(&mut source_root, &entry.meta, &entry.text));
73+
position = Some(self.add_file_with_position(
74+
source_root_id,
75+
&mut source_root,
76+
&entry.meta,
77+
&entry.text,
78+
));
6479
} else {
65-
db.add_file(&mut source_root, &entry.meta, &entry.text);
80+
self.add_file(source_root_id, &mut source_root, &entry.meta, &entry.text);
6681
}
6782
}
68-
db.query_mut(ra_db::SourceRootQuery)
69-
.set(WORKSPACE, Arc::new(source_root.clone()));
70-
(db, source_root, position)
83+
self.query_mut(ra_db::SourceRootQuery)
84+
.set(source_root_id, Arc::new(source_root.clone()));
85+
(source_root, position)
7186
}
7287

73-
fn add_file(&mut self, source_root: &mut SourceRoot, path: &str, text: &str) -> FileId {
88+
fn add_file(
89+
&mut self,
90+
source_root_id: SourceRootId,
91+
source_root: &mut SourceRoot,
92+
path: &str,
93+
text: &str,
94+
) -> FileId {
7495
assert!(path.starts_with('/'));
7596
let path = RelativePathBuf::from_path(&path[1..]).unwrap();
76-
let file_id = FileId(source_root.files.len() as u32);
97+
let file_id = FileId(self.file_counter);
98+
self.file_counter += 1;
7799
let text = Arc::new(text.to_string());
78100
self.query_mut(ra_db::FileTextQuery).set(file_id, text);
79101
self.query_mut(ra_db::FileRelativePathQuery)
80102
.set(file_id, path.clone());
81103
self.query_mut(ra_db::FileSourceRootQuery)
82-
.set(file_id, WORKSPACE);
104+
.set(file_id, source_root_id);
83105
source_root.files.insert(path, file_id);
84106
file_id
85107
}
86108

87109
fn add_file_with_position(
88110
&mut self,
111+
source_root_id: SourceRootId,
89112
source_root: &mut SourceRoot,
90113
path: &str,
91114
text: &str,
92115
) -> FilePosition {
93116
let (offset, text) = extract_offset(text);
94-
let file_id = self.add_file(source_root, path, &text);
117+
let file_id = self.add_file(source_root_id, source_root, path, &text);
95118
FilePosition { file_id, offset }
96119
}
97120
}
@@ -121,6 +144,7 @@ impl Default for MockDatabase {
121144
events: Default::default(),
122145
runtime: salsa::Runtime::default(),
123146
id_maps: Default::default(),
147+
file_counter: 0,
124148
};
125149
db.query_mut(ra_db::CrateGraphQuery)
126150
.set((), Default::default());
@@ -138,6 +162,7 @@ impl salsa::ParallelDatabase for MockDatabase {
138162
events: Default::default(),
139163
runtime: self.runtime.snapshot(self),
140164
id_maps: self.id_maps.clone(),
165+
file_counter: self.file_counter,
141166
})
142167
}
143168
}

‎crates/ra_hir/src/nameres.rs

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -433,13 +433,15 @@ where
433433
continue;
434434
}
435435
if self.resolve_import(module_id, import)? {
436+
log::debug!("import {:?} resolved (or definite error)", import);
436437
self.processed_imports.insert((module_id, i));
437438
}
438439
}
439440
Ok(())
440441
}
441442

442443
fn resolve_import(&mut self, module_id: ModuleId, import: &Import) -> Cancelable<bool> {
444+
log::debug!("resolving import: {:?}", import);
443445
let ptr = match import.kind {
444446
ImportKind::Glob => return Ok(false),
445447
ImportKind::Named(ptr) => ptr,
@@ -450,8 +452,11 @@ where
450452
PathKind::Super => {
451453
match module_id.parent(&self.module_tree) {
452454
Some(it) => it,
453-
// TODO: error
454-
None => return Ok(true), // this can't suddenly resolve if we just resolve some other imports
455+
None => {
456+
// TODO: error
457+
log::debug!("super path in root module");
458+
return Ok(true); // this can't suddenly resolve if we just resolve some other imports
459+
}
455460
}
456461
}
457462
PathKind::Crate => module_id.crate_root(&self.module_tree),
@@ -462,13 +467,20 @@ where
462467

463468
let def_id = match self.result.per_module[&curr].items.get(name) {
464469
Some(res) if !res.def_id.is_none() => res.def_id,
465-
_ => return Ok(false),
470+
_ => {
471+
log::debug!("path segment {:?} not found", name);
472+
return Ok(false);
473+
}
466474
};
467475

468476
if !is_last {
469477
let type_def_id = if let Some(d) = def_id.take(Namespace::Types) {
470478
d
471479
} else {
480+
log::debug!(
481+
"path segment {:?} resolved to value only, but is not last",
482+
name
483+
);
472484
return Ok(false);
473485
};
474486
curr = match type_def_id.loc(self.db) {
@@ -486,27 +498,49 @@ where
486498
segments: import.path.segments[i + 1..].iter().cloned().collect(),
487499
kind: PathKind::Crate,
488500
};
501+
log::debug!("resolving {:?} in other source root", path);
489502
let def_id = module.resolve_path(self.db, &path)?;
490503
if !def_id.is_none() {
504+
let name = path.segments.last().unwrap();
491505
self.update(module_id, |items| {
492506
let res = Resolution {
493-
def_id: def_id,
507+
def_id,
494508
import: Some(ptr),
495509
};
496510
items.items.insert(name.clone(), res);
497511
});
512+
log::debug!(
513+
"resolved import {:?} ({:?}) cross-source root to {:?}",
514+
name,
515+
import,
516+
def_id.map(|did| did.loc(self.db))
517+
);
498518
return Ok(true);
499519
} else {
500-
return Ok(false);
520+
log::debug!("rest of path did not resolve in other source root");
521+
return Ok(true);
501522
}
502523
}
503524
}
504-
_ => return Ok(true), // this resolved to a non-module, so the path won't ever resolve
525+
_ => {
526+
log::debug!(
527+
"path segment {:?} resolved to non-module {:?}, but is not last",
528+
name,
529+
type_def_id.loc(self.db)
530+
);
531+
return Ok(true); // this resolved to a non-module, so the path won't ever resolve
532+
}
505533
}
506534
} else {
535+
log::debug!(
536+
"resolved import {:?} ({:?}) within source root to {:?}",
537+
name,
538+
import,
539+
def_id.map(|did| did.loc(self.db))
540+
);
507541
self.update(module_id, |items| {
508542
let res = Resolution {
509-
def_id: def_id,
543+
def_id,
510544
import: Some(ptr),
511545
};
512546
items.items.insert(name.clone(), res);

‎crates/ra_hir/src/nameres/tests.rs

Lines changed: 125 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use std::sync::Arc;
22

33
use salsa::Database;
4-
use ra_db::{FilesDatabase, CrateGraph};
4+
use ra_db::{FilesDatabase, CrateGraph, SourceRootId};
55
use relative_path::RelativePath;
66
use test_utils::assert_eq_text;
77

@@ -78,6 +78,35 @@ fn item_map_smoke_test() {
7878
);
7979
}
8080

81+
#[test]
82+
fn use_trees() {
83+
let (item_map, module_id) = item_map(
84+
"
85+
//- /lib.rs
86+
mod foo;
87+
88+
use crate::foo::bar::{Baz, Quux};
89+
<|>
90+
91+
//- /foo/mod.rs
92+
pub mod bar;
93+
94+
//- /foo/bar.rs
95+
pub struct Baz;
96+
pub enum Quux {};
97+
",
98+
);
99+
check_module_item_map(
100+
&item_map,
101+
module_id,
102+
"
103+
Baz: t v
104+
Quux: t
105+
foo: t
106+
",
107+
);
108+
}
109+
81110
#[test]
82111
fn re_exports() {
83112
let (item_map, module_id) = item_map(
@@ -198,6 +227,101 @@ fn item_map_across_crates() {
198227
);
199228
}
200229

230+
#[test]
231+
fn import_across_source_roots() {
232+
let (mut db, sr) = MockDatabase::with_files(
233+
"
234+
//- /lib.rs
235+
pub mod a {
236+
pub mod b {
237+
pub struct C;
238+
}
239+
}
240+
",
241+
);
242+
let lib_id = sr.files[RelativePath::new("/lib.rs")];
243+
244+
let source_root = SourceRootId(1);
245+
246+
let (sr2, pos) = db.add_fixture(
247+
source_root,
248+
"
249+
//- /main.rs
250+
use test_crate::a::b::C;
251+
",
252+
);
253+
assert!(pos.is_none());
254+
255+
let main_id = sr2.files[RelativePath::new("/main.rs")];
256+
257+
eprintln!("lib = {:?}, main = {:?}", lib_id, main_id);
258+
259+
let mut crate_graph = CrateGraph::default();
260+
let main_crate = crate_graph.add_crate_root(main_id);
261+
let lib_crate = crate_graph.add_crate_root(lib_id);
262+
crate_graph.add_dep(main_crate, "test_crate".into(), lib_crate);
263+
264+
db.set_crate_graph(crate_graph);
265+
266+
let module = crate::source_binder::module_from_file_id(&db, main_id)
267+
.unwrap()
268+
.unwrap();
269+
let module_id = module.def_id.loc(&db).module_id;
270+
let item_map = db.item_map(source_root).unwrap();
271+
272+
check_module_item_map(
273+
&item_map,
274+
module_id,
275+
"
276+
C: t v
277+
test_crate: t
278+
",
279+
);
280+
}
281+
282+
#[test]
283+
fn reexport_across_crates() {
284+
let (mut db, sr) = MockDatabase::with_files(
285+
"
286+
//- /main.rs
287+
use test_crate::Baz;
288+
289+
//- /lib.rs
290+
pub use foo::Baz;
291+
292+
mod foo;
293+
294+
//- /foo.rs
295+
pub struct Baz;
296+
",
297+
);
298+
let main_id = sr.files[RelativePath::new("/main.rs")];
299+
let lib_id = sr.files[RelativePath::new("/lib.rs")];
300+
301+
let mut crate_graph = CrateGraph::default();
302+
let main_crate = crate_graph.add_crate_root(main_id);
303+
let lib_crate = crate_graph.add_crate_root(lib_id);
304+
crate_graph.add_dep(main_crate, "test_crate".into(), lib_crate);
305+
306+
db.set_crate_graph(crate_graph);
307+
308+
let source_root = db.file_source_root(main_id);
309+
let module = crate::source_binder::module_from_file_id(&db, main_id)
310+
.unwrap()
311+
.unwrap();
312+
let module_id = module.def_id.loc(&db).module_id;
313+
let item_map = db.item_map(source_root).unwrap();
314+
315+
check_module_item_map(
316+
&item_map,
317+
module_id,
318+
"
319+
Baz: t v
320+
test_crate: t
321+
",
322+
);
323+
}
324+
201325
#[test]
202326
fn typing_inside_a_function_should_not_invalidate_item_map() {
203327
let (mut db, pos) = MockDatabase::with_position(

‎crates/ra_hir/src/path.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ fn convert_path(prefix: Option<Path>, path: &ast::Path) -> Option<Path> {
150150
let prefix = if let Some(qual) = path.qualifier() {
151151
Some(convert_path(prefix, qual)?)
152152
} else {
153-
None
153+
prefix
154154
};
155155
let segment = path.segment()?;
156156
let res = match segment.kind()? {

0 commit comments

Comments
 (0)