Skip to content

Commit

Permalink
Rollup merge of rust-lang#73379 - pnkfelix:issue-70819-disallow-overr…
Browse files Browse the repository at this point in the history
…ide-forbid-in-same-scope, r=petrochenkov

Disallow later override of forbid lint in same scope

Fix rust-lang#70819

When building lint specs map for a given scope, check if forbid present on each insert.

Drive-by changes:

  1. make `LintLevelsBuilder::push` private to the crate.

  2. Add methods to `LintSource` for extracting its name symbol or its span.

  3. Rewrote old test so that its use of lint 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...)
  • Loading branch information
tmandry authored Aug 19, 2020
2 parents c03c213 + 4ec4c4f commit 6af1838
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 11 deletions.
47 changes: 43 additions & 4 deletions src/librustc_lint/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 src/librustc_middle/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`.
12 changes: 6 additions & 6 deletions src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,25 @@ 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)] ...
);
}

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
Expand Down

0 comments on commit 6af1838

Please sign in to comment.