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

Revert diagnostics hack to fix ICE 132920 #133304

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Conversation

lqd
Copy link
Member

@lqd lqd commented Nov 21, 2024

This reverts 8a568d9 from #128849 to fix the diagnostics ICE in #132920.

The hack mentioned in that commit was supposed to be tailored to E277, but that codepath is used actually shared with other errors, e.g. at least the E283 from the linked issue.

We may have to eat the slightly worse diagnostics until a non-hacky way to make this error less verbose is implemented (or I guess a different hack specializing even more to E277's structure).

Sorry @estebank 🙏. I can close this if you'd prefer to fix it in a different way.

Since it seems unexpected that #128849 would impact the repro, here's how the error used to look before that PR.

warning: unused import: `minirapier::Ray`
 --> src/main.rs:2:5
  |
2 | use minirapier::Ray;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error[E0283]: type annotations needed
  --> src/main.rs:10:5
   |
10 |     insert_resource(Res.into());
   |     ^^^^^^^^^^^^^^^ ---------- type must be known at this point
   |     |
   |     cannot infer type of the type parameter `R` declared on the function `insert_resource`
   |
   = note: cannot satisfy `_: Resource`
   = help: the trait `Resource` is implemented for `Res`
note: required by a bound in `insert_resource`
  --> src/main.rs:4:23
   |
4  | fn insert_resource<R: Resource>(_resource: R) {}
   |                       ^^^^^^^^ required by this bound in `insert_resource`
help: consider specifying the generic argument
   |
10 |     insert_resource::<R>(Res.into());
   |                    +++++
help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds
   |
10 -     insert_resource(Res.into());
10 +     insert_resource(Res);

And how it looks now without the ICE.

warning: unused import: `minirapier::Ray`
 --> src/main.rs:2:5
  |
2 | use minirapier::Ray;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error[E0283]: type annotations needed
  --> src/main.rs:10:5
   |
10 |     insert_resource(Res.into());
   |     ^^^^^^^^^^^^^^^ ---------- type must be known at this point
   |     |
   |     cannot infer type of the type parameter `R` declared on the function `insert_resource`
   |
   = note: cannot satisfy `_: Resource`
note: there are multiple different versions of crate `minibevy` in the dependency graph
  --> /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_b/src/lib.rs:1:1
   |
1  | pub trait Resource {}
   | ^^^^^^^^^^^^^^^^^^ this is the required trait
   |
  ::: src/main.rs:1:5
   |
1  | use minibevy::Resource;
   |     -------- one version of crate `minibevy` is used here, as a direct dependency of the current crate
2  | use minirapier::Ray;
   |     ---------- one version of crate `minibevy` is used here, as a dependency of crate `minirapier`
   |
  ::: /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_a/src/lib.rs:1:1
   |
1  | pub trait Resource {}
   | ------------------ this is the found trait
   = help: you can use `cargo tree` to explore your dependency tree
note: required by a bound in `insert_resource`
  --> src/main.rs:4:23
   |
4  | fn insert_resource<R: Resource>(_resource: R) {}
   |                       ^^^^^^^^ required by this bound in `insert_resource`
help: consider specifying the generic argument
   |
10 |     insert_resource::<R>(Res.into());
   |                    +++++
help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds
   |
10 -     insert_resource(Res.into());
10 +     insert_resource(Res);
   |

The improvements from #128849 are still present and the note about the trait coming from the 2 versions of bevy is more explanatory/helpful than before, albeit a bit verbosely.

Fixes #132920.

@rustbot
Copy link
Collaborator

rustbot commented Nov 21, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, unfortunate diagnostics change, but ICEing isn't ideal either. You can r=me after PR CI is green.

Copy link
Member

Choose a reason for hiding this comment

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

Remark: this is indeed quite the setup... Thanks for the comments.

@jieyouxu
Copy link
Member

Actually I just saw estebank wanted to look at this on zulip, so r? @estebank

@rustbot rustbot assigned estebank and unassigned jieyouxu Nov 21, 2024
@lqd
Copy link
Member Author

lqd commented Nov 21, 2024

Thanks for the quick review @jieyouxu

Esteban may take a look at it soon as well, according to https://rust-lang.zulipchat.com/#narrow/channel/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-11-21/near/483761338 so r? @estebank until then.

@rustbot

This comment was marked as resolved.

@jieyouxu
Copy link
Member

(We raced 😆)

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 25, 2024

📌 Commit 764e3e2 has been approved by estebank

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 25, 2024
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 26, 2024
Revert diagnostics hack to fix ICE 132920

This reverts 8a568d9 from rust-lang#128849 to fix the diagnostics ICE in rust-lang#132920.

The hack mentioned in that commit was supposed to be tailored to E277, but that codepath is used actually shared with other errors, e.g. at least the E283 from the linked issue.

We may have to eat the slightly worse diagnostics until a non-hacky way to make this error less verbose is implemented (or I guess a different hack specializing even more to E277's structure).

Sorry `@estebank` 🙏. I can close this if you'd prefer to fix it in a different way.

Since it seems unexpected that rust-lang#128849 would impact the repro, here's how the error used to look before that PR.

```console
warning: unused import: `minirapier::Ray`
 --> src/main.rs:2:5
  |
2 | use minirapier::Ray;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error[E0283]: type annotations needed
  --> src/main.rs:10:5
   |
10 |     insert_resource(Res.into());
   |     ^^^^^^^^^^^^^^^ ---------- type must be known at this point
   |     |
   |     cannot infer type of the type parameter `R` declared on the function `insert_resource`
   |
   = note: cannot satisfy `_: Resource`
   = help: the trait `Resource` is implemented for `Res`
note: required by a bound in `insert_resource`
  --> src/main.rs:4:23
   |
4  | fn insert_resource<R: Resource>(_resource: R) {}
   |                       ^^^^^^^^ required by this bound in `insert_resource`
help: consider specifying the generic argument
   |
10 |     insert_resource::<R>(Res.into());
   |                    +++++
help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds
   |
10 -     insert_resource(Res.into());
10 +     insert_resource(Res);
```

And how it looks now without the ICE.

```console
warning: unused import: `minirapier::Ray`
 --> src/main.rs:2:5
  |
2 | use minirapier::Ray;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error[E0283]: type annotations needed
  --> src/main.rs:10:5
   |
10 |     insert_resource(Res.into());
   |     ^^^^^^^^^^^^^^^ ---------- type must be known at this point
   |     |
   |     cannot infer type of the type parameter `R` declared on the function `insert_resource`
   |
   = note: cannot satisfy `_: Resource`
note: there are multiple different versions of crate `minibevy` in the dependency graph
  --> /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_b/src/lib.rs:1:1
   |
1  | pub trait Resource {}
   | ^^^^^^^^^^^^^^^^^^ this is the required trait
   |
  ::: src/main.rs:1:5
   |
1  | use minibevy::Resource;
   |     -------- one version of crate `minibevy` is used here, as a direct dependency of the current crate
2  | use minirapier::Ray;
   |     ---------- one version of crate `minibevy` is used here, as a dependency of crate `minirapier`
   |
  ::: /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_a/src/lib.rs:1:1
   |
1  | pub trait Resource {}
   | ------------------ this is the found trait
   = help: you can use `cargo tree` to explore your dependency tree
note: required by a bound in `insert_resource`
  --> src/main.rs:4:23
   |
4  | fn insert_resource<R: Resource>(_resource: R) {}
   |                       ^^^^^^^^ required by this bound in `insert_resource`
help: consider specifying the generic argument
   |
10 |     insert_resource::<R>(Res.into());
   |                    +++++
help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds
   |
10 -     insert_resource(Res.into());
10 +     insert_resource(Res);
   |
```

The improvements from rust-lang#128849 are still present and the note about the trait coming from the 2 versions of bevy is more explanatory/helpful than before, albeit a bit verbosely.

Fixes rust-lang#132920.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
Rollup of 28 pull requests

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133136 (Support ranges in `<[T]>::get_many_mut()`)
 - rust-lang#133140 (Inline ExprPrecedence::order into Expr::precedence)
 - rust-lang#133248 (CI: split x86_64-msvc-ext job)
 - rust-lang#133282 (Shorten the `MaybeUninit` `Debug` implementation)
 - rust-lang#133304 (Revert diagnostics hack to fix ICE 132920)
 - rust-lang#133326 (Remove the `DefinitelyInitializedPlaces` analysis.)
 - rust-lang#133362 (No need to re-sort existential preds in relate impl)
 - rust-lang#133367 (Simplify array length mismatch error reporting (to not try to turn consts into target usizes))
 - rust-lang#133394 (Bail on more errors in dyn ty lowering)
 - rust-lang#133410 (target check_consistency: ensure target feature string makes some basic sense)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)
 - rust-lang#133443 (Remove dead code stemming from the old effects desugaring (II))
 - rust-lang#133449 (std: expose `const_io_error!` as `const_error!`)
 - rust-lang#133450 (remove "onur-ozkan" from  users_on_vacation)
 - rust-lang#133454 (Update test expectations to accept LLVM 'initializes' attribute)
 - rust-lang#133458 (Fix `Result` and `Option` not getting a jump to def link generated)
 - rust-lang#133462 (Use ReadCache for archive reading in bootstrap)
 - rust-lang#133464 (std::thread: avoid leading whitespace in some panic messages)
 - rust-lang#133467 (tests: Add recursive associated type bound regression tests)
 - rust-lang#133470 (Cleanup: delete `//@ pretty-expanded` directive)
 - rust-lang#133473 (tests: Add regression test for recursive enum with Cow and Clone)
 - rust-lang#133481 (Disable `avr-rjmp-offset` on Windows for now)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
Rollup of 28 pull requests

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133136 (Support ranges in `<[T]>::get_many_mut()`)
 - rust-lang#133140 (Inline ExprPrecedence::order into Expr::precedence)
 - rust-lang#133248 (CI: split x86_64-msvc-ext job)
 - rust-lang#133282 (Shorten the `MaybeUninit` `Debug` implementation)
 - rust-lang#133304 (Revert diagnostics hack to fix ICE 132920)
 - rust-lang#133326 (Remove the `DefinitelyInitializedPlaces` analysis.)
 - rust-lang#133362 (No need to re-sort existential preds in relate impl)
 - rust-lang#133367 (Simplify array length mismatch error reporting (to not try to turn consts into target usizes))
 - rust-lang#133394 (Bail on more errors in dyn ty lowering)
 - rust-lang#133410 (target check_consistency: ensure target feature string makes some basic sense)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)
 - rust-lang#133443 (Remove dead code stemming from the old effects desugaring (II))
 - rust-lang#133449 (std: expose `const_io_error!` as `const_error!`)
 - rust-lang#133450 (remove "onur-ozkan" from  users_on_vacation)
 - rust-lang#133454 (Update test expectations to accept LLVM 'initializes' attribute)
 - rust-lang#133458 (Fix `Result` and `Option` not getting a jump to def link generated)
 - rust-lang#133462 (Use ReadCache for archive reading in bootstrap)
 - rust-lang#133464 (std::thread: avoid leading whitespace in some panic messages)
 - rust-lang#133467 (tests: Add recursive associated type bound regression tests)
 - rust-lang#133470 (Cleanup: delete `//@ pretty-expanded` directive)
 - rust-lang#133473 (tests: Add regression test for recursive enum with Cow and Clone)
 - rust-lang#133481 (Disable `avr-rjmp-offset` on Windows for now)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
Rollup of 28 pull requests

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133136 (Support ranges in `<[T]>::get_many_mut()`)
 - rust-lang#133140 (Inline ExprPrecedence::order into Expr::precedence)
 - rust-lang#133248 (CI: split x86_64-msvc-ext job)
 - rust-lang#133282 (Shorten the `MaybeUninit` `Debug` implementation)
 - rust-lang#133304 (Revert diagnostics hack to fix ICE 132920)
 - rust-lang#133326 (Remove the `DefinitelyInitializedPlaces` analysis.)
 - rust-lang#133362 (No need to re-sort existential preds in relate impl)
 - rust-lang#133367 (Simplify array length mismatch error reporting (to not try to turn consts into target usizes))
 - rust-lang#133394 (Bail on more errors in dyn ty lowering)
 - rust-lang#133410 (target check_consistency: ensure target feature string makes some basic sense)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)
 - rust-lang#133443 (Remove dead code stemming from the old effects desugaring (II))
 - rust-lang#133449 (std: expose `const_io_error!` as `const_error!`)
 - rust-lang#133450 (remove "onur-ozkan" from  users_on_vacation)
 - rust-lang#133454 (Update test expectations to accept LLVM 'initializes' attribute)
 - rust-lang#133458 (Fix `Result` and `Option` not getting a jump to def link generated)
 - rust-lang#133462 (Use ReadCache for archive reading in bootstrap)
 - rust-lang#133464 (std::thread: avoid leading whitespace in some panic messages)
 - rust-lang#133467 (tests: Add recursive associated type bound regression tests)
 - rust-lang#133470 (Cleanup: delete `//@ pretty-expanded` directive)
 - rust-lang#133473 (tests: Add regression test for recursive enum with Cow and Clone)
 - rust-lang#133481 (Disable `avr-rjmp-offset` on Windows for now)

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

Rollup of 8 pull requests

Successful merges:

 - rust-lang#115293 (Remove -Zfuel.)
 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133304 (Revert diagnostics hack to fix ICE 132920)
 - rust-lang#133402 (Constify `Drop` and `Destruct`)
 - rust-lang#133458 (Fix `Result` and `Option` not getting a jump to def link generated)
 - rust-lang#133471 (gce: fix typing_mode mismatch)
 - rust-lang#133475 (`MaybeStorage` improvements)
 - rust-lang#133513 (Only ignore windows-gnu in avr-jmp-offset)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f101562 into rust-lang:master Nov 27, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Nov 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 27, 2024
Rollup merge of rust-lang#133304 - lqd:issue-132920, r=estebank

Revert diagnostics hack to fix ICE 132920

This reverts 8a568d9 from rust-lang#128849 to fix the diagnostics ICE in rust-lang#132920.

The hack mentioned in that commit was supposed to be tailored to E277, but that codepath is used actually shared with other errors, e.g. at least the E283 from the linked issue.

We may have to eat the slightly worse diagnostics until a non-hacky way to make this error less verbose is implemented (or I guess a different hack specializing even more to E277's structure).

Sorry ``@estebank`` 🙏. I can close this if you'd prefer to fix it in a different way.

Since it seems unexpected that rust-lang#128849 would impact the repro, here's how the error used to look before that PR.

```console
warning: unused import: `minirapier::Ray`
 --> src/main.rs:2:5
  |
2 | use minirapier::Ray;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error[E0283]: type annotations needed
  --> src/main.rs:10:5
   |
10 |     insert_resource(Res.into());
   |     ^^^^^^^^^^^^^^^ ---------- type must be known at this point
   |     |
   |     cannot infer type of the type parameter `R` declared on the function `insert_resource`
   |
   = note: cannot satisfy `_: Resource`
   = help: the trait `Resource` is implemented for `Res`
note: required by a bound in `insert_resource`
  --> src/main.rs:4:23
   |
4  | fn insert_resource<R: Resource>(_resource: R) {}
   |                       ^^^^^^^^ required by this bound in `insert_resource`
help: consider specifying the generic argument
   |
10 |     insert_resource::<R>(Res.into());
   |                    +++++
help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds
   |
10 -     insert_resource(Res.into());
10 +     insert_resource(Res);
```

And how it looks now without the ICE.

```console
warning: unused import: `minirapier::Ray`
 --> src/main.rs:2:5
  |
2 | use minirapier::Ray;
  |     ^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

error[E0283]: type annotations needed
  --> src/main.rs:10:5
   |
10 |     insert_resource(Res.into());
   |     ^^^^^^^^^^^^^^^ ---------- type must be known at this point
   |     |
   |     cannot infer type of the type parameter `R` declared on the function `insert_resource`
   |
   = note: cannot satisfy `_: Resource`
note: there are multiple different versions of crate `minibevy` in the dependency graph
  --> /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_b/src/lib.rs:1:1
   |
1  | pub trait Resource {}
   | ^^^^^^^^^^^^^^^^^^ this is the required trait
   |
  ::: src/main.rs:1:5
   |
1  | use minibevy::Resource;
   |     -------- one version of crate `minibevy` is used here, as a direct dependency of the current crate
2  | use minirapier::Ray;
   |     ---------- one version of crate `minibevy` is used here, as a dependency of crate `minirapier`
   |
  ::: /home/lqd/rust/tmp/minimization/issue-132920/rustc-ice-version-conflict/minibevy_a/src/lib.rs:1:1
   |
1  | pub trait Resource {}
   | ------------------ this is the found trait
   = help: you can use `cargo tree` to explore your dependency tree
note: required by a bound in `insert_resource`
  --> src/main.rs:4:23
   |
4  | fn insert_resource<R: Resource>(_resource: R) {}
   |                       ^^^^^^^^ required by this bound in `insert_resource`
help: consider specifying the generic argument
   |
10 |     insert_resource::<R>(Res.into());
   |                    +++++
help: consider removing this method call, as the receiver has type `Res` and `Res: Resource` trivially holds
   |
10 -     insert_resource(Res.into());
10 +     insert_resource(Res);
   |
```

The improvements from rust-lang#128849 are still present and the note about the trait coming from the 2 versions of bevy is more explanatory/helpful than before, albeit a bit verbosely.

Fixes rust-lang#132920.
@lqd lqd deleted the issue-132920 branch January 12, 2025 22:28
@lqd
Copy link
Member Author

lqd commented Jan 12, 2025

Unfortunately, I hadn't noticed that #128849 was in beta already when we landed this.

So now #132920 could be encountered on stable, and while the setup was quite involved there, another issue with code unrelated to it was opened for the same ICE in #135410.

I guess we can stable nominate this PR, even though it doesn't seem worthy of a point release on its own. A lukewarm nomination.

@lqd lqd added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Jan 12, 2025
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 16, 2025
@apiraino
Copy link
Contributor

Stable backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team. Backport labels handled by them.

This backport alone probably does not justify a dot release.

@rustbot label +stable-accepted +T-compiler

@rustbot rustbot added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Jan 16, 2025
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. stable-accepted Accepted for backporting to the compiler in the stable channel. stable-nominated Nominated for backporting to the compiler in the stable channel. 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.

error: the compiler unexpectedly panicked. this is a bug.
6 participants