Rework dyn trait lowering to stop being so intertwined with trait alias expansion #133830

compiler-errors · 2024-12-03T23:53:09Z

This PR reworks the trait object lowering code to stop handling trait aliases so funky, and removes the TraitAliasExpander in favor of a much simpler design. This refactoring is important for making the code that I'm writing in #133397 understandable and easy to maintain, so the diagnostics regressions are IMO inevitable.

In the old trait object lowering code, we used to be a bit sloppy with the lists of traits in their unexpanded and expanded forms. This PR largely rewrites this logic to expand the trait aliases once and handle them more responsibly throughout afterwards.

Please review this with whitespace disabled.

r? lcnr

rustbot · 2024-12-03T23:53:18Z

HIR ty lowering was modified

cc @fmease

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs

lcnr · 2024-12-04T13:56:30Z

second commit is more than I thought, might take a few days to fully review that one

compiler-errors · 2024-12-04T15:23:43Z

@bors try @rust-timer queue

Rework dyn trait lowering to stop being so intertwined with trait alias expansion This PR reworks the trait object lowering code to stop handling trait aliases so funky, and removes the `TraitAliasExpander` in favor of a much simpler design. This refactoring is important for making the code that I'm writing in rust-lang#133397 understandable and easy to maintain, so the diagnostics regressions are IMO inevitable. ##### First commit: We used to use Span to maintain a per-principal list of missing associated types. We don't support multiple object principals, and I don't believe we intend to do so anytime soon. Let's remove this to greatly simplify the bookkeeping of this function. ##### Second commit: In the old trait object lowering code, we used to be a bit sloppy with the lists of traits in their unexpanded and expanded forms. This PR largely rewrites this logic to expand the trait aliases *once* and handle them more responsibly throughout afterwards. Please review both of these with whitespace disabled. r? lcnr

bors · 2024-12-04T18:03:35Z

⌛ Trying commit af94314 with merge b020ef4...

bors · 2024-12-04T19:48:01Z

☀️ Try build successful - checks-actions
Build commit: b020ef4 (b020ef4229c5573e06021c20dc22f314db4f0782)

rust-timer · 2024-12-04T21:04:51Z

Finished benchmarking commit (b020ef4): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary -1.0%)

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)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.0%	[-1.0%, -1.0%]	1

Cycles

Results (secondary -1.5%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 767.015s -> 767.953s (0.12%)
Artifact size: 330.85 MiB -> 330.86 MiB (0.00%)

lcnr

a few nits and (more than a few) further cleanups, though all of them should hopefully be mechanical.

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

lcnr · 2024-12-17T09:13:43Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

-        for (base_trait_ref, original_span) in regular_traits_refs_spans {
-            let base_pred: ty::Predicate<'tcx> = base_trait_ref.upcast(tcx);
+        let mut needed_associated_types = FxIndexSet::default();
+        if let Some((principal_trait, spans)) = &principal_trait {


can you move this into a separate fn collect_needed_associated_types function?

also, the comment further down is incorrect, preexisting, but pls fix

// Here, the user could theoretically write `dyn MyTrait<Output = X>`,

should be

// Here, the user could theoretically write `dyn MyTrait<MyOutput = X>`,

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

lcnr · 2024-12-17T09:19:40Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

-                    *span,
-                    crate::errors::UnusedAssociatedTypeBounds { span: *span },
+                    span,
+                    crate::errors::UnusedAssociatedTypeBounds { span },
                );
            }
        }

        if let Err(guar) = self.check_for_required_assoc_tys(


I feel like this should be part of fn check_for_required_assoc_tys which should get moved to this file?

Not gonna do this I think.

lcnr · 2024-12-17T09:24:46Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

@@ -254,7 +233,8 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
                    .args
                    .iter()
                    .enumerate()
-                    .skip(1) // Remove `Self` for `ExistentialPredicate`.
+                    // Skip `Self`
+                    .skip(1)


can you move the

// Verify that `dummy_self` did not leak inside default type parameters. This // could not be done at path creation, since we need to see through trait aliases.

part into a sub-fn

I don't think I will do this.

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

lcnr · 2024-12-17T09:43:53Z

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs

-            )
+        let mut v = principal_trait_ref
+            .into_iter()
+            .chain(existential_projections)
            .chain(auto_trait_predicates)
            .collect::<SmallVec<[_; 8]>>();
        v.sort_by(|a, b| a.skip_binder().stable_cmp(tcx, &b.skip_binder()));


can you move the // Use explicitly-specified region bound, unless the bound is missing. part into a sub-fn as well?

I don't think I will do this.

compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs

tests/ui/error-codes/E0225.stderr

lcnr · 2024-12-17T10:15:09Z

