From 9a7841251115c79fe7e1c12e2e5d637e19ad940a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 14 Dec 2023 14:13:35 +1100 Subject: [PATCH] Split `Handler::emit_diagnostic` in two. Currently, `emit_diagnostic` takes `&mut self`. This commit changes it so `emit_diagnostic` takes `self` and the new `emit_diagnostic_without_consuming` function takes `&mut self`. I find the distinction useful. The former case is much more common, and avoids a bunch of `mut` and `&mut` occurrences. We can also restrict the latter with `pub(crate)` which is nice. --- compiler/rustc_borrowck/src/lib.rs | 4 +- compiler/rustc_codegen_ssa/src/back/write.rs | 2 +- .../src/transform/check_consts/check.rs | 4 +- .../rustc_errors/src/diagnostic_builder.rs | 20 ++++---- compiler/rustc_errors/src/emitter.rs | 2 +- compiler/rustc_errors/src/lib.rs | 51 +++++++++++++------ .../rustc_expand/src/proc_macro_server.rs | 2 +- compiler/rustc_hir_typeck/src/writeback.rs | 4 +- compiler/rustc_parse/src/lib.rs | 8 +-- .../rustc_query_system/src/dep_graph/graph.rs | 4 +- src/tools/miri/src/diagnostics.rs | 2 +- src/tools/rustfmt/src/parse/session.rs | 6 +-- 12 files changed, 64 insertions(+), 45 deletions(-) diff --git a/compiler/rustc_borrowck/src/lib.rs b/compiler/rustc_borrowck/src/lib.rs index 3d3fd412ae0d1..7e0e598cd9f7d 100644 --- a/compiler/rustc_borrowck/src/lib.rs +++ b/compiler/rustc_borrowck/src/lib.rs @@ -2502,8 +2502,8 @@ mod error { if !self.errors.buffered.is_empty() { self.errors.buffered.sort_by_key(|diag| diag.sort_span); - for mut diag in self.errors.buffered.drain(..) { - self.infcx.tcx.sess.diagnostic().emit_diagnostic(&mut diag); + for diag in self.errors.buffered.drain(..) { + self.infcx.tcx.sess.diagnostic().emit_diagnostic(diag); } } diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index 618a72272e58f..40fd8c5c1d692 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -1848,7 +1848,7 @@ impl SharedEmitterMain { d.code(code); } d.replace_args(diag.args); - handler.emit_diagnostic(&mut d); + handler.emit_diagnostic(d); } Ok(SharedEmitterMessage::InlineAsmError(cookie, msg, level, source)) => { let msg = msg.strip_prefix("error: ").unwrap_or(&msg).to_string(); diff --git a/compiler/rustc_const_eval/src/transform/check_consts/check.rs b/compiler/rustc_const_eval/src/transform/check_consts/check.rs index 5380d3071d6c6..bb17602d3ba01 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/check.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/check.rs @@ -277,8 +277,8 @@ impl<'mir, 'tcx> Checker<'mir, 'tcx> { // "secondary" errors if they occurred. let secondary_errors = mem::take(&mut self.secondary_errors); if self.error_emitted.is_none() { - for mut error in secondary_errors { - self.tcx.sess.diagnostic().emit_diagnostic(&mut error); + for error in secondary_errors { + self.tcx.sess.diagnostic().emit_diagnostic(error); } } else { assert!(self.tcx.sess.has_errors().is_some()); diff --git a/compiler/rustc_errors/src/diagnostic_builder.rs b/compiler/rustc_errors/src/diagnostic_builder.rs index b8bd86a72e4df..315e47c097156 100644 --- a/compiler/rustc_errors/src/diagnostic_builder.rs +++ b/compiler/rustc_errors/src/diagnostic_builder.rs @@ -132,7 +132,7 @@ impl EmissionGuarantee for ErrorGuaranteed { DiagnosticBuilderState::Emittable(handler) => { db.inner.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation; - let guar = handler.emit_diagnostic(&mut db.inner.diagnostic); + let guar = handler.emit_diagnostic_without_consuming(&mut db.inner.diagnostic); // Only allow a guarantee if the `level` wasn't switched to a // non-error - the field isn't `pub`, but the whole `Diagnostic` @@ -181,7 +181,7 @@ impl EmissionGuarantee for () { DiagnosticBuilderState::Emittable(handler) => { db.inner.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation; - handler.emit_diagnostic(&mut db.inner.diagnostic); + handler.emit_diagnostic_without_consuming(&mut db.inner.diagnostic); } // `.emit()` was previously called, disallowed from repeating it. DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {} @@ -207,7 +207,7 @@ impl EmissionGuarantee for Noted { // First `.emit()` call, the `&Handler` is still available. DiagnosticBuilderState::Emittable(handler) => { db.inner.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation; - handler.emit_diagnostic(&mut db.inner.diagnostic); + handler.emit_diagnostic_without_consuming(&mut db.inner.diagnostic); } // `.emit()` was previously called, disallowed from repeating it. DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {} @@ -236,7 +236,7 @@ impl EmissionGuarantee for Bug { DiagnosticBuilderState::Emittable(handler) => { db.inner.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation; - handler.emit_diagnostic(&mut db.inner.diagnostic); + handler.emit_diagnostic_without_consuming(&mut db.inner.diagnostic); } // `.emit()` was previously called, disallowed from repeating it. DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {} @@ -260,7 +260,7 @@ impl EmissionGuarantee for ! { DiagnosticBuilderState::Emittable(handler) => { db.inner.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation; - handler.emit_diagnostic(&mut db.inner.diagnostic); + handler.emit_diagnostic_without_consuming(&mut db.inner.diagnostic); } // `.emit()` was previously called, disallowed from repeating it. DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {} @@ -284,7 +284,7 @@ impl EmissionGuarantee for rustc_span::fatal_error::FatalError { DiagnosticBuilderState::Emittable(handler) => { db.inner.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation; - handler.emit_diagnostic(&mut db.inner.diagnostic); + handler.emit_diagnostic_without_consuming(&mut db.inner.diagnostic); } // `.emit()` was previously called, disallowed from repeating it. DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {} @@ -365,7 +365,9 @@ impl<'a, G: EmissionGuarantee> DiagnosticBuilder<'a, G> { } } - /// Emit the diagnostic. + /// Emit the diagnostic. Does not consume `self`, which may be surprising, + /// but there are various places that rely on continuing to use `self` + /// after calling `emit`. #[track_caller] pub fn emit(&mut self) -> G { G::diagnostic_builder_emit_producing_guarantee(self) @@ -640,13 +642,13 @@ impl Drop for DiagnosticBuilderInner<'_> { // No `.emit()` or `.cancel()` calls. DiagnosticBuilderState::Emittable(handler) => { if !panicking() { - handler.emit_diagnostic(&mut Diagnostic::new( + handler.emit_diagnostic(Diagnostic::new( Level::Bug, DiagnosticMessage::from( "the following error was constructed but not emitted", ), )); - handler.emit_diagnostic(&mut self.diagnostic); + handler.emit_diagnostic_without_consuming(&mut self.diagnostic); panic!("error was constructed but not emitted"); } } diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index 3f257fdd9cf27..379883a0c18b7 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -581,7 +581,7 @@ impl Emitter for SilentEmitter { if let Some(ref note) = self.fatal_note { d.note(note.clone()); } - self.fatal_handler.emit_diagnostic(&mut d); + self.fatal_handler.emit_diagnostic(d); } } } diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 508fa7c2e8e3a..cf73c638d85e1 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -1054,7 +1054,7 @@ impl Handler { } let mut diagnostic = Diagnostic::new(Level::DelayedBug, msg); diagnostic.set_span(sp); - inner.emit_diagnostic(&mut diagnostic).unwrap() + inner.emit_diagnostic(diagnostic).unwrap() } // FIXME(eddyb) note the comment inside `impl Drop for HandlerInner`, that's @@ -1064,7 +1064,7 @@ impl Handler { let mut diagnostic = Diagnostic::new(Level::DelayedBug, msg); if inner.flags.report_delayed_bugs { - inner.emit_diagnostic(&mut diagnostic); + inner.emit_diagnostic_without_consuming(&mut diagnostic); } let backtrace = std::backtrace::Backtrace::capture(); inner.good_path_delayed_bugs.push(DelayedDiagnostic::with_backtrace(diagnostic, backtrace)); @@ -1072,7 +1072,9 @@ impl Handler { #[track_caller] pub fn span_bug_no_panic(&self, span: impl Into, msg: impl Into) { - self.emit_diagnostic(Diagnostic::new(Bug, msg).set_span(span)); + let mut diag = Diagnostic::new(Bug, msg); + diag.set_span(span); + self.emit_diagnostic(diag); } #[track_caller] @@ -1185,10 +1187,10 @@ impl Handler { DiagnosticMessage::Str(warnings), )), (_, 0) => { - inner.emit_diagnostic(&mut Diagnostic::new(Fatal, errors)); + inner.emit_diagnostic(Diagnostic::new(Fatal, errors)); } (_, _) => { - inner.emit_diagnostic(&mut Diagnostic::new(Fatal, format!("{errors}; {warnings}"))); + inner.emit_diagnostic(Diagnostic::new(Fatal, format!("{errors}; {warnings}"))); } } @@ -1255,8 +1257,17 @@ impl Handler { self.inner.borrow_mut().emitter.emit_diagnostic(&db); } - pub fn emit_diagnostic(&self, diagnostic: &mut Diagnostic) -> Option { - self.inner.borrow_mut().emit_diagnostic(diagnostic) + pub fn emit_diagnostic(&self, mut diagnostic: Diagnostic) -> Option { + self.emit_diagnostic_without_consuming(&mut diagnostic) + } + + // It's unfortunate this exists. `emit_diagnostic` is preferred, because it + // consumes the diagnostic, thus ensuring it is emitted just once. + pub(crate) fn emit_diagnostic_without_consuming( + &self, + diagnostic: &mut Diagnostic, + ) -> Option { + self.inner.borrow_mut().emit_diagnostic_without_consuming(diagnostic) } pub fn emit_err<'a>(&'a self, err: impl IntoDiagnostic<'a>) -> ErrorGuaranteed { @@ -1370,7 +1381,7 @@ impl Handler { // Here the diagnostic is given back to `emit_diagnostic` where it was first // intercepted. Now it should be processed as usual, since the unstable expectation // id is now stable. - inner.emit_diagnostic(&mut diag); + inner.emit_diagnostic(diag); } } @@ -1412,7 +1423,7 @@ impl HandlerInner { let has_errors = self.has_errors(); let diags = self.stashed_diagnostics.drain(..).map(|x| x.1).collect::>(); let mut reported = None; - for mut diag in diags { + for diag in diags { // Decrement the count tracking the stash; emitting will increment it. if diag.is_error() { if matches!(diag.level, Level::Error { lint: true }) { @@ -1432,14 +1443,20 @@ impl HandlerInner { } } } - let reported_this = self.emit_diagnostic(&mut diag); + let reported_this = self.emit_diagnostic(diag); reported = reported.or(reported_this); } reported } - // FIXME(eddyb) this should ideally take `diagnostic` by value. - fn emit_diagnostic(&mut self, diagnostic: &mut Diagnostic) -> Option { + fn emit_diagnostic(&mut self, mut diagnostic: Diagnostic) -> Option { + self.emit_diagnostic_without_consuming(&mut diagnostic) + } + + fn emit_diagnostic_without_consuming( + &mut self, + diagnostic: &mut Diagnostic, + ) -> Option { if matches!(diagnostic.level, Level::Error { .. } | Level::Fatal) && self.treat_err_as_bug() { diagnostic.level = Level::Bug; @@ -1576,12 +1593,14 @@ impl HandlerInner { #[track_caller] fn span_bug(&mut self, sp: impl Into, msg: impl Into) -> ! { - self.emit_diagnostic(Diagnostic::new(Bug, msg).set_span(sp)); + let mut diag = Diagnostic::new(Bug, msg); + diag.set_span(sp); + self.emit_diagnostic(diag); panic::panic_any(ExplicitBug); } fn failure_note(&mut self, msg: impl Into) { - self.emit_diagnostic(&mut Diagnostic::new(FailureNote, msg)); + self.emit_diagnostic(Diagnostic::new(FailureNote, msg)); } fn flush_delayed( @@ -1613,7 +1632,7 @@ impl HandlerInner { if no_bugs { // Put the overall explanation before the `DelayedBug`s, to // frame them better (e.g. separate warnings from them). - self.emit_diagnostic(&mut Diagnostic::new(Bug, explanation)); + self.emit_diagnostic(Diagnostic::new(Bug, explanation)); no_bugs = false; } @@ -1628,7 +1647,7 @@ impl HandlerInner { } bug.level = Level::Bug; - self.emit_diagnostic(&mut bug); + self.emit_diagnostic(bug); } // Panic with `DelayedBugPanic` to avoid "unexpected panic" messages. diff --git a/compiler/rustc_expand/src/proc_macro_server.rs b/compiler/rustc_expand/src/proc_macro_server.rs index 5308e338d7f8b..d2c26668ea83a 100644 --- a/compiler/rustc_expand/src/proc_macro_server.rs +++ b/compiler/rustc_expand/src/proc_macro_server.rs @@ -502,7 +502,7 @@ impl server::FreeFunctions for Rustc<'_, '_> { None, ); } - self.sess().span_diagnostic.emit_diagnostic(&mut diag); + self.sess().span_diagnostic.emit_diagnostic(diag); } } diff --git a/compiler/rustc_hir_typeck/src/writeback.rs b/compiler/rustc_hir_typeck/src/writeback.rs index 1609c036fbd95..5e562d9453f15 100644 --- a/compiler/rustc_hir_typeck/src/writeback.rs +++ b/compiler/rustc_hir_typeck/src/writeback.rs @@ -504,8 +504,8 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> { if !errors_buffer.is_empty() { errors_buffer.sort_by_key(|diag| diag.span.primary_span()); - for mut diag in errors_buffer { - self.tcx().sess.diagnostic().emit_diagnostic(&mut diag); + for diag in errors_buffer { + self.tcx().sess.diagnostic().emit_diagnostic(diag); } } } diff --git a/compiler/rustc_parse/src/lib.rs b/compiler/rustc_parse/src/lib.rs index 95352dbdc134f..9887a85e6a40c 100644 --- a/compiler/rustc_parse/src/lib.rs +++ b/compiler/rustc_parse/src/lib.rs @@ -51,8 +51,8 @@ macro_rules! panictry_buffer { match $e { Ok(e) => e, Err(errs) => { - for mut e in errs { - $handler.emit_diagnostic(&mut e); + for e in errs { + $handler.emit_diagnostic(e); } FatalError.raise() } @@ -165,8 +165,8 @@ fn try_file_to_source_file( fn file_to_source_file(sess: &ParseSess, path: &Path, spanopt: Option) -> Lrc { match try_file_to_source_file(sess, path, spanopt) { Ok(source_file) => source_file, - Err(mut d) => { - sess.span_diagnostic.emit_diagnostic(&mut d); + Err(d) => { + sess.span_diagnostic.emit_diagnostic(d); FatalError.raise(); } } diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index bc09972185a62..3b8ccb67bbefd 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -926,8 +926,8 @@ impl DepGraphData { let handle = qcx.dep_context().sess().diagnostic(); - for mut diagnostic in side_effects.diagnostics { - handle.emit_diagnostic(&mut diagnostic); + for diagnostic in side_effects.diagnostics { + handle.emit_diagnostic(diagnostic); } } } diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index ef394b163103b..e0f5914497502 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -512,7 +512,7 @@ pub fn report_msg<'tcx>( } } - handler.emit_diagnostic(&mut err); + handler.emit_diagnostic(err); } impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { diff --git a/src/tools/rustfmt/src/parse/session.rs b/src/tools/rustfmt/src/parse/session.rs index 0573df9de2f6d..06f9c4c624336 100644 --- a/src/tools/rustfmt/src/parse/session.rs +++ b/src/tools/rustfmt/src/parse/session.rs @@ -284,10 +284,8 @@ impl ParseSess { // Methods that should be restricted within the parse module. impl ParseSess { pub(super) fn emit_diagnostics(&self, diagnostics: Vec) { - for mut diagnostic in diagnostics { - self.parse_sess - .span_diagnostic - .emit_diagnostic(&mut diagnostic); + for diagnostic in diagnostics { + self.parse_sess.span_diagnostic.emit_diagnostic(diagnostic); } }