From 1bfa1d51fd220ee5281176f565581cd9dfeaa380 Mon Sep 17 00:00:00 2001
From: "Zack M. Davis" <code@zackmdavis.net>
Date: Sat, 15 Oct 2016 10:28:12 -0700
Subject: [PATCH 1/4] introing one-time diagnostics: only emit "lint level
defined here" once
We introduce a new `one_time_diagnostics` field on
`rustc::session::Session` to hold a hashset of diagnostic messages we've
set once but don't want to see again (as uniquified by span and message
text), "lint level defined here" being the motivating example dealt with
here.
This is in the matter of #24690.
---
src/librustc/lint/context.rs | 3 +--
src/librustc/session/mod.rs | 27 ++++++++++++++++++-
src/test/compile-fail/lint-group-style.rs | 2 --
.../lint-unconditional-recursion.rs | 14 +---------
4 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs
index 81d3d440b566f..eebf4d32ab26f 100644
--- a/src/librustc/lint/context.rs
+++ b/src/librustc/lint/context.rs
@@ -452,8 +452,7 @@ pub fn raw_struct_lint<'a>(sess: &'a Session,
}
if let Some(span) = def {
- let explanation = "lint level defined here";
- err.span_note(span, &explanation);
+ sess.diag_span_note_once(&mut err, span, "lint level defined here");
}
err
diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs
index d002aba595bca..7917964c8f1b6 100644
--- a/src/librustc/session/mod.rs
+++ b/src/librustc/session/mod.rs
@@ -17,7 +17,7 @@ use middle::dependency_format;
use session::search_paths::PathKind;
use session::config::DebugInfoLevel;
use ty::tls;
-use util::nodemap::{NodeMap, FnvHashMap};
+use util::nodemap::{NodeMap, FnvHashMap, FnvHashSet};
use util::common::duration_to_secs_str;
use mir::transform as mir_pass;
@@ -75,6 +75,10 @@ pub struct Session {
pub working_dir: PathBuf,
pub lint_store: RefCell<lint::LintStore>,
pub lints: RefCell<NodeMap<Vec<(lint::LintId, Span, String)>>>,
+ /// Set of (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<FnvHashSet<(Span, String)>>,
pub plugin_llvm_passes: RefCell<Vec<String>>,
pub mir_passes: RefCell<mir_pass::Passes>,
pub plugin_attributes: RefCell<Vec<(String, AttributeType)>>,
@@ -288,6 +292,26 @@ impl Session {
pub fn diagnostic<'a>(&'a self) -> &'a errors::Handler {
&self.parse_sess.span_diagnostic
}
+
+ /// Analogous to calling `.span_note` on the given DiagnosticBuilder, but
+ /// deduplicates on span and message for this `Session`.
+ //
+ // FIXME: if the need arises for one-time diagnostics other than
+ // `span_note`, we almost certainly want to generalize this "check 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>,
+ span: Span, message: &str) {
+ let span_message = (span, message.to_owned());
+ let already_noted: bool = self.one_time_diagnostics.borrow()
+ .contains(&span_message);
+ if !already_noted {
+ diag_builder.span_note(span, &message);
+ self.one_time_diagnostics.borrow_mut().insert(span_message);
+ }
+ }
+
pub fn codemap<'a>(&'a self) -> &'a codemap::CodeMap {
self.parse_sess.codemap()
}
@@ -561,6 +585,7 @@ pub fn build_session_(sopts: config::Options,
working_dir: env::current_dir().unwrap(),
lint_store: RefCell::new(lint::LintStore::new()),
lints: RefCell::new(NodeMap()),
+ one_time_diagnostics: RefCell::new(FnvHashSet()),
plugin_llvm_passes: RefCell::new(Vec::new()),
mir_passes: RefCell::new(mir_pass::Passes::new()),
plugin_attributes: RefCell::new(Vec::new()),
diff --git a/src/test/compile-fail/lint-group-style.rs b/src/test/compile-fail/lint-group-style.rs
index 393e46ab5394c..a88e0c63ac374 100644
--- a/src/test/compile-fail/lint-group-style.rs
+++ b/src/test/compile-fail/lint-group-style.rs
@@ -20,7 +20,6 @@ mod test {
#[forbid(bad_style)]
//~^ NOTE lint level defined here
- //~^^ NOTE lint level defined here
mod bad {
fn CamelCase() {} //~ ERROR function `CamelCase` should have a snake case name
@@ -30,7 +29,6 @@ mod test {
mod warn {
#![warn(bad_style)]
//~^ NOTE lint level defined here
- //~| NOTE lint level defined here
fn CamelCase() {} //~ WARN function `CamelCase` should have a snake case name
diff --git a/src/test/compile-fail/lint-unconditional-recursion.rs b/src/test/compile-fail/lint-unconditional-recursion.rs
index 94e189aa47f6f..bee5a2c45be6d 100644
--- a/src/test/compile-fail/lint-unconditional-recursion.rs
+++ b/src/test/compile-fail/lint-unconditional-recursion.rs
@@ -10,19 +10,7 @@
#![deny(unconditional_recursion)]
//~^ NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
-//~| NOTE lint level defined here
+
#![allow(dead_code)]
fn foo() { //~ ERROR function cannot return without recurring
foo(); //~ NOTE recursive call site
From 95805ee6275b4fe12cdb8845e699e7a6aa6b26eb Mon Sep 17 00:00:00 2001
From: "Zack M. Davis" <code@zackmdavis.net>
Date: Wed, 26 Oct 2016 20:16:58 -0700
Subject: [PATCH 2/4] save a borrow by using return value of HashSet::insert
Thanks to Niko Matsakis's review for the suggestion.
---
src/librustc/session/mod.rs | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs
index 7917964c8f1b6..f4029fde471b9 100644
--- a/src/librustc/session/mod.rs
+++ b/src/librustc/session/mod.rs
@@ -297,18 +297,16 @@ impl Session {
/// deduplicates on span and message for this `Session`.
//
// FIXME: if the need arises for one-time diagnostics other than
- // `span_note`, we almost certainly want to generalize this "check the
- // one-time diagnostics map, then set message if it's not already there"
- // code to accomodate all of them
+ // `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>,
span: Span, message: &str) {
let span_message = (span, message.to_owned());
- let already_noted: bool = self.one_time_diagnostics.borrow()
- .contains(&span_message);
- if !already_noted {
+ let fresh = self.one_time_diagnostics.borrow_mut().insert(span_message);
+ if fresh {
diag_builder.span_note(span, &message);
- self.one_time_diagnostics.borrow_mut().insert(span_message);
}
}
From 8d1da8452cc725443c43c44e0f4ec64a73944cdf Mon Sep 17 00:00:00 2001
From: "Zack M. Davis" <code@zackmdavis.net>
Date: Wed, 26 Oct 2016 21:03:18 -0700
Subject: [PATCH 3/4] one-time diagnostics are only one-time for humans, not
JSON-eating tools
Jonathan D. Turner pointed out that we don't want to dedup in JSON
mode. Since the compile-test runner uses JSON output, we regrettably
need to revert the edits to existing tests; one imagines that testing
for one-time diagnosticity for humans will have to be added as a UI
test.
This remains in the matter of #24690.
---
src/librustc/session/mod.rs | 20 ++++++++++++++-----
src/test/compile-fail/lint-group-style.rs | 2 ++
.../lint-unconditional-recursion.rs | 14 ++++++++++++-
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs
index f4029fde471b9..154fe9af79a9a 100644
--- a/src/librustc/session/mod.rs
+++ b/src/librustc/session/mod.rs
@@ -294,7 +294,8 @@ impl Session {
}
/// Analogous to calling `.span_note` on the given DiagnosticBuilder, but
- /// deduplicates on span and message for this `Session`.
+ /// deduplicates on 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
@@ -303,10 +304,19 @@ impl Session {
pub fn diag_span_note_once<'a, 'b>(&'a self,
diag_builder: &'b mut DiagnosticBuilder<'a>,
span: Span, message: &str) {
- let span_message = (span, message.to_owned());
- let fresh = self.one_time_diagnostics.borrow_mut().insert(span_message);
- if fresh {
- diag_builder.span_note(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);
+ },
+ _ => {
+ let span_message = (span, message.to_owned());
+ let fresh = self.one_time_diagnostics.borrow_mut().insert(span_message);
+ if fresh {
+ diag_builder.span_note(span, &message);
+ }
+ }
}
}
diff --git a/src/test/compile-fail/lint-group-style.rs b/src/test/compile-fail/lint-group-style.rs
index a88e0c63ac374..393e46ab5394c 100644
--- a/src/test/compile-fail/lint-group-style.rs
+++ b/src/test/compile-fail/lint-group-style.rs
@@ -20,6 +20,7 @@ mod test {
#[forbid(bad_style)]
//~^ NOTE lint level defined here
+ //~^^ NOTE lint level defined here
mod bad {
fn CamelCase() {} //~ ERROR function `CamelCase` should have a snake case name
@@ -29,6 +30,7 @@ mod test {
mod warn {
#![warn(bad_style)]
//~^ NOTE lint level defined here
+ //~| NOTE lint level defined here
fn CamelCase() {} //~ WARN function `CamelCase` should have a snake case name
diff --git a/src/test/compile-fail/lint-unconditional-recursion.rs b/src/test/compile-fail/lint-unconditional-recursion.rs
index bee5a2c45be6d..94e189aa47f6f 100644
--- a/src/test/compile-fail/lint-unconditional-recursion.rs
+++ b/src/test/compile-fail/lint-unconditional-recursion.rs
@@ -10,7 +10,19 @@
#![deny(unconditional_recursion)]
//~^ NOTE lint level defined here
-
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
+//~| NOTE lint level defined here
#![allow(dead_code)]
fn foo() { //~ ERROR function cannot return without recurring
foo(); //~ NOTE recursive call site
From ef6a07221d66bff1ef8edac4f9ffc39013abf256 Mon Sep 17 00:00:00 2001
From: "Zack M. Davis" <code@zackmdavis.net>
Date: Wed, 26 Oct 2016 23:07:38 -0700
Subject: [PATCH 4/4] deduplicate one-time diagnostics on lint ID as well as
span and message
Some lint-level attributes (like `bad-style`, or, more dramatically,
`warnings`) can affect more than one lint; it seems fairer to point out
the attribute once for each distinct lint affected. Also, a UI test is
added. This remains in the matter of #24690.
---
src/librustc/lint/context.rs | 2 +-
src/librustc/session/mod.rs | 17 +++++++--------
src/test/ui/span/issue-24690.rs | 22 ++++++++++++++++++++
src/test/ui/span/issue-24690.stderr | 32 +++++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 9 deletions(-)
create mode 100644 src/test/ui/span/issue-24690.rs
create mode 100644 src/test/ui/span/issue-24690.stderr
diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs
index eebf4d32ab26f..cf4115e37cd98 100644
--- a/src/librustc/lint/context.rs
+++ b/src/librustc/lint/context.rs
@@ -452,7 +452,7 @@ pub fn raw_struct_lint<'a>(sess: &'a Session,
}
if let Some(span) = def {
- sess.diag_span_note_once(&mut err, span, "lint level defined here");
+ 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 154fe9af79a9a..ce3f2be54b2d5 100644
--- a/src/librustc/session/mod.rs
+++ b/src/librustc/session/mod.rs
@@ -75,10 +75,10 @@ pub struct Session {
pub working_dir: PathBuf,
pub lint_store: RefCell<lint::LintStore>,
pub lints: RefCell<NodeMap<Vec<(lint::LintId, Span, String)>>>,
- /// Set of (span, message) tuples tracking lint (sub)diagnostics that have
- /// been set once, but should not be set again, in order to avoid
+ /// 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<FnvHashSet<(Span, String)>>,
+ pub one_time_diagnostics: RefCell<FnvHashSet<(lint::LintId, Span, String)>>,
pub plugin_llvm_passes: RefCell<Vec<String>>,
pub mir_passes: RefCell<mir_pass::Passes>,
pub plugin_attributes: RefCell<Vec<(String, AttributeType)>>,
@@ -294,8 +294,8 @@ impl Session {
}
/// Analogous to calling `.span_note` on the given DiagnosticBuilder, but
- /// deduplicates on span and message for this `Session` if we're not
- /// outputting in JSON mode.
+ /// 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
@@ -303,7 +303,7 @@ impl Session {
// 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>,
- span: Span, message: &str) {
+ lint: &'static lint::Lint, span: Span, message: &str) {
match self.opts.error_format {
// when outputting JSON for tool consumption, the tool might want
// the duplicates
@@ -311,8 +311,9 @@ impl Session {
diag_builder.span_note(span, &message);
},
_ => {
- let span_message = (span, message.to_owned());
- let fresh = self.one_time_diagnostics.borrow_mut().insert(span_message);
+ 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);
}
diff --git a/src/test/ui/span/issue-24690.rs b/src/test/ui/span/issue-24690.rs
new file mode 100644
index 0000000000000..def0d9aced3c3
--- /dev/null
+++ b/src/test/ui/span/issue-24690.rs
@@ -0,0 +1,22 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+//! A test to ensure that helpful `note` messages aren't emitted more often
+//! than necessary.
+
+// Although there are three errors, we should only get two "lint level defined
+// here" notes pointing at the `warnings` span, one for each error type.
+#![deny(warnings)]
+
+fn main() {
+ let theTwo = 2;
+ let theOtherTwo = 2;
+ println!("{}", theTwo);
+}
diff --git a/src/test/ui/span/issue-24690.stderr b/src/test/ui/span/issue-24690.stderr
new file mode 100644
index 0000000000000..0d2a2ef751666
--- /dev/null
+++ b/src/test/ui/span/issue-24690.stderr
@@ -0,0 +1,32 @@
+error: unused variable: `theOtherTwo`
+ --> $DIR/issue-24690.rs:20:9
+ |
+20 | let theOtherTwo = 2;
+ | ^^^^^^^^^^^
+ |
+note: lint level defined here
+ --> $DIR/issue-24690.rs:16:9
+ |
+16 | #![deny(warnings)]
+ | ^^^^^^^^
+
+error: variable `theTwo` should have a snake case name such as `the_two`
+ --> $DIR/issue-24690.rs:19:9
+ |
+19 | let theTwo = 2;
+ | ^^^^^^
+ |
+note: lint level defined here
+ --> $DIR/issue-24690.rs:16:9
+ |
+16 | #![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;
+ | ^^^^^^^^^^^
+
+error: aborting due to 3 previous errors
+