Improve ICE message for forbidden dep-graph reads. #124252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-query-system
bors
added
S-waiting-on-bors
RalfJung
reviewed
Apr 22, 2024
|
|
||
| panic!( | ||
| "Error: trying to record dependency on DepNode {dep_node} in a \ | ||
| context that does not allow it (e.g. during query deserialization)." |
There was a problem hiding this comment.
FWIW, as someone who knows nothing about the query/incremental system, I still don't know that "record dependency" is the same as "invoke query". Or maybe it's not the same, but the explanation you gave me that I actually understood was "you invoked a query in a context that does not allow it". But maybe there are cases where saying "invoked a query" here would be wrong?
There was a problem hiding this comment.
Good point. Invoking a query is only of several ways of recording something in the dependency graph, but it is by far the most common. I'll update the error message.
There was a problem hiding this comment.
Is the new message better?
RalfJung
reviewed
Apr 22, 2024
| The most common case of recording a dependency on a DepNode `foo` is \ | ||
| when the correspondng query `foo` is invoked. Invoking queries is not \ | ||
| allowed as part of loading something from the incremental on-disk cache. \ | ||
| See <https://github.com/rust-lang/rust/pull/91919>." |
There was a problem hiding this comment.
That's a lot better, thanks!
Not sure if linking to the PR is very useful, though.
There was a problem hiding this comment.
I'll leave the link in unless you or @oli-obk think that it's actively hurting understandability.
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Apr 22, 2024
…read-ice, r=oli-obk Improve ICE message for forbidden dep-graph reads. The new message mentions the main context that the ICE might occur in and it mentions the query/dep-node that is being read. cc rust-lang#123781, where this would have been helpful.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 22, 2024
…llaumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#124178 ([cleanup] [llvm backend] Prevent creating the same `Instance::mono` multiple times) - rust-lang#124183 (Stop taking `ParamTy`/`ParamConst`/`EarlyParamRegion`/`AliasTy` by ref) - rust-lang#124217 (coverage: Prepare for improved branch coverage) - rust-lang#124220 (Miri: detect wrong vtables in wide pointers) - rust-lang#124252 (Improve ICE message for forbidden dep-graph reads.) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 22, 2024
…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#115913 (checked_ilog: improve performance) - rust-lang#124178 ([cleanup] [llvm backend] Prevent creating the same `Instance::mono` multiple times) - rust-lang#124183 (Stop taking `ParamTy`/`ParamConst`/`EarlyParamRegion`/`AliasTy` by ref) - rust-lang#124217 (coverage: Prepare for improved branch coverage) - rust-lang#124230 (Stabilize generic `NonZero`.) - rust-lang#124252 (Improve ICE message for forbidden dep-graph reads.) - rust-lang#124268 (Update books) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 23, 2024
…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#115913 (checked_ilog: improve performance) - rust-lang#124178 ([cleanup] [llvm backend] Prevent creating the same `Instance::mono` multiple times) - rust-lang#124183 (Stop taking `ParamTy`/`ParamConst`/`EarlyParamRegion`/`AliasTy` by ref) - rust-lang#124217 (coverage: Prepare for improved branch coverage) - rust-lang#124230 (Stabilize generic `NonZero`.) - rust-lang#124252 (Improve ICE message for forbidden dep-graph reads.) - rust-lang#124268 (Update books) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 23, 2024
Rollup merge of rust-lang#124252 - michaelwoerister:better-forbidden-read-ice, r=oli-obk Improve ICE message for forbidden dep-graph reads. The new message mentions the main context that the ICE might occur in and it mentions the query/dep-node that is being read. cc rust-lang#123781, where this would have been helpful.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The new message mentions the main context that the ICE might occur in and it mentions the query/dep-node that is being read.
cc #123781, where this would have been helpful.