Opaque types should not be HIR items (and thus not be HIR owners) #129023
Comments
camelid
added
C-cleanup
rustbot
added
the
needs-triage
camelid
removed
the
needs-triage
compiler-errors
added
E-hard
|
I talked to Boxy and I'd like to take a look at this, unless someone like @cjgillot knows how to fix it and has the time. I've relabeled this as E-hard since it likely requires a lot of patience going through the HIR visitors to ensure they're handling opaques correctly now. |
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`.
|
I posted a draft here: #129244 |
What are the complications exactly?
Ideally I'd like to see a design document (maybe a compiler MCP) describing what are the requirements on the Ideally the whole If creation is delayed for some |
|
Def id creation for |
|
For
The "created early" alternatives seem preferable to me, because the node is properly integrated into the def id tree this way. |
The directly opposite alternative is to delay creation of
Some schemes in the middle between these two opposite alternatives are possible, I'm again not sure why we'd choose them (but that's again a question of what we want from the def id tree in general #129023 (comment)). |
Make opaque types regular HIR nodes Having opaque types as HIR owner introduces all sorts of complications. This PR proposes to make them regular HIR nodes instead. I haven't gone through all the test changes yet, so there may be a few surprises. Many thanks to `@camelid` for the first draft. Fixes rust-lang#129023 Fixes rust-lang#129099 Fixes rust-lang#125843 Fixes rust-lang#119716 Fixes rust-lang#121422
|
On the general direction, I agree we should strive to have all non-leaf definitions created in the defcollector. For the simple reason that this ensures consistency of the def tree. One reason we create impltrait definitions late is that the defcollector was buggy, and having the desugaring in a single place was simpler. But I'd like to try reverting it and fix def collector. |
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`.
…chenkov Create opaque definitions in resolver. Implementing rust-lang#129023 (comment) That was easier than I expected. r? `@petrochenkov`
Rollup merge of rust-lang#129493 - cjgillot:early-opaque-def, r=petrochenkov Create opaque definitions in resolver. Implementing rust-lang#129023 (comment) That was easier than I expected. r? `@petrochenkov`
Create opaque definitions in resolver. Implementing rust-lang/rust#129023 (comment) That was easier than I expected. r? `@petrochenkov`
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`.
|
With #129493 having landed I think we can close this ^^ The problem of "its not possible to parent defs inside of opaque types correctly" has been resolved |
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`
Create opaque definitions in resolver. Implementing rust-lang/rust#129023 (comment) That was easier than I expected. r? `@petrochenkov`
Make opaque types regular HIR nodes Having opaque types as HIR owner introduces all sorts of complications. This PR proposes to make them regular HIR nodes instead. I haven't gone through all the test changes yet, so there may be a few surprises. Many thanks to `@camelid` for the first draft. Fixes rust-lang#129023 Fixes rust-lang#129099 Fixes rust-lang#125843 Fixes rust-lang#119716 Fixes rust-lang#121422
Make opaque types regular HIR nodes Having opaque types as HIR owner introduces all sorts of complications. This PR proposes to make them regular HIR nodes instead. I haven't gone through all the test changes yet, so there may be a few surprises. Many thanks to `@camelid` for the first draft. Fixes rust-lang/rust#129023 Fixes #129099 Fixes #125843 Fixes #119716 Fixes #121422
Summary
hir::ItemKind::OpaqueTy, addhir::Node::OpaqueTyItemIdinhir::TyKind::OpaqueDefwith&'hir OpaqueTyA WIP version of much of this can be seen in 79a3e7a...dcb9e9a, but it's incomplete. The reason is that many parts of the compiler assume that opaques are items and handle them through implicit codepaths.
Background
Currently, opaque types (e.g., return-position
impl Trait) are lowered into ahir::ItemKind::OpaqueTyand then ahir::TyKind::OpaqueDefthat references that item's ID. However, because opaques'DefIds are created inrustc_ast_lowering, yet opaques can contain nodes whoseDefIds are created inDefCollector(such as anon consts, which may in turn contain arbitrary nested items), this causes an incorrect parenting structure. This issue was uncovered in #128844, and it impeded that change, which delayed creation of defs for all expression-like nodes (anon consts, closures, etc.) untilast_loweringto fix other issues relating to def parents. @BoxyUwU summarized the issue in that PR's thread, reproduced here:As mentioned above, the alternative is to create
DefIds for opaques ahead-of-time, inDefCollector, but that is unsatisfactory because lowering of opaque types is quite complicated and that logic exists inrustc_ast_lowering. IMO it also makes more intuitive sense to create defs for things that don't themselves participate in resolution, such as anon consts and opaque types, duringast_loweringrather than def collection.The text was updated successfully, but these errors were encountered: