about summary refs log tree commit diff
path: root/src/tools/rustfmt
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2022-02-25 00:46:04 +0000
committerbors <bors@rust-lang.org>2022-02-25 00:46:04 +0000
commitd4de1f230ca30b7ce08fbf453daebf8b2e7ffcc9 (patch)
tree922a1d8bf2850b2f35dda5bc6cc14e9194c375c1 /src/tools/rustfmt
parent4e82f35492ea5c78e19609bf4468f0a686d9a756 (diff)
parentb7e95dee65c35db8f8e07046d445b12d92cbae12 (diff)
downloadrust-d4de1f230ca30b7ce08fbf453daebf8b2e7ffcc9.tar.gz
rust-d4de1f230ca30b7ce08fbf453daebf8b2e7ffcc9.zip
Auto merge of #93368 - eddyb:diagbld-guarantee, r=estebank
rustc_errors: let `DiagnosticBuilder::emit` return a "guarantee of emission".

That is, `DiagnosticBuilder` is now generic over the return type of `.emit()`, so we'll now have:
* `DiagnosticBuilder<ErrorReported>` for error (incl. fatal/bug) diagnostics
  * can only be created via a `const L: Level`-generic constructor, that limits allowed variants via a `where` clause, so not even `rustc_errors` can accidentally bypass this limitation
  * asserts `diagnostic.is_error()` on emission, just in case the construction restriction was bypassed (e.g. by replacing the whole `Diagnostic` inside `DiagnosticBuilder`)
  * `.emit()` returns `ErrorReported`, as a "proof" token that `.emit()` was called
    (though note that this isn't a real guarantee until after completing the work on
     #69426)
* `DiagnosticBuilder<()>` for everything else (warnings, notes, etc.)
  * can also be obtained from other `DiagnosticBuilder`s by calling `.forget_guarantee()`

This PR is a companion to other ongoing work, namely:
* #69426
  and it's ongoing implementation:
  #93222
  the API changes in this PR are needed to get statically-checked "only errors produce `ErrorReported` from `.emit()`", but doesn't itself provide any really strong guarantees without those other `ErrorReported` changes
* #93244
  would make the choices of API changes (esp. naming) in this PR fit better overall

In order to be able to let `.emit()` return anything trustable, several changes had to be made:
* `Diagnostic`'s `level` field is now private to `rustc_errors`, to disallow arbitrary "downgrade"s from "some kind of error" to "warning" (or anything else that doesn't cause compilation to fail)
  * it's still possible to replace the whole `Diagnostic` inside the `DiagnosticBuilder`, sadly, that's harder to fix, but it's unlikely enough that we can paper over it with asserts on `.emit()`
* `.cancel()` now consumes `DiagnosticBuilder`, preventing `.emit()` calls on a cancelled diagnostic
  * it's also now done internally, through `DiagnosticBuilder`-private state, instead of having a `Level::Cancelled` variant that can be read (or worse, written) by the user
  * this removes a hazard of calling `.cancel()` on an error then continuing to attach details to it, and even expect to be able to `.emit()` it
  * warnings were switched to *only* `can_emit_warnings` on emission (instead of pre-cancelling early)
  * `struct_dummy` was removed (as it relied on a pre-`Cancelled` `Diagnostic`)
* since `.emit()` doesn't consume the `DiagnosticBuilder` <sub>(I tried and gave up, it's much more work than this PR)</sub>,
  we have to make `.emit()` idempotent wrt the guarantees it returns
  * thankfully, `err.emit(); err.emit();` can return `ErrorReported` both times, as the second `.emit()` call has no side-effects *only* because the first one did do the appropriate emission
* `&mut Diagnostic` is now used in a lot of function signatures, which used to take `&mut DiagnosticBuilder` (in the interest of not having to make those functions generic)
  * the APIs were already mostly identical, allowing for low-effort porting to this new setup
  * only some of the suggestion methods needed some rework, to have the extra `DiagnosticBuilder` functionality on the `Diagnostic` methods themselves (that change is also present in #93259)
  * `.emit()`/`.cancel()` aren't available, but IMO calling them from an "error decorator/annotator" function isn't a good practice, and can lead to strange behavior (from the caller's perspective)
  * `.downgrade_to_delayed_bug()` was added, letting you convert any `.is_error()` diagnostic into a `delay_span_bug` one (which works because in both cases the guarantees available are the same)

This PR should ideally be reviewed commit-by-commit, since there is a lot of fallout in each.

r? `@estebank` cc `@Manishearth` `@nikomatsakis` `@mark-i-m`
Diffstat (limited to 'src/tools/rustfmt')
-rw-r--r--src/tools/rustfmt/src/modules.rs2
-rw-r--r--src/tools/rustfmt/src/parse/macros/cfg_if.rs2
-rw-r--r--src/tools/rustfmt/src/parse/macros/lazy_static.rs2
-rw-r--r--src/tools/rustfmt/src/parse/macros/mod.rs2
-rw-r--r--src/tools/rustfmt/src/parse/parser.rs2
-rw-r--r--src/tools/rustfmt/src/parse/session.rs29
6 files changed, 12 insertions, 27 deletions
diff --git a/src/tools/rustfmt/src/modules.rs b/src/tools/rustfmt/src/modules.rs
index 9c964b274e0..d4bddd95785 100644
--- a/src/tools/rustfmt/src/modules.rs
+++ b/src/tools/rustfmt/src/modules.rs
@@ -439,7 +439,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                 }
             }
             Err(mod_err) if !mods_outside_ast.is_empty() => {
-                if let ModError::ParserError(mut e) = mod_err {
+                if let ModError::ParserError(e) = mod_err {
                     e.cancel();
                 }
                 Ok(Some(SubModKind::MultiExternal(mods_outside_ast)))
diff --git a/src/tools/rustfmt/src/parse/macros/cfg_if.rs b/src/tools/rustfmt/src/parse/macros/cfg_if.rs
index e10fbe64bcd..306b6bb745e 100644
--- a/src/tools/rustfmt/src/parse/macros/cfg_if.rs
+++ b/src/tools/rustfmt/src/parse/macros/cfg_if.rs
@@ -57,7 +57,7 @@ fn parse_cfg_if_inner<'a>(
             let item = match parser.parse_item(ForceCollect::No) {
                 Ok(Some(item_ptr)) => item_ptr.into_inner(),
                 Ok(None) => continue,
-                Err(mut err) => {
+                Err(err) => {
                     err.cancel();
                     parser.sess.span_diagnostic.reset_err_count();
                     return Err(
diff --git a/src/tools/rustfmt/src/parse/macros/lazy_static.rs b/src/tools/rustfmt/src/parse/macros/lazy_static.rs
index 9c8651aa3fa..4c541de04be 100644
--- a/src/tools/rustfmt/src/parse/macros/lazy_static.rs
+++ b/src/tools/rustfmt/src/parse/macros/lazy_static.rs
@@ -23,7 +23,7 @@ pub(crate) fn parse_lazy_static(
                         val
                     }
                 }
-                Err(mut err) => {
+                Err(err) => {
                     err.cancel();
                     parser.sess.span_diagnostic.reset_err_count();
                     return None;
diff --git a/src/tools/rustfmt/src/parse/macros/mod.rs b/src/tools/rustfmt/src/parse/macros/mod.rs
index 2e9ce1d35f4..fd738908170 100644
--- a/src/tools/rustfmt/src/parse/macros/mod.rs
+++ b/src/tools/rustfmt/src/parse/macros/mod.rs
@@ -36,7 +36,7 @@ fn parse_macro_arg<'a, 'b: 'a>(parser: &'a mut Parser<'b>) -> Option<MacroArg> {
                         return Some(MacroArg::$macro_arg($f(x)?));
                     }
                 }
-                Err(mut e) => {
+                Err(e) => {
                     e.cancel();
                     parser.sess.span_diagnostic.reset_err_count();
                 }
diff --git a/src/tools/rustfmt/src/parse/parser.rs b/src/tools/rustfmt/src/parse/parser.rs
index 657217633f4..f0944a88d2f 100644
--- a/src/tools/rustfmt/src/parse/parser.rs
+++ b/src/tools/rustfmt/src/parse/parser.rs
@@ -115,7 +115,7 @@ impl<'a> Parser<'a> {
             match parser.parse_mod(&TokenKind::Eof) {
                 Ok(result) => Some(result),
                 Err(mut e) => {
-                    sess.emit_or_cancel_diagnostic(&mut e);
+                    e.emit();
                     if sess.can_reset_errors() {
                         sess.reset_errors();
                     }
diff --git a/src/tools/rustfmt/src/parse/session.rs b/src/tools/rustfmt/src/parse/session.rs
index 7fc3778376c..40a6d708d8c 100644
--- a/src/tools/rustfmt/src/parse/session.rs
+++ b/src/tools/rustfmt/src/parse/session.rs
@@ -60,7 +60,7 @@ impl Emitter for SilentOnIgnoredFilesEmitter {
         None
     }
     fn emit_diagnostic(&mut self, db: &Diagnostic) {
-        if db.level == DiagnosticLevel::Fatal {
+        if db.level() == DiagnosticLevel::Fatal {
             return self.handle_non_ignoreable_error(db);
         }
         if let Some(primary_span) = &db.span.primary_span() {
@@ -230,17 +230,6 @@ impl ParseSess {
         }
     }
 
-    pub(crate) fn emit_or_cancel_diagnostic(&self, diagnostic: &mut Diagnostic) {
-        self.parse_sess.span_diagnostic.emit_diagnostic(diagnostic);
-        // The Handler will check whether the diagnostic should be emitted
-        // based on the user's rustfmt configuration and the originating file
-        // that caused the parser error. If the Handler determined it should skip
-        // emission then we need to ensure the diagnostic is cancelled.
-        if !diagnostic.cancelled() {
-            diagnostic.cancel();
-        }
-    }
-
     pub(super) fn can_reset_errors(&self) -> bool {
         self.can_reset_errors.load(Ordering::Acquire)
     }
@@ -292,7 +281,7 @@ mod tests {
         use super::*;
         use crate::config::IgnoreList;
         use crate::utils::mk_sp;
-        use rustc_span::{FileName as SourceMapFileName, MultiSpan, RealFileName, DUMMY_SP};
+        use rustc_span::{FileName as SourceMapFileName, MultiSpan, RealFileName};
         use std::path::PathBuf;
         use std::sync::atomic::AtomicU32;
 
@@ -310,16 +299,12 @@ mod tests {
         }
 
         fn build_diagnostic(level: DiagnosticLevel, span: Option<MultiSpan>) -> Diagnostic {
-            Diagnostic {
-                level,
-                code: None,
-                message: vec![],
-                children: vec![],
-                suggestions: Ok(vec![]),
-                span: span.unwrap_or_else(MultiSpan::new),
-                sort_span: DUMMY_SP,
-                is_lint: false,
+            let mut diag = Diagnostic::new(level, "");
+            diag.message.clear();
+            if let Some(span) = span {
+                diag.span = span;
             }
+            diag
         }
 
         fn build_emitter(