Auto merge of #15148 - lowr:fix/super-nameres-in-block, r=Veykril

bors · bors · commit 4b06d3c595a7 · 2023-06-28T10:43:56.000Z
Fix `self` and `super` path resolution in block modules

This PR fixes `self` and `super` path resolution with block modules involved.

Previously, we were just going up the module tree count-of-`super` times without considering block modules in the way, and then if we ended up in a block `DefMap`, we adjust "to the containing crate-rooted module". While this seems to work in most real-world cases, we failed to resolve them within peculiar module structures.

`self` and `super` should actually be resolved to the nearest non-block module, and the paths don't necessarily resolve to a crate-rooted module. This PR makes sure every `self` and `super` segment in paths are resolved to a non-block module.
diff --git a/crates/hir-def/src/body/tests.rs b/crates/hir-def/src/body/tests.rs
@@ -3,12 +3,12 @@ mod block;
 use base_db::{fixture::WithFixture, SourceDatabase};
 use expect_test::Expect;
 
-use crate::ModuleDefId;
+use crate::{test_db::TestDB, ModuleDefId};
 
 use super::*;
 
 fn lower(ra_fixture: &str) -> Arc<Body> {
-    let db = crate::test_db::TestDB::with_files(ra_fixture);
+    let db = TestDB::with_files(ra_fixture);
 
     let krate = db.crate_graph().iter().next().unwrap();
     let def_map = db.crate_def_map(krate);
@@ -25,23 +25,23 @@ fn lower(ra_fixture: &str) -> Arc<Body> {
     db.body(fn_def.unwrap().into())
 }
 
-fn block_def_map_at(ra_fixture: &str) -> String {
-    let (db, position) = crate::test_db::TestDB::with_position(ra_fixture);
+fn def_map_at(ra_fixture: &str) -> String {
+    let (db, position) = TestDB::with_position(ra_fixture);
 
     let module = db.module_at_position(position);
     module.def_map(&db).dump(&db)
 }
 
 fn check_block_scopes_at(ra_fixture: &str, expect: Expect) {
-    let (db, position) = crate::test_db::TestDB::with_position(ra_fixture);
+    let (db, position) = TestDB::with_position(ra_fixture);
 
     let module = db.module_at_position(position);
     let actual = module.def_map(&db).dump_block_scopes(&db);
     expect.assert_eq(&actual);
 }
 
 fn check_at(ra_fixture: &str, expect: Expect) {
-    let actual = block_def_map_at(ra_fixture);
+    let actual = def_map_at(ra_fixture);
     expect.assert_eq(&actual);
 }
 
diff --git a/crates/hir-def/src/body/tests/block.rs b/crates/hir-def/src/body/tests/block.rs
@@ -133,6 +133,47 @@ struct Struct {}
     );
 }
 
