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

consider bannig empty panic/unreachable/ bugs from rustc codebase #118955

Open
matthiaskrgr opened this issue Dec 14, 2023 · 5 comments
Open

consider bannig empty panic/unreachable/ bugs from rustc codebase #118955

matthiaskrgr opened this issue Dec 14, 2023 · 5 comments
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-meta Area: Issues & PRs about the rust-lang/rust repository itself C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@matthiaskrgr
Copy link
Member

git grep "[^e]bug\!()" compiler   130 
git grep "panic\!()" compiler  43 
git grep "unreachable\!()" compiler  384

I wonder if it makes sense to try to fill these up with at least somewhat contextual error/ice messages instead of just a "panicked at" or "impossible case reached"?

Unfortunately I could not find a rustc lint that warns on empty panics like these

@matthiaskrgr matthiaskrgr added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ A-meta Area: Issues & PRs about the rust-lang/rust repository itself labels Dec 14, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 14, 2023
@workingjubilee
Copy link
Member

13 of the panic!() cases are in comments, 4 are not in .rs files, and 2 are in rustc_codegen_cranelift as example code.

That leaves 24 unlabeled panics, as far as I can tell:

rustc_expand/src/parse/tests.rs
325:        let ast::ItemKind::Mod(_, mod_kind) = &item.kind else { panic!() };

rustc_hir_pretty/src/lib.rs
2182:                            _ => panic!(),

rustc_metadata/src/creader.rs
599:            _ => panic!(),

rustc_resolve/src/late.rs
2540:            panic!()

rustc_hir_analysis/src/collect/type_of.rs
23:    let Node::AnonConst(_) = tcx.hir().get(hir_id) else { panic!() };

rustc_span/src/caching_source_map_view.rs
208:                panic!();

rustc_middle/src/dep_graph/dep_node.rs
110:                    panic!();

rustc_ast_pretty/src/pprust/state.rs
1558:                _ => panic!(),

rustc_ast_lowering/src/path.rs
288:            Some(_) => panic!(),

rustc_abi/src/layout.rs
603:                    _ => panic!(),
631:                    panic!();
684:                    _ => panic!(),
761:            _ => panic!(),
1157:                            _ => panic!(),

rustc_lint/src/early.rs
317:        panic!()

rustc_lint/src/passes.rs
128:                panic!()
243:                panic!()

rustc_lint/src/late.rs
330:        panic!()

rustc_query_system/src/query/plumbing.rs
174:                QueryResult::Poisoned => panic!(),
195:                QueryResult::Poisoned => panic!(),

rustc_codegen_gcc/src/common.rs
254:                    panic!()

rustc_codegen_gcc/src/builder.rs
1160:            panic!();
1191:                panic!();

@compiler-errors
Copy link
Member

compiler-errors commented Dec 15, 2023

I'll take a stab at annotating some of the ones in rustc_hir*, rustc_trait*, and rustc_infer.

@workingjubilee
Copy link
Member

Assuming the currently outstanding PRs are accepted, the remaining unlabeled panic!() instances are in rustc_codegen_gcc, rustc_lint, rustc_metadata, rustc_query_system, and rustc_resolve.

I have not even glanced at where all the unreachable!() instances are since there are... many.

@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-error-handling Area: Error handling and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ A-error-handling Area: Error handling labels Dec 15, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 15, 2023
Annotate some bugs

Gives a semi-helpful message to some `bug!()`/`unreachable!()`/`panic!()`. This also works around some other bugs/panics/etc that weren't needed, and also makes some of them into `span_bug!`s so they also have a useful span.

Note to reviewer: best to disable whitespace when comparing for some cases where indentation changed.

cc rust-lang#118955
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 15, 2023
…m-abi, r=davidtwco

Annotate panic! reasons during enum layout

Add some reasons to the panics, and use more exhaustive matches.

Also see: rust-lang#118955
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 15, 2023
Rollup merge of rust-lang#118974 - workingjubilee:why-worry-about-enum-abi, r=davidtwco

Annotate panic! reasons during enum layout

Add some reasons to the panics, and use more exhaustive matches.

Also see: rust-lang#118955
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 15, 2023
Rollup merge of rust-lang#118962 - compiler-errors:bugs, r=TaKO8Ki

Annotate some bugs

Gives a semi-helpful message to some `bug!()`/`unreachable!()`/`panic!()`. This also works around some other bugs/panics/etc that weren't needed, and also makes some of them into `span_bug!`s so they also have a useful span.

Note to reviewer: best to disable whitespace when comparing for some cases where indentation changed.

cc rust-lang#118955
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 18, 2023
Add better ICE messages for some undescriptive panics

Add some better messages at some panics

re: rust-lang#118955

I took a look at some others but either was not able to figure out what they did, or it was unclear what they should say instead. For example in the query system whether each time a poisoned value is matched upon if they should all just call `FatalError.raise()`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 18, 2023
Rollup merge of rust-lang#118967 - RossSmyth:panic-messages, r=TaKO8Ki

Add better ICE messages for some undescriptive panics

Add some better messages at some panics

re: rust-lang#118955

I took a look at some others but either was not able to figure out what they did, or it was unclear what they should say instead. For example in the query system whether each time a poisoned value is matched upon if they should all just call `FatalError.raise()`
@fee1-dead
Copy link
Member

would it be worth it to add an internal lint for this?

fmease added a commit to fmease/rust that referenced this issue Jan 3, 2024
Query panic!() to useful diagnostic

Changes some more ICEs from bare panic!()s

Adds an `expect_job()` helper method as that is a moral equivalent of what was happening at the uses.

re:rust-lang#118955
fmease added a commit to fmease/rust that referenced this issue Jan 3, 2024
…-errors

Query panic!() to useful diagnostic

Changes some more ICEs from bare panic!()s

Adds an `expect_job()` helper method as that is a moral equivalent of what was happening at the uses.

re:rust-lang#118955
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 3, 2024
Rollup merge of rust-lang#119086 - RossSmyth:query_panics, r=compiler-errors

Query panic!() to useful diagnostic

Changes some more ICEs from bare panic!()s

Adds an `expect_job()` helper method as that is a moral equivalent of what was happening at the uses.

re:rust-lang#118955
@jieyouxu
Copy link
Member

Update (2024-02-27):

git grep "[^e]bug\!()" compiler | count       111
git grep "panic\!()" compiler | count         30
git grep "unreachable\!()" compiler | count   358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-meta Area: Issues & PRs about the rust-lang/rust repository itself C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants