-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Remove manual WF hack #140557
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
Remove manual WF hack #140557
Conversation
@bors try |
Remove manual WF hack r? lcnr
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚨 Report generation of 🆘 Can someone from the infra team check in on this? @rust-lang/infra |
@craterbot retry-report |
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚨 Report generation of 🆘 Can someone from the infra team check in on this? @rust-lang/infra |
@craterbot retry-report |
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚨 Report generation of 🆘 Can someone from the infra team check in on this? @rust-lang/infra |
@craterbot retry-report |
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
@rfcbot fcp merge |
Team member @compiler-errors 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! See this document for info about what commands tagged team members can give me. |
I am in favor of reverting this hack and we can easily revert it if we encounter a new regression. I think maintaining such warts long time is bound to cause issues at some point. @rfcbot reviewed |
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.
yay hack gone
36cd73d
to
df47958
Compare
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@bors r+ rollup= |
@bors r+ rollup=never |
💡 This pull request was already approved, no need to approve it again.
|
☀️ 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 6d1875f (parent) -> 7e19eef (this PR) Test differencesNo test diffs found Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 7e19eef048ba58c28c70afbf5f95da4829c15796 --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 (7e19eef): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -5.6%, secondary -1.7%)This 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 772.375s -> 772.011s (-0.05%) |
We do not need this hack anymore since we fixed the candidate selection problems with
Sized
bounds. We prefer built-in sized bounds now since #138176, which fixes the only regression this hack was intended to fix.While this theoretically is broken for some code, for example, when there a param-env bound that shadows an impl or built-in trait, we don't see it in practice and IMO it's not worth the burden of having to maintain this wart in
compare_method_predicate_entailment
.The code that regresses is, for example:
Specifically, I don't believe this is really going to be encountered in practice. For this to fail, there must be a where clause in the trait method that would shadow an impl or built-in (non-
Sized
) candidate in the trait, and this shadowing would need to be encountered when solving a nested WF goal from the impl self type.See #108544 for the original regression. Crater run is clean!
r? lcnr