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.

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

Issue-125323: ICE non-ADT in struct pattern when long time constant evaluation is in for loop #138679

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Shunpoco
Copy link
Contributor

@Shunpoco Shunpoco commented Mar 18, 2025

This PR fixes #125323

Context

According to the issue, the ICE happens since #121206.
In the PR, some error methods were reorganized. For example, has_errors() was renamed to has_errors_exclude_lint_errors(). However, some codes which used the original has_errors() were not switched to has_errors_exclude_lint_errors(). I finally found that report_error() in writeback.rs causes this ICE. Currently the method uses tainted_by_errors() to get guar (ErrorGuaranteed), but originally it used dcx().has_errors() but it wasn't changed to has_errors_exclude_lint_errors() when changes in #121206 were merged. I don't think I fully understand how an error is propagated, but I suppose that the error from long time constant evaluation is unexpectedly propagated other parts (in this ICE, for loop), then cause the non-ADT in struct pattern ICE.

Change

  • Fix report_error() in writeback.rs: use dcx().has_errors_exclude_lint_errors() instead of tainted_by_errors() to prevent error propagation from constant evaluation.
  • Add test for the ICE
  • Modify some tests to align the change: Due to this fix, E0282 error happens (or not happen anymore) in some tests.

NOTE

The 4th commit aims to revert the fix in #123516 because I confirmed that the ICE solved by the PR doesn't happen if I modify report_error(). I think the root cause of that ICE is the same as #125323 . But I can discard this commit since we can fix #125323 without it.

…report_error

When constant evaluation fails, the error is propagated to other places if report_error uses tainted_by_errors.

Signed-off-by: Shunpoco <tkngsnsk313320@gmail.com>
Signed-off-by: Shunpoco <tkngsnsk313320@gmail.com>
Signed-off-by: Shunpoco <tkngsnsk313320@gmail.com>
This ty::Error was added in rust-lang#123428, but the root cause is the same as rust-lang#125323.
After report_error is fixed, the arm isn't needed anymore.

Signed-off-by: Shunpoco <tkngsnsk313320@gmail.com>
@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 18, 2025
@rust-log-analyzer

This comment has been minimized.

Signed-off-by: Shunpoco <tkngsnsk313320@gmail.com>
@Shunpoco Shunpoco marked this pull request as ready for review March 19, 2025 00:51
@rustbot
Copy link
Collaborator

rustbot commented Mar 19, 2025

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

@compiler-errors
Copy link
Member

I don't believe this is the right solution, because using tainted_by_errors in Resolver::report_error is intentional to make sure we don't have non-local dependence on error reporting.

The real root cause of this issue is this:

// But we exclude lint errors from this, because lint errors are typically
// less serious and we're more likely to want to continue (#87337).
if let Some(guar) = sess.dcx().has_errors_excluding_lint_errors() {
guar.raise_fatal();
}

...which causes us to enter check_mod_deathness if the only error that was reported was through the const eval lint. I think the code in compiler/rustc_passes/src/dead.rs should handle errors more gracefully, and not walk into modules if the body typeck results are tainted. But I'll pass this onto oli who can confirm my suspicions and guide this PR along, since they've been responsible for some of this error tainting and cleanup work.

r? oli-obk

@rustbot rustbot assigned oli-obk and unassigned estebank Mar 19, 2025
@compiler-errors
Copy link
Member

Please ensure that after this PR is fully reworked that it's squashed into a single commit.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 19, 2025

Yea errs' analysis is spot on. Skipping the visit_body call if the typeck results are tainted in visit_nested_body would avoid any such ICEs. This would have the fun side effect of causing everything with errors to be marked as dead though, so we likely have to record that we didn't do the analysis properly. The easiest way to do that would be to make the Visitor::Result be an actual Result<(), ErrorGuaranteed> and recording that in the query result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: dead: non-ADT in struct pattern
6 participants