Make opaque types regular HIR nodes #129244

cjgillot · 2024-08-18T16:55:08Z

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 #129023

Fixes #129099
Fixes #125843
Fixes #119716
Fixes #121422

rustbot · 2024-08-18T16:55:16Z

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-08-18T16:55:18Z

HIR ty lowering was modified

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler/rustc_hir_analysis/src/collect.rs

compiler/rustc_privacy/src/lib.rs

tests/ui/async-await/in-trait/async-generics-and-bounds.stderr

tests/ui/async-await/multiple-lifetimes/ret-impl-trait-one.stderr

compiler/rustc_ast_lowering/src/index.rs

compiler/rustc_middle/src/hir/mod.rs

compiler/rustc_middle/src/query/mod.rs

compiler/rustc_privacy/src/lib.rs

petrochenkov · 2024-08-20T13:45:59Z

Ignoring all the ID parenting questions, the change looks reasonable to me because

It reduces weirdness
- We have some other nodes, like closures, that could could technically be "outlined" into items, but they are typically kept inline, and opaques now behave analogously to closures
It doesn't complicate the compiler logic while reducing weirdness

Regarding DefIds, HirIds and parenting, the change is not strictly necessary (#129023 (comment)) to fix the HIR ownership issues, and I'd like to discuss that topic separately (see my other messages in #129023).

cjgillot · 2024-08-22T21:11:32Z

Until I fix clippy:
@bors try @rust-timer queue

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

bors · 2024-08-22T21:12:44Z

⌛ Trying commit b66cc38 with merge b64686e...

bors · 2024-08-22T23:08:09Z

☀️ Try build successful - checks-actions
Build commit: b64686e (b64686e62be3a3e957a5559eae0d8cd2f7cbee0c)

bors · 2024-09-07T21:21:51Z

☔ The latest upstream changes (presumably #126161) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors

This looks great to me. The fact that opaques were separate items was a constant source of annoyance and I'm glad to see this being settled.

I reviewed everything and only had a few questions. I am not totally caught up on the DefId mismatch problem, was that resolved?

This needs a rebase, and I'd like to take a quick look afterwards, so please ping me when that happens.

compiler-errors · 2024-10-02T02:18:54Z

compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs

@@ -686,22 +678,19 @@ impl<'a, 'tcx> Visitor<'tcx> for BoundVarContext<'a, 'tcx> {
            hir::TyKind::Ref(lifetime_ref, ref mt) => {
                self.visit_lifetime(lifetime_ref);
                let scope = Scope::ObjectLifetimeDefault {
-                    lifetime: self.map.defs.get(&lifetime_ref.hir_id).cloned(),
+                    lifetime: self.map.defs.get(&lifetime_ref.hir_id.local_id).cloned(),


Is there any way we can assert that the lifetime's OwnerId is the same owner that we're resolving here?

And elsewhere throughout this file.

I'm possibly just being paranoid, though.

I'm not convinced this is useful. We don't track the root OwnerId, so we'd need to add it, for limited benefit IMHO.

compiler/rustc_ast_lowering/src/index.rs

This introduce an additional collection of opaques on HIR, as they can no longer be listed using the free item list.

compiler-errors · 2024-10-05T00:39:53Z

@bors r+ rollup=never

bors · 2024-10-05T00:39:56Z

📌 Commit ef17eb7 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-10-05T06:19:39Z

⌛ Testing commit ef17eb7 with merge 5a4ee43...

bors · 2024-10-05T08:48:00Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 5a4ee43 to master...

rust-timer · 2024-10-05T10:07:10Z

Finished benchmarking commit (5a4ee43): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	2
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.1%]	13
Improvements ✅ (secondary)	-0.6%	[-1.4%, -0.2%]	23
All ❌✅ (primary)	-0.2%	[-0.3%, -0.1%]	13

Max RSS (memory usage)

Results (primary -1.9%, secondary -1.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-3.5%, -0.8%]	19
Improvements ✅ (secondary)	-1.2%	[-3.1%, -0.6%]	4
All ❌✅ (primary)	-1.9%	[-3.5%, 2.7%]	20

Cycles

Results (secondary -2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-4.3%, -1.4%]	9
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 770.7s -> 774.178s (0.45%)
Artifact size: 342.04 MiB -> 342.23 MiB (0.06%)

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

…r-errors,petrochenkov Remap impl-trait lifetimes on HIR instead of AST lowering Current AST->HIR lowering goes out of its way to remap lifetimes for opaque types. This is complicated and leaks into upstream and downstream code. This PR stops trying to be clever during lowering, and prefers to do this remapping during the HIR->ty lowering. The remapping computation easily fits into the bound var resolution code. Its result can be used in by `generics_of` and `hir_ty_lowering::new_opaque` to add the proper parameters and arguments. See an example on the doc for query `opaque_captured_lifetimes`. Based on rust-lang#129244 Fixes rust-lang#125249 Fixes rust-lang#126850 cc `@compiler-errors` `@spastorino` r? `@petrochenkov`

Rollup merge of rust-lang#129383 - cjgillot:opaque-noremap, r=compiler-errors,petrochenkov Remap impl-trait lifetimes on HIR instead of AST lowering Current AST->HIR lowering goes out of its way to remap lifetimes for opaque types. This is complicated and leaks into upstream and downstream code. This PR stops trying to be clever during lowering, and prefers to do this remapping during the HIR->ty lowering. The remapping computation easily fits into the bound var resolution code. Its result can be used in by `generics_of` and `hir_ty_lowering::new_opaque` to add the proper parameters and arguments. See an example on the doc for query `opaque_captured_lifetimes`. Based on rust-lang#129244 Fixes rust-lang#125249 Fixes rust-lang#126850 cc `@compiler-errors` `@spastorino` r? `@petrochenkov`

rustbot assigned michaelwoerister Aug 18, 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 18, 2024

cjgillot mentioned this pull request Aug 18, 2024

Opaque types should not be HIR items (and thus not be HIR owners) #129023

Closed

compiler-errors self-assigned this Aug 18, 2024

This comment has been minimized.

Sign in to view

cjgillot commented Aug 18, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

camelid mentioned this pull request Aug 18, 2024

Delay def creation for expression-like nodes #129247

Open

petrochenkov self-assigned this Aug 18, 2024

michaelwoerister removed their assignment Aug 19, 2024

petrochenkov reviewed Aug 20, 2024

View reviewed changes

compiler/rustc_ast_lowering/src/index.rs Show resolved Hide resolved

compiler/rustc_middle/src/hir/mod.rs Show resolved Hide resolved

compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_privacy/src/lib.rs Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2024

This comment was marked as resolved.

Sign in to view

petrochenkov mentioned this pull request Aug 20, 2024

Fix anon const def-creation when macros are involved #129137

Merged

cjgillot mentioned this pull request Aug 22, 2024

Remap impl-trait lifetimes on HIR instead of AST lowering #129383

Merged

cjgillot force-pushed the opaque-hir branch from 90b8f3a to b66cc38 Compare August 22, 2024 01:20

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 22, 2024

This comment has been minimized.

Sign in to view

compiler-errors approved these changes Oct 2, 2024

View reviewed changes

cjgillot and others added 11 commits October 4, 2024 23:01

Make query backtrace more useful.

9914472

Make naming more consistent.

4ec7839

rm ItemKind::OpaqueTy

d6f247f

This introduce an additional collection of opaques on HIR, as they can no longer be listed using the free item list.

WfCheck opaques.

68f7ed4

Visit opaques for visibilities.

e740c7b

Bless ui tests.

d9f15fa

Remove stray fixmes.

c7cb45a

Simplify bound var resolution.

6ec58a4

Promote crash tests to ui.

6278e0f

Bless incremental tests.

98941f7

Adapt clippy.

ef17eb7

cjgillot force-pushed the opaque-hir branch from 7782194 to ef17eb7 Compare October 4, 2024 23:50

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 5, 2024

bors merged commit 5a4ee43 into rust-lang:master Oct 5, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Oct 5, 2024

matthiaskrgr mentioned this pull request Oct 5, 2024

ICE: assertion failed: unique #125249

Closed

cjgillot deleted the opaque-hir branch October 5, 2024 11:44

matthiaskrgr mentioned this pull request Oct 8, 2024

ICE: cannot convert "'a/#4" to a region vid #131295

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make opaque types regular HIR nodes #129244

Make opaque types regular HIR nodes #129244

cjgillot commented Aug 18, 2024 •

edited

Loading

rustbot commented Aug 18, 2024

rustbot commented Aug 18, 2024

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Aug 20, 2024 •

edited

Loading

This comment was marked as resolved.

This comment has been minimized.

cjgillot commented Aug 22, 2024

This comment has been minimized.

bors commented Aug 22, 2024

bors commented Aug 22, 2024

This comment has been minimized.

bors commented Sep 7, 2024

compiler-errors left a comment

compiler-errors Oct 2, 2024

compiler-errors Oct 2, 2024

compiler-errors Oct 2, 2024

cjgillot Oct 5, 2024

compiler-errors commented Oct 5, 2024

bors commented Oct 5, 2024

bors commented Oct 5, 2024

bors commented Oct 5, 2024

rust-timer commented Oct 5, 2024

Make opaque types regular HIR nodes #129244

Make opaque types regular HIR nodes #129244

Conversation

cjgillot commented Aug 18, 2024 • edited Loading

rustbot commented Aug 18, 2024

rustbot commented Aug 18, 2024

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Aug 20, 2024 • edited Loading

This comment was marked as resolved.

This comment has been minimized.

cjgillot commented Aug 22, 2024

This comment has been minimized.

bors commented Aug 22, 2024

bors commented Aug 22, 2024

This comment has been minimized.

bors commented Sep 7, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Oct 5, 2024

bors commented Oct 5, 2024

bors commented Oct 5, 2024

bors commented Oct 5, 2024

rust-timer commented Oct 5, 2024

Overall result: ✅ improvements - no action needed

cjgillot commented Aug 18, 2024 •

edited

Loading

petrochenkov commented Aug 20, 2024 •

edited

Loading