Skip to content

Commit

Permalink
Prevent forbid from being ignored if overriden at the same level.
Browse files Browse the repository at this point in the history
That is, this changes `#[forbid(foo)] #[allow(foo)]` from allowing foo to
forbidding foo.
  • Loading branch information
pnkfelix authored and Mark-Simulacrum committed Oct 4, 2020
1 parent b4e77d2 commit afa2a67
Show file tree
Hide file tree
Showing 6 changed files with 144 additions and 18 deletions.
47 changes: 43 additions & 4 deletions compiler/rustc_lint/src/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<LintId, LevelSource>,
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
Expand All @@ -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,
Expand Down Expand Up @@ -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));
}
}

Expand All @@ -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)) => {
Expand Down Expand Up @@ -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, _)) => {
Expand Down
20 changes: 19 additions & 1 deletion compiler/rustc_middle/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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 {
Expand Down
49 changes: 49 additions & 0 deletions src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.rs
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
@@ -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`.
1 change: 0 additions & 1 deletion src/tools/clippy/tests/ui/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
16 changes: 4 additions & 12 deletions src/tools/clippy/tests/ui/attrs.stderr
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
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)]
| ^^^^^^^^^^^^^^^^^
|
= 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")]
| ^^^^^^^^^^^^^^^^^
|
= 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")]
| ^^^^^^^^^^^
Expand All @@ -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

0 comments on commit afa2a67

Please sign in to comment.