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
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions compiler/rustc_query_system/src/query/plumbing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@ enum QueryResult {
Poisoned,
}

impl QueryResult {
/// Unwraps the query job expecting that it has started.
fn expect_job(self) -> QueryJob {
match self {
Self::Started(job) => job,
Self::Poisoned => {
panic!("job for query failed to start and was poisoned")
}
}
}
}

impl<K> QueryState<K>
where
K: Eq + Hash + Copy + Debug,
Expand Down Expand Up @@ -169,10 +181,7 @@ where

let job = {
let mut lock = state.active.lock_shard_by_value(&key);
match lock.remove(&key).unwrap() {
QueryResult::Started(job) => job,
QueryResult::Poisoned => panic!(),
}
lock.remove(&key).unwrap().expect_job()
};

job.signal_complete();
Expand All @@ -190,10 +199,8 @@ where
let state = self.state;
let job = {
let mut shard = state.active.lock_shard_by_value(&self.key);
let job = match shard.remove(&self.key).unwrap() {
QueryResult::Started(job) => job,
QueryResult::Poisoned => panic!(),
};
let job = shard.remove(&self.key).unwrap().expect_job();

shard.insert(self.key, QueryResult::Poisoned);
job
};
Expand Down Expand Up @@ -277,11 +284,14 @@ where
// We didn't find the query result in the query cache. Check if it was
// poisoned due to a panic instead.
let lock = query.query_state(qcx).active.get_shard_by_value(&key).lock();

match lock.get(&key) {
// 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.

}
_ => panic!(
"query result must in the cache or the query must be poisoned after a wait"
"query '{}' result must be in the cache or the query must be poisoned after a wait",
query.name()
),
}
})
Expand Down
Loading