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

Document TRACK_DIAGNOSTIC calls. #120699

Merged
merged 7 commits into from
Mar 14, 2024

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Feb 6, 2024

@rustbot rustbot added 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 Feb 6, 2024
@nnethercote
Copy link
Contributor Author

@cjgillot: I'm fairly confident this is correct, but not 100%. I'm hoping you can confirm it.

@bors rollup=always

@bors
Copy link
Contributor

bors commented Feb 6, 2024

☔ The latest upstream changes (presumably #120715) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote nnethercote force-pushed the rm-useless-TRACK_DIAGNOSTIC-calls branch from e8fcc8e to 091091b Compare February 7, 2024 03:12
@nnethercote
Copy link
Contributor Author

It's a tiny change, but I still managed to make this conflict with one of my own PRs :)

Anyway, I rebased.

@nnethercote nnethercote force-pushed the rm-useless-TRACK_DIAGNOSTIC-calls branch from 091091b to fd40d67 Compare February 9, 2024 05:44
@nnethercote
Copy link
Contributor Author

I added a small follow-up refactoring.

@bors
Copy link
Contributor

bors commented Feb 12, 2024

☔ The latest upstream changes (presumably #120980) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

I took a bit of time before getting to this. I think the reasoning is correct: we only need to track actual side effects, not potential side effects. However, I have a doubt about the Expect case.

compiler/rustc_errors/src/lib.rs Show resolved Hide resolved
compiler/rustc_errors/src/lib.rs Show resolved Hide resolved
compiler/rustc_errors/src/lib.rs Show resolved Hide resolved
@nnethercote nnethercote force-pushed the rm-useless-TRACK_DIAGNOSTIC-calls branch from fd40d67 to 4d9ce73 Compare February 15, 2024 04:31
@nnethercote nnethercote changed the title Remove useless TRACK_DIAGNOSTIC calls. Document TRACK_DIAGNOSTIC calls. Feb 15, 2024
@nnethercote
Copy link
Contributor Author

I have redone this PR. There's a bunch of refactoring, plus some addition of comments. Quite different from the original version, but I think it adds some welcome clarity to emit_diagnostic.

// Is saving the diagnostic in `delayed_bugs` a notable
// side-effect? Should `TRACK_DIAGNOSTIC` be called?
// Unclear. Currently we err on the side of "no" to avoid
// having to clone the diagnostic.
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need to call TRACK_DIAGNOSTIC: if there is a delayed bug, the incremental session is deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have updated the comment.

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2024
@nnethercote nnethercote force-pushed the rm-useless-TRACK_DIAGNOSTIC-calls branch from 4d9ce73 to c2b6e91 Compare February 18, 2024 18:29
@bors
Copy link
Contributor

bors commented Feb 22, 2024

☔ The latest upstream changes (presumably #121415) made this pull request unmergeable. Please resolve the merge conflicts.

This means `DiagCtxtInner::emit_diagnostic` can return its result
directly, rather than having to modify a local variable.
It has a single call site, and this will enable subsequent refactorings.
It results in a tiny bit of duplication (another
`self.treat_next_err_as_bug()` condition) but I think it's worth it to
get more code into the main `match`.
This will enable additional refactorings.
Note that `self.suppressed_expected_diag` is no longer set for
`ForceWarning`, which is good. Nor is `TRACK_DIAGNOSTIC` called for
`Allow`, which is also good.
Also add an assertion for the levels allowed with `has_future_breakage`.
This match is complex enough that it's a good idea to enumerate every
variant.

This also means `can_be_top_or_sub` can just be `can_be_subdiag`.
@nnethercote nnethercote force-pushed the rm-useless-TRACK_DIAGNOSTIC-calls branch from c2b6e91 to 7ef605b Compare March 1, 2024 02:57
@nnethercote
Copy link
Contributor Author

I rebased and added one new commit, to make the match in emit_diagnostic enumerate every variant. I think this is good to merge now, I like how much it has clarified all the different cases in emit_diagnostic.

@nnethercote
Copy link
Contributor Author

@cjgillot has gone on vacation for two weeks, so I will reassign:

r? @oli-obk

We have already gone through one round of review feedback and updates, I think @cjgillot was close to approving it.

@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 Mar 13, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2024
…OSTIC-calls, r=oli-obk

Document `TRACK_DIAGNOSTIC` calls.

r? `@cjgillot`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#121207 (Add `-Z external-clangrt`)
 - rust-lang#122397 (Various cleanups around the const eval query providers)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122422 (compiletest: Allow `only-unix` in test headers)
 - rust-lang#122424 (fix: typos)
 - rust-lang#122425 (Increase timeout for new bors bot)
 - rust-lang#122426 (Fix StableMIR `WrappingRange::is_full` computation)
 - rust-lang#122430 (Generate link to `Local` in `hir::Let` documentation)
 - rust-lang#122434 (pattern analysis: rename a few types)
 - rust-lang#122437 (pattern analysis: remove `MaybeInfiniteInt::JustAfterMax`)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2024
…OSTIC-calls, r=oli-obk

Document `TRACK_DIAGNOSTIC` calls.

r? ``@cjgillot``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2024
…OSTIC-calls, r=oli-obk

Document `TRACK_DIAGNOSTIC` calls.

r? ```@cjgillot```
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#120943 (Create some minimal HIR for associated opaque types)
 - rust-lang#121764 (Make incremental sessions identity no longer depend on the crate names provided by source code)
 - rust-lang#122375 (CFI: Break tests into smaller files)
 - rust-lang#122397 (Various cleanups around the const eval query providers)
 - rust-lang#122405 (Add methods to create StableMIR constant)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122440 (const-eval: organize and extend tests for required-consts)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 14, 2024
…OSTIC-calls, r=oli-obk

Document `TRACK_DIAGNOSTIC` calls.

r? ````@cjgillot````
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#114038 (unix time module now return result)
 - rust-lang#119676 (rustdoc-search: search types by higher-order functions)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#121899 (Document how removing a type's field can be bad and what to do instead)
 - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`)
 - rust-lang#122397 (Various cleanups around the const eval query providers)
 - rust-lang#122405 (Add methods to create StableMIR constant)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122440 (const-eval: organize and extend tests for required-consts)
 - rust-lang#122461 (fix unsoundness in Step::forward_unchecked for signed integers)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 14, 2024
…OSTIC-calls, r=oli-obk

Document `TRACK_DIAGNOSTIC` calls.

r? `````@cjgillot`````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 14, 2024
…OSTIC-calls, r=oli-obk

Document `TRACK_DIAGNOSTIC` calls.

r? ``````@cjgillot``````
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#119676 (rustdoc-search: search types by higher-order functions)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#121899 (Document how removing a type's field can be bad and what to do instead)
 - rust-lang#122405 (Add methods to create StableMIR constant)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122421 (Improve `Step` docs)
 - rust-lang#122440 (const-eval: organize and extend tests for required-consts)
 - rust-lang#122461 (fix unsoundness in Step::forward_unchecked for signed integers)

Failed merges:

 - rust-lang#122397 (Various cleanups around the const eval query providers)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fce6e75 into rust-lang:master Mar 14, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2024
Rollup merge of rust-lang#120699 - nnethercote:rm-useless-TRACK_DIAGNOSTIC-calls, r=oli-obk

Document `TRACK_DIAGNOSTIC` calls.

r? ```````@cjgillot```````
@nnethercote nnethercote deleted the rm-useless-TRACK_DIAGNOSTIC-calls branch March 18, 2024 01:13
nnethercote added a commit to nnethercote/rust that referenced this pull request Jun 20, 2024
In rust-lang#120699 I moved some code dealing with `has_future_breakage` earlier
in `emit_diagnostic`. Issue rust-lang#126521 identified a case where that
reordering was invalid (leading to an assertion failure) for some `Expect`
diagnostics.

This commit partially undoes the change, by moving the handling of
unstable `Expect` diagnostics earlier again. This makes
`emit_diagnostic` a bit uglier, but is necessary to fix the problem.

Fixes rust-lang#126521.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 20, 2024
Fix assertion failure for some `Expect` diagnostics.

In rust-lang#120699 I moved some code dealing with `has_future_breakage` earlier in `emit_diagnostic`. Issue rust-lang#126521 identified a case where that reordering was invalid (leading to an assertion failure) for some `Expect` diagnostics.

This commit partially undoes the change, by moving the handling of unstable `Expect` diagnostics earlier again. This makes `emit_diagnostic` a bit uglier, but is necessary to fix the problem.

Fixes rust-lang#126521.

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 20, 2024
Fix assertion failure for some `Expect` diagnostics.

In rust-lang#120699 I moved some code dealing with `has_future_breakage` earlier in `emit_diagnostic`. Issue rust-lang#126521 identified a case where that reordering was invalid (leading to an assertion failure) for some `Expect` diagnostics.

This commit partially undoes the change, by moving the handling of unstable `Expect` diagnostics earlier again. This makes `emit_diagnostic` a bit uglier, but is necessary to fix the problem.

Fixes rust-lang#126521.

r? ``@oli-obk``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2024
Rollup merge of rust-lang#126719 - nnethercote:fix-126521, r=oli-obk

Fix assertion failure for some `Expect` diagnostics.

In rust-lang#120699 I moved some code dealing with `has_future_breakage` earlier in `emit_diagnostic`. Issue rust-lang#126521 identified a case where that reordering was invalid (leading to an assertion failure) for some `Expect` diagnostics.

This commit partially undoes the change, by moving the handling of unstable `Expect` diagnostics earlier again. This makes `emit_diagnostic` a bit uglier, but is necessary to fix the problem.

Fixes rust-lang#126521.

r? ``@oli-obk``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants