Skip to content

Commit

Permalink
Fix anon const def-creation when macros are involved
Browse files Browse the repository at this point in the history
Ever since rust-lang#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 rust-lang#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 rust-lang#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`.
  • Loading branch information
camelid committed Sep 8, 2024
1 parent 878f49f commit fae1882
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 52 deletions.
20 changes: 12 additions & 8 deletions compiler/rustc_ast/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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<GenericBound> {
match &self.kind {
ExprKind::Path(None, path) => Some(GenericBound::Trait(
Expand Down
110 changes: 76 additions & 34 deletions compiler/rustc_resolve/src/def_collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,37 @@ 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);
}

/// Creates `DefId`s for nodes in the AST.
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<PendingAnonConstInfo>,
impl_trait_context: ImplTraitContext,
in_attr: bool,
expansion: LocalExpnId,
Expand Down Expand Up @@ -111,10 +126,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");
}
}
Expand Down Expand Up @@ -326,46 +347,67 @@ impl<'a, 'b, 'tcx> visit::Visitor<'a> for DefCollector<'a, 'b, 'tcx> {
}

fn visit_anon_const(&mut self, constant: &'a AnonConst) {
if self.resolver.tcx.features().const_arg_path
&& constant.value.is_potential_trivial_const_arg()
{
if self.resolver.tcx.features().const_arg_path {
// HACK(min_generic_const_args): don't create defs for anon consts if we think they will
// later be turned into ConstArgKind::Path's. because this is before resolve is done, we
// may accidentally identify a construction of a unit struct as a param and not create a
// 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.
visit::walk_anon_const(self, constant)
} else {
let def =
self.create_def(constant.id, kw::Empty, DefKind::AnonConst, constant.value.span);
self.with_parent(def, |this| visit::walk_anon_const(this, constant));

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 });
return visit::walk_anon_const(self, constant);
} else if constant.value.is_potential_trivial_const_arg() {
return visit::walk_anon_const(self, constant);
}
}

let def = self.create_def(constant.id, kw::Empty, DefKind::AnonConst, constant.value.span);
self.with_parent(def, |this| visit::walk_anon_const(this, constant));
}

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(..) | 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);
}
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;
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
}
_ => self.parent_def,
} else {
self.parent_def
};

self.with_parent(parent_def, |this| visit::walk_expr(this, expr));
self.with_parent(grandparent_def, |this| {
let parent_def = match expr.kind {
ExprKind::Closure(..) | ExprKind::Gen(..) => {
this.create_def(expr.id, kw::Empty, DefKind::Closure, expr.span)
}
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,
};

this.with_parent(parent_def, |this| visit::walk_expr(this, expr))
})
}

fn visit_ty(&mut self, ty: &'a Ty) {
Expand Down
28 changes: 25 additions & 3 deletions compiler/rustc_resolve/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,29 @@ impl<'a> ParentScope<'a> {
}
}

#[derive(Copy, Debug, Clone)]
struct InvocationParent {
parent_def: LocalDefId,
pending_anon_const_info: Option<PendingAnonConstInfo>,
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,
Expand Down Expand Up @@ -1144,7 +1167,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<LocalExpnId, (LocalDefId, ImplTraitContext, bool /*in_attr*/)>,
invocation_parents: FxHashMap<LocalExpnId, InvocationParent>,

/// 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).
Expand Down Expand Up @@ -1381,8 +1404,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<Ident, ExternPreludeEntry<'_>> = tcx
.sess
Expand Down
16 changes: 9 additions & 7 deletions compiler/rustc_resolve/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<NodeId>;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit fae1882

Please sign in to comment.