-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Make sure to treat only param where clauses as inherent #145262
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
Conversation
@@ -1945,6 +1945,29 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> { | |||
); | |||
(xform_self_ty, xform_ret_ty) = | |||
self.xform_self_ty(probe.item, trait_ref.self_ty(), trait_ref.args); | |||
|
|||
if matches!(probe.kind, WhereClauseCandidate(_)) { |
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.
We could fast path this for the old trait solver, since we expect things to be normalized always.
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.
hm 🤔
so what's going on is:
- if the normalized self type is a param we
assemble_inherent_candidates_from_param
- this uses fast-reject in an attempt to filter to where-clauses which only apply for the self type
- doesn't handle unnormalized where-clauses or alias self types
We can't normalize in assembly because it doesn't use a probe, so we instead do it here. This totally makes sense to me, r=me on this impl
Can you add a comment to assemble_inherent_candidates_from_param
that we only filter the bounds
as an fast-path optimization and we properly reject non-param self type where-clauses in consider_probe
?
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.
actually, move this structurally_normalize_ty
above the xform
and then use the normalized ty
in xform_self_ty
? This should avoid some work given that this change seems to actually impact perf in the new solver
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.
I don't see how moving it would change things for the better here, and it makes the structure kinda worse, so I'm not going to do this. I'm happy to review the change if you'd like to make a follow-up.
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.
My theory here is that the perf improvement derives from us pruning candidates with this check that would otherwise need to do unnecessary work below.
Maybe structurally normalizing eagerly here would mean we wouldn't normalize when proving things in the un-pruned (i.e. actually param) candidates, but I didn't want to tangle those changes into this PR.
@bors2 try @rust-timer queue I also want to crater this since it may cause some code to fail due to new ambiguity and missing imports. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Make sure to treat only param where clauses as inherent
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (987a165): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient 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 1.2%, secondary -2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 464.993s -> 463.982s (-0.22%) |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
0.0% perf regression is definitely important good job perfbot 👍 |
This code compiles on 1.89.0. But it doesn't compile on 1.88.0, and doesn't compile with this PR. Is this intended? I don't think the test checks for this case properly. pub mod module {
pub trait Trait {
fn method(&self);
}
}
// No import of Trait
use std::ops::Deref;
pub fn foo(x: impl Deref<Target: module::Trait>) {
x.method();
} |
Yes, this is intended breakage. The whole point of this PR is that there was a regression in 1.89 where we were accepting code that we shouldn't have. As a side effect, this regression also affects how we compute the lint for unused imports, but that's kinda the least important part of this PR. This is the whole point of the crater run I started. I'd like to ensure that nobody is relying on this behavior.
The test I included is effectively equivalent to that one, since they are both originating from the same root cause around how we treat where clauses from param types specially in method selection. Both the test I included and the one you've shared go from fail (1.88) -> pass (1.89) -> fail (this PR). |
Given that there was an accidental stabilization here and that we'll be accepting a breaking change to take it back, let's go ahead and nominate for review. @rustbot labels +I-lang-nominated It hasn't been out there long, but it is particularly the kind of thing that would be easy for people to unknowingly rely on. In fact, I'm going to go ahead kick off the proposed FCP because I don't suspect the crater run is going to tell us very much. The release has been out for less than a week. Most of the people who are going to accidentally rely on this probably haven't done so yet1 -- they'll do it after we land this but before we release it and they adopt that release. @rfcbot fcp merge Footnotes
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
please also add more tests use std::ops::Deref;
trait Trait1 {
fn call_me(&self) {}
}
impl<T> Trait1 for T {}
trait Trait2 {
fn call_me(&self) {}
}
impl<T> Trait2 for T {}
pub fn foo<T, U>(x: T)
where
T: Deref<Target = U>,
U: Trait1,
{
// This should be ambiguous. The fact that there's an inherent where-bound
// candidate for `U` should not impact the candidates for `T`
x.call_me();
} |
[BETA] Revert "Use DeepRejectCtxt in assemble_inherent_candidates_from_param" Fixes #145185. Backporting this to stable and beta in favor of #145262 (comment). r? lcnr
[BETA] Revert "Use DeepRejectCtxt in assemble_inherent_candidates_from_param" Fixes #145185. Backporting this to stable and beta in favor of #145262 (comment). r? lcnr
[BETA] Revert "Use DeepRejectCtxt in assemble_inherent_candidates_from_param" Fixes #145185. Backporting this to stable and beta in favor of #145262 (comment). r? lcnr
[BETA] Revert "Use DeepRejectCtxt in assemble_inherent_candidates_from_param" Fixes #145185. Backporting this to stable and beta in favor of #145262 (comment). r? lcnr
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
04329e3
to
c2b9a8a
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 809200e (parent) -> a1dbb44 (this PR) Test differencesShow 16 test diffsStage 1
Stage 2
Additionally, 8 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 a1dbb443527bd126452875eb5d5860c1d001d761 --output-dir test-dashboard And 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 (a1dbb44): comparison URL. Overall result: ✅ improvements - no action needed@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 (secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 469.654s -> 469.015s (-0.14%) |
See the description in the test file.
This PR fixes a bug introduced by #141333, where we considered non-
Param
where clauses to be "inherent" for the purpose of method probing, which leads to both changes in method ambiguity (see test) and also import usage linting (and thus fixes #145185).r? @lcnr