-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework upcasting confirmation to support upcasting to fewer projections in target bounds #114036
Conversation
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bunch of somewhat involved cleanup suggestions
compiler/rustc_trait_selection/src/traits/select/confirmation.rs
Outdated
Show resolved
Hide resolved
☔ The latest upstream changes (presumably #113393) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment was marked as resolved.
This comment was marked as resolved.
5c3fc7b
to
dc2f85f
Compare
compiler/rustc_trait_selection/src/traits/select/confirmation.rs
Outdated
Show resolved
Hide resolved
2c14a0a
to
e6b5a9d
Compare
@bors try @rust-timer queueu |
@rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit e6b5a9db9b20cb1ec59a08de9252c15eea250968 with merge 0a272aa2cb590369372a550e471024d5c98edccc... |
fdcab31
to
7c942cc
Compare
Let's try perf one more time. This new code path actually shouldn't ever be hit without trait upcasting -- @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 7c942cc with merge c6d58f5a53b24b41706d35d7118db315a699da6e... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c6d58f5a53b24b41706d35d7118db315a699da6e): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 649.462s -> 649.886s (0.07%) |
This regression is small and affects just a single crate. The actual changes here pretty much exclusively affect unstable code so this feels like it may be noise. I believe this to be some inlining change and don't think it's worth it to look deeper into it. @bors r+ rollup=never |
☀️ Test successful - checks-actions |
Finished benchmarking commit (4f7bb98): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 647.543s -> 647.162s (-0.06%) |
Bubble up nested goals from equation in `predicates_for_object_candidate` This used to be needed for rust-lang#114036 (comment), but since it's no longer, I'm opening this as a separate PR. This also fixes one ICEing UI test: (`tests/ui/unboxed-closures/issue-53448.rs`) r? `@lcnr`
Bubble up nested goals from equation in `predicates_for_object_candidate` This used to be needed for rust-lang/rust#114036 (comment), but since it's no longer, I'm opening this as a separate PR. This also fixes one ICEing UI test: (`tests/ui/unboxed-closures/issue-53448.rs`) r? `@lcnr`
Bubble up nested goals from equation in `predicates_for_object_candidate` This used to be needed for rust-lang/rust#114036 (comment), but since it's no longer, I'm opening this as a separate PR. This also fixes one ICEing UI test: (`tests/ui/unboxed-closures/issue-53448.rs`) r? `@lcnr`
Bubble up nested goals from equation in `predicates_for_object_candidate` This used to be needed for rust-lang/rust#114036 (comment), but since it's no longer, I'm opening this as a separate PR. This also fixes one ICEing UI test: (`tests/ui/unboxed-closures/issue-53448.rs`) r? `@lcnr`
This PR implements a modified trait upcasting algorithm that is resilient to changes in the number of associated types in the bounds of the source and target trait objects.
It does this by equating each bound of the target trait ref individually against the bounds of the source trait ref, rather than doing them all together by constructing a new trait object.
The new way we do trait upcasting confirmation
Equate the target trait object's principal trait ref with one of the supertraits of the source trait object's principal.
https://github.com/rust-lang/rust/blob/fdcab310b2a57a4e9cc0b2629abd27afda49cd80/compiler/rustc_trait_selection/src/traits/select/mod.rs#L2509-L2525
Make sure that every auto trait in the target trait object is present in the source trait ref's bounds.
https://github.com/rust-lang/rust/blob/fdcab310b2a57a4e9cc0b2629abd27afda49cd80/compiler/rustc_trait_selection/src/traits/select/mod.rs#L2559-L2562
For each projection in the target trait object, make sure there is exactly one projection that equates with it in the source trait ref's bound. If there is more than one, bail with ambiguity.
https://github.com/rust-lang/rust/blob/fdcab310b2a57a4e9cc0b2629abd27afda49cd80/compiler/rustc_trait_selection/src/traits/select/mod.rs#L2526-L2557
Make sure the lifetime of the source trait object outlives the lifetime of the target.
Meanwhile, this is how we used to do upcasting:
For each supertrait of the source trait object, take that supertrait, append the source object's projection bounds, and the target trait object's auto trait bounds, and make this into a new object type:
rust/compiler/rustc_trait_selection/src/traits/select/confirmation.rs
Lines 915 to 929 in d12c6e9
Then equate it with the target trait object:
rust/compiler/rustc_trait_selection/src/traits/select/confirmation.rs
Line 936 in d12c6e9
This will be a type mismatch if the target trait object has fewer projection bounds, since we compare the bounds structurally in relate:
rust/compiler/rustc_middle/src/ty/relate.rs
Lines 696 to 698 in d12c6e9
Fixes #114035
Also fixes #114113, because I added a normalize call in the old solver.
r? types