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

Count stashed errors again #121669

Merged
merged 6 commits into from
Feb 29, 2024
Merged

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Feb 27, 2024

Stashed diagnostics are such a pain. Their "might be emitted, might not" semantics messes with lots of things.

#120828 and #121206 made some big changes to how they work, improving some things, but still leaving some problems, as seen by the issues caused by #121206. This PR aims to fix all of them by restricting them in a way that eliminates the "might be emitted, might not" semantics while still allowing 98% of their benefit. Details in the individual commit logs.

r? @oli-obk

@rustbot
Copy link
Collaborator

rustbot commented Feb 27, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Feb 27, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 27, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

The code and output changes look reasonable to me. Left some nitpicks, but r=me on the approach.

self.tcx.dcx().span_delayed_bug(expr.span, "no adt_def for expression");
return;
};
let adt = self.typeck_results().expr_ty(expr).ty_adt_def().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly concerned that this will lead to an ICE that used not to happen (span_delayed_bug has been the go-to solution for "this is invalid state but other errors will likely have been emitted so an ICE isn't necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just reverting to what was there before #121482.

Comment on lines +849 to +858
// We used to cancel here for slightly better error messages, but
// cancelling stashed diagnostics is no longer allowed because it
// causes problems when tracking whether errors have actually
// occurred.
tcx.sess.dcx().try_steal_modify_and_emit_err(
tcx.def_span(opaque_def_id),
StashKey::OpaqueHiddenTypeMismatch,
|_err| {},
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is fine.

{
return;
}
let def_id = cx.typeck_results().type_dependent_def_id(expr.hir_id).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very weary of .unwrap() in the compiler. I've had cases where I leaned on it and later changes caused my expectations to not be met.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this is just reverting to what was there before #121586.

I understand the purpose of span_delayed_bug, but it shouldn't be used on genuinely unreachable code paths, right? I don't know if you saw #121208 where I deliberately changed a bunch of definitely/probably/possibly unreachable span_delayed_bug calls to span_bug... some of them were found with subsequent fuzzing to be reachable, which means we got test cases for those paths.

@nnethercote
Copy link
Contributor Author

I have updated to move the clippy test. I haven't made any other changes, not sure if my responses to the other comments will satisfy the reviewers.

(And apologies for messing up the syntax on the review request in the original description, it was meant to be for @oli-obk, but luck would have it that @estebank is a good reviewer for this PR as well.)

@rustbot rustbot assigned oli-obk and unassigned estebank Feb 27, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 28, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@nnethercote
Copy link
Contributor Author

I added one more commit, reinstating the emit_stashed_diagnostics call in DiagCtxtInner::drop, to fix all the assertion failures that we were getting (details in the commit message). That should fix all the problems that #121206 caused, I hope.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 28, 2024

@bors r=estebank

@fmease fmease closed this Feb 28, 2024
@fmease fmease reopened this Feb 28, 2024
@fmease
Copy link
Member

fmease commented Feb 28, 2024

@bors r=estebank

@bors
Copy link
Contributor

bors commented Feb 28, 2024

📌 Commit 1c5d1c7 has been approved by estebank

It is now in the queue for this repository.

@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 Feb 28, 2024
@bors
Copy link
Contributor

bors commented Feb 28, 2024

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 28, 2024
This gives one extra error message on one test, but is necessary to fix
bigger problems caused by the cancellation of stashed errors.

(Note: why not just avoid stashing altogether? Because that resulted in
additional output changes.)
This gives one extra error message on two tests, but is necessary to fix
bigger problems caused by the cancellation of stashed errors.

(Note: why not just avoid stashing altogether? Because that resulted in
additional output changes.)
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.

This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
  errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
  disallowing the use of `steal_diagnostic` with errors, and introducing
  the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
  that can be used instead.

Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
  return `Option<ErrorGuaranteed>`, which enables the removal of two
  `delayed_bug` calls and one `Ty::new_error_with_message` call. This is
  possible because we store error guarantees in
  `DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
  alongside calls to `has_errors`, which is a nice simplification, and
  eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
  error message.

Fixes rust-lang#121451.
Fixes rust-lang#121477.
Fixes rust-lang#121504.
Fixes rust-lang#121508.
This undoes the changes from rust-lang#121482 and rust-lang#121586, now that stashed errors
will trigger more early aborts.
Seems wise, since it shouldn't proceed in that case.
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
Copy link
Contributor Author

I rebased.

@bors r=estebank

@bors
Copy link
Contributor

bors commented Feb 29, 2024

📌 Commit 82961c0 has been approved by estebank

It is now in the queue for this repository.

@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 Feb 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 29, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#110543 (Make `ReentrantLock` public)
 - rust-lang#121689 ([rustdoc] Prevent inclusion of whitespace character after macro_rules ident)
 - rust-lang#121724 (Use `LitKind::Err` for malformed floats)
 - rust-lang#121735 (pattern analysis: Don't panic when encountering unexpected constructor)
 - rust-lang#121743 (Opportunistically resolve regions when processing region outlives obligations)

Failed merges:

 - rust-lang#121326 (Detect empty leading where clauses on type aliases)
 - rust-lang#121416 (Improve error messages for generics with default parameters)
 - rust-lang#121669 (Count stashed errors again)
 - rust-lang#121723 (Two diagnostic things)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 29, 2024
…llaumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#119748 (Increase visibility of `join_path` and `split_paths`)
 - rust-lang#120820 (Enable CMPXCHG16B, SSE3, SAHF/LAHF and 128-bit Atomics (in nightly) in Windows x64)
 - rust-lang#121000 (pattern_analysis: rework how we hide empty private fields)
 - rust-lang#121376 (Skip unnecessary comparison with half-open range patterns)
 - rust-lang#121596 (Use volatile access instead of `#[used]` for `on_tls_callback`)
 - rust-lang#121669 (Count stashed errors again)
 - rust-lang#121783 (Emitter cleanups)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a5945b5 into rust-lang:master Feb 29, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 29, 2024
Rollup merge of rust-lang#121669 - nnethercote:count-stashed-errs-again, r=estebank

Count stashed errors again

Stashed diagnostics are such a pain. Their "might be emitted, might not" semantics messes with lots of things.

rust-lang#120828 and rust-lang#121206 made some big changes to how they work, improving some things, but still leaving some problems, as seen by the issues caused by rust-lang#121206. This PR aims to fix all of them by restricting them in a way that eliminates the "might be emitted, might not" semantics while still allowing 98% of their benefit. Details in the individual commit logs.

r? `@oli-obk`
@nnethercote nnethercote deleted the count-stashed-errs-again branch February 29, 2024 20:54
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 7, 2024
…in, r=estebank

Count stashed errors again

Stashed diagnostics are such a pain. Their "might be emitted, might not" semantics messes with lots of things.

rust-lang#120828 and rust-lang#121206 made some big changes to how they work, improving some things, but still leaving some problems, as seen by the issues caused by rust-lang#121206. This PR aims to fix all of them by restricting them in a way that eliminates the "might be emitted, might not" semantics while still allowing 98% of their benefit. Details in the individual commit logs.

r? `@oli-obk`
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