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

Tweak ICE message #107771

Merged
merged 1 commit into from
Feb 8, 2023
Merged

Tweak ICE message #107771

merged 1 commit into from
Feb 8, 2023

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Feb 7, 2023

Modify main message to be more conversational and emit one fewer note.

Modify main message to be more conversational and emit one fewer note.
@rustbot
Copy link
Collaborator

rustbot commented Feb 7, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@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 Feb 7, 2023
@estebank
Copy link
Contributor Author

estebank commented Feb 7, 2023

Before
Screenshot 2023-02-07 at 11 18 13 AM

After
Screenshot 2023-02-07 at 11 17 46 AM

@compiler-errors
Copy link
Member

Thanks. This is clearer.

r? @compiler-errors @bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 7, 2023

📌 Commit a7597a1 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 Feb 7, 2023
@matthiaskrgr
Copy link
Member

imo "internal compiler error" and "unexpected panic" are very greppable lines where you can almost be 100% sure if you find this in rustcs output, you found a bug in rustc (and not some proc macro or build.rs script..)

Does this change anything about the output of delay_span_bugs or is this only for the error after the stacktrace?

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Good point @matthiaskrgr, maybe we should emit the same message as a note on the delayed/non-delayed bug cases like:

@@ -1200,11 +1200,9 @@ pub fn report_ice(info: &panic::PanicInfo<'_>, bug_report_url: &str) {
if !info.payload().is::<rustc_errors::ExplicitBug>()
&& !info.payload().is::<rustc_errors::DelayedBugPanic>()
{
let mut d = rustc_errors::Diagnostic::new(rustc_errors::Level::Bug, "unexpected panic");
handler.emit_diagnostic(&mut d);
handler.emit_err(session_diagnostics::Ice);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
} else {
handler.emit_note(session_diagnostics::Ice);
}

@compiler-errors
Copy link
Member

compiler-errors commented Feb 7, 2023

On the other hand, I'm surprised we don't have even one unit test testing delayed bugs.

edit: I'm dumb, yes we do.

@@ -4,7 +4,7 @@ error: internal compiler error: delayed span bug triggered by #[rustc_error(dela
LL | fn main() {}
| ^^^^^^^^^

error: internal compiler error: unexpected panic
error: the compiler unexpectedly panicked. this is a bug.
Copy link
Member

Choose a reason for hiding this comment

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

Actually, now that I think of it, @matthiaskrgr it should be equally valid to grep for something like "this is a bug" for delayed bug cases like this, rather than "unexpected panic". Is that ok?

Copy link
Member

Choose a reason for hiding this comment

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

rg'ed in my local cargo cache (3K crates, 81 git checkouts)

  • internal compiler error: 0
  • unexpected panic 14
  • this is a bug 109
  • the compiler unexpectedly panicked: 0

so "internal compiler error" and "the compiler unexpectedly panicked" gives us the most information because we can almost always be sure, that this is NOT a false positive from for example a format!("internal compiler error") that needed an extra & and sneaked into a diagnostic that way...
"this is a bug" however gives us the least amount of information because a bunch of other crates actually use this in their code.

Copy link
Member

Choose a reason for hiding this comment

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

well "unexpectedly panicked" should also work as a good grep string then?

@estebank
Copy link
Contributor Author

estebank commented Feb 7, 2023

Does this change anything about the output of delay_span_bugs or is this only for the error after the stacktrace?
compiler-errors

emit_bug will continue to write error: internal compiler error: <message from the error>, so that doesn't change.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 8, 2023
Tweak ICE message

Modify main message to be more conversational and emit one fewer note.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#107656 (Bump rust-installer)
 - rust-lang#107757 (Allow automatically creating vscode `settings.json` with `x setup`)
 - rust-lang#107769 (Rename `PointerSized` to `PointerLike`)
 - rust-lang#107770 (rustdoc: use a newline instead of `<br>` to format code headers)
 - rust-lang#107771 (Tweak ICE message)
 - rust-lang#107773 (Clearly signal purpose of the yaml template)
 - rust-lang#107776 (Docs: Fix format of headings in String::reserve)
 - rust-lang#107779 (Remove astconv usage in diagnostic)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 31345cd into rust-lang:master Feb 8, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 8, 2023
@estebank estebank deleted the ice-msg branch November 9, 2023 05:11
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.

6 participants