From 7575e06403d77f6c03febd853cbca54b0db9d14a Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 15 Aug 2024 14:36:10 -0700 Subject: [PATCH] Fix anon const def-creation when macros are involved Ever since #125915, some `ast::AnonConst`s turn into `hir::ConstArgKind::Path`s, which don't have associated `DefId`s. To deal with the fact that we don't have resolution information in `DefCollector`, we decided to implement a process where if the anon const *appeared* to be trivial (i.e., `N` or `{ N }`), we would avoid creating a def for it in `DefCollector`. If later, in AST lowering, we realized it turned out to be a unit struct literal, or we were lowering it to something that didn't use `hir::ConstArg`, we'd create its def there. However, let's say we have a macro `m!()` that expands to a reference to a free constant `FOO`. If we use `m!()` in the body of an anon const (e.g., `Foo<{ m!() }>`), then in def collection, it appears to be a nontrivial anon const and we create a def. But the macro expands to something that looks like a trivial const arg, but is not, so in AST lowering we "fix" the mistake we assumed def collection made and create a def for it. This causes a duplicate definition ICE. The long-term fix for this is to delay the creation of defs for all expression-like nodes until AST lowering (see #128844 for an incomplete attempt at this). This would avoid issues like this one that are caused by hacky workarounds. However, doing this uncovers a pre-existing bug with opaque types that is quite involved to fix (see #129023). In the meantime, this PR fixes the bug by delaying def creation for anon consts whose bodies are macro invocations until after we expand the macro and know what is inside it. This is accomplished by adding information to create the anon const's def to the data in `Resolver.invocation_parents`. --- compiler/rustc_ast/src/ast.rs | 20 ++- compiler/rustc_resolve/src/def_collector.rs | 138 ++++++++++++------ compiler/rustc_resolve/src/lib.rs | 28 +++- compiler/rustc_resolve/src/macros.rs | 16 +- tests/crashes/128016.rs | 10 -- .../early/trivial-const-arg-macro-nested.rs | 21 +++ .../early/trivial-const-arg-macro-param.rs | 13 ++ .../trivial-const-arg-macro-res-error.rs | 13 ++ .../trivial-const-arg-macro-res-error.stderr | 24 +++ .../early/trivial-const-arg-macro.rs | 15 ++ 10 files changed, 224 insertions(+), 74 deletions(-) delete mode 100644 tests/crashes/128016.rs create mode 100644 tests/ui/const-generics/early/trivial-const-arg-macro-nested.rs create mode 100644 tests/ui/const-generics/early/trivial-const-arg-macro-param.rs create mode 100644 tests/ui/const-generics/early/trivial-const-arg-macro-res-error.rs create mode 100644 tests/ui/const-generics/early/trivial-const-arg-macro-res-error.stderr create mode 100644 tests/ui/const-generics/early/trivial-const-arg-macro.rs diff --git a/compiler/rustc_ast/src/ast.rs b/compiler/rustc_ast/src/ast.rs index a44ed82850480..46d68d2054125 100644 --- a/compiler/rustc_ast/src/ast.rs +++ b/compiler/rustc_ast/src/ast.rs @@ -1188,14 +1188,7 @@ impl Expr { /// /// Does not ensure that the path resolves to a const param, the caller should check this. pub fn is_potential_trivial_const_arg(&self) -> bool { - let this = if let ExprKind::Block(block, None) = &self.kind - && let [stmt] = block.stmts.as_slice() - && let StmtKind::Expr(expr) = &stmt.kind - { - expr - } else { - self - }; + let this = self.maybe_unwrap_block(); if let ExprKind::Path(None, path) = &this.kind && path.is_potential_trivial_const_arg() @@ -1206,6 +1199,17 @@ impl Expr { } } + pub fn maybe_unwrap_block(&self) -> &Expr { + if let ExprKind::Block(block, None) = &self.kind + && let [stmt] = block.stmts.as_slice() + && let StmtKind::Expr(expr) = &stmt.kind + { + expr + } else { + self + } + } + pub fn to_bound(&self) -> Option { match &self.kind { ExprKind::Path(None, path) => Some(GenericBound::Trait( diff --git a/compiler/rustc_resolve/src/def_collector.rs b/compiler/rustc_resolve/src/def_collector.rs index ed23870dfdf8b..e32c76ed7594f 100644 --- a/compiler/rustc_resolve/src/def_collector.rs +++ b/compiler/rustc_resolve/src/def_collector.rs @@ -11,15 +11,23 @@ use rustc_span::symbol::{kw, sym, Symbol}; use rustc_span::Span; use tracing::debug; -use crate::{ImplTraitContext, Resolver}; +use crate::{ImplTraitContext, InvocationParent, PendingAnonConstInfo, Resolver}; pub(crate) fn collect_definitions( resolver: &mut Resolver<'_, '_>, fragment: &AstFragment, expansion: LocalExpnId, ) { - let (parent_def, impl_trait_context, in_attr) = resolver.invocation_parents[&expansion]; - let mut visitor = DefCollector { resolver, parent_def, expansion, impl_trait_context, in_attr }; + let InvocationParent { parent_def, pending_anon_const_info, impl_trait_context, in_attr } = + resolver.invocation_parents[&expansion]; + let mut visitor = DefCollector { + resolver, + parent_def, + pending_anon_const_info, + expansion, + impl_trait_context, + in_attr, + }; fragment.visit_with(&mut visitor); } @@ -27,6 +35,13 @@ pub(crate) fn collect_definitions( struct DefCollector<'a, 'b, 'tcx> { resolver: &'a mut Resolver<'b, 'tcx>, parent_def: LocalDefId, + /// If we have an anon const that consists of a macro invocation, e.g. `Foo<{ m!() }>`, + /// we need to wait until we know what the macro expands to before we create the def for + /// the anon const. That's because we lower some anon consts into `hir::ConstArgKind::Path`, + /// which don't have defs. + /// + /// See `Self::visit_anon_const()`. + pending_anon_const_info: Option, impl_trait_context: ImplTraitContext, in_attr: bool, expansion: LocalExpnId, @@ -110,10 +125,16 @@ impl<'a, 'b, 'tcx> DefCollector<'a, 'b, 'tcx> { fn visit_macro_invoc(&mut self, id: NodeId) { let id = id.placeholder_to_expn_id(); - let old_parent = self - .resolver - .invocation_parents - .insert(id, (self.parent_def, self.impl_trait_context, self.in_attr)); + let pending_anon_const_info = self.pending_anon_const_info.take(); + let old_parent = self.resolver.invocation_parents.insert( + id, + InvocationParent { + parent_def: self.parent_def, + pending_anon_const_info, + impl_trait_context: self.impl_trait_context, + in_attr: self.in_attr, + }, + ); assert!(old_parent.is_none(), "parent `LocalDefId` is reset for an invocation"); } } @@ -320,7 +341,13 @@ impl<'a, 'b, 'tcx> visit::Visitor<'a> for DefCollector<'a, 'b, 'tcx> { // def. we'll then create a def later in ast lowering in this case. the parent of nested // items will be messed up, but that's ok because there can't be any if we're just looking // for bare idents. - if constant.value.is_potential_trivial_const_arg() { + + if matches!(constant.value.maybe_unwrap_block().kind, ExprKind::MacCall(..)) { + // See self.pending_anon_const_info for explanation + self.pending_anon_const_info = + Some(PendingAnonConstInfo { id: constant.id, span: constant.value.span }); + visit::walk_anon_const(self, constant) + } else if constant.value.is_potential_trivial_const_arg() { visit::walk_anon_const(self, constant) } else { let def = @@ -330,48 +357,67 @@ impl<'a, 'b, 'tcx> visit::Visitor<'a> for DefCollector<'a, 'b, 'tcx> { } fn visit_expr(&mut self, expr: &'a Expr) { - let parent_def = match expr.kind { - ExprKind::MacCall(..) => return self.visit_macro_invoc(expr.id), - ExprKind::Closure(ref closure) => { - // Async closures desugar to closures inside of closures, so - // we must create two defs. - let closure_def = self.create_def(expr.id, kw::Empty, DefKind::Closure, expr.span); - match closure.coroutine_kind { - Some(coroutine_kind) => { - self.with_parent(closure_def, |this| { - let coroutine_def = this.create_def( - coroutine_kind.closure_id(), - kw::Empty, - DefKind::Closure, - expr.span, - ); - this.with_parent(coroutine_def, |this| visit::walk_expr(this, expr)); - }); - return; + if matches!(expr.kind, ExprKind::MacCall(..)) { + return self.visit_macro_invoc(expr.id); + } + + let grandparent_def = if let Some(pending_anon) = self.pending_anon_const_info.take() { + // See self.pending_anon_const_info for explanation + if !expr.is_potential_trivial_const_arg() { + self.create_def(pending_anon.id, kw::Empty, DefKind::AnonConst, pending_anon.span) + } else { + self.parent_def + } + } else { + self.parent_def + }; + + self.with_parent(grandparent_def, |this| { + let parent_def = match expr.kind { + ExprKind::Closure(ref closure) => { + // Async closures desugar to closures inside of closures, so + // we must create two defs. + let closure_def = + this.create_def(expr.id, kw::Empty, DefKind::Closure, expr.span); + match closure.coroutine_kind { + Some(coroutine_kind) => { + this.with_parent(closure_def, |this| { + let coroutine_def = this.create_def( + coroutine_kind.closure_id(), + kw::Empty, + DefKind::Closure, + expr.span, + ); + this.with_parent(coroutine_def, |this| { + visit::walk_expr(this, expr) + }); + }); + return; + } + None => closure_def, } - None => closure_def, } - } - ExprKind::Gen(_, _, _, _) => { - self.create_def(expr.id, kw::Empty, DefKind::Closure, expr.span) - } - ExprKind::ConstBlock(ref constant) => { - for attr in &expr.attrs { - visit::walk_attribute(self, attr); + ExprKind::Gen(_, _, _, _) => { + this.create_def(expr.id, kw::Empty, DefKind::Closure, expr.span) } - let def = self.create_def( - constant.id, - kw::Empty, - DefKind::InlineConst, - constant.value.span, - ); - self.with_parent(def, |this| visit::walk_anon_const(this, constant)); - return; - } - _ => self.parent_def, - }; + ExprKind::ConstBlock(ref constant) => { + for attr in &expr.attrs { + visit::walk_attribute(this, attr); + } + let def = this.create_def( + constant.id, + kw::Empty, + DefKind::InlineConst, + constant.value.span, + ); + this.with_parent(def, |this| visit::walk_anon_const(this, constant)); + return; + } + _ => this.parent_def, + }; - self.with_parent(parent_def, |this| visit::walk_expr(this, expr)); + this.with_parent(parent_def, |this| visit::walk_expr(this, expr)) + }) } fn visit_ty(&mut self, ty: &'a Ty) { diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 02fdc1ae6685e..92c807f945120 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -170,6 +170,29 @@ impl<'a> ParentScope<'a> { } } +#[derive(Copy, Debug, Clone)] +struct InvocationParent { + parent_def: LocalDefId, + pending_anon_const_info: Option, + impl_trait_context: ImplTraitContext, + in_attr: bool, +} + +impl InvocationParent { + const ROOT: Self = Self { + parent_def: CRATE_DEF_ID, + pending_anon_const_info: None, + impl_trait_context: ImplTraitContext::Existential, + in_attr: false, + }; +} + +#[derive(Copy, Debug, Clone)] +struct PendingAnonConstInfo { + id: NodeId, + span: Span, +} + #[derive(Copy, Debug, Clone)] enum ImplTraitContext { Existential, @@ -1143,7 +1166,7 @@ pub struct Resolver<'a, 'tcx> { /// When collecting definitions from an AST fragment produced by a macro invocation `ExpnId` /// we know what parent node that fragment should be attached to thanks to this table, /// and how the `impl Trait` fragments were introduced. - invocation_parents: FxHashMap, + invocation_parents: FxHashMap, /// Some way to know that we are in a *trait* impl in `visit_assoc_item`. /// FIXME: Replace with a more general AST map (together with some other fields). @@ -1380,8 +1403,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { node_id_to_def_id.insert(CRATE_NODE_ID, crate_feed); let mut invocation_parents = FxHashMap::default(); - invocation_parents - .insert(LocalExpnId::ROOT, (CRATE_DEF_ID, ImplTraitContext::Existential, false)); + invocation_parents.insert(LocalExpnId::ROOT, InvocationParent::ROOT); let mut extern_prelude: FxHashMap> = tcx .sess diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index 7203fbe4a0c18..d6afd01f48748 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -42,9 +42,9 @@ use crate::errors::{ use crate::imports::Import; use crate::Namespace::*; use crate::{ - BindingKey, BuiltinMacroState, DeriveData, Determinacy, Finalize, MacroData, ModuleKind, - ModuleOrUniformRoot, NameBinding, NameBindingKind, ParentScope, PathResult, ResolutionError, - Resolver, ScopeSet, Segment, ToNameBinding, Used, + BindingKey, BuiltinMacroState, DeriveData, Determinacy, Finalize, InvocationParent, MacroData, + ModuleKind, ModuleOrUniformRoot, NameBinding, NameBindingKind, ParentScope, PathResult, + ResolutionError, Resolver, ScopeSet, Segment, ToNameBinding, Used, }; type Res = def::Res; @@ -183,7 +183,7 @@ impl<'a, 'tcx> ResolverExpand for Resolver<'a, 'tcx> { } fn invocation_parent(&self, id: LocalExpnId) -> LocalDefId { - self.invocation_parents[&id].0 + self.invocation_parents[&id].parent_def } fn resolve_dollar_crates(&mut self) { @@ -303,12 +303,12 @@ impl<'a, 'tcx> ResolverExpand for Resolver<'a, 'tcx> { .invocation_parents .get(&invoc_id) .or_else(|| self.invocation_parents.get(&eager_expansion_root)) - .filter(|&&(mod_def_id, _, in_attr)| { + .filter(|&&InvocationParent { parent_def: mod_def_id, in_attr, .. }| { in_attr && invoc.fragment_kind == AstFragmentKind::Expr && self.tcx.def_kind(mod_def_id) == DefKind::Mod }) - .map(|&(mod_def_id, ..)| mod_def_id); + .map(|&InvocationParent { parent_def: mod_def_id, .. }| mod_def_id); let (ext, res) = self.smart_resolve_macro_path( path, kind, @@ -951,7 +951,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let node_id = self .invocation_parents .get(&parent_scope.expansion) - .map_or(ast::CRATE_NODE_ID, |id| self.def_id_to_node_id[id.0]); + .map_or(ast::CRATE_NODE_ID, |parent| { + self.def_id_to_node_id[parent.parent_def] + }); self.lint_buffer.buffer_lint( LEGACY_DERIVE_HELPERS, node_id, diff --git a/tests/crashes/128016.rs b/tests/crashes/128016.rs deleted file mode 100644 index d23721ae14e3a..0000000000000 --- a/tests/crashes/128016.rs +++ /dev/null @@ -1,10 +0,0 @@ -//@ known-bug: #128016 -macro_rules! len { - () => { - target - }; -} - -fn main() { - let val: [str; len!()] = []; -} diff --git a/tests/ui/const-generics/early/trivial-const-arg-macro-nested.rs b/tests/ui/const-generics/early/trivial-const-arg-macro-nested.rs new file mode 100644 index 0000000000000..f9730cf85661a --- /dev/null +++ b/tests/ui/const-generics/early/trivial-const-arg-macro-nested.rs @@ -0,0 +1,21 @@ +//@ check-pass + +// This is a regression test for #128016. + +macro_rules! len_inner { + () => { + BAR + }; +} + +macro_rules! len { + () => { + len_inner!() + }; +} + +const BAR: usize = 0; + +fn main() { + let val: [bool; len!()] = []; +} diff --git a/tests/ui/const-generics/early/trivial-const-arg-macro-param.rs b/tests/ui/const-generics/early/trivial-const-arg-macro-param.rs new file mode 100644 index 0000000000000..f123e55c028c3 --- /dev/null +++ b/tests/ui/const-generics/early/trivial-const-arg-macro-param.rs @@ -0,0 +1,13 @@ +//@ check-pass + +macro_rules! len { + ($x:ident) => { + $x + }; +} + +fn bar() { + let val: [bool; len!(N)] = [true; N]; +} + +fn main() {} diff --git a/tests/ui/const-generics/early/trivial-const-arg-macro-res-error.rs b/tests/ui/const-generics/early/trivial-const-arg-macro-res-error.rs new file mode 100644 index 0000000000000..f218caac0cf98 --- /dev/null +++ b/tests/ui/const-generics/early/trivial-const-arg-macro-res-error.rs @@ -0,0 +1,13 @@ +// This is a regression test for #128016. + +macro_rules! len { + () => { + target + //~^ ERROR cannot find value `target` + }; +} + +fn main() { + let val: [str; len!()] = []; + //~^ ERROR the size for values +} diff --git a/tests/ui/const-generics/early/trivial-const-arg-macro-res-error.stderr b/tests/ui/const-generics/early/trivial-const-arg-macro-res-error.stderr new file mode 100644 index 0000000000000..ab289e5a6b78f --- /dev/null +++ b/tests/ui/const-generics/early/trivial-const-arg-macro-res-error.stderr @@ -0,0 +1,24 @@ +error[E0425]: cannot find value `target` in this scope + --> $DIR/trivial-const-arg-macro-res-error.rs:5:9 + | +LL | target + | ^^^^^^ not found in this scope +... +LL | let val: [str; len!()] = []; + | ------ in this macro invocation + | + = note: this error originates in the macro `len` (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: the size for values of type `str` cannot be known at compilation time + --> $DIR/trivial-const-arg-macro-res-error.rs:11:14 + | +LL | let val: [str; len!()] = []; + | ^^^^^^^^^^^^^ doesn't have a size known at compile-time + | + = help: the trait `Sized` is not implemented for `str` + = note: slice and array elements must have `Sized` type + +error: aborting due to 2 previous errors + +Some errors have detailed explanations: E0277, E0425. +For more information about an error, try `rustc --explain E0277`. diff --git a/tests/ui/const-generics/early/trivial-const-arg-macro.rs b/tests/ui/const-generics/early/trivial-const-arg-macro.rs new file mode 100644 index 0000000000000..a19d9abfdcb05 --- /dev/null +++ b/tests/ui/const-generics/early/trivial-const-arg-macro.rs @@ -0,0 +1,15 @@ +//@ check-pass + +// This is a regression test for #128016. + +macro_rules! len { + () => { + BAR + }; +} + +const BAR: usize = 0; + +fn main() { + let val: [bool; len!()] = []; +}