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

(Re-)return adjustment target if adjust kind is never-to-any #134279

Merged
merged 2 commits into from
Dec 14, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Dec 13, 2024

This PR fixes #134162 where we ICE'd on

fn main() {
    struct X;
    let _ = [X] == [panic!(); 2];
}

In #121208 (comment), there was a change

- if let Some(adjustments) = self.typeck_results.borrow().adjustments().get(expr.hir_id) {
-     let reported = self.dcx().span_delayed_bug(
-         expr.span,
-         "expression with never type wound up being adjusted",
-     );
-     return if let [Adjustment { kind: Adjust::NeverToAny, target }] = &adjustments[..] {
-         target.to_owned()
-     } else {
-         Ty::new_error(self.tcx(), reported)
-     };
- }
+ if let Some(_) = self.typeck_results.borrow().adjustments().get(expr.hir_id) {
+     self.dcx()
+         .span_bug(expr.span, "expression with never type wound up being adjusted");
+ }

It turned out returning the adjustment target if the adjustment kind is NeverToAny is necessary, as otherwise we will go through a series of delay_bugs and eventually find that we constructed a TyKind::Error without having actually emitted an error.

This PR addresses that by re-returning the adjustment target if the adjustment kind is NeverToAny, partially reverting this change from #121208.

This PR has two commits:

  1. The first commit adds a regression test for ICE: this path really should be doomed #134162, which will ICE (on stable 1.83.0, beta and nightly 2024-12-13).
  2. The second commit is the partial revert, which will fix the ICE.

cc @nnethercote FYI as this is related to #121208 changes. The changes from #121208 exposed that we lacked test coverage for the code pattern reported in #134162.

@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2024

r? @davidtwco

rustbot has assigned @davidtwco.
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 Dec 13, 2024
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 14, 2024

📌 Commit 2e8ce2a has been approved by compiler-errors

It is now in the queue for this repository.

@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 Dec 14, 2024
@matthiaskrgr
Copy link
Member

@bors r-
a test has been added tests/crashes/134162.rs , need to be removed and re-r+'d I think

@bors bors added S-waiting-on-author 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 Dec 14, 2024
Without doing so, we'll run into a series of delayed bugs then find that
we have a `TyKind::Error` constructed yet fail to emit an error.

This partially reverts a change in
<rust-lang#121208> related to never type
adjustments in expr typecheck errors.
@jieyouxu jieyouxu force-pushed the return-adjustment-target branch from 2e8ce2a to d15315c Compare December 14, 2024 09:07
@jieyouxu
Copy link
Member Author

Removed the redundant crashes test (which is the larger version of the regression test added).

@bors r=@compiler-errors

@bors
Copy link
Contributor

bors commented Dec 14, 2024

📌 Commit d15315c has been approved by compiler-errors

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#133221 (Add external macros specific diagnostics for check-cfg)
 - rust-lang#133386 (Update linux_musl base to dynamically link the crt by default)
 - rust-lang#134191 (Make some types and methods related to Polonius + Miri public)
 - rust-lang#134227 (Update wasi-sdk used to build WASI targets)
 - rust-lang#134279 ((Re-)return adjustment target if adjust kind is never-to-any)
 - rust-lang#134295 (Encode coroutine-closures in SMIR)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#133221 (Add external macros specific diagnostics for check-cfg)
 - rust-lang#133386 (Update linux_musl base to dynamically link the crt by default)
 - rust-lang#134191 (Make some types and methods related to Polonius + Miri public)
 - rust-lang#134227 (Update wasi-sdk used to build WASI targets)
 - rust-lang#134279 ((Re-)return adjustment target if adjust kind is never-to-any)
 - rust-lang#134295 (Encode coroutine-closures in SMIR)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 752f79a into rust-lang:master Dec 14, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
Rollup merge of rust-lang#134279 - jieyouxu:return-adjustment-target, r=compiler-errors

(Re-)return adjustment target if adjust kind is never-to-any

This PR fixes rust-lang#134162 where we ICE'd on

```rs
fn main() {
    struct X;
    let _ = [X] == [panic!(); 2];
}
```

In rust-lang#121208 (comment), there was a change

```diff
- if let Some(adjustments) = self.typeck_results.borrow().adjustments().get(expr.hir_id) {
-     let reported = self.dcx().span_delayed_bug(
-         expr.span,
-         "expression with never type wound up being adjusted",
-     );
-     return if let [Adjustment { kind: Adjust::NeverToAny, target }] = &adjustments[..] {
-         target.to_owned()
-     } else {
-         Ty::new_error(self.tcx(), reported)
-     };
- }
+ if let Some(_) = self.typeck_results.borrow().adjustments().get(expr.hir_id) {
+     self.dcx()
+         .span_bug(expr.span, "expression with never type wound up being adjusted");
+ }
```

It turned out returning the adjustment target if the adjustment kind is `NeverToAny` is necessary, as otherwise we will go through a series of `delay_bug`s and eventually find that we constructed a `TyKind::Error` without having actually emitted an error.

This PR addresses that by re-returning the adjustment target if the adjustment kind is `NeverToAny`, partially reverting this change from rust-lang#121208.

This PR has two commits:

1. The first commit adds a regression test for rust-lang#134162, which will ICE (on stable 1.83.0, beta and nightly 2024-12-13).
2. The second commit is the partial revert, which will fix the ICE.

cc `@nnethercote` FYI as this is related to rust-lang#121208 changes. The changes from rust-lang#121208 exposed that we lacked test coverage for the code pattern reported in rust-lang#134162.
@jieyouxu jieyouxu deleted the return-adjustment-target branch December 15, 2024 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

ICE: this path really should be doomed
6 participants