From 857c35ddb03ee5db97bbb4743dfeedeb3df350ec Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 25 Jan 2019 10:08:21 +0300 Subject: [PATCH] refactor import resolution extract path resolution use enums instead of bools --- crates/ra_hir/src/marks.rs | 7 +- crates/ra_hir/src/nameres.rs | 221 ++++++++++++++++------------- crates/ra_hir/src/nameres/tests.rs | 21 +++ crates/test_utils/src/marks.rs | 6 +- 4 files changed, 151 insertions(+), 104 deletions(-) diff --git a/crates/ra_hir/src/marks.rs b/crates/ra_hir/src/marks.rs index f4d0c3e596..338ed0516a 100644 --- a/crates/ra_hir/src/marks.rs +++ b/crates/ra_hir/src/marks.rs @@ -1,3 +1,4 @@ -use test_utils::mark; - -mark!(name_res_works_for_broken_modules); +test_utils::marks!( + name_res_works_for_broken_modules + item_map_enum_importing +); diff --git a/crates/ra_hir/src/nameres.rs b/crates/ra_hir/src/nameres.rs index a3bc989580..8c8494b460 100644 --- a/crates/ra_hir/src/nameres.rs +++ b/crates/ra_hir/src/nameres.rs @@ -19,6 +19,7 @@ pub(crate) mod lower; use std::sync::Arc; use ra_db::CrateId; +use test_utils::tested_by; use rustc_hash::{FxHashMap, FxHashSet}; use crate::{ @@ -273,7 +274,7 @@ where // already done continue; } - if self.resolve_import(module_id, import_id, import_data) { + if self.resolve_import(module_id, import_id, import_data) == ReachedFixedPoint::Yes { log::debug!("import {:?} resolved (or definite error)", import_id); self.processed_imports.insert((module_id, import_id)); } @@ -285,111 +286,36 @@ where module_id: ModuleId, import_id: ImportId, import: &ImportData, - ) -> bool { + ) -> ReachedFixedPoint { log::debug!("resolving import: {:?}", import); if import.is_glob { - return false; + return ReachedFixedPoint::Yes; }; - - let mut curr: ModuleId = match import.path.kind { - PathKind::Plain | PathKind::Self_ => module_id, - PathKind::Super => { - match module_id.parent(&self.module_tree) { - Some(it) => it, - None => { - // TODO: error - log::debug!("super path in root module"); - return true; // this can't suddenly resolve if we just resolve some other imports - } - } - } - PathKind::Crate => module_id.crate_root(&self.module_tree), - PathKind::Abs => { - // TODO: absolute use is not supported for now - return false; - } + let original_module = Module { + krate: self.krate, + module_id, }; + let (def_id, reached_fixedpoint) = + self.result + .resolve_path(self.db, original_module, &import.path); - for (i, segment) in import.path.segments.iter().enumerate() { - let is_last = i == import.path.segments.len() - 1; - - let def_id = match self.result.per_module[&curr].items.get(&segment.name) { - Some(res) if !res.def_id.is_none() => res.def_id, - _ => { - log::debug!("path segment {:?} not found", segment.name); - return 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", - segment.name - ); - return false; - }; - curr = match type_def_id { - ModuleDef::Module(module) => { - if module.krate == self.krate { - module.module_id - } else { - let path = Path { - 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 last_segment = path.segments.last().unwrap(); - self.update(module_id, |items| { - let res = Resolution { - def_id, - import: Some(import_id), - }; - items.items.insert(last_segment.name.clone(), res); - }); - log::debug!( - "resolved import {:?} ({:?}) cross-source root to {:?}", - last_segment.name, - import, - def_id, - ); - return true; - } else { - log::debug!("rest of path did not resolve in other source root"); - return true; - } - } - } - _ => { - log::debug!( - "path segment {:?} resolved to non-module {:?}, but is not last", - segment.name, - type_def_id, - ); - return true; // this resolved to a non-module, so the path won't ever resolve - } - } - } else { - log::debug!( - "resolved import {:?} ({:?}) within source root to {:?}", - segment.name, - import, + if reached_fixedpoint == ReachedFixedPoint::Yes { + let last_segment = import.path.segments.last().unwrap(); + self.update(module_id, |items| { + let res = Resolution { def_id, - ); - self.update(module_id, |items| { - let res = Resolution { - def_id, - import: Some(import_id), - }; - items.items.insert(segment.name.clone(), res); - }) - } + import: Some(import_id), + }; + items.items.insert(last_segment.name.clone(), res); + }); + log::debug!( + "resolved import {:?} ({:?}) cross-source root to {:?}", + last_segment.name, + import, + def_id, + ); } - true + reached_fixedpoint } fn update(&mut self, module_id: ModuleId, f: impl FnOnce(&mut ModuleScope)) { @@ -398,5 +324,102 @@ where } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ReachedFixedPoint { + Yes, + No, +} + +impl ItemMap { + // returns true if we are sure that additions to `ItemMap` wouldn't change + // the result. That is, if we've reached fixed point at this particular + // import. + fn resolve_path( + &self, + db: &impl HirDatabase, + original_module: Module, + path: &Path, + ) -> (PerNs, ReachedFixedPoint) { + let mut curr_per_ns: PerNs = PerNs::types(match path.kind { + PathKind::Crate => original_module.crate_root(db).into(), + PathKind::Self_ | PathKind::Plain => original_module.into(), + PathKind::Super => { + if let Some(p) = original_module.parent(db) { + p.into() + } else { + log::debug!("super path in root module"); + return (PerNs::none(), ReachedFixedPoint::Yes); + } + } + PathKind::Abs => { + // TODO: absolute use is not supported + return (PerNs::none(), ReachedFixedPoint::Yes); + } + }); + + for (i, segment) in path.segments.iter().enumerate() { + let curr = match curr_per_ns.as_ref().take_types() { + Some(r) => r, + None => { + // we still have path segments left, but the path so far + // didn't resolve in the types namespace => no resolution + // (don't break here because curr_per_ns might contain + // something in the value namespace, and it would be wrong + // to return that) + return (PerNs::none(), ReachedFixedPoint::No); + } + }; + // resolve segment in curr + + curr_per_ns = match curr { + ModuleDef::Module(module) => { + if module.krate != original_module.krate { + let path = Path { + segments: path.segments[i..].iter().cloned().collect(), + kind: PathKind::Crate, + }; + log::debug!("resolving {:?} in other crate", path); + let def_id = module.resolve_path(db, &path); + return (def_id, ReachedFixedPoint::Yes); + } + + match self.per_module[&module.module_id].items.get(&segment.name) { + Some(res) if !res.def_id.is_none() => res.def_id, + _ => { + log::debug!("path segment {:?} not found", segment.name); + return (PerNs::none(), ReachedFixedPoint::No); + } + } + } + ModuleDef::Enum(e) => { + // enum variant + tested_by!(item_map_enum_importing); + let matching_variant = e + .variants(db) + .into_iter() + .find(|(n, _variant)| n == &segment.name); + + match matching_variant { + Some((_n, variant)) => PerNs::both(variant.into(), (*e).into()), + None => PerNs::none(), + } + } + _ => { + // could be an inherent method call in UFCS form + // (`Struct::method`), or some other kind of associated + // item... Which we currently don't handle (TODO) + log::debug!( + "path segment {:?} resolved to non-module {:?}, but is not last", + segment.name, + curr, + ); + return (PerNs::none(), ReachedFixedPoint::Yes); + } + }; + } + (curr_per_ns, ReachedFixedPoint::Yes) + } +} + #[cfg(test)] mod tests; diff --git a/crates/ra_hir/src/nameres/tests.rs b/crates/ra_hir/src/nameres/tests.rs index 9322bf08ce..430d16a2e4 100644 --- a/crates/ra_hir/src/nameres/tests.rs +++ b/crates/ra_hir/src/nameres/tests.rs @@ -215,6 +215,27 @@ fn item_map_using_self() { ); } +#[test] +fn item_map_enum_importing() { + covers!(item_map_enum_importing); + let (item_map, module_id) = item_map( + " + //- /lib.rs + enum E { V } + use self::E::V; + <|> + ", + ); + check_module_item_map( + &item_map, + module_id, + " + E: t + V: t v + ", + ); +} + #[test] fn item_map_across_crates() { let (mut db, sr) = MockDatabase::with_files( diff --git a/crates/test_utils/src/marks.rs b/crates/test_utils/src/marks.rs index 79ffedf69b..ee47b52198 100644 --- a/crates/test_utils/src/marks.rs +++ b/crates/test_utils/src/marks.rs @@ -46,11 +46,13 @@ macro_rules! covers { } #[macro_export] -macro_rules! mark { - ($ident:ident) => { +macro_rules! marks { + ($($ident:ident)*) => { + $( #[allow(bad_style)] pub(crate) static $ident: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + )* }; }