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

Query panic!() to useful diagnostic #119086

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

RossSmyth
Copy link
Contributor

@RossSmyth RossSmyth commented Dec 18, 2023

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:#118955

@rustbot
Copy link
Collaborator

rustbot commented Dec 18, 2023

r? @davidtwco

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

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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 18, 2023
@RossSmyth RossSmyth changed the title Query panic!() Query panic!() to FatalError Dec 18, 2023
@RossSmyth
Copy link
Contributor Author

RossSmyth commented Dec 18, 2023

In the same vein as #118955 FatalError.raise() is very similar to a non-descriptive panic. So if we want these to have nicer messages then FatalError.raise() will probably need the same treatment as panic!(). The main difference in handling is that there are a couple instances where panics are caught, and they have some special handling. I mainly converted these over to normalize them when I was able to.

if !value.is::<rustc_errors::FatalErrorMarker>() {
print_markframe_trace(self.dep_graph(), frame);
}

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@RossSmyth RossSmyth changed the title Query panic!() to FatalError Query panic!() to useful diagnostic Dec 18, 2023
@rust-log-analyzer

This comment has been minimized.

@RossSmyth
Copy link
Contributor Author

RossSmyth commented Dec 19, 2023

I see, so there are well defined errors that occur that will lead to a query being poisoned, then when checked they will be QueryResult::Poisoned, so then it leads to the panic message and stacktrace, which FatalError does not have.

So as I have it right now it is useful for compiler bugs, but not useful for user-facing errors like those testing as it may be more confusing.

I don't know enough about the query system to say if those are actually mutually exclusive. Maybe there's a way to signal there's a graceful error? Maybe a new QueryResult variant, and setting it in the JobOwner drop on some condition?

compiler/rustc_query_system/src/query/plumbing.rs Outdated Show resolved Hide resolved
compiler/rustc_query_system/src/query/plumbing.rs Outdated Show resolved Hide resolved
compiler/rustc_query_system/src/query/plumbing.rs Outdated Show resolved Hide resolved
compiler/rustc_query_system/src/query/plumbing.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@davidtwco
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 3, 2024

📌 Commit abe9c14 has been approved by davidtwco

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 3, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Jan 3, 2024
fmease added a commit to fmease/rust that referenced this pull request 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
Copy link
Member

fmease commented Jan 3, 2024

10 commits for a +21/-11 diff is quite a lot. I've already rolled it up but I think that this should've been squashed 🙈

@compiler-errors
Copy link
Member

Wait yes, please squash this

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 3, 2024
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 3, 2024
@RossSmyth
Copy link
Contributor Author

Okie dokie, forgot about this

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 3, 2024

📌 Commit 0d421c5 has been approved by compiler-errors

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 3, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Jan 3, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 3, 2024
Rollup of 21 pull requests

Successful merges:

 - rust-lang#119086 (Query panic!() to useful diagnostic)
 - rust-lang#119239 (Remove unnecessary arm in `check_expr_yield`)
 - rust-lang#119298 (suppress change-tracker warnings in CI containers)
 - rust-lang#119319 (Document that File does not buffer reads/writes)
 - rust-lang#119434 (rc: Take *const T in is_dangling)
 - rust-lang#119444 (Rename `TyCtxt::is_closure` to `TyCtxt::is_closure_or_coroutine`)
 - rust-lang#119474 (Update tracking issue of naked_functions)
 - rust-lang#119476 (Pretty-print always-const trait predicates correctly)
 - rust-lang#119477 (rustdoc ui: adjust tooltip z-index to be above sidebar)
 - rust-lang#119479 (Remove two unused feature gates from rustc_query_impl)
 - rust-lang#119487 (Minor improvements in comment on `freshen.rs`)
 - rust-lang#119492 (Update books)
 - rust-lang#119494 (Deny defaults for higher-ranked generic parameters)
 - rust-lang#119498 (Update deadlinks of `strict_provenance` lints)
 - rust-lang#119505 (Don't synthesize host effect params for trait associated functions marked const)
 - rust-lang#119510 (Report I/O errors from rmeta encoding with emit_fatal)
 - rust-lang#119512 (Mark myself as back from leave)
 - rust-lang#119514 (coverage: Avoid a query stability hazard in `function_coverage_map`)
 - rust-lang#119523 (llvm: Allow `noundef` in codegen tests)
 - rust-lang#119534 (Update `thread_local` examples to use `local_key_cell_methods`)
 - rust-lang#119544 (Fix: Properly set vendor in i686-win7-windows-msvc target)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 093bd08 into rust-lang:master Jan 3, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 3, 2024
@RossSmyth RossSmyth deleted the query_panics branch January 3, 2024 20:35
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request 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
// The query we waited on panicked. Continue unwinding here.
Some(QueryResult::Poisoned) => FatalError.raise(),
Some(QueryResult::Poisoned) => {
panic!("query '{}' not cached due to poisoning", query.name())
Copy link
Contributor

Choose a reason for hiding this comment

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

This branch now introduces a panic, which is not correct.

jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 5, 2024
Don't panic when waiting on poisoned queries

This fixes a bug introduced in rust-lang#119086.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
Don't panic when waiting on poisoned queries

This fixes a bug introduced in rust-lang#119086.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 5, 2024
Don't panic when waiting on poisoned queries

This fixes a bug introduced in rust-lang#119086.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
Rollup merge of rust-lang#121913 - Zoxc:query-fix, r=compiler-errors

Don't panic when waiting on poisoned queries

This fixes a bug introduced in rust-lang#119086.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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.

8 participants