+#[test]
+fn super_imports_2() {
+    check_at(
+        r#"
+fn outer() {
+    mod m {
+        struct ResolveMe {}
+        fn middle() {
+            mod m2 {
+                fn inner() {
+                    use super::ResolveMe;
+                    $0
+                }
+            }
+        }
+    }
+}
+"#,
+        expect![[r#"
+            block scope
+            ResolveMe: t
+
+            block scope
+            m2: t
+
+            block scope::m2
+            inner: v
+
+            block scope
+            m: t
+
+            block scope::m
+            ResolveMe: t
+            middle: v
+
+            crate
+            outer: v
+        "#]],
+    );
+}
+
 #[test]
 fn nested_module_scoping() {
     check_block_scopes_at(
@@ -155,6 +196,42 @@ fn f() {
     );
 }
 
+#[test]
+fn self_imports() {
+    check_at(
+        r#"
+fn f() {
+    mod m {
+        struct ResolveMe {}
+        fn g() {
+            fn h() {
+                use self::ResolveMe;
+                $0
+            }
+        }
+    }
+}
+"#,
+        expect![[r#"
+            block scope
+            ResolveMe: t
+
+            block scope
+            h: v
+
+            block scope
+            m: t
+
+            block scope::m
+            ResolveMe: t
+            g: v
+
+            crate
+            f: v
+        "#]],
+    );
+}
+
 #[test]
 fn legacy_macro_items() {
     // Checks that legacy-scoped `macro_rules!` from parent namespaces are resolved and expanded
diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs
@@ -145,24 +145,28 @@ pub struct ModuleId {
 }
 
 impl ModuleId {
-    pub fn def_map(&self, db: &dyn db::DefDatabase) -> Arc<DefMap> {
+    pub fn def_map(self, db: &dyn db::DefDatabase) -> Arc<DefMap> {
         match self.block {
             Some(block) => db.block_def_map(block),
             None => db.crate_def_map(self.krate),
         }
     }
 
-    pub fn krate(&self) -> CrateId {
+    pub fn krate(self) -> CrateId {
         self.krate
     }
 
-    pub fn containing_module(&self, db: &dyn db::DefDatabase) -> Option<ModuleId> {
+    pub fn containing_module(self, db: &dyn db::DefDatabase) -> Option<ModuleId> {
         self.def_map(db).containing_module(self.local_id)
     }
 
-    pub fn containing_block(&self) -> Option<BlockId> {
+    pub fn containing_block(self) -> Option<BlockId> {
         self.block
     }
+
+    pub fn is_block_module(self) -> bool {
+        self.block.is_some() && self.local_id == DefMap::ROOT
+    }
 }
 
 /// An ID of a module, **local** to a `DefMap`.
diff --git a/crates/hir-def/src/nameres.rs b/crates/hir-def/src/nameres.rs
@@ -196,6 +196,10 @@ impl BlockRelativeModuleId {
     fn into_module(self, krate: CrateId) -> ModuleId {
         ModuleId { krate, block: self.block, local_id: self.local_id }
     }
+
+    fn is_block_module(self) -> bool {
+        self.block.is_some() && self.local_id == DefMap::ROOT
+    }
 }
 
 impl std::ops::Index<LocalModuleId> for DefMap {
@@ -278,7 +282,9 @@ pub struct ModuleData {
     pub origin: ModuleOrigin,
     /// Declared visibility of this module.
     pub visibility: Visibility,
-    /// Always [`None`] for block modules
+    /// Parent module in the same `DefMap`.
+    ///
+    /// [`None`] for block modules because they are always its `DefMap`'s root.
     pub parent: Option<LocalModuleId>,
     pub children: FxHashMap<Name, LocalModuleId>,
     pub scope: ItemScope,
diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs
@@ -808,11 +808,8 @@ impl DefCollector<'_> {
                 }
             }
 
-            // Check whether all namespace is resolved
-            if def.take_types().is_some()
-                && def.take_values().is_some()
-                && def.take_macros().is_some()
-            {
+            // Check whether all namespaces are resolved.
+            if def.is_full() {
                 PartialResolvedImport::Resolved(def)
             } else {
                 PartialResolvedImport::Indeterminate(def)
@@ -821,7 +818,7 @@ impl DefCollector<'_> {
     }
 
     fn resolve_extern_crate(&self, name: &Name) -> Option<CrateRootModuleId> {
-        if *name == name!(self) {
+        if *name == name![self] {
             cov_mark::hit!(extern_crate_self_as);
             Some(self.def_map.crate_root())
         } else {
diff --git a/crates/hir-def/src/nameres/path_resolution.rs b/crates/hir-def/src/nameres/path_resolution.rs
@@ -12,11 +12,12 @@
 
 use base_db::Edition;
 use hir_expand::name::Name;
+use triomphe::Arc;
 
 use crate::{
     db::DefDatabase,
     item_scope::BUILTIN_SCOPE,
-    nameres::{sub_namespace_match, BuiltinShadowMode, DefMap, MacroSubNs},
+    nameres::{sub_namespace_match, BlockInfo, BuiltinShadowMode, DefMap, MacroSubNs},
     path::{ModPath, PathKind},
     per_ns::PerNs,
     visibility::{RawVisibility, Visibility},
@@ -159,13 +160,15 @@ impl DefMap {
                 (None, new) => new,
             };
 
-            match &current_map.block {
-                Some(block) => {
+            match current_map.block {
+                Some(block) if original_module == Self::ROOT => {
+                    // Block modules "inherit" names from its parent module.
                     original_module = block.parent.local_id;
                     arc = block.parent.def_map(db, current_map.krate);
-                    current_map = &*arc;
+                    current_map = &arc;
                 }
-                None => return result,
+                // Proper (non-block) modules, including those in block `DefMap`s, don't.
+                _ => return result,
             }
         }
     }
@@ -189,7 +192,7 @@ impl DefMap {
         ));
 
         let mut segments = path.segments().iter().enumerate();
-        let mut curr_per_ns: PerNs = match path.kind {
+        let mut curr_per_ns = match path.kind {
             PathKind::DollarCrate(krate) => {
                 if krate == self.krate {
                     cov_mark::hit!(macro_dollar_crate_self);
@@ -241,51 +244,54 @@ impl DefMap {
                 )
             }
             PathKind::Super(lvl) => {
-                let mut module = original_module;
-                for i in 0..lvl {
-                    match self.modules[module].parent {
-                        Some(it) => module = it,
-                        None => match &self.block {
-                            Some(block) => {
-                                // Look up remaining path in parent `DefMap`
-                                let new_path = ModPath::from_segments(
-                                    PathKind::Super(lvl - i),
-                                    path.segments().to_vec(),
-                                );
-                                tracing::debug!(
-                                    "`super` path: {} -> {} in parent map",
-                                    path.display(db.upcast()),
-                                    new_path.display(db.upcast())
-                                );
-                                return block
-                                    .parent
-                                    .def_map(db, self.krate)
-                                    .resolve_path_fp_with_macro(
-                                        db,
-                                        mode,
-                                        block.parent.local_id,
-                                        &new_path,
-                                        shadow,
-                                        expected_macro_subns,
-                                    );
-                            }
-                            None => {
-                                tracing::debug!("super path in root module");
-                                return ResolvePathResult::empty(ReachedFixedPoint::Yes);
-                            }
-                        },
-                    }
+                let mut local_id = original_module;
+                let mut ext;
+                let mut def_map = self;
+
+                // Adjust `local_id` to `self`, i.e. the nearest non-block module.
+                if def_map.module_id(local_id).is_block_module() {
+                    (ext, local_id) = adjust_to_nearest_non_block_module(db, def_map, local_id);
+                    def_map = &ext;
                 }
 
-                // Resolve `self` to the containing crate-rooted module if we're a block
-                self.with_ancestor_maps(db, module, &mut |def_map, module| {
-                    if def_map.block.is_some() {
-                        None // keep ascending
+                // Go up the module tree but skip block modules as `super` always refers to the
+                // nearest non-block module.
+                for _ in 0..lvl {
+                    // Loop invariant: at the beginning of each loop, `local_id` must refer to a
+                    // non-block module.
+                    if let Some(parent) = def_map.modules[local_id].parent {
+                        local_id = parent;
+                        if def_map.module_id(local_id).is_block_module() {
+                            (ext, local_id) =
+                                adjust_to_nearest_non_block_module(db, def_map, local_id);
+                            def_map = &ext;
+                        }
                     } else {
-                        Some(PerNs::types(def_map.module_id(module).into(), Visibility::Public))
+                        stdx::always!(def_map.block.is_none());
+                        tracing::debug!("super path in root module");
+                        return ResolvePathResult::empty(ReachedFixedPoint::Yes);
                     }
-                })
-                .expect("block DefMap not rooted in crate DefMap")
+                }
+
+                let module = def_map.module_id(local_id);
+                stdx::never!(module.is_block_module());
+
+                if self.block != def_map.block {
+                    // If we have a different `DefMap` from `self` (the orignal `DefMap` we started
+                    // with), resolve the remaining path segments in that `DefMap`.
+                    let path =
+                        ModPath::from_segments(PathKind::Super(0), path.segments().iter().cloned());
+                    return def_map.resolve_path_fp_with_macro(
+                        db,
+                        mode,
+                        local_id,
+                        &path,
+                        shadow,
+                        expected_macro_subns,
+                    );
+                }
+
+                PerNs::types(module.into(), Visibility::Public)
             }
             PathKind::Abs => {
                 // 2018-style absolute path -- only extern prelude
@@ -508,3 +514,27 @@ impl DefMap {
         }
     }
 }
+
+/// Given a block module, returns its nearest non-block module and the `DefMap` it blongs to.
+fn adjust_to_nearest_non_block_module(
+    db: &dyn DefDatabase,
+    def_map: &DefMap,
+    mut local_id: LocalModuleId,
+) -> (Arc<DefMap>, LocalModuleId) {
+    // INVARIANT: `local_id` in `def_map` must be a block module.
+    stdx::always!(def_map.module_id(local_id).is_block_module());
+
+    let mut ext;
+    // This needs to be a local variable due to our mighty lifetime.
+    let mut def_map = def_map;
+    loop {
+        let BlockInfo { parent, .. } = def_map.block.expect("block module without parent module");
+
+        ext = parent.def_map(db, def_map.krate);
+        def_map = &ext;
+        local_id = parent.local_id;
+        if !parent.is_block_module() {
+            return (ext, local_id);
+        }
+    }
+}
diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs

Original file line number	Diff line number	Diff line change
`@@ -145,24 +145,28 @@ pub struct ModuleId {`
`145`	`145`	`}`
`146`	`146`
`147`	`147`	`impl ModuleId {`
`148`		`- pub fn def_map(&self, db: &dyn db::DefDatabase) -> Arc<DefMap> {`
	`148`	`+ pub fn def_map(self, db: &dyn db::DefDatabase) -> Arc<DefMap> {`
`149`	`149`	`match self.block {`
`150`	`150`	`Some(block) => db.block_def_map(block),`
`151`	`151`	`None => db.crate_def_map(self.krate),`
`152`	`152`	`}`
`153`	`153`	`}`
`154`	`154`
`155`		`- pub fn krate(&self) -> CrateId {`
	`155`	`+ pub fn krate(self) -> CrateId {`
`156`	`156`	`self.krate`
`157`	`157`	`}`
`158`	`158`
`159`		`- pub fn containing_module(&self, db: &dyn db::DefDatabase) -> Option<ModuleId> {`
	`159`	`+ pub fn containing_module(self, db: &dyn db::DefDatabase) -> Option<ModuleId> {`
`160`	`160`	`self.def_map(db).containing_module(self.local_id)`
`161`	`161`	`}`
`162`	`162`
`163`		`- pub fn containing_block(&self) -> Option<BlockId> {`
	`163`	`+ pub fn containing_block(self) -> Option<BlockId> {`
`164`	`164`	`self.block`
`165`	`165`	`}`
	`166`	`+`
	`167`	`+ pub fn is_block_module(self) -> bool {`
	`168`	`+ self.block.is_some() && self.local_id == DefMap::ROOT`
	`169`	`+ }`
`166`	`170`	`}`
`167`	`171`
`168`	`172`	/// An ID of a module, local to a `DefMap`.