-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Remove context dependant ! fallback
#148871
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
This comment has been minimized.
This comment has been minimized.
|
I have really low confidence in the never type tests, IMO they require a major refactoring, as a lot of them are based on outdated ideas and features... |
This comment has been minimized.
This comment has been minimized.
|
For the tests, I would expect us to have revisions for edition 2021 and edition 2024 (for the different fallbacks) |
3f4ec16 to
b128eae
Compare
|
@jackh726 I'm not sure 2021vs2015 is relevant. But either way, yes, I agree that we should have tests for both current fallback behaviors based on the edition. I'll make a followup which cleans up the tests, wanna make sure that we have relevant and comprehensive tests. |
This comment has been minimized.
This comment has been minimized.
b128eae to
6b36e86
Compare
| // phase of fallback. This means that we only replace | ||
| // inference variables with their underlying opaque types as a | ||
| // last resort. |
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.
existing, but I do not understand that comment?
Do you understand what this wants to say. It feels like this is a general "if fallback occurred, previously stalled goals may make progress again".
I really don't see how there's anything specific to opaques here.
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 get it either .-.
the example below is supposed to explain this, but I still don't get it
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.
hmm, want to rip this out and replace it with
if fallback occurred, previously stalled goals may make progress again
even if this comment was somehow relevant, given that we both don't get what it means, I don't think it does us any good
| }); | ||
| let unit_obligation = obligation.with(tcx, predicate); | ||
| if self.predicate_may_hold(&unit_obligation) { | ||
| // FIXME: make a new issue for this |
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.
Can you elaborate on that FIXME? it feels unactionable for anyone who isn't you right now... or maybe just do that if you can xd
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.
Sure, I opened #148922 and linked it from here. I tried my best to write a good explanation, but it's just confusing :(
| @@ -1,9 +1,8 @@ | |||
| //@ revisions: nofallback fallback | |||
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.
want to rename the revisions to "unit_fallback never_fallback"?
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.
In a follow up
| fallback_to(self.tcx.types.unit); | ||
| } | ||
| DivergingFallbackBehavior::ContextDependent => { | ||
| if found_infer_var_info.self_in_trait && found_infer_var_info.output { |
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.
found_infer_var_info is dead code now?
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.
Did you mean InferVarInfo? Yes, it is. Removed.
I don't think we need to worry about |
0f425a6 to
39ac890
Compare
|
@rustbot review |
|
@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.
Remove context dependant `!` fallback
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (2fb4a7a): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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. @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.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.8%)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: 473.917s -> 472.516s (-0.30%) |
|
@rustbot author |
The code supporting it is extremely confusing. At the same time, we have no plans to use this scheme, so there is no value in supporting it.
the only diagnostic that was using this field specifically сares for the never type fallback, not the integer fallback.
I think in this case the early return is harder to read than just an if. "if fallback hasn't occured we don't do..."
One is about context dependant case which I removed a few commits back, the other neither me nor lcnr could understand, soooo :upsidedown_smile:
... now that it's always the same for a certain crate
39ac890 to
37ecc8e
Compare
|
This PR was rebased onto a different main 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+ rollup=never |
|
☀️ 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 88bd39b (parent) -> d645a4c (this PR) Test differencesShow 29 test diffsStage 1
Stage 2
Additionally, 23 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 d645a4c9c563b80048ce5f32845e754a67f11efa --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 (d645a4c): 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 (primary 2.0%, secondary 1.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -3.4%)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: 472.29s -> 473.697s (0.30%) |
... and minor cleanup.
r? lcnr