diff options
| author | bors <bors@rust-lang.org> | 2022-02-25 00:46:04 +0000 |
|---|---|---|
| committer | bors <bors@rust-lang.org> | 2022-02-25 00:46:04 +0000 |
| commit | d4de1f230ca30b7ce08fbf453daebf8b2e7ffcc9 (patch) | |
| tree | 922a1d8bf2850b2f35dda5bc6cc14e9194c375c1 /compiler/rustc_lint/src | |
| parent | 4e82f35492ea5c78e19609bf4468f0a686d9a756 (diff) | |
| parent | b7e95dee65c35db8f8e07046d445b12d92cbae12 (diff) | |
| download | rust-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.rs | 12 | ||||
| -rw-r--r-- | compiler/rustc_lint/src/context.rs | 22 | ||||
| -rw-r--r-- | compiler/rustc_lint/src/levels.rs | 25 |
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(); }, ); } |
