refactor import resolution

extract path resolution
use enums instead of bools

4 files changed, 146 insertions, 99 deletions

diff --git a/crates/ra_hir/src/marks.rs b/crates/ra_hir/src/marks.rs
index f4d0c3e5966..338ed0516a8 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 a3bc989580a..8c8494b4603 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,116 +286,138 @@ 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 original_module = Module {
+            krate: self.krate,
+            module_id,
+        };
+        let (def_id, reached_fixedpoint) =
+            self.result
+                .resolve_path(self.db, original_module, &import.path);
+
+        if reached_fixedpoint == ReachedFixedPoint::Yes {
+            let last_segment = import.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,
+            );
+        }
+        reached_fixedpoint
+    }
+
+    fn update(&mut self, module_id: ModuleId, f: impl FnOnce(&mut ModuleScope)) {
+        let module_items = self.result.per_module.get_mut(&module_id).unwrap();
+        f(module_items)
+    }
+}
 
-        let mut curr: ModuleId = match import.path.kind {
-            PathKind::Plain | PathKind::Self_ => module_id,
+#[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<ModuleDef>, ReachedFixedPoint) {
+        let mut curr_per_ns: PerNs<ModuleDef> = PerNs::types(match path.kind {
+            PathKind::Crate => original_module.crate_root(db).into(),
+            PathKind::Self_ | PathKind::Plain => original_module.into(),
             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
-                    }
+                if let Some(p) = original_module.parent(db) {
+                    p.into()
+                } else {
+                    log::debug!("super path in root module");
+                    return (PerNs::none(), ReachedFixedPoint::Yes);
                 }
             }
-            PathKind::Crate => module_id.crate_root(&self.module_tree),
             PathKind::Abs => {
-                // TODO: absolute use is not supported for now
-                return false;
+                // TODO: absolute use is not supported
+                return (PerNs::none(), ReachedFixedPoint::Yes);
             }
-        };
-
-        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;
+        });
+
+        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);
+                    }
 
-            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;
-                            }
+                    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);
                         }
                     }
-                    _ => {
-                        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
+                }
+                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(),
                     }
                 }
-            } else {
-                log::debug!(
-                    "resolved import {:?} ({:?}) within source root to {:?}",
-                    segment.name,
-                    import,
-                    def_id,
-                );
-                self.update(module_id, |items| {
-                    let res = Resolution {
-                        def_id,
-                        import: Some(import_id),
-                    };
-                    items.items.insert(segment.name.clone(), res);
-                })
-            }
+                _ => {
+                    // 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);
+                }
+            };
         }
-        true
-    }
-
-    fn update(&mut self, module_id: ModuleId, f: impl FnOnce(&mut ModuleScope)) {
-        let module_items = self.result.per_module.get_mut(&module_id).unwrap();
-        f(module_items)
+        (curr_per_ns, ReachedFixedPoint::Yes)
     }
 }
 
diff --git a/crates/ra_hir/src/nameres/tests.rs b/crates/ra_hir/src/nameres/tests.rs
index 9322bf08ce9..430d16a2e4b 100644
--- a/crates/ra_hir/src/nameres/tests.rs
+++ b/crates/ra_hir/src/nameres/tests.rs
@@ -216,6 +216,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 79ffedf69bc..ee47b521981 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);
+        )*
     };
 }