From 32b8579b6826091e11ea6d90a2d64f4975894032 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Mon, 26 Jun 2017 13:30:21 -0700 Subject: [PATCH] make lint on-by-default/implied-by messages appear only once From review discussion on #38103 (https://github.com/rust-lang/rust/pull/38103#discussion_r94845060). --- src/librustc/lint/context.rs | 26 +++++----- src/librustc/session/mod.rs | 60 +++++++++++++++++------- src/test/ui/lint/lint-group-style.stderr | 10 ++-- src/test/ui/path-lookahead.stderr | 2 - src/test/ui/span/issue-24690.stderr | 6 +-- 5 files changed, 63 insertions(+), 41 deletions(-) diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index a9e0ef51102..1a0ab8c413d 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -513,7 +513,6 @@ pub fn raw_struct_lint<'a, S>(sess: &'a Session, } let name = lint.name_lower(); - let mut def = None; // Except for possible note details, forbid behaves like deny. let effective_level = if level == Forbid { Deny } else { level }; @@ -528,7 +527,8 @@ pub fn raw_struct_lint<'a, S>(sess: &'a Session, match source { Default => { - err.note(&format!("#[{}({})] on by default", level.as_str(), name)); + sess.diag_note_once(&mut err, lint, + &format!("#[{}({})] on by default", level.as_str(), name)); }, CommandLine(lint_flag_val) => { let flag = match level { @@ -537,20 +537,24 @@ pub fn raw_struct_lint<'a, S>(sess: &'a Session, }; let hyphen_case_lint_name = name.replace("_", "-"); if lint_flag_val.as_str() == name { - err.note(&format!("requested on the command line with `{} {}`", - flag, hyphen_case_lint_name)); + sess.diag_note_once(&mut err, lint, + &format!("requested on the command line with `{} {}`", + flag, hyphen_case_lint_name)); } else { let hyphen_case_flag_val = lint_flag_val.as_str().replace("_", "-"); - err.note(&format!("`{} {}` implied by `{} {}`", - flag, hyphen_case_lint_name, flag, hyphen_case_flag_val)); + sess.diag_note_once(&mut err, lint, + &format!("`{} {}` implied by `{} {}`", + flag, hyphen_case_lint_name, flag, + hyphen_case_flag_val)); } }, Node(lint_attr_name, src) => { - def = Some(src); + sess.diag_span_note_once(&mut err, lint, src, "lint level defined here"); if lint_attr_name.as_str() != name { let level_str = level.as_str(); - err.note(&format!("#[{}({})] implied by #[{}({})]", - level_str, name, level_str, lint_attr_name)); + sess.diag_note_once(&mut err, lint, + &format!("#[{}({})] implied by #[{}({})]", + level_str, name, level_str, lint_attr_name)); } } } @@ -566,10 +570,6 @@ pub fn raw_struct_lint<'a, S>(sess: &'a Session, err.note(&citation); } - if let Some(span) = def { - sess.diag_span_note_once(&mut err, lint, span, "lint level defined here"); - } - err } diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs index 70c07982f83..fb513f573d7 100644 --- a/src/librustc/session/mod.rs +++ b/src/librustc/session/mod.rs @@ -79,10 +79,10 @@ pub struct Session { pub working_dir: (String, bool), pub lint_store: RefCell, pub lints: RefCell, - /// Set of (LintId, span, message) tuples tracking lint (sub)diagnostics - /// that have been set once, but should not be set again, in order to avoid - /// redundantly verbose output (Issue #24690). - pub one_time_diagnostics: RefCell>, + /// Set of (LintId, Option, message) tuples tracking lint + /// (sub)diagnostics that have been set once, but should not be set again, + /// in order to avoid redundantly verbose output (Issue #24690). + pub one_time_diagnostics: RefCell, String)>>, pub plugin_llvm_passes: RefCell>, pub plugin_attributes: RefCell>, pub crate_types: RefCell>, @@ -157,6 +157,13 @@ pub struct PerfStats { pub decode_def_path_tables_time: Cell, } +/// Enum to support dispatch of one-time diagnostics (in Session.diag_once) +enum DiagnosticBuilderMethod { + Note, + SpanNote, + // add more variants as needed to support one-time diagnostics +} + impl Session { pub fn local_crate_disambiguator(&self) -> Symbol { *self.crate_disambiguator.borrow() @@ -329,34 +336,53 @@ impl Session { &self.parse_sess.span_diagnostic } - /// Analogous to calling `.span_note` on the given DiagnosticBuilder, but - /// deduplicates on lint ID, span, and message for this `Session` if we're - /// not outputting in JSON mode. - // - // FIXME: if the need arises for one-time diagnostics other than - // `span_note`, we almost certainly want to generalize this - // "check/insert-into the one-time diagnostics map, then set message if - // it's not already there" code to accomodate all of them - pub fn diag_span_note_once<'a, 'b>(&'a self, - diag_builder: &'b mut DiagnosticBuilder<'a>, - lint: &'static lint::Lint, span: Span, message: &str) { + /// Analogous to calling methods on the given `DiagnosticBuilder`, but + /// deduplicates on lint ID, span (if any), and message for this `Session` + /// if we're not outputting in JSON mode. + fn diag_once<'a, 'b>(&'a self, + diag_builder: &'b mut DiagnosticBuilder<'a>, + method: DiagnosticBuilderMethod, + lint: &'static lint::Lint, message: &str, span: Option) { + let mut do_method = || { + match method { + DiagnosticBuilderMethod::Note => { + diag_builder.note(message); + }, + DiagnosticBuilderMethod::SpanNote => { + diag_builder.span_note(span.expect("span_note expects a span"), message); + } + } + }; + match self.opts.error_format { // when outputting JSON for tool consumption, the tool might want // the duplicates config::ErrorOutputType::Json => { - diag_builder.span_note(span, &message); + do_method() }, _ => { let lint_id = lint::LintId::of(lint); let id_span_message = (lint_id, span, message.to_owned()); let fresh = self.one_time_diagnostics.borrow_mut().insert(id_span_message); if fresh { - diag_builder.span_note(span, &message); + do_method() } } } } + pub fn diag_span_note_once<'a, 'b>(&'a self, + diag_builder: &'b mut DiagnosticBuilder<'a>, + lint: &'static lint::Lint, span: Span, message: &str) { + self.diag_once(diag_builder, DiagnosticBuilderMethod::SpanNote, lint, message, Some(span)); + } + + pub fn diag_note_once<'a, 'b>(&'a self, + diag_builder: &'b mut DiagnosticBuilder<'a>, + lint: &'static lint::Lint, message: &str) { + self.diag_once(diag_builder, DiagnosticBuilderMethod::Note, lint, message, None); + } + pub fn codemap<'a>(&'a self) -> &'a codemap::CodeMap { self.parse_sess.codemap() } diff --git a/src/test/ui/lint/lint-group-style.stderr b/src/test/ui/lint/lint-group-style.stderr index dec44c317e4..636370de302 100644 --- a/src/test/ui/lint/lint-group-style.stderr +++ b/src/test/ui/lint/lint-group-style.stderr @@ -4,12 +4,12 @@ error: function `CamelCase` should have a snake case name such as `camel_case` 14 | fn CamelCase() {} | ^^^^^^^^^^^^^^^^^ | - = note: #[deny(non_snake_case)] implied by #[deny(bad_style)] note: lint level defined here --> $DIR/lint-group-style.rs:11:9 | 11 | #![deny(bad_style)] | ^^^^^^^^^ + = note: #[deny(non_snake_case)] implied by #[deny(bad_style)] error: function `CamelCase` should have a snake case name such as `camel_case` --> $DIR/lint-group-style.rs:22:9 @@ -17,12 +17,12 @@ error: function `CamelCase` should have a snake case name such as `camel_case` 22 | fn CamelCase() {} | ^^^^^^^^^^^^^^^^^ | - = note: #[forbid(non_snake_case)] implied by #[forbid(bad_style)] note: lint level defined here --> $DIR/lint-group-style.rs:20:14 | 20 | #[forbid(bad_style)] | ^^^^^^^^^ + = note: #[forbid(non_snake_case)] implied by #[forbid(bad_style)] error: static variable `bad` should have an upper case name such as `BAD` --> $DIR/lint-group-style.rs:24:9 @@ -30,12 +30,12 @@ error: static variable `bad` should have an upper case name such as `BAD` 24 | static bad: isize = 1; | ^^^^^^^^^^^^^^^^^^^^^^ | - = note: #[forbid(non_upper_case_globals)] implied by #[forbid(bad_style)] note: lint level defined here --> $DIR/lint-group-style.rs:20:14 | 20 | #[forbid(bad_style)] | ^^^^^^^^^ + = note: #[forbid(non_upper_case_globals)] implied by #[forbid(bad_style)] warning: function `CamelCase` should have a snake case name such as `camel_case` --> $DIR/lint-group-style.rs:30:9 @@ -43,12 +43,12 @@ warning: function `CamelCase` should have a snake case name such as `camel_case` 30 | fn CamelCase() {} | ^^^^^^^^^^^^^^^^^ | - = note: #[warn(non_snake_case)] implied by #[warn(bad_style)] note: lint level defined here --> $DIR/lint-group-style.rs:28:17 | 28 | #![warn(bad_style)] | ^^^^^^^^^ + = note: #[warn(non_snake_case)] implied by #[warn(bad_style)] warning: type `snake_case` should have a camel case name such as `SnakeCase` --> $DIR/lint-group-style.rs:32:9 @@ -56,12 +56,12 @@ warning: type `snake_case` should have a camel case name such as `SnakeCase` 32 | struct snake_case; | ^^^^^^^^^^^^^^^^^^ | - = note: #[warn(non_camel_case_types)] implied by #[warn(bad_style)] note: lint level defined here --> $DIR/lint-group-style.rs:28:17 | 28 | #![warn(bad_style)] | ^^^^^^^^^ + = note: #[warn(non_camel_case_types)] implied by #[warn(bad_style)] error: aborting due to previous error(s) diff --git a/src/test/ui/path-lookahead.stderr b/src/test/ui/path-lookahead.stderr index 1e19977e84a..8fd1b8de687 100644 --- a/src/test/ui/path-lookahead.stderr +++ b/src/test/ui/path-lookahead.stderr @@ -23,6 +23,4 @@ warning: function is never used: `no_parens` 20 | | return ::to_string(&arg); 21 | | } | |_^ - | - = note: #[warn(dead_code)] on by default diff --git a/src/test/ui/span/issue-24690.stderr b/src/test/ui/span/issue-24690.stderr index 598f9f51307..edc150f65ea 100644 --- a/src/test/ui/span/issue-24690.stderr +++ b/src/test/ui/span/issue-24690.stderr @@ -4,20 +4,18 @@ error: variable `theTwo` should have a snake case name such as `the_two` 19 | let theTwo = 2; | ^^^^^^ | - = note: #[deny(non_snake_case)] implied by #[deny(warnings)] note: lint level defined here --> $DIR/issue-24690.rs:16:9 | 16 | #![deny(warnings)] | ^^^^^^^^ + = note: #[deny(non_snake_case)] implied by #[deny(warnings)] error: variable `theOtherTwo` should have a snake case name such as `the_other_two` --> $DIR/issue-24690.rs:20:9 | 20 | let theOtherTwo = 2; | ^^^^^^^^^^^ - | - = note: #[deny(non_snake_case)] implied by #[deny(warnings)] error: unused variable: `theOtherTwo` --> $DIR/issue-24690.rs:20:9 @@ -25,12 +23,12 @@ error: unused variable: `theOtherTwo` 20 | let theOtherTwo = 2; | ^^^^^^^^^^^ | - = note: #[deny(unused_variables)] implied by #[deny(warnings)] note: lint level defined here --> $DIR/issue-24690.rs:16:9 | 16 | #![deny(warnings)] | ^^^^^^^^ + = note: #[deny(unused_variables)] implied by #[deny(warnings)] error: aborting due to previous error(s)