Skip to content

Commit 0cb66bd

Browse files
rust-timerrust-timer
authored
Unrolled build for rust-lang#121669
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`
2 parents 384d26f + 82961c0 commit 0cb66bd

File tree

40 files changed

+475
-342
lines changed

40 files changed

+475
-342
lines changed

‎compiler/rustc_errors/src/diagnostic.rs

+3-5
Original file line numberDiff line numberDiff line change
@@ -1308,11 +1308,9 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> {
13081308
drop(self);
13091309
}
13101310

1311-
/// Stashes diagnostic for possible later improvement in a different,
1312-
/// later stage of the compiler. The diagnostic can be accessed with
1313-
/// the provided `span` and `key` through [`DiagCtxt::steal_diagnostic()`].
1314-
pub fn stash(mut self, span: Span, key: StashKey) {
1315-
self.dcx.stash_diagnostic(span, key, self.take_diag());
1311+
/// See `DiagCtxt::stash_diagnostic` for details.
1312+
pub fn stash(mut self, span: Span, key: StashKey) -> Option<ErrorGuaranteed> {
1313+
self.dcx.stash_diagnostic(span, key, self.take_diag())
13161314
}
13171315

13181316
/// Delay emission of this diagnostic as a bug.

‎compiler/rustc_errors/src/lib.rs

+154-65
Original file line numberDiff line numberDiff line change
@@ -434,10 +434,6 @@ struct DiagCtxtInner {
434434
/// The delayed bugs and their error guarantees.
435435
delayed_bugs: Vec<(DelayedDiagInner, ErrorGuaranteed)>,
436436

437-
/// The number of stashed errors. Unlike the other counts, this can go up
438-
/// and down, so it doesn't guarantee anything.
439-
stashed_err_count: usize,
440-
441437
/// The error count shown to the user at the end.
442438
deduplicated_err_count: usize,
443439
/// The warning count shown to the user at the end.
@@ -475,7 +471,7 @@ struct DiagCtxtInner {
475471
/// add more information). All stashed diagnostics must be emitted with
476472
/// `emit_stashed_diagnostics` by the time the `DiagCtxtInner` is dropped,
477473
/// otherwise an assertion failure will occur.
478-
stashed_diagnostics: FxIndexMap<(Span, StashKey), DiagInner>,
474+
stashed_diagnostics: FxIndexMap<(Span, StashKey), (DiagInner, Option<ErrorGuaranteed>)>,
479475

480476
future_breakage_diagnostics: Vec<DiagInner>,
481477

@@ -559,10 +555,18 @@ pub struct DiagCtxtFlags {
559555

560556
impl Drop for DiagCtxtInner {
561557
fn drop(&mut self) {
562-
// Any stashed diagnostics should have been handled by
563-
// `emit_stashed_diagnostics` by now.
564-
assert!(self.stashed_diagnostics.is_empty());
558+
// For tools using `interface::run_compiler` (e.g. rustc, rustdoc)
559+
// stashed diagnostics will have already been emitted. But for others
560+
// that don't use `interface::run_compiler` (e.g. rustfmt, some clippy
561+
// lints) this fallback is necessary.
562+
//
563+
// Important: it is sound to produce an `ErrorGuaranteed` when stashing
564+
// errors because they are guaranteed to be emitted here or earlier.
565+
self.emit_stashed_diagnostics();
565566

567+
// Important: it is sound to produce an `ErrorGuaranteed` when emitting
568+
// delayed bugs because they are guaranteed to be emitted here if
569+
// necessary.
566570
if self.err_guars.is_empty() {
567571
self.flush_delayed()
568572
}
@@ -615,7 +619,6 @@ impl DiagCtxt {
615619
err_guars: Vec::new(),
616620
lint_err_guars: Vec::new(),
617621
delayed_bugs: Vec::new(),
618-
stashed_err_count: 0,
619622
deduplicated_err_count: 0,
620623
deduplicated_warn_count: 0,
621624
emitter,
@@ -676,7 +679,6 @@ impl DiagCtxt {
676679
err_guars,
677680
lint_err_guars,
678681
delayed_bugs,
679-
stashed_err_count,
680682
deduplicated_err_count,
681683
deduplicated_warn_count,
682684
emitter: _,
@@ -699,7 +701,6 @@ impl DiagCtxt {
699701
*err_guars = Default::default();
700702
*lint_err_guars = Default::default();
701703
*delayed_bugs = Default::default();
702-
*stashed_err_count = 0;
703704
*deduplicated_err_count = 0;
704705
*deduplicated_warn_count = 0;
705706
*must_produce_diag = false;
@@ -715,39 +716,111 @@ impl DiagCtxt {
715716
*fulfilled_expectations = Default::default();
716717
}
717718

718-
/// Stash a given diagnostic with the given `Span` and [`StashKey`] as the key.
719-
/// Retrieve a stashed diagnostic with `steal_diagnostic`.
720-
pub fn stash_diagnostic(&self, span: Span, key: StashKey, diag: DiagInner) {
721-
let mut inner = self.inner.borrow_mut();
722-
723-
let key = (span.with_parent(None), key);
724-
725-
if diag.is_error() {
726-
if diag.is_lint.is_none() {
727-
inner.stashed_err_count += 1;
728-
}
729-
}
719+
/// Stashes a diagnostic for possible later improvement in a different,
720+
/// later stage of the compiler. Possible actions depend on the diagnostic
721+
/// level:
722+
/// - Level::Error: immediately counted as an error that has occurred, because it
723+
/// is guaranteed to be emitted eventually. Can be later accessed with the
724+
/// provided `span` and `key` through
725+
/// [`DiagCtxt::try_steal_modify_and_emit_err`] or
726+
/// [`DiagCtxt::try_steal_replace_and_emit_err`]. These do not allow
727+
/// cancellation or downgrading of the error. Returns
728+
/// `Some(ErrorGuaranteed)`.
729+
/// - Level::Warning and lower (i.e. !is_error()): can be accessed with the
730+
/// provided `span` and `key` through [`DiagCtxt::steal_non_err()`]. This
731+
/// allows cancelling and downgrading of the diagnostic. Returns `None`.
732+
/// - Others: not allowed, will trigger a panic.
733+
pub fn stash_diagnostic(
734+
&self,
735+
span: Span,
736+
key: StashKey,
737+
diag: DiagInner,
738+
) -> Option<ErrorGuaranteed> {
739+
let guar = if diag.level() == Level::Error {
740+
// This `unchecked_error_guaranteed` is valid. It is where the
741+
// `ErrorGuaranteed` for stashed errors originates. See
742+
// `DiagCtxtInner::drop`.
743+
#[allow(deprecated)]
744+
Some(ErrorGuaranteed::unchecked_error_guaranteed())
745+
} else if !diag.is_error() {
746+
None
747+
} else {
748+
self.span_bug(span, format!("invalid level in `stash_diagnostic`: {}", diag.level));
749+
};
730750

731751
// FIXME(Centril, #69537): Consider reintroducing panic on overwriting a stashed diagnostic
732752
// if/when we have a more robust macro-friendly replacement for `(span, key)` as a key.
733753
// See the PR for a discussion.
734-
inner.stashed_diagnostics.insert(key, diag);
754+
let key = (span.with_parent(None), key);
755+
self.inner.borrow_mut().stashed_diagnostics.insert(key, (diag, guar));
756+
757+
guar
735758
}
736759

737-
/// Steal a previously stashed diagnostic with the given `Span` and [`StashKey`] as the key.
738-
pub fn steal_diagnostic(&self, span: Span, key: StashKey) -> Option<Diag<'_, ()>> {
739-
let mut inner = self.inner.borrow_mut();
760+
/// Steal a previously stashed non-error diagnostic with the given `Span`
761+
/// and [`StashKey`] as the key. Panics if the found diagnostic is an
762+
/// error.
763+
pub fn steal_non_err(&self, span: Span, key: StashKey) -> Option<Diag<'_, ()>> {
740764
let key = (span.with_parent(None), key);
741765
// FIXME(#120456) - is `swap_remove` correct?
742-
let diag = inner.stashed_diagnostics.swap_remove(&key)?;
743-
if diag.is_error() {
744-
if diag.is_lint.is_none() {
745-
inner.stashed_err_count -= 1;
746-
}
747-
}
766+
let (diag, guar) = self.inner.borrow_mut().stashed_diagnostics.swap_remove(&key)?;
767+
assert!(!diag.is_error());
768+
assert!(guar.is_none());
748769
Some(Diag::new_diagnostic(self, diag))
749770
}
750771

772+
/// Steals a previously stashed error with the given `Span` and
773+
/// [`StashKey`] as the key, modifies it, and emits it. Returns `None` if
774+
/// no matching diagnostic is found. Panics if the found diagnostic's level
775+
/// isn't `Level::Error`.
776+
pub fn try_steal_modify_and_emit_err<F>(
777+
&self,
778+
span: Span,
779+
key: StashKey,
780+
mut modify_err: F,
781+
) -> Option<ErrorGuaranteed>
782+
where
783+
F: FnMut(&mut Diag<'_>),
784+
{
785+
let key = (span.with_parent(None), key);
786+
// FIXME(#120456) - is `swap_remove` correct?
787+
let err = self.inner.borrow_mut().stashed_diagnostics.swap_remove(&key);
788+
err.map(|(err, guar)| {
789+
// The use of `::<ErrorGuaranteed>` is safe because level is `Level::Error`.
790+
assert_eq!(err.level, Level::Error);
791+
assert!(guar.is_some());
792+
let mut err = Diag::<ErrorGuaranteed>::new_diagnostic(self, err);
793+
modify_err(&mut err);
794+
assert_eq!(err.level, Level::Error);
795+
err.emit()
796+
})
797+
}
798+
799+
/// Steals a previously stashed error with the given `Span` and
800+
/// [`StashKey`] as the key, cancels it if found, and emits `new_err`.
801+
/// Panics if the found diagnostic's level isn't `Level::Error`.
802+
pub fn try_steal_replace_and_emit_err(
803+
&self,
804+
span: Span,
805+
key: StashKey,
806+
new_err: Diag<'_>,
807+
) -> ErrorGuaranteed {
808+
let key = (span.with_parent(None), key);
809+
// FIXME(#120456) - is `swap_remove` correct?
810+
let old_err = self.inner.borrow_mut().stashed_diagnostics.swap_remove(&key);
811+
match old_err {
812+
Some((old_err, guar)) => {
813+
assert_eq!(old_err.level, Level::Error);
814+
assert!(guar.is_some());
815+
// Because `old_err` has already been counted, it can only be
816+
// safely cancelled because the `new_err` supplants it.
817+
Diag::<ErrorGuaranteed>::new_diagnostic(self, old_err).cancel();
818+
}
819+
None => {}
820+
};
821+
new_err.emit()
822+
}
823+
751824
pub fn has_stashed_diagnostic(&self, span: Span, key: StashKey) -> bool {
752825
self.inner.borrow().stashed_diagnostics.get(&(span.with_parent(None), key)).is_some()
753826
}
@@ -757,41 +830,40 @@ impl DiagCtxt {
757830
self.inner.borrow_mut().emit_stashed_diagnostics()
758831
}
759832

760-
/// This excludes lint errors, delayed bugs and stashed errors.
833+
/// This excludes lint errors, and delayed bugs.
761834
#[inline]
762835
pub fn err_count_excluding_lint_errs(&self) -> usize {
763-
self.inner.borrow().err_guars.len()
836+
let inner = self.inner.borrow();
837+
inner.err_guars.len()
838+
+ inner
839+
.stashed_diagnostics
840+
.values()
841+
.filter(|(diag, guar)| guar.is_some() && diag.is_lint.is_none())
842+
.count()
764843
}
765844

766-
/// This excludes delayed bugs and stashed errors.
845+
/// This excludes delayed bugs.
767846
#[inline]
768847
pub fn err_count(&self) -> usize {
769848
let inner = self.inner.borrow();
770-
inner.err_guars.len() + inner.lint_err_guars.len()
771-
}
772-
773-
/// This excludes normal errors, lint errors, and delayed bugs. Unless
774-
/// absolutely necessary, avoid using this. It's dubious because stashed
775-
/// errors can later be cancelled, so the presence of a stashed error at
776-
/// some point of time doesn't guarantee anything -- there are no
777-
/// `ErrorGuaranteed`s here.
778-
pub fn stashed_err_count(&self) -> usize {
779-
self.inner.borrow().stashed_err_count
849+
inner.err_guars.len()
850+
+ inner.lint_err_guars.len()
851+
+ inner.stashed_diagnostics.values().filter(|(_diag, guar)| guar.is_some()).count()
780852
}
781853

782-
/// This excludes lint errors, delayed bugs, and stashed errors. Unless
783-
/// absolutely necessary, prefer `has_errors` to this method.
854+
/// This excludes lint errors and delayed bugs. Unless absolutely
855+
/// necessary, prefer `has_errors` to this method.
784856
pub fn has_errors_excluding_lint_errors(&self) -> Option<ErrorGuaranteed> {
785857
self.inner.borrow().has_errors_excluding_lint_errors()
786858
}
787859

788-
/// This excludes delayed bugs and stashed errors.
860+
/// This excludes delayed bugs.
789861
pub fn has_errors(&self) -> Option<ErrorGuaranteed> {
790862
self.inner.borrow().has_errors()
791863
}
792864

793-
/// This excludes stashed errors. Unless absolutely necessary, prefer
794-
/// `has_errors` to this method.
865+
/// This excludes nothing. Unless absolutely necessary, prefer `has_errors`
866+
/// to this method.
795867
pub fn has_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {
796868
self.inner.borrow().has_errors_or_delayed_bugs()
797869
}
@@ -876,10 +948,10 @@ impl DiagCtxt {
876948
}
877949
}
878950

879-
/// This excludes delayed bugs and stashed errors. Used for early aborts
880-
/// after errors occurred -- e.g. because continuing in the face of errors is
881-
/// likely to lead to bad results, such as spurious/uninteresting
882-
/// additional errors -- when returning an error `Result` is difficult.
951+
/// This excludes delayed bugs. Used for early aborts after errors occurred
952+
/// -- e.g. because continuing in the face of errors is likely to lead to
953+
/// bad results, such as spurious/uninteresting additional errors -- when
954+
/// returning an error `Result` is difficult.
883955
pub fn abort_if_errors(&self) {
884956
if self.has_errors().is_some() {
885957
FatalError.raise();
@@ -963,7 +1035,7 @@ impl DiagCtxt {
9631035
inner
9641036
.stashed_diagnostics
9651037
.values_mut()
966-
.for_each(|diag| diag.update_unstable_expectation_id(unstable_to_stable));
1038+
.for_each(|(diag, _guar)| diag.update_unstable_expectation_id(unstable_to_stable));
9671039
inner
9681040
.future_breakage_diagnostics
9691041
.iter_mut()
@@ -1270,12 +1342,8 @@ impl DiagCtxtInner {
12701342
fn emit_stashed_diagnostics(&mut self) -> Option<ErrorGuaranteed> {
12711343
let mut guar = None;
12721344
let has_errors = !self.err_guars.is_empty();
1273-
for (_, diag) in std::mem::take(&mut self.stashed_diagnostics).into_iter() {
1274-
if diag.is_error() {
1275-
if diag.is_lint.is_none() {
1276-
self.stashed_err_count -= 1;
1277-
}
1278-
} else {
1345+
for (_, (diag, _guar)) in std::mem::take(&mut self.stashed_diagnostics).into_iter() {
1346+
if !diag.is_error() {
12791347
// Unless they're forced, don't flush stashed warnings when
12801348
// there are errors, to avoid causing warning overload. The
12811349
// stash would've been stolen already if it were important.
@@ -1334,7 +1402,8 @@ impl DiagCtxtInner {
13341402
} else {
13351403
let backtrace = std::backtrace::Backtrace::capture();
13361404
// This `unchecked_error_guaranteed` is valid. It is where the
1337-
// `ErrorGuaranteed` for delayed bugs originates.
1405+
// `ErrorGuaranteed` for delayed bugs originates. See
1406+
// `DiagCtxtInner::drop`.
13381407
#[allow(deprecated)]
13391408
let guar = ErrorGuaranteed::unchecked_error_guaranteed();
13401409
self.delayed_bugs
@@ -1446,11 +1515,31 @@ impl DiagCtxtInner {
14461515
}
14471516

14481517
fn has_errors_excluding_lint_errors(&self) -> Option<ErrorGuaranteed> {
1449-
self.err_guars.get(0).copied()
1518+
self.err_guars.get(0).copied().or_else(|| {
1519+
if let Some((_diag, guar)) = self
1520+
.stashed_diagnostics
1521+
.values()
1522+
.find(|(diag, guar)| guar.is_some() && diag.is_lint.is_none())
1523+
{
1524+
*guar
1525+
} else {
1526+
None
1527+
}
1528+
})
14501529
}
14511530

14521531
fn has_errors(&self) -> Option<ErrorGuaranteed> {
1453-
self.has_errors_excluding_lint_errors().or_else(|| self.lint_err_guars.get(0).copied())
1532+
self.err_guars.get(0).copied().or_else(|| self.lint_err_guars.get(0).copied()).or_else(
1533+
|| {
1534+
if let Some((_diag, guar)) =
1535+
self.stashed_diagnostics.values().find(|(_diag, guar)| guar.is_some())
1536+
{
1537+
*guar
1538+
} else {
1539+
None
1540+
}
1541+
},
1542+
)
14541543
}
14551544

14561545
fn has_errors_or_delayed_bugs(&self) -> Option<ErrorGuaranteed> {

‎compiler/rustc_hir_analysis/src/astconv/lint.rs

+10-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use rustc_ast::TraitObjectSyntax;
2-
use rustc_errors::{codes::*, Diag, EmissionGuarantee, StashKey};
2+
use rustc_errors::{codes::*, Diag, EmissionGuarantee, Level, StashKey};
33
use rustc_hir as hir;
44
use rustc_hir::def::{DefKind, Res};
55
use rustc_lint_defs::{builtin::BARE_TRAIT_OBJECTS, Applicability};
@@ -237,7 +237,15 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
237237
}
238238
// check if the impl trait that we are considering is a impl of a local trait
239239
self.maybe_lint_blanket_trait_impl(self_ty, &mut diag);
240-
diag.stash(self_ty.span, StashKey::TraitMissingMethod);
240+
match diag.level() {
241+
Level::Error => {
242+
diag.stash(self_ty.span, StashKey::TraitMissingMethod);
243+
}
244+
Level::DelayedBug => {
245+
diag.emit();
246+
}
247+
_ => unreachable!(),
248+
}
241249
} else {
242250
let msg = "trait objects without an explicit `dyn` are deprecated";
243251
tcx.node_span_lint(BARE_TRAIT_OBJECTS, self_ty.hir_id, self_ty.span, msg, |lint| {

‎compiler/rustc_hir_analysis/src/check/check.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -1350,8 +1350,7 @@ fn check_type_alias_type_params_are_used<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalD
13501350
let ty = tcx.type_of(def_id).instantiate_identity();
13511351
if ty.references_error() {
13521352
// If there is already another error, do not emit an error for not using a type parameter.
1353-
// Without the `stashed_err_count` part this can fail (#120856).
1354-
assert!(tcx.dcx().has_errors().is_some() || tcx.dcx().stashed_err_count() > 0);
1353+
assert!(tcx.dcx().has_errors().is_some());
13551354
return;
13561355
}
13571356

0 commit comments

Comments
 (0)