@rustbot author

lcnr · 2025-01-21T10:04:32Z

@bors r+

bors · 2025-01-21T10:04:35Z

📌 Commit 824a867 has been approved by lcnr

It is now in the queue for this repository.

bors · 2025-01-21T12:33:38Z

⌛ Testing commit 824a867 with merge cd805f0...

bors · 2025-01-21T15:27:35Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing cd805f0 to master...

rust-timer · 2025-01-21T17:16:44Z

Finished benchmarking commit (cd805f0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 5.1%)

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)	5.1%	[5.1%, 5.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.1%	[5.1%, 5.1%]	1

Cycles

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

Binary size

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

Bootstrap: 763.46s -> 764.328s (0.11%)
Artifact size: 326.05 MiB -> 326.09 MiB (0.01%)

rustbot assigned lcnr Dec 3, 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 Dec 3, 2024

compiler-errors force-pushed the span-key branch from b72f1d9 to d7c6ee0 Compare December 4, 2024 02:28

compiler-errors changed the title ~~Dont use span as key when collecting missing associated types from dyn~~ Rework dyn trait lowering to stop being so intertwined with trait alias expansion Dec 4, 2024

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the span-key branch from d7c6ee0 to af94314 Compare December 4, 2024 03:38

lcnr reviewed Dec 4, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs Outdated Show resolved Hide resolved

lcnr reviewed Dec 4, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs Show resolved Hide resolved

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 Dec 4, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 4, 2024

compiler-errors force-pushed the span-key branch 2 times, most recently from c91411b to 9ec2929 Compare December 16, 2024 18:15

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the span-key branch from 9ec2929 to 4a38b55 Compare December 16, 2024 19:21

lcnr reviewed Dec 17, 2024

View reviewed changes

rustbot 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 Dec 17, 2024

Rework trait expansion to happen once explicitly

824a867

compiler-errors force-pushed the span-key branch from 4a38b55 to 824a867 Compare January 15, 2025 01:26

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 21, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 21, 2025

bors merged commit cd805f0 into rust-lang:master Jan 21, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 21, 2025

This was referenced Jan 21, 2025

Refactor dyn-compatibility error and suggestions #133372

Merged

Move supertrait_def_ids into the elaborate module like all other fns #135706

Merged

matthiaskrgr mentioned this pull request Jan 23, 2025

ICE: trait object trait bounds reference Self #135918

Closed

moxian mentioned this pull request Mar 24, 2025

ICE: encountered a fresh type during canonicalization #138891

Open

matthiaskrgr mentioned this pull request Mar 28, 2025

ICE: not using infcx #139082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework dyn trait lowering to stop being so intertwined with trait alias expansion #133830

Rework dyn trait lowering to stop being so intertwined with trait alias expansion #133830

compiler-errors commented Dec 3, 2024 •

edited

Loading

rustbot commented Dec 3, 2024

This comment has been minimized.

lcnr commented Dec 4, 2024

compiler-errors commented Dec 4, 2024

This comment has been minimized.

bors commented Dec 4, 2024

bors commented Dec 4, 2024

This comment has been minimized.

rust-timer commented Dec 4, 2024

This comment has been minimized.

lcnr left a comment

lcnr Dec 17, 2024

lcnr Dec 17, 2024

lcnr Dec 17, 2024

compiler-errors Jan 15, 2025

lcnr Dec 17, 2024

compiler-errors Jan 15, 2025

lcnr Dec 17, 2024

compiler-errors Jan 15, 2025

lcnr commented Dec 17, 2024

lcnr commented Jan 21, 2025

bors commented Jan 21, 2025

bors commented Jan 21, 2025

bors commented Jan 21, 2025

rust-timer commented Jan 21, 2025

Rework dyn trait lowering to stop being so intertwined with trait alias expansion #133830

Rework dyn trait lowering to stop being so intertwined with trait alias expansion #133830

Conversation

compiler-errors commented Dec 3, 2024 • edited Loading

rustbot commented Dec 3, 2024

This comment has been minimized.

lcnr commented Dec 4, 2024

compiler-errors commented Dec 4, 2024

This comment has been minimized.

bors commented Dec 4, 2024

bors commented Dec 4, 2024

This comment has been minimized.

rust-timer commented Dec 4, 2024

Overall result: no relevant changes - no action needed

This comment has been minimized.

lcnr 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

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

lcnr commented Dec 17, 2024

lcnr commented Jan 21, 2025

bors commented Jan 21, 2025

bors commented Jan 21, 2025

bors commented Jan 21, 2025

rust-timer commented Jan 21, 2025

Overall result: no relevant changes - no action needed

compiler-errors commented Dec 3, 2024 •

edited

Loading