Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix ice when error reporting recursion errors #94391

Merged
merged 1 commit into from
Mar 25, 2022
Merged

Fix ice when error reporting recursion errors #94391

merged 1 commit into from
Mar 25, 2022

Conversation

light4
Copy link
Contributor

@light4 light4 commented Feb 26, 2022

Fixes: #90319, #92148, #93955

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 26, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2022
@light4 light4 closed this Feb 26, 2022
@light4 light4 reopened this Feb 26, 2022
@light4 light4 marked this pull request as ready for review February 26, 2022 07:20
@light4
Copy link
Contributor Author

light4 commented Feb 26, 2022

Note there is another PR #91238 fixes the ICE in a different way.

tom7980
tom7980 reviewed Feb 26, 2022
View reviewed changes
compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
Overflow => {
// Already reported.
Overflow(OverflowError::Error(ErrorReported)) => {
return;
Copy link
Contributor

@tom7980 tom7980 Feb 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm fairly sure that this is the line that fixes the ICE, if you change this back to a bug! invocation does the ICE come back? We're still technically reporting the same error but instead of handling it we ignore it which is probably fine because we've technically already errored somewhere else before this if we are getting to this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this back to a bug! will cause ICE.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we have a delay_span_bug here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@tom7980
Copy link
Contributor

tom7980 commented Feb 26, 2022

@estebank I think this fix is cleaner than mine, see my comment in the changed files but I'd go with this over #91238 as mine introduces a bunch of overhead for this specific issue that we don't really need.

@Dylan-DPC
Copy link
Member

closing this based on comment by author #94391 (comment)

@Dylan-DPC Dylan-DPC closed this Feb 27, 2022
@light4
Copy link
Contributor Author

light4 commented Feb 27, 2022

@Dylan-DPC should not we close #91238 instead of this?

@Dylan-DPC
Copy link
Member

ah yes :D

@Dylan-DPC Dylan-DPC reopened this Feb 27, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Feb 27, 2022
Closed
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@bors
Copy link
Contributor

bors commented Mar 23, 2022

☔ The latest upstream changes (presumably #94901) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor

Apologies for the delay in reviewing. Can you check if adding the delay_span_bug call before the return causes any issues? Having that call protects us from every accidentally compiling if other parts of the codebase change in the future. Either way, once we know the answer to that, r=me.

@estebank estebank added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 24, 2022
@estebank
Copy link
Contributor

Tepidly nominating for backport, as this fixes a stable regression that has existed for multiple stable releases.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 24, 2022

📌 Commit 85e67b9 has been approved by estebank

@bors bors added S-waiting-on-bors 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 Mar 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 24, 2022
@Dylan-DPC
Rollup merge of rust-lang#94391 - light4:issue-90319, r=estebank
5333dee 
Fix ice when error reporting recursion errors

Fixes: rust-lang#90319, rust-lang#92148, rust-lang#93955
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 24, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 24, 2022
@Dylan-DPC
Rollup merge of rust-lang#94391 - light4:issue-90319, r=estebank
f5e5ca2 
Fix ice when error reporting recursion errors

Fixes: rust-lang#90319, rust-lang#92148, rust-lang#93955
This was referenced Mar 24, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2022
@bors
Auto merge of rust-lang#95291 - Dylan-DPC:rollup-vrb4wlw, r=Dylan-DPC
661e8be 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#94391 (Fix ice when error reporting recursion errors)
 - rust-lang#94655 (Clarify which kinds of MIR are allowed during which phases.)
 - rust-lang#95179 (Try to evaluate in try unify and postpone resolution of constants that contain inference variables)
 - rust-lang#95270 (debuginfo: Fix debuginfo for Box<T> where T is unsized.)
 - rust-lang#95276 (add diagnostic items for clippy's `trim_split_whitespace`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d1d4613 into rust-lang:master Mar 25, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 25, 2022
This was referenced Mar 26, 2022
Closed
Closed
@apiraino
Copy link
Contributor

Beta backport declined, discussion of compiler team on Zulip.

@estebank as mentioned in the discussion, please feel free to advocate otherwise

@rustbot label -beta-nominated

@rustbot rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 31, 2022
@JohnTitor JohnTitor mentioned this pull request May 1, 2022
Closed
@light4 light4 deleted the issue-90319 branch November 10, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom7980 tom7980 tom7980 left review comments

@estebank estebank estebank left review comments

@Hamidb14 Hamidb14 Hamidb14 approved these changes

Assignees

@matthewjasper matthewjasper

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

regression: ICE ErrorReporting Overflow should not reach report_selection_err call