Visit param_env field in Obligation's TypeFoldable impl by Aaron1011 · Pull Request #91205 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This oversight appears to have gone unnoticed for a long time
without causing issues, but it should still be fixed.
r? @jackh726
(rust-highfive has picked a reviewer for you, use r? to override)
Relevant to the compiler team, which will review and decide on the PR/issue.
label
the output probably changed cause the param_env of the obligation also contained an erroneous anon const which we previously did not compute for whatever reason 🤷
slightly prefer to keep the old formatting here, apart from that r=me
r? @lcnr
📌 Commit d7e8212 has been approved by lcnr
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Visit param_env field in Obligation's TypeFoldable impl
This oversight appears to have gone unnoticed for a long time without causing issues, but it should still be fixed.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Visit param_env field in Obligation's TypeFoldable impl
This oversight appears to have gone unnoticed for a long time without causing issues, but it should still be fixed.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
This oversight appears to have gone unnoticed for a long time without causing issues, but it should still be fixed.
I've blessed the Clippy test.
@bors r=lcnr
📌 Commit a7cc6bc has been approved by lcnr
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request
Prior to PR rust-lang#91205, checking for errors in the overall obligation
would check checking the ParamEnv, due to an incorrect
super_visit_with impl. With this bug fixed, we will now
bail out of impl candidate assembly if the ParamEnv contains
any error types.
In practice, this appears to be overly conservative - when an error occurs early in compilation, we end up giving up early for some predicates that we could have successfully evaluated without overflow. By only checking for errors in the predicate itself, we avoid causing additional spurious 'type annotations needed' errors after a 'real' error has already occurred.
With this PR, the diagnostic changes caused by PR rust-lang#91205 are reverted.
Finished benchmarking commit (1e79d79): comparison url.
Summary: This change led to very large relevant regressions 😿 in compiler performance.
- Very large regression in instruction counts (up to 6.6% on
fullbuilds ofhyper-2)
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.
@rustbot label: +perf-regression
Can someone file a ticket to follow up with changing the output here? Inference errors outside of expression context are very confusing to people.
The perf regression is really unfortunate - I had hoped that #91254 would allow us to recover some of the loss, but it appears to have no impact.
I'll see if there are any call sites that don't actually care about the ParamEnv, and could be adjusted to just visit the predicate. However, this is a correctness fix, do I'm not sure how much can be done to improve the performance.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…lcnr
Only check for errors in predicate when skipping impl assembly
Prior to PR rust-lang#91205, checking for errors in the overall obligation
would check checking the ParamEnv, due to an incorrect
super_visit_with impl. With this bug fixed, we will now
bail out of impl candidate assembly if the ParamEnv contains
any error types.
In practice, this appears to be overly conservative - when an error occurs early in compilation, we end up giving up early for some predicates that we could have successfully evaluated without overflow. By only checking for errors in the predicate itself, we avoid causing additional spurious 'type annotations needed' errors after a 'real' error has already occurred.
With this PR, the diagnostic changes caused by PR rust-lang#91205 are reverted.
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request
Prior to PR rust-lang#91205, checking for errors in the overall obligation
would check checking the ParamEnv, due to an incorrect
super_visit_with impl. With this bug fixed, we will now
bail out of impl candidate assembly if the ParamEnv contains
any error types.
In practice, this appears to be overly conservative - when an error occurs early in compilation, we end up giving up early for some predicates that we could have successfully evaluated without overflow. By only checking for errors in the predicate itself, we avoid causing additional spurious 'type annotations needed' errors after a 'real' error has already occurred.
With this PR, the diagnostic changes caused by PR rust-lang#91205 are reverted.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…lcnr
Only check for errors in predicate when skipping impl assembly
Prior to PR rust-lang#91205, checking for errors in the overall obligation
would check checking the ParamEnv, due to an incorrect
super_visit_with impl. With this bug fixed, we will now
bail out of impl candidate assembly if the ParamEnv contains
any error types.
In practice, this appears to be overly conservative - when an error occurs early in compilation, we end up giving up early for some predicates that we could have successfully evaluated without overflow. By only checking for errors in the predicate itself, we avoid causing additional spurious 'type annotations needed' errors after a 'real' error has already occurred.
With this PR, the diagnostic changes caused by PR rust-lang#91205 are reverted.
flip1995 pushed a commit to flip1995/rust that referenced this pull request
Visit param_env field in Obligation's TypeFoldable impl
This oversight appears to have gone unnoticed for a long time without causing issues, but it should still be fixed.
flip1995 pushed a commit to flip1995/rust that referenced this pull request
Prior to PR rust-lang#91205, checking for errors in the overall obligation
would check checking the ParamEnv, due to an incorrect
super_visit_with impl. With this bug fixed, we will now
bail out of impl candidate assembly if the ParamEnv contains
any error types.
In practice, this appears to be overly conservative - when an error occurs early in compilation, we end up giving up early for some predicates that we could have successfully evaluated without overflow. By only checking for errors in the predicate itself, we avoid causing additional spurious 'type annotations needed' errors after a 'real' error has already occurred.
With this PR, the diagnostic changes caused by PR rust-lang#91205 are reverted.
pietroalbini pushed a commit to pietroalbini/rust that referenced this pull request
Prior to PR rust-lang#91205, checking for errors in the overall obligation
would check checking the ParamEnv, due to an incorrect
super_visit_with impl. With this bug fixed, we will now
bail out of impl candidate assembly if the ParamEnv contains
any error types.
In practice, this appears to be overly conservative - when an error occurs early in compilation, we end up giving up early for some predicates that we could have successfully evaluated without overflow. By only checking for errors in the predicate itself, we avoid causing additional spurious 'type annotations needed' errors after a 'real' error has already occurred.
With this PR, the diagnostic changes caused by PR rust-lang#91205 are reverted.
Labels
This PR was explicitly merged by bors.
Performance regression.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.