Merge #10960

10960: fix: fix handling of macros in `extern` blocks r=jonas-schievink a=jonas-schievink

- Removes knowledge of what's in an extern block from `ItemTree`.
- Treats extern blocks as `AssocContainerId` and renames the latter to `ItemContainerId`.
- Threads the container through name resolution (which is a bit messy).
- Considers statics to be associated items, since they can be in an extern block. (it might be better to further distinguish potentially-associated-items from potentially-in-an-extern-block-items, but currently I don't expect much of a benefit)
- Adds a test for the incorrect-case diagnostic demonstrating the impact of this change.

Fixes #8913

bors r+

Co-authored-by: Jonas Schievink <[email protected]>

Author: bors[bot]

crates/hir/src/attrs.rs

@@ -137,6 +137,7 @@ fn resolve_doc_path(
         AttrDefId::TraitId(it) => it.resolver(db.upcast()),
         AttrDefId::TypeAliasId(it) => it.resolver(db.upcast()),
         AttrDefId::ImplId(it) => it.resolver(db.upcast()),
+        AttrDefId::ExternBlockId(it) => it.resolver(db.upcast()),
         AttrDefId::GenericParamId(it) => match it {
             GenericParamId::TypeParamId(it) => it.parent,
             GenericParamId::LifetimeParamId(it) => it.parent,

crates/hir/src/lib.rs

@@ -114,11 +114,11 @@ pub use {
         type_ref::{Mutability, TypeRef},
         visibility::Visibility,
         AdtId,
-        AssocContainerId,
         AssocItemId,
         AssocItemLoc,
         DefWithBodyId,
         ImplId,
+        ItemContainerId,
         ItemLoc,
         Lookup,
         ModuleDefId,
@@ -1550,7 +1550,7 @@ impl Static {
     pub fn ty(self, db: &dyn HirDatabase) -> Type {
         let data = db.static_data(self.id);
         let resolver = self.id.resolver(db.upcast());
-        let krate = self.id.lookup(db.upcast()).container.krate();
+        let krate = self.id.lookup(db.upcast()).container.module(db.upcast()).krate();
         let ctx = hir_ty::TyLoweringContext::new(db, &resolver);
         let ty = ctx.lower_ty(&data.type_ref);
         Type::new_with_resolver_inner(db, krate, &resolver, ty)
@@ -1820,8 +1820,8 @@ where
     AST: ItemTreeNode,
 {
     match id.lookup(db.upcast()).container {
-        AssocContainerId::TraitId(_) | AssocContainerId::ImplId(_) => Some(ctor(DEF::from(id))),
-        AssocContainerId::ModuleId(_) => None,
+        ItemContainerId::TraitId(_) | ItemContainerId::ImplId(_) => Some(ctor(DEF::from(id))),
+        ItemContainerId::ModuleId(_) | ItemContainerId::ExternBlockId(_) => None,
     }
 }
 
@@ -1847,9 +1847,11 @@ impl AssocItem {
             AssocItem::TypeAlias(it) => it.id.lookup(db.upcast()).container,
         };
         match container {
-            AssocContainerId::TraitId(id) => AssocItemContainer::Trait(id.into()),
-            AssocContainerId::ImplId(id) => AssocItemContainer::Impl(id.into()),
-            AssocContainerId::ModuleId(_) => panic!("invalid AssocItem"),
+            ItemContainerId::TraitId(id) => AssocItemContainer::Trait(id.into()),
+            ItemContainerId::ImplId(id) => AssocItemContainer::Impl(id.into()),
+            ItemContainerId::ModuleId(_) | ItemContainerId::ExternBlockId(_) => {
+                panic!("invalid AssocItem")
+            }
         }
     }
 

crates/hir_def/src/attr.rs

@@ -362,6 +362,7 @@ impl AttrsWithOwner {
                     RawAttrs::from_attrs_owner(db, src.with_value(&src.value[it.local_id]))
                 }
             },
+            AttrDefId::ExternBlockId(it) => attrs_from_item_tree(it.lookup(db).id, db),
         };
 
         let attrs = raw_attrs.filter(db, def.krate(db));
@@ -443,6 +444,7 @@ impl AttrsWithOwner {
                     .child_source(db)
                     .map(|source| ast::AnyHasAttrs::new(source[id.local_id].clone())),
             },
+            AttrDefId::ExternBlockId(id) => id.lookup(db).source(db).map(ast::AnyHasAttrs::new),
         };
 
         AttrSourceMap::new(owner.as_ref().map(|node| node as &dyn HasAttrs))

crates/hir_def/src/data.rs

@@ -13,8 +13,8 @@ use crate::{
     item_tree::{self, AssocItem, FnFlags, ItemTreeId, ModItem, Param},
     type_ref::{TraitRef, TypeBound, TypeRef},
     visibility::RawVisibility,
-    AssocContainerId, AssocItemId, ConstId, ConstLoc, FunctionId, FunctionLoc, HasModule, ImplId,
-    Intern, Lookup, ModuleId, StaticId, TraitId, TypeAliasId, TypeAliasLoc,
+    AssocItemId, ConstId, ConstLoc, FunctionId, FunctionLoc, HasModule, ImplId, Intern,
+    ItemContainerId, Lookup, ModuleId, StaticId, TraitId, TypeAliasId, TypeAliasLoc,
 };
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -54,6 +54,10 @@ impl FunctionData {
             flags.bits |= FnFlags::IS_VARARGS;
         }
 
+        if matches!(loc.container, ItemContainerId::ExternBlockId(_)) {
+            flags.bits |= FnFlags::IS_IN_EXTERN_BLOCK;
+        }
+
         Arc::new(FunctionData {
             name: func.name.clone(),
             params: enabled_params
@@ -130,7 +134,7 @@ impl TypeAliasData {
             name: typ.name.clone(),
             type_ref: typ.type_ref.clone(),
             visibility: item_tree[typ.visibility].clone(),
-            is_extern: typ.is_extern,
+            is_extern: matches!(loc.container, ItemContainerId::ExternBlockId(_)),
             bounds: typ.bounds.to_vec(),
         })
     }
@@ -162,7 +166,7 @@ impl TraitData {
         let is_auto = tr_def.is_auto;
         let is_unsafe = tr_def.is_unsafe;
         let module_id = tr_loc.container;
-        let container = AssocContainerId::TraitId(tr);
+        let container = ItemContainerId::TraitId(tr);
         let visibility = item_tree[tr_def.visibility].clone();
         let mut expander = Expander::new(db, tr_loc.id.file_id(), module_id);
         let skip_array_during_method_dispatch = item_tree
@@ -231,7 +235,7 @@ impl ImplData {
         let self_ty = impl_def.self_ty.clone();
         let is_negative = impl_def.is_negative;
         let module_id = impl_loc.container;
-        let container = AssocContainerId::ImplId(id);
+        let container = ItemContainerId::ImplId(id);
         let mut expander = Expander::new(db, impl_loc.id.file_id(), module_id);
 
         let items = collect_items(
@@ -282,16 +286,16 @@ pub struct StaticData {
 
 impl StaticData {
     pub(crate) fn static_data_query(db: &dyn DefDatabase, konst: StaticId) -> Arc<StaticData> {
-        let node = konst.lookup(db);
-        let item_tree = node.id.item_tree(db);
-        let statik = &item_tree[node.id.value];
+        let loc = konst.lookup(db);
+        let item_tree = loc.id.item_tree(db);
+        let statik = &item_tree[loc.id.value];
 
         Arc::new(StaticData {
             name: statik.name.clone(),
             type_ref: statik.type_ref.clone(),
             visibility: item_tree[statik.visibility].clone(),
             mutable: statik.mutable,
-            is_extern: statik.is_extern,
+            is_extern: matches!(loc.container, ItemContainerId::ExternBlockId(_)),
         })
     }
 }
@@ -302,7 +306,7 @@ fn collect_items(
     expander: &mut Expander,
     assoc_items: impl Iterator<Item = AssocItem>,
     tree_id: item_tree::TreeId,
-    container: AssocContainerId,
+    container: ItemContainerId,
     limit: usize,
 ) -> Vec<(Name, AssocItemId)> {
     if limit == 0 {

crates/hir_def/src/db.rs

@@ -19,10 +19,10 @@ use crate::{
     lang_item::{LangItemTarget, LangItems},
     nameres::DefMap,
     visibility::{self, Visibility},
-    AttrDefId, BlockId, BlockLoc, ConstId, ConstLoc, DefWithBodyId, EnumId, EnumLoc, FunctionId,
-    FunctionLoc, GenericDefId, ImplId, ImplLoc, LocalEnumVariantId, LocalFieldId, StaticId,
-    StaticLoc, StructId, StructLoc, TraitId, TraitLoc, TypeAliasId, TypeAliasLoc, UnionId,
-    UnionLoc, VariantId,
+    AttrDefId, BlockId, BlockLoc, ConstId, ConstLoc, DefWithBodyId, EnumId, EnumLoc, ExternBlockId,
+    ExternBlockLoc, FunctionId, FunctionLoc, GenericDefId, ImplId, ImplLoc, LocalEnumVariantId,
+    LocalFieldId, StaticId, StaticLoc, StructId, StructLoc, TraitId, TraitLoc, TypeAliasId,
+    TypeAliasLoc, UnionId, UnionLoc, VariantId,
 };
 
 #[salsa::query_group(InternDatabaseStorage)]
@@ -46,6 +46,8 @@ pub trait InternDatabase: SourceDatabase {
     #[salsa::interned]
     fn intern_impl(&self, loc: ImplLoc) -> ImplId;
     #[salsa::interned]
+    fn intern_extern_block(&self, loc: ExternBlockLoc) -> ExternBlockId;
+    #[salsa::interned]
     fn intern_block(&self, loc: BlockLoc) -> BlockId;
 }
 

crates/hir_def/src/import_map.rs

@@ -468,7 +468,7 @@ mod tests {
     use base_db::{fixture::WithFixture, SourceDatabase, Upcast};
     use expect_test::{expect, Expect};
 
-    use crate::{test_db::TestDB, AssocContainerId, Lookup};
+    use crate::{test_db::TestDB, ItemContainerId, Lookup};
 
     use super::*;
 
@@ -563,7 +563,7 @@ mod tests {
         };
 
         match container {
-            AssocContainerId::TraitId(it) => Some(ItemInNs::Types(it.into())),
+            ItemContainerId::TraitId(it) => Some(ItemInNs::Types(it.into())),
             _ => None,
         }
     }

crates/hir_def/src/item_tree.rs

@@ -660,8 +660,6 @@ pub struct Static {
     pub name: Name,
     pub visibility: RawVisibilityId,
     pub mutable: bool,
-    /// Whether the static is in an `extern` block.
-    pub is_extern: bool,
     pub type_ref: Interned<TypeRef>,
     pub ast_id: FileAstId<ast::Static>,
 }
@@ -695,7 +693,6 @@ pub struct TypeAlias {
     pub bounds: Box<[Interned<TypeBound>]>,
     pub generic_params: Interned<GenericParams>,
     pub type_ref: Option<Interned<TypeRef>>,
-    pub is_extern: bool,
     pub ast_id: FileAstId<ast::TypeAlias>,
 }
 

crates/hir_def/src/item_tree/lower.rs

@@ -360,7 +360,6 @@ impl<'a> Ctx<'a> {
             generic_params,
             type_ref,
             ast_id,
-            is_extern: false,
         };
         Some(id(self.data().type_aliases.alloc(res)))
     }
@@ -371,7 +370,7 @@ impl<'a> Ctx<'a> {
         let visibility = self.lower_visibility(static_);
         let mutable = static_.mut_token().is_some();
         let ast_id = self.source_ast_id_map.ast_id(static_);
-        let res = Static { name, visibility, mutable, type_ref, ast_id, is_extern: false };
+        let res = Static { name, visibility, mutable, type_ref, ast_id };
         Some(id(self.data().statics.alloc(res)))
     }
 
@@ -525,27 +524,23 @@ impl<'a> Ctx<'a> {
         let children: Box<[_]> = block.extern_item_list().map_or(Box::new([]), |list| {
             list.extern_items()
                 .filter_map(|item| {
+                    // Note: All items in an `extern` block need to be lowered as if they're outside of one
+                    // (in other words, the knowledge that they're in an extern block must not be used).
+                    // This is because an extern block can contain macros whose ItemTree's top-level items
+                    // should be considered to be in an extern block too.
                     let attrs = RawAttrs::new(self.db, &item, &self.hygiene);
                     let id: ModItem = match item {
                         ast::ExternItem::Fn(ast) => {
                             let func_id = self.lower_function(&ast)?;
                             let func = &mut self.data().functions[func_id.index];
                             if is_intrinsic_fn_unsafe(&func.name) {
+                                // FIXME: this breaks in macros
                                 func.flags.bits |= FnFlags::IS_UNSAFE;
                             }
-                            func.flags.bits |= FnFlags::IS_IN_EXTERN_BLOCK;
                             func_id.into()
                         }
-                        ast::ExternItem::Static(ast) => {
-                            let statik = self.lower_static(&ast)?;
-                            self.data().statics[statik.index].is_extern = true;
-                            statik.into()
-                        }
-                        ast::ExternItem::TypeAlias(ty) => {
-                            let foreign_ty = self.lower_type_alias(&ty)?;
-                            self.data().type_aliases[foreign_ty.index].is_extern = true;
-                            foreign_ty.into()
-                        }
+                        ast::ExternItem::Static(ast) => self.lower_static(&ast)?.into(),
+                        ast::ExternItem::TypeAlias(ty) => self.lower_type_alias(&ty)?.into(),
                         ast::ExternItem::MacroCall(call) => {
                             // FIXME: we need some way of tracking that the macro call is in an
                             // extern block

crates/hir_def/src/item_tree/pretty.rs

@@ -328,8 +328,7 @@ impl<'a> Printer<'a> {
                 wln!(self, " = _;");
             }
             ModItem::Static(it) => {
-                let Static { name, visibility, mutable, is_extern, type_ref, ast_id: _ } =
-                    &self.tree[it];
+                let Static { name, visibility, mutable, type_ref, ast_id: _ } = &self.tree[it];
                 self.print_visibility(*visibility);
                 w!(self, "static ");
                 if *mutable {
@@ -338,9 +337,6 @@ impl<'a> Printer<'a> {
                 w!(self, "{}: ", name);
                 self.print_type_ref(type_ref);
                 w!(self, " = _;");
-                if *is_extern {
-                    w!(self, "  // extern");
-                }
                 wln!(self);
             }
             ModItem::Trait(it) => {
@@ -393,15 +389,8 @@ impl<'a> Printer<'a> {
                 wln!(self, "}}");
             }
             ModItem::TypeAlias(it) => {
-                let TypeAlias {
-                    name,
-                    visibility,
-                    bounds,
-                    type_ref,
-                    is_extern,
-                    generic_params,
-                    ast_id: _,
-                } = &self.tree[it];
+                let TypeAlias { name, visibility, bounds, type_ref, generic_params, ast_id: _ } =
+                    &self.tree[it];
                 self.print_visibility(*visibility);
                 w!(self, "type {}", name);
                 self.print_generic_params(generic_params);
@@ -415,9 +404,6 @@ impl<'a> Printer<'a> {
                 }
                 self.print_where_clause(generic_params);
                 w!(self, ";");
-                if *is_extern {
-                    w!(self, "  // extern");
-                }
                 wln!(self);
             }
             ModItem::Mod(it) => {

crates/hir_def/src/item_tree/tests.rs

@@ -70,13 +70,13 @@ extern "C" {
             #[on_extern_block]  // AttrId { is_doc_comment: false, ast_index: 0 }
             extern "C" {
                 #[on_extern_type]  // AttrId { is_doc_comment: false, ast_index: 0 }
-                pub(self) type ExType;  // extern
+                pub(self) type ExType;
 
                 #[on_extern_static]  // AttrId { is_doc_comment: false, ast_index: 0 }
-                pub(self) static EX_STATIC: u8 = _;  // extern
+                pub(self) static EX_STATIC: u8 = _;
 
                 #[on_extern_fn]  // AttrId { is_doc_comment: false, ast_index: 0 }
-                // flags = 0x60
+                // flags = 0x20
                 pub(self) fn ex_fn() -> ();
             }
         "##]],