[WIP] Delay def collection for expression-like nodes #128844

camelid · 2024-08-08T19:27:34Z

These expression-like nodes are anon const args, const blocks, closures,
and generators. They need to have their def collection delayed because
some anon const args turn into ConstArgKind::Path, which has no DefId.
Thus defs that could have an anon const as their parent must be delayed
as well.

r? @BoxyUwU

These expression-like nodes are anon const args, const blocks, closures, and generators. They need to have their def collection delayed because some anon const args turn into `ConstArgKind::Path`, which has no DefId. Thus defs that could have an anon const as their parent must be delayed as well.

This ensures the correct DefId <-> HirId mapping is made.

rustbot · 2024-08-10T03:43:45Z

HIR ty lowering was modified

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

cjgillot · 2024-08-10T11:16:14Z

compiler/rustc_ast_lowering/src/expr.rs

            let hir_id = self.lower_node_id(e.id);
            self.lower_attrs(hir_id, &e.attrs);

            let kind = match &e.kind {
                ExprKind::Array(exprs) => hir::ExprKind::Array(self.lower_exprs(exprs)),
                ExprKind::ConstBlock(c) => {
                    let c = self.with_new_scopes(c.value.span, |this| {
-                        let def_id = this.create_def(


If the NodeId has already been lowered, create_def should either ICE or register it correctly.

What do you want to see changed here? I'm a little confused by the purpose of this comment haha
edit: oh this is on an old commit I see 😅 nevermind

cjgillot · 2024-08-10T11:18:36Z

compiler/rustc_ast_lowering/src/lib.rs

@@ -828,7 +828,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
            LifetimeRes::Fresh { param, kind, .. } => {
                // Late resolution delegates to us the creation of the `LocalDefId`.
                let _def_id = self.create_def(
-                    self.current_hir_id_owner.def_id, // FIXME: should this use self.current_def_id_parent?
+                    self.current_def_id_parent,


Should we remove that parameter and have create_def always use current_def_id_parent?

That would make sense to me 👍 I think using current_hir_id_owner only makes sense if either:

We statically know that the current_def_id_parent is the innermost hir owner

All definitions participating in the def parenting tree are also hir owners (aka, the former point is always trivially true)

I think right now there should never be any cases where we're creating defs that wants to skip a parent. The struct definition in fn foo() -> Foo<{ struct Bar; 1 }> does have that behaviour since its parent is foo not constant#0 but that' happens via creating the DefId in DefCollector.

BoxyUwU · 2024-08-10T17:33:27Z

Context for why the Item::OpaqueTy changes since @cjgillot has started looking at this PR:

note: "this PR" indicates "this PR before the changes to opaques" as this was written from before those changes were made

Given the code example:

trait Trait<const N: usize> {}

#[rustfmt::skip]
fn foo() -> impl Trait<{ struct Bar; 1 }> {}

impl Trait<1> for () {}

The free_items for this module are:

free_items: [
  ItemId { owner_id: DefId(0:1 ~ playground_1[9aa8]::{use#0}) },
  ItemId { owner_id: DefId(0:2 ~ playground_1[9aa8]::std) },
  ItemId { owner_id: DefId(0:3 ~ playground_1[9aa8]::Trait) },
  ItemId { owner_id: DefId(0:7 ~ playground_1[9aa8]::foo) },
  ItemId { owner_id: DefId(0:11 ~ playground_1[9aa8]::foo::{opaque#0}) },
  ItemId { owner_id: DefId(0:9 ~ playground_1[9aa8]::foo::{constant#0}::Bar) },
  ItemId { owner_id: DefId(0:5 ~ playground_1[9aa8]::{impl#0}) }
]

so, HirIdValidator gets used on foo, opaque#0 and Bar, but not the anon const

foo it will recurse and call visit_nested_item on the opaque which doesn't recurse, and just checks that the parent DefId's hir's owner is foo
opaque#0it will recurse and... check the HirId of the anon const is right (for some definition of right, unrelated to any of this), then recurse inside it, then visit Bar as a nested item and check that: Bar's def parent's hir's owner is opaque#0,
- On nightly that def parent is the anon const, and the anon consts hir is indeed owned by opaque#0
- On this PR where items (that arent nested bodies) that are in anon consts are not parented to the anon const, Bar's def parent is foo, so we attempt to check that foo's hir's owner is opaque#0 which it is not so we ICE

On nightly:
The def parent setup for Bar is foo::{constant}::Bar, foo and opaque#0 are hir owners. When we validate opaque's HirId's, it succeeds as we ignore anon consts, and the fact that constant exists means that Bar's parent's hir is owned opaque.
On this PR:
The def parent setup for bar is foo::Bar, foo and opaque#0 are hir owners. When we validate opaques HirIds, it fails as constant#0 is no longer the parent of Bar so it correctly detects that we have a nested thing in opaque#0 with a parent that is "outside" of opaque in a different hir owner's hir

Conclusion: Opaque types either need to have their DefId created in DefCollector or needs to stop being a hir owner. Or, generally, if something is to be a hir owner node, the DefId for it should be created before all children DefIds or else you cannot create the parent structure correctly.

We need to recurse into them since they may contain anon consts with code inside, e.g.

Note that the new test output is in most cases incorrect or undesirable. This should be fixed before merging. The purpose of this commit is merely to see what changes occurred.

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`.

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 ideal long-term fix for this is a bit unclear. One option is to delay def creation 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, this approach has some downsides as well, and the best approach is yet to be determined. 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`.

Fix anon const def-creation when macros are involved Fixes rust-lang#128016. 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`. r? `@BoxyUwU`

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 ideal long-term fix for this is a bit unclear. One option is to delay def creation 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, this approach has some downsides as well, and the best approach is yet to be determined. 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`.

rustbot assigned BoxyUwU Aug 8, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 8, 2024