Uh oh!
There was an error while loading. Please reload this page.
AliasTerm refactor - #155392
Conversation
rustbot
commented
Apr 16, 2026
Some changes occurred in src/tools/clippy cc @rust-lang/clippy changes to the core type system cc @lcnr HIR ty lowering was modified cc @fmease Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor changes to the core type system cc @lcnr This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 Some changes occurred in compiler/rustc_sanitizers cc @rcvalle |
BoxyUwU
commented
Apr 17, 2026
r? BoxyUwU |
Kivooeo
commented
Apr 18, 2026
a bit concerned about PredicateKind size increase, let's do a perf to make sure it's fine @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-timer
commented
Apr 18, 2026
Finished benchmarking commit (8206b39): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.5%, secondary 0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 491.618s -> 491.67s (0.01%) |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I feel a bit unsure about the with_args method, i feel like it makes the callsites which are obligation.predicate.with_args much less clear what's going on. but I guess thats a more general issue that projection predicates have really confusingly fields.
this PR needs a rebase and can you link to the tracking issue in the description. thx
WaffleLapkin
commented
Apr 21, 2026
Rebased & added the tracking issue to the PR description. |
BoxyUwU
commented
Apr 21, 2026
@bors r+ rollup=never p=1 (bitrotty) |
jhpratt
commented
Apr 22, 2026
threading between rollups @bors p=6 |
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing f9988fe (parent) -> 1bfcb28 (this PR) Test differencesShow 23 test diffs23 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 1bfcb284f7a2199ad322daa463e29e708d5bc635 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
rust-timer
commented
Apr 22, 2026
Finished benchmarking commit (1bfcb28): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.8%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 490.294s -> 491.717s (0.29%) |
Mark-Simulacrum
commented
Apr 27, 2026
@WaffleLapkin / @BoxyUwU it looks like there were some concerns over the size increases (#155392 (comment)), it looks like those have born out in both pre-merge and after-merge results. Do we think the refactor here is worth the slight regressions in instruction counts? I think it's reasonable to accept them given the lack of cycle counts. |
| goal.param_env, | ||
| ty::UnevaluatedConst::new( | ||
| goal.predicate.alias.def_id.try_into().unwrap(), | ||
| goal.predicate.alias.def_id().try_into().unwrap(), |
There was a problem hiding this comment.
we should pass in the DefId from the match where we decide to call normalize_anon_const here? 🤔
| self.add_goals( | ||
| GoalSource::Misc, | ||
| cx.predicates_of(free_alias.def_id) | ||
| cx.predicates_of(free_alias.def_id()) |
There was a problem hiding this comment.
same here
| cx.type_of(inherent.def_id()).instantiate(cx, inherent_args).skip_norm_wip().into() | ||
| } else { | ||
| cx.const_of_item(inherent.def_id).instantiate(cx, inherent_args).skip_norm_wip().into() | ||
| cx.const_of_item(inherent.def_id()) |
There was a problem hiding this comment.
and here, I guess in general, I feel like most calls of AliasXXX::def_id are meh
There was a problem hiding this comment.
i would expect in the long run we just get rid of this method since not everything will have a DefId anymore
BoxyUwU
commented
Apr 28, 2026
It would probably be good to look into if we can reduce the size of |
View all comments
follow up to #154758
tracking issue: #154941