-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
hir_analysis: add missing sizedness bounds #142712
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
hir_analysis: add missing sizedness bounds #142712
Conversation
|
rustbot has assigned @petrochenkov. Use |
|
HIR ty lowering was modified cc @fmease |
This comment has been minimized.
This comment has been minimized.
|
Wasn't expecting that failure, looking into it. |
|
A smaller example of one of the failing crates (which happens with a stage two build): #![crate_type = "lib"]
use std::marker::PhantomData;
pub trait ZeroMapKV<'a> {
type Container;
}
pub trait ZeroFrom<'zf, C: ?Sized> {}
pub struct ZeroMap<'a, K: ZeroMapKV<'a>>(PhantomData<&'a K>);
impl<'zf, 's, K> ZeroFrom<'zf, ZeroMap<'s, K>> for ZeroMap<'zf, K>
where
K: for<'b> ZeroMapKV<'b>,
<K as ZeroMapKV<'zf>>::Container: ZeroFrom<'zf, <K as ZeroMapKV<'s>>::Container>,
{
} |
|
r? types |
|
I'd like to think about this... Can you remind me, is |
Yes, it is |
|
I think this highlights a problematic interaction with trait aliases, and I'm tempted to say that trait aliases should have their behavior reworked to stop expanding into their bounds in FOr now, I think it's inconsistent to not add |
60d5044 to
7684842
Compare
Changed to this, thanks. Still hitting the stage two failure, though I've added a UI test with my reproduction so that will fail first, not had much progress on working out what's going wrong with it. |
This comment has been minimized.
This comment has been minimized.
7684842 to
eac7da1
Compare
|
See discussion on Zulip (#t-types > relaxing associated type bounds @ 💬) for background behind the last commit's changes. |
|
@bors try |
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
3901931 to
c4cf6ba
Compare
This comment has been minimized.
This comment has been minimized.
c4cf6ba to
4440b69
Compare
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.
final nits, then r=me
Default sizedness bounds were not being added to `explicit_super_predicates_of` and `explicit_implied_predicates_of` which meant that a trait bound added to a associated type projection would be missing the implied predicate of the default sizedness supertrait of that trait. An unexpected consequence of this change was that the check for multiple principals was now finding an additional `MetaSized` principal when eagerly expanding trait aliases. Instead of special-casing trait aliases as different from traits and not adding a `MetaSized` supertrait to trait aliases, filter out `MetaSized` when lowering `dyn Trait`.
4440b69 to
82a4049
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r=lcnr |
|
☀️ Test successful - checks-actions |
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 2aaa62b (parent) -> f435972 (this PR) Test differencesShow 10 test diffsStage 1
Stage 2
Additionally, 4 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard f435972085b697a1ece8ee6a1ac76efff8d1df7b --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 |
|
Finished benchmarking commit (f435972): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur 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.4%, secondary -0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.4%, secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.3%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.049s -> 474.979s (-0.01%) |
|
perf triage: This seems to have caused quite a lot of regressions. @davidtwco Is this expected/justified? Can we do something to mitigate it or should we even revert? |
|
These regressions aren't especially surprising given the change, but are required for correctness of this feature, there may be an optimisation we could add but we'd need to investigate. |
Depends on #144064
Default sizedness bounds were not being added to
explicit_super_predicates_ofandexplicit_implied_predicates_ofwhich meant that a trait bound added to a associated type projection would be missing the implied predicate of the default sizedness supertrait of that trait.An unexpected consequence of this change was that the check for multiple principals was now finding an additional
MetaSizedprincipal when eagerly expanding trait aliases - which is fixed by skippingMetaSizedwhen elaborating trait aliases in loweringdyn TraitAlias.