Explicitly call emit_stashed_diagnostics.#121487
Merged
bors merged 1 commit intorust-lang:masterfrom Feb 23, 2024
Merged
Conversation
Commit 72b172b in rust-lang#121206 changed things so that `emit_stashed_diagnostics` is only called from `run_compiler`. But rustfmt doesn't use `run_compiler`, so it needs to call `emit_stashed_diagnostics` itself to avoid an abort in `DiagCtxtInner::drop` when stashed diagnostics occur. Fixes rust-lang#121450.
Collaborator
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
rustbot
added
the
S-waiting-on-review
Contributor
Author
|
The changes are all within rustfmt so there's an argument that this PR should be against the rustfmt repo. But I think the changes in rustc that caused this bug haven't yet made it into the rustfmt repo, whereas this crash is manifesting in the rust repo. So I think the rust repo is appropriate. |
Contributor
|
@nnethercote Yeah, I think it makes sense to address this in rust-lang/rust since it's impacting nightly rustfmt users. |
ytmimi
reviewed
Feb 23, 2024
| // when dropped. The final result here combines the parsing result and | ||
| // the `emit_stashed_diagnostics` result. | ||
| let parse_res = catch_unwind(move || parser.parse_crate_mod()); | ||
| let stashed_res = self.parser.dcx().emit_stashed_diagnostics(); |
Contributor
There was a problem hiding this comment.
I'm wondering will this emit anything when using the silent emitter? I'm guessing it won't, but just want to double check.
Contributor
Author
There was a problem hiding this comment.
It shouldn't. DiagCtxt::emit_stashed_diagnostics calls DiagCtxtInner:emit_diagnostic on each diagnostic, which calls emit_diagnostic on the emitter held by the DiagCtxt. So if that's the silent emitter, then nothing will be printed.
Contributor
There was a problem hiding this comment.
Thanks for the explanation 😁
Contributor
|
@bors r+ rollup |
Collaborator
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 23, 2024
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#121434 (Fix rust-lang#121208 fallout) - rust-lang#121471 (When encountering `<&T as Clone>::clone(x)` because `T: Clone`, suggest `#[derive(Clone)]`) - rust-lang#121476 (remove `llvm.assertions=true` in compiler profile) - rust-lang#121479 (fix generalizer unsoundness) - rust-lang#121480 (Fix more rust-lang#121208 fallout) - rust-lang#121482 (Allow for a missing `adt_def` in `NamePrivacyVisitor`.) - rust-lang#121484 (coverage: Use variable name `this` in `CoverageGraph::from_mir`) - rust-lang#121487 (Explicitly call `emit_stashed_diagnostics`.) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 23, 2024
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#121434 (Fix rust-lang#121208 fallout) - rust-lang#121471 (When encountering `<&T as Clone>::clone(x)` because `T: Clone`, suggest `#[derive(Clone)]`) - rust-lang#121476 (remove `llvm.assertions=true` in compiler profile) - rust-lang#121479 (fix generalizer unsoundness) - rust-lang#121480 (Fix more rust-lang#121208 fallout) - rust-lang#121482 (Allow for a missing `adt_def` in `NamePrivacyVisitor`.) - rust-lang#121484 (coverage: Use variable name `this` in `CoverageGraph::from_mir`) - rust-lang#121487 (Explicitly call `emit_stashed_diagnostics`.) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 23, 2024
Rollup merge of rust-lang#121487 - nnethercote:fix-121450, r=oli-obk Explicitly call `emit_stashed_diagnostics`. Commit 72b172b in rust-lang#121206 changed things so that `emit_stashed_diagnostics` is only called from `run_compiler`. But rustfmt doesn't use `run_compiler`, so it needs to call `emit_stashed_diagnostics` itself to avoid an abort in `DiagCtxtInner::drop` when stashed diagnostics occur. Fixes rust-lang#121450. r? `@oli-obk`
Contributor
|
@nnethercote does this PR solve the same issue #121301 was looking into? |
Contributor
Author
I suspect that is a different problem, probably caused by #121085. |
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Feb 25, 2024
This call was added to `parse_crate_mod` in rust-lang#121487, to fix a case where a stashed diagnostic wasn't emitted. But there is another path where a stashed diagnostic might fail to be emitted if there's a parse error, if the `build` call in `parse_crate_inner` fails before `parse_crate_mod` is reached. So this commit moves the `emit_stashed_diagnostic` call outwards, from `parse_crate_mod` to `format_project`, just after the `Parser::parse_crate` call. This should be far out enough to catch any parsing errors. Fixes rust-lang#121517.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 26, 2024
Move `emit_stashed_diagnostic` call in rustfmt. This call was added to `parse_crate_mod` in rust-lang#121487, to fix a case where a stashed diagnostic wasn't emitted. But there is another path where a stashed diagnostic might fail to be emitted if there's a parse error, if the `build` call in `parse_crate_inner` fails before `parse_crate_mod` is reached. So this commit moves the `emit_stashed_diagnostic` call outwards, from `parse_crate_mod` to `format_project`, just after the `Parser::parse_crate` call. This should be far out enough to catch any parsing errors. Fixes rust-lang#121517. r? `@oli-obk` cc `@ytmimi`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 26, 2024
Rollup merge of rust-lang#121615 - nnethercote:fix-121517, r=oli-obk Move `emit_stashed_diagnostic` call in rustfmt. This call was added to `parse_crate_mod` in rust-lang#121487, to fix a case where a stashed diagnostic wasn't emitted. But there is another path where a stashed diagnostic might fail to be emitted if there's a parse error, if the `build` call in `parse_crate_inner` fails before `parse_crate_mod` is reached. So this commit moves the `emit_stashed_diagnostic` call outwards, from `parse_crate_mod` to `format_project`, just after the `Parser::parse_crate` call. This should be far out enough to catch any parsing errors. Fixes rust-lang#121517. r? `@oli-obk` cc `@ytmimi`
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Feb 28, 2024
I removed it in rust-lang#121206 because I thought thought it wasn't necessary. But then I had to add an `emit_stashed_diagnostics` call elsewhere in rustfmt to avoid the assertion failure (which took two attempts to get right, rust-lang#121487 and rust-lang#121615), and now there's an assertion failure in clippy as well (rust-lang/rust-clippy#12364). So this commit just reinstates the call in `DiagCtxtInner::drop`. It also reverts the rustfmt changes from rust-lang#121487 and rust-lang#121615, though it keeps the tests added for those PRs.
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Feb 29, 2024
I removed it in rust-lang#121206 because I thought thought it wasn't necessary. But then I had to add an `emit_stashed_diagnostics` call elsewhere in rustfmt to avoid the assertion failure (which took two attempts to get right, rust-lang#121487 and rust-lang#121615), and now there's an assertion failure in clippy as well (rust-lang/rust-clippy#12364). So this commit just reinstates the call in `DiagCtxtInner::drop`. It also reverts the rustfmt changes from rust-lang#121487 and rust-lang#121615, though it keeps the tests added for those PRs.
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Mar 1, 2024
I removed it in rust-lang#121206 because I thought thought it wasn't necessary. But then I had to add an `emit_stashed_diagnostics` call elsewhere in rustfmt to avoid the assertion failure (which took two attempts to get right, rust-lang#121487 and rust-lang#121615), and now there's an assertion failure in clippy as well (rust-lang/rust-clippy#12364). So this commit just reinstates the call in `DiagCtxtInner::drop`. It also reverts the rustfmt changes from rust-lang#121487 and rust-lang#121615, though it keeps the tests added for those PRs.
calebcartwright
pushed a commit
to calebcartwright/rust
that referenced
this pull request
Jun 22, 2024
This call was added to `parse_crate_mod` in rust-lang#121487, to fix a case where a stashed diagnostic wasn't emitted. But there is another path where a stashed diagnostic might fail to be emitted if there's a parse error, if the `build` call in `parse_crate_inner` fails before `parse_crate_mod` is reached. So this commit moves the `emit_stashed_diagnostic` call outwards, from `parse_crate_mod` to `format_project`, just after the `Parser::parse_crate` call. This should be far out enough to catch any parsing errors. Fixes rust-lang#121517.
calebcartwright
pushed a commit
to calebcartwright/rust
that referenced
this pull request
Jun 22, 2024
Move `emit_stashed_diagnostic` call in rustfmt. This call was added to `parse_crate_mod` in rust-lang#121487, to fix a case where a stashed diagnostic wasn't emitted. But there is another path where a stashed diagnostic might fail to be emitted if there's a parse error, if the `build` call in `parse_crate_inner` fails before `parse_crate_mod` is reached. So this commit moves the `emit_stashed_diagnostic` call outwards, from `parse_crate_mod` to `format_project`, just after the `Parser::parse_crate` call. This should be far out enough to catch any parsing errors. Fixes rust-lang#121517. r? `@oli-obk` cc `@ytmimi`
calebcartwright
pushed a commit
to calebcartwright/rust
that referenced
this pull request
Jun 22, 2024
I removed it in rust-lang#121206 because I thought thought it wasn't necessary. But then I had to add an `emit_stashed_diagnostics` call elsewhere in rustfmt to avoid the assertion failure (which took two attempts to get right, rust-lang#121487 and rust-lang#121615), and now there's an assertion failure in clippy as well (rust-lang/rust-clippy#12364). So this commit just reinstates the call in `DiagCtxtInner::drop`. It also reverts the rustfmt changes from rust-lang#121487 and rust-lang#121615, though it keeps the tests added for those PRs.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Commit 72b172b in #121206 changed things so that
emit_stashed_diagnosticsis only called fromrun_compiler. But rustfmt doesn't userun_compiler, so it needs to callemit_stashed_diagnosticsitself to avoid an abort inDiagCtxtInner::dropwhen stashed diagnostics occur.Fixes #121450.
r? @oli-obk