Repository navigation
Opaque types should not be HIR items (and thus not be HIR owners) #129023
Description
Summary
- Delete
hir::ItemKind::OpaqueTy, addhir::Node::OpaqueTy - Replace the
ItemIdinhir::TyKind::OpaqueDefwith&'hir OpaqueTy - Fix the fallout
A 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 a hir::ItemKind::OpaqueTy and then a hir::TyKind::OpaqueDef that references that item's ID. However, because opaques' DefIds are created in rustc_ast_lowering, yet opaques can contain nodes whose DefIds are created in DefCollector (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.) until ast_lowering to fix other issues relating to def parents. @BoxyUwU summarized the issue in that PR's thread, reproduced here:
Context for why the
Item::OpaqueTychanges 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_itemsfor 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,
HirIdValidatorgets used onfoo,opaque#0andBar,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#0`it 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 ICEOn nightly: The def parent setup for
Barisfoo::{constant}::Bar,fooandopaque#0are hir owners. When we validateopaque's HirId's, it succeeds as we ignore anon consts, and the fact thatconstantexists means thatBar's parent's hir is ownedopaque. On this PR: The def parent setup forbarisfoo::Bar,fooandopaque#0are hir owners. When we validateopaques HirIds, it fails asconstant#0is no longer the parent ofBarso it correctly detects that we have a nested thing inopaque#0with a parent that is "outside" ofopaquein a different hir owner's hirConclusion: Opaque types either need to have their
DefIdcreated inDefCollectoror needs to stop being a hir owner. Or, generally, if something is to be a hir owner node, theDefIdfor it should be created before all childrenDefIds or else you cannot create the parent structure correctly.
As mentioned above, the alternative is to create DefIds for opaques ahead-of-time, in DefCollector, but that is unsatisfactory because lowering of opaque types is quite complicated and that logic exists in rustc_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, during ast_lowering rather than def collection.
Activity
compiler-errors commentedon Aug 12, 2024
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.
Fix anon const def-creation when macros are involved
cjgillot commentedon Aug 18, 2024
I posted a draft here: #129244
petrochenkov commentedon Aug 19, 2024
What are the complications exactly?
To created a
DefIdyou need to 1) seeimpl Traitwhich is visible syntactically and 2) know that it is in return position and not argument position, that information is already tracked by def collector.Ideally I'd like to see a design document (maybe a compiler MCP) describing what are the requirements on the
DefIdtree, what we want from theDefIdparenting relation in particular.I haven't read all the relevant PRs yet (#129137 (comment)), but so far everything starting from #125915 seems to be going into a wrong direction to me.
Ideally the whole
DefIdtree should be built in def collector, as I see it, because we can create "fully capable" def ids there, connected by parenting relations with macro calls in particular.We can create additional "dummy"
DefIds if for some node it is unclear at def collection time whether it really needs theDefIdor not (this is relevant tomin_generic_const_args).If creation is delayed for some
DefId, then it will suffer from inability to have children subtrees. Is it acceptable for some nodes? Maybe, then it should be documented why it is acceptable.For some nodes, like closures, it doesn't look acceptable - if we skip closures from the parenting relations then why not continue and skip e.g.
fnitems as well.1 remaining item
petrochenkov commentedon Aug 19, 2024
For
impl Traitthe questions of being or not being an item, and late or earlyDefIdcreation are sort of orthogonal (not entirely).All four alternatives are possible:
compiler\rustc_ty_utils\src\assoc.rs- associated items, HIR owners (own trivial synthetic HIR with no issues), created lateThe "created early" alternatives seem preferable to me, because the node is properly integrated into the def id tree this way.
The choice between "item" and "not item" needs to be made based on some other criteria, maybe "not item" is indeed better, I haven't carefully looked at #129244 yet.
petrochenkov commentedon Aug 20, 2024
The directly opposite alternative is to delay creation of
DefIds for all nodes that do not own nontrivial HIR.What the consequences of that will be:
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)).
Creating everything in def collector (maybe creating more than strictly necessary) still looks like the simplest solution to me.
Auto merge of rust-lang#129244 - cjgillot:opaque-hir, r=<try>
cjgillot commentedon Aug 23, 2024
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.
Fix anon const def-creation when macros are involved
Rollup merge of rust-lang#129493 - cjgillot:early-opaque-def, r=petro…
Unrolled build for rust-lang#129493
Rollup merge of #129493 - cjgillot:early-opaque-def, r=petrochenkov
Fix anon const def-creation when macros are involved
BoxyUwU commentedon Sep 12, 2024
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
Auto merge of rust-lang#129137 - camelid:lazy-def-macro-const, r=BoxyUwU
Rollup merge of #129493 - cjgillot:early-opaque-def, r=petrochenkov
Auto merge of rust-lang#129244 - cjgillot:opaque-hir, r=compiler-errors
Auto merge of #129244 - cjgillot:opaque-hir, r=compiler-errors