From b4e77d21bcf8b15ef7d873005382ba8ca309faf5 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 15 Jun 2020 11:50:58 -0400 Subject: [PATCH 1/2] rewrite old test so that its attributes are consistent with what we want in the language. (Note that the fact this test existed is a slight sign that we may need a crater run on this bugfix...) --- .../ui/rfc-2565-param-attrs/param-attrs-allowed.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs b/src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs index 1217f89cb3168..a547d09d04822 100644 --- a/src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs +++ b/src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs @@ -8,8 +8,8 @@ extern "C" { #[allow(unused_mut)] a: i32, #[cfg(something)] b: i32, #[cfg_attr(something, cfg(nothing))] c: i32, - #[deny(unused_mut)] d: i32, - #[forbid(unused_mut)] #[warn(unused_mut)] ... + #[forbid(unused_mut)] d: i32, + #[deny(unused_mut)] #[warn(unused_mut)] ... ); } @@ -17,16 +17,16 @@ type FnType = fn( #[allow(unused_mut)] a: i32, #[cfg(something)] b: i32, #[cfg_attr(something, cfg(nothing))] c: i32, - #[deny(unused_mut)] d: i32, - #[forbid(unused_mut)] #[warn(unused_mut)] e: i32 + #[forbid(unused_mut)] d: i32, + #[deny(unused_mut)] #[warn(unused_mut)] e: i32 ); pub fn foo( #[allow(unused_mut)] a: i32, #[cfg(something)] b: i32, #[cfg_attr(something, cfg(nothing))] c: i32, - #[deny(unused_mut)] d: i32, - #[forbid(unused_mut)] #[warn(unused_mut)] _e: i32 + #[forbid(unused_mut)] d: i32, + #[deny(unused_mut)] #[warn(unused_mut)] _e: i32 ) {} // self From afa2a675453091773eb9dd1b19389725526224b9 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 15 Jun 2020 14:17:35 -0400 Subject: [PATCH 2/2] Prevent forbid from being ignored if overriden at the same level. That is, this changes `#[forbid(foo)] #[allow(foo)]` from allowing foo to forbidding foo. --- compiler/rustc_lint/src/levels.rs | 47 ++++++++++++++++-- compiler/rustc_middle/src/lint.rs | 20 +++++++- ...0819-dont-override-forbid-in-same-scope.rs | 49 +++++++++++++++++++ ...-dont-override-forbid-in-same-scope.stderr | 29 +++++++++++ src/tools/clippy/tests/ui/attrs.rs | 1 - src/tools/clippy/tests/ui/attrs.stderr | 16 ++---- 6 files changed, 144 insertions(+), 18 deletions(-) create mode 100644 src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.rs create mode 100644 src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.stderr diff --git a/compiler/rustc_lint/src/levels.rs b/compiler/rustc_lint/src/levels.rs index 48254dcee82fe..222333a578b7d 100644 --- a/compiler/rustc_lint/src/levels.rs +++ b/compiler/rustc_lint/src/levels.rs @@ -10,6 +10,7 @@ use rustc_hir as hir; use rustc_hir::def_id::{CrateNum, LOCAL_CRATE}; use rustc_hir::{intravisit, HirId}; use rustc_middle::hir::map::Map; +use rustc_middle::lint::LevelSource; use rustc_middle::lint::LintDiagnosticBuilder; use rustc_middle::lint::{struct_lint_level, LintLevelMap, LintLevelSets, LintSet, LintSource}; use rustc_middle::ty::query::Providers; @@ -95,6 +96,44 @@ impl<'s> LintLevelsBuilder<'s> { self.sets.list.push(LintSet::CommandLine { specs }); } + /// Attempts to insert the `id` to `level_src` map entry. If unsuccessful + /// (e.g. if a forbid was already inserted on the same scope), then emits a + /// diagnostic with no change to `specs`. + fn insert_spec( + &mut self, + specs: &mut FxHashMap, + id: LintId, + (level, src): LevelSource, + ) { + if let Some((old_level, old_src)) = specs.get(&id) { + if old_level == &Level::Forbid && level != Level::Forbid { + let mut diag_builder = struct_span_err!( + self.sess, + src.span(), + E0453, + "{}({}) incompatible with previous forbid in same scope", + level.as_str(), + src.name(), + ); + match *old_src { + LintSource::Default => {} + LintSource::Node(_, forbid_source_span, reason) => { + diag_builder.span_label(forbid_source_span, "`forbid` level set here"); + if let Some(rationale) = reason { + diag_builder.note(&rationale.as_str()); + } + } + LintSource::CommandLine(_) => { + diag_builder.note("`forbid` lint level was set on command line"); + } + } + diag_builder.emit(); + return; + } + } + specs.insert(id, (level, src)); + } + /// Pushes a list of AST lint attributes onto this context. /// /// This function will return a `BuilderPush` object which should be passed @@ -109,7 +148,7 @@ impl<'s> LintLevelsBuilder<'s> { /// `#[allow]` /// /// Don't forget to call `pop`! - pub fn push( + pub(crate) fn push( &mut self, attrs: &[ast::Attribute], store: &LintStore, @@ -221,7 +260,7 @@ impl<'s> LintLevelsBuilder<'s> { let src = LintSource::Node(name, li.span(), reason); for &id in ids { self.check_gated_lint(id, attr.span); - specs.insert(id, (level, src)); + self.insert_spec(&mut specs, id, (level, src)); } } @@ -235,7 +274,7 @@ impl<'s> LintLevelsBuilder<'s> { reason, ); for id in ids { - specs.insert(*id, (level, src)); + self.insert_spec(&mut specs, *id, (level, src)); } } Err((Some(ids), new_lint_name)) => { @@ -272,7 +311,7 @@ impl<'s> LintLevelsBuilder<'s> { reason, ); for id in ids { - specs.insert(*id, (level, src)); + self.insert_spec(&mut specs, *id, (level, src)); } } Err((None, _)) => { diff --git a/compiler/rustc_middle/src/lint.rs b/compiler/rustc_middle/src/lint.rs index 25e5379881e70..91e1d6e0b0b72 100644 --- a/compiler/rustc_middle/src/lint.rs +++ b/compiler/rustc_middle/src/lint.rs @@ -9,7 +9,7 @@ use rustc_session::lint::{builtin, Level, Lint, LintId}; use rustc_session::{DiagnosticMessageId, Session}; use rustc_span::hygiene::MacroKind; use rustc_span::source_map::{DesugaringKind, ExpnKind, MultiSpan}; -use rustc_span::{Span, Symbol}; +use rustc_span::{symbol, Span, Symbol, DUMMY_SP}; /// How a lint level was set. #[derive(Clone, Copy, PartialEq, Eq, HashStable)] @@ -25,6 +25,24 @@ pub enum LintSource { CommandLine(Symbol), } +impl LintSource { + pub fn name(&self) -> Symbol { + match *self { + LintSource::Default => symbol::kw::Default, + LintSource::Node(name, _, _) => name, + LintSource::CommandLine(name) => name, + } + } + + pub fn span(&self) -> Span { + match *self { + LintSource::Default => DUMMY_SP, + LintSource::Node(_, span, _) => span, + LintSource::CommandLine(_) => DUMMY_SP, + } + } +} + pub type LevelSource = (Level, LintSource); pub struct LintLevelSets { diff --git a/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.rs b/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.rs new file mode 100644 index 0000000000000..8e25227b59e63 --- /dev/null +++ b/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.rs @@ -0,0 +1,49 @@ +// This test is checking that you cannot override a `forbid` by adding in other +// attributes later in the same scope. (We already ensure that you cannot +// override it in nested scopes). + +// If you turn off deduplicate diagnostics (which rustc turns on by default but +// compiletest turns off when it runs ui tests), then the errors are +// (unfortunately) repeated here because the checking is done as we read in the +// errors, and curretly that happens two or three different times, depending on +// compiler flags. +// +// I decided avoiding the redundant output was not worth the time in engineering +// effort for bug like this, which 1. end users are unlikely to run into in the +// first place, and 2. they won't see the redundant output anyway. + +// compile-flags: -Z deduplicate-diagnostics=yes + +fn forbid_first(num: i32) -> i32 { + #![forbid(unused)] + #![deny(unused)] + //~^ ERROR: deny(unused) incompatible with previous forbid in same scope [E0453] + #![warn(unused)] + //~^ ERROR: warn(unused) incompatible with previous forbid in same scope [E0453] + #![allow(unused)] + //~^ ERROR: allow(unused) incompatible with previous forbid in same scope [E0453] + + num * num +} + +fn forbid_last(num: i32) -> i32 { + #![deny(unused)] + #![warn(unused)] + #![allow(unused)] + #![forbid(unused)] + + num * num +} + +fn forbid_multiple(num: i32) -> i32 { + #![forbid(unused)] + #![forbid(unused)] + + num * num +} + +fn main() { + forbid_first(10); + forbid_last(10); + forbid_multiple(10); +} diff --git a/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.stderr b/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.stderr new file mode 100644 index 0000000000000..3951c511bf432 --- /dev/null +++ b/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.stderr @@ -0,0 +1,29 @@ +error[E0453]: deny(unused) incompatible with previous forbid in same scope + --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:19:13 + | +LL | #![forbid(unused)] + | ------ `forbid` level set here +LL | #![deny(unused)] + | ^^^^^^ + +error[E0453]: warn(unused) incompatible with previous forbid in same scope + --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:21:13 + | +LL | #![forbid(unused)] + | ------ `forbid` level set here +... +LL | #![warn(unused)] + | ^^^^^^ + +error[E0453]: allow(unused) incompatible with previous forbid in same scope + --> $DIR/issue-70819-dont-override-forbid-in-same-scope.rs:23:14 + | +LL | #![forbid(unused)] + | ------ `forbid` level set here +... +LL | #![allow(unused)] + | ^^^^^^ + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0453`. diff --git a/src/tools/clippy/tests/ui/attrs.rs b/src/tools/clippy/tests/ui/attrs.rs index 908d063729f45..32685038067d6 100644 --- a/src/tools/clippy/tests/ui/attrs.rs +++ b/src/tools/clippy/tests/ui/attrs.rs @@ -3,7 +3,6 @@ // Test that the whole restriction group is not enabled #![warn(clippy::restriction)] #![deny(clippy::restriction)] -#![forbid(clippy::restriction)] #![allow(clippy::missing_docs_in_private_items, clippy::panic, clippy::unreachable)] #[inline(always)] diff --git a/src/tools/clippy/tests/ui/attrs.stderr b/src/tools/clippy/tests/ui/attrs.stderr index ef4b89eaa6dee..4324984dd60eb 100644 --- a/src/tools/clippy/tests/ui/attrs.stderr +++ b/src/tools/clippy/tests/ui/attrs.stderr @@ -1,5 +1,5 @@ error: you have declared `#[inline(always)]` on `test_attr_lint`. This is usually a bad idea - --> $DIR/attrs.rs:9:1 + --> $DIR/attrs.rs:8:1 | LL | #[inline(always)] | ^^^^^^^^^^^^^^^^^ @@ -7,7 +7,7 @@ LL | #[inline(always)] = note: `-D clippy::inline-always` implied by `-D warnings` error: the since field must contain a semver-compliant version - --> $DIR/attrs.rs:29:14 + --> $DIR/attrs.rs:28:14 | LL | #[deprecated(since = "forever")] | ^^^^^^^^^^^^^^^^^ @@ -15,7 +15,7 @@ LL | #[deprecated(since = "forever")] = note: `-D clippy::deprecated-semver` implied by `-D warnings` error: the since field must contain a semver-compliant version - --> $DIR/attrs.rs:32:14 + --> $DIR/attrs.rs:31:14 | LL | #[deprecated(since = "1")] | ^^^^^^^^^^^ @@ -37,13 +37,5 @@ LL | #![deny(clippy::restriction)] | = help: try enabling only the lints you really need -error: restriction lints are not meant to be all enabled - --> $DIR/attrs.rs:6:11 - | -LL | #![forbid(clippy::restriction)] - | ^^^^^^^^^^^^^^^^^^^ - | - = help: try enabling only the lints you really need - -error: aborting due to 6 previous errors +error: aborting due to 5 previous errors