about summary refs log tree commit diff
path: root/compiler/rustc_lint/src
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 /compiler/rustc_lint/src
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 'compiler/rustc_lint/src')
-rw-r--r--compiler/rustc_lint/src/builtin.rs12
-rw-r--r--compiler/rustc_lint/src/context.rs22
-rw-r--r--compiler/rustc_lint/src/levels.rs25
3 files changed, 32 insertions, 27 deletions
diff --git a/compiler/rustc_lint/src/builtin.rs b/compiler/rustc_lint/src/builtin.rs
index 0a218c2d255..46b90baa585 100644
--- a/compiler/rustc_lint/src/builtin.rs
+++ b/compiler/rustc_lint/src/builtin.rs
@@ -31,7 +31,7 @@ use rustc_ast::{self as ast, *};
 use rustc_ast_pretty::pprust::{self, expr_to_string};
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_data_structures::stack::ensure_sufficient_stack;
-use rustc_errors::{Applicability, DiagnosticBuilder, DiagnosticStyledString};
+use rustc_errors::{Applicability, Diagnostic, DiagnosticStyledString};
 use rustc_feature::{deprecated_attributes, AttributeGate, BuiltinAttribute, GateIssue, Stability};
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
@@ -1476,17 +1476,17 @@ impl TypeAliasBounds {
         }
     }
 
-    fn suggest_changing_assoc_types(ty: &hir::Ty<'_>, err: &mut DiagnosticBuilder<'_>) {
+    fn suggest_changing_assoc_types(ty: &hir::Ty<'_>, err: &mut Diagnostic) {
         // Access to associates types should use `<T as Bound>::Assoc`, which does not need a
         // bound.  Let's see if this type does that.
 
         // We use a HIR visitor to walk the type.
         use rustc_hir::intravisit::{self, Visitor};
-        struct WalkAssocTypes<'a, 'db> {
-            err: &'a mut DiagnosticBuilder<'db>,
+        struct WalkAssocTypes<'a> {
+            err: &'a mut Diagnostic,
         }
-        impl<'a, 'db, 'v> Visitor<'v> for WalkAssocTypes<'a, 'db> {
-            fn visit_qpath(&mut self, qpath: &'v hir::QPath<'v>, id: hir::HirId, span: Span) {
+        impl Visitor<'_> for WalkAssocTypes<'_> {
+            fn visit_qpath(&mut self, qpath: &hir::QPath<'_>, id: hir::HirId, span: Span) {
                 if TypeAliasBounds::is_type_variable_assoc(qpath) {
                     self.err.span_help(
                         span,
diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs
index 0c9defc4cea..ac0f4bd8a4d 100644
--- a/compiler/rustc_lint/src/context.rs
+++ b/compiler/rustc_lint/src/context.rs
@@ -319,7 +319,7 @@ impl LintStore {
     ) {
         let (tool_name, lint_name_only) = parse_lint_and_tool_name(lint_name);
         if lint_name_only == crate::WARNINGS.name_lower() && level == Level::ForceWarn {
-            return struct_span_err!(
+            struct_span_err!(
                 sess,
                 DUMMY_SP,
                 E0602,
@@ -327,6 +327,7 @@ impl LintStore {
                 crate::WARNINGS.name_lower()
             )
             .emit();
+            return;
         }
         let db = match self.check_lint_name(lint_name_only, tool_name, registered_tools) {
             CheckLintNameResult::Ok(_) => None,
@@ -339,7 +340,7 @@ impl LintStore {
                     err.help(&format!("did you mean: `{}`", suggestion));
                 }
 
-                Some(err)
+                Some(err.forget_guarantee())
             }
             CheckLintNameResult::Tool(result) => match result {
                 Err((Some(_), new_name)) => Some(sess.struct_warn(&format!(
@@ -350,13 +351,16 @@ impl LintStore {
                 ))),
                 _ => None,
             },
-            CheckLintNameResult::NoTool => Some(struct_span_err!(
-                sess,
-                DUMMY_SP,
-                E0602,
-                "unknown lint tool: `{}`",
-                tool_name.unwrap()
-            )),
+            CheckLintNameResult::NoTool => Some(
+                struct_span_err!(
+                    sess,
+                    DUMMY_SP,
+                    E0602,
+                    "unknown lint tool: `{}`",
+                    tool_name.unwrap()
+                )
+                .forget_guarantee(),
+            ),
         };
 
         if let Some(mut db) = db {
diff --git a/compiler/rustc_lint/src/levels.rs b/compiler/rustc_lint/src/levels.rs
index 35c7d885e1d..d7cdb08d817 100644
--- a/compiler/rustc_lint/src/levels.rs
+++ b/compiler/rustc_lint/src/levels.rs
@@ -3,7 +3,7 @@ use crate::late::unerased_lint_store;
 use rustc_ast as ast;
 use rustc_ast_pretty::pprust;
 use rustc_data_structures::fx::FxHashMap;
-use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
+use rustc_errors::{struct_span_err, Applicability, Diagnostic};
 use rustc_hir as hir;
 use rustc_hir::{intravisit, HirId};
 use rustc_middle::hir::nested_filter;
@@ -150,29 +150,28 @@ impl<'s> LintLevelsBuilder<'s> {
                     fcw_warning, specs, old_src, id_name
                 );
 
-                let decorate_diag_builder = |mut diag_builder: DiagnosticBuilder<'_>| {
-                    diag_builder.span_label(src.span(), "overruled by previous forbid");
+                let decorate_diag = |diag: &mut Diagnostic| {
+                    diag.span_label(src.span(), "overruled by previous forbid");
                     match old_src {
                         LintLevelSource::Default => {
-                            diag_builder.note(&format!(
+                            diag.note(&format!(
                                 "`forbid` lint level is the default for {}",
                                 id.to_string()
                             ));
                         }
                         LintLevelSource::Node(_, forbid_source_span, reason) => {
-                            diag_builder.span_label(forbid_source_span, "`forbid` level set here");
+                            diag.span_label(forbid_source_span, "`forbid` level set here");
                             if let Some(rationale) = reason {
-                                diag_builder.note(rationale.as_str());
+                                diag.note(rationale.as_str());
                             }
                         }
                         LintLevelSource::CommandLine(_, _) => {
-                            diag_builder.note("`forbid` lint level was set on command line");
+                            diag.note("`forbid` lint level was set on command line");
                         }
                     }
-                    diag_builder.emit();
                 };
                 if !fcw_warning {
-                    let diag_builder = struct_span_err!(
+                    let mut diag_builder = struct_span_err!(
                         self.sess,
                         src.span(),
                         E0453,
@@ -180,18 +179,20 @@ impl<'s> LintLevelsBuilder<'s> {
                         level.as_str(),
                         src.name(),
                     );
-                    decorate_diag_builder(diag_builder);
+                    decorate_diag(&mut diag_builder);
+                    diag_builder.emit();
                 } else {
                     self.struct_lint(
                         FORBIDDEN_LINT_GROUPS,
                         Some(src.span().into()),
                         |diag_builder| {
-                            let diag_builder = diag_builder.build(&format!(
+                            let mut diag_builder = diag_builder.build(&format!(
                                 "{}({}) incompatible with previous forbid",
                                 level.as_str(),
                                 src.name(),
                             ));
-                            decorate_diag_builder(diag_builder);
+                            decorate_diag(&mut diag_builder);
+                            diag_builder.emit();
                         },
                     );
                 }