Skip to content

Commit

Permalink
Rollup merge of rust-lang#81556 - nikomatsakis:forbidden-lint-groups-…
Browse files Browse the repository at this point in the history
…lint, r=pnkfelix

introduce future-compatibility warning for forbidden lint groups

We used to ignore `forbid(group)` scenarios completely. This changed in rust-lang#78864, but that led to a number of regressions (rust-lang#80988, rust-lang#81218).

This PR introduces a future compatibility warning for the case where a group is forbidden but then an individual lint within that group is allowed. We now issue a FCW when we see the "allow", but permit it to take effect.

r? `@Mark-Simulacrum`
  • Loading branch information
m-ou-se authored Feb 3, 2021
2 parents 3b6dee2 + b6b897b commit 54fa9ea
Show file tree
Hide file tree
Showing 19 changed files with 554 additions and 54 deletions.
15 changes: 15 additions & 0 deletions compiler/rustc_lint/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ use rustc_session::SessionLintStore;
use rustc_span::lev_distance::find_best_match_for_name;
use rustc_span::{symbol::Symbol, MultiSpan, Span, DUMMY_SP};
use rustc_target::abi::LayoutOf;
use tracing::debug;

use std::cell::Cell;
use std::slice;
Expand Down Expand Up @@ -336,6 +337,20 @@ impl LintStore {
}
}

/// True if this symbol represents a lint group name.
pub fn is_lint_group(&self, lint_name: Symbol) -> bool {
debug!(
"is_lint_group(lint_name={:?}, lint_groups={:?})",
lint_name,
self.lint_groups.keys().collect::<Vec<_>>()
);
let lint_name_str = &*lint_name.as_str();
self.lint_groups.contains_key(&lint_name_str) || {
let warnings_name_str = crate::WARNINGS.name_lower();
lint_name_str == &*warnings_name_str
}
}

/// Checks the name of a lint for its existence, and whether it was
/// renamed or removed. Generates a DiagnosticBuilder containing a
/// warning for renamed and removed lints. This is over both lint
Expand Down
101 changes: 73 additions & 28 deletions compiler/rustc_lint/src/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use rustc_ast::attr;
use rustc_ast::unwrap_or;
use rustc_ast_pretty::pprust;
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::{struct_span_err, Applicability};
use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
use rustc_hir as hir;
use rustc_hir::def_id::{CrateNum, LOCAL_CRATE};
use rustc_hir::{intravisit, HirId};
Expand All @@ -17,11 +17,15 @@ use rustc_middle::lint::{
};
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::TyCtxt;
use rustc_session::lint::{builtin, Level, Lint, LintId};
use rustc_session::lint::{
builtin::{self, FORBIDDEN_LINT_GROUPS},
Level, Lint, LintId,
};
use rustc_session::parse::feature_err;
use rustc_session::Session;
use rustc_span::symbol::{sym, Symbol};
use rustc_span::{source_map::MultiSpan, Span, DUMMY_SP};
use tracing::debug;

use std::cmp;

Expand Down Expand Up @@ -51,6 +55,7 @@ pub struct LintLevelsBuilder<'s> {
id_to_set: FxHashMap<HirId, u32>,
cur: u32,
warn_about_weird_lints: bool,
store: &'s LintStore,
}

pub struct BuilderPush {
Expand All @@ -59,13 +64,14 @@ pub struct BuilderPush {
}

impl<'s> LintLevelsBuilder<'s> {
pub fn new(sess: &'s Session, warn_about_weird_lints: bool, store: &LintStore) -> Self {
pub fn new(sess: &'s Session, warn_about_weird_lints: bool, store: &'s LintStore) -> Self {
let mut builder = LintLevelsBuilder {
sess,
sets: LintLevelSets::new(),
cur: 0,
id_to_set: Default::default(),
warn_about_weird_lints,
store,
};
builder.process_command_line(sess, store);
assert_eq!(builder.sets.list.len(), 1);
Expand Down Expand Up @@ -120,36 +126,75 @@ impl<'s> LintLevelsBuilder<'s> {
if let (Level::Forbid, old_src) =
self.sets.get_lint_level(id.lint, self.cur, Some(&specs), &self.sess)
{
let mut diag_builder = struct_span_err!(
self.sess,
src.span(),
E0453,
"{}({}) incompatible with previous forbid",
level.as_str(),
src.name(),
// Backwards compatibility check:
//
// We used to not consider `forbid(lint_group)`
// as preventing `allow(lint)` for some lint `lint` in
// `lint_group`. For now, issue a future-compatibility
// warning for this case.
let id_name = id.lint.name_lower();
let fcw_warning = match old_src {
LintLevelSource::Default => false,
LintLevelSource::Node(symbol, _, _) => self.store.is_lint_group(symbol),
LintLevelSource::CommandLine(symbol, _) => self.store.is_lint_group(symbol),
};
debug!(
"fcw_warning={:?}, specs.get(&id) = {:?}, old_src={:?}, id_name={:?}",
fcw_warning, specs, old_src, id_name
);
diag_builder.span_label(src.span(), "overruled by previous forbid");
match old_src {
LintLevelSource::Default => {
diag_builder.note(&format!(
"`forbid` lint level is the default for {}",
id.to_string()
));
}
LintLevelSource::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());

let decorate_diag_builder = |mut diag_builder: DiagnosticBuilder<'_>| {
diag_builder.span_label(src.span(), "overruled by previous forbid");
match old_src {
LintLevelSource::Default => {
diag_builder.note(&format!(
"`forbid` lint level is the default for {}",
id.to_string()
));
}
LintLevelSource::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());
}
}
LintLevelSource::CommandLine(_, _) => {
diag_builder.note("`forbid` lint level was set on command line");
}
}
LintLevelSource::CommandLine(_, _) => {
diag_builder.note("`forbid` lint level was set on command line");
}
diag_builder.emit();
};
if !fcw_warning {
let diag_builder = struct_span_err!(
self.sess,
src.span(),
E0453,
"{}({}) incompatible with previous forbid",
level.as_str(),
src.name(),
);
decorate_diag_builder(diag_builder);
} else {
self.struct_lint(
FORBIDDEN_LINT_GROUPS,
Some(src.span().into()),
|diag_builder| {
let diag_builder = diag_builder.build(&format!(
"{}({}) incompatible with previous forbid",
level.as_str(),
src.name(),
));
decorate_diag_builder(diag_builder);
},
);
}
diag_builder.emit();

// Retain the forbid lint level
return;
// Retain the forbid lint level, unless we are
// issuing a FCW. In the FCW case, we want to
// respect the new setting.
if !fcw_warning {
return;
}
}
}
specs.insert(id, (level, src));
Expand Down
39 changes: 39 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,48 @@
//! compiler code, rather than using their own custom pass. Those
//! lints are all available in `rustc_lint::builtin`.

// ignore-tidy-filelength

use crate::{declare_lint, declare_lint_pass};
use rustc_span::edition::Edition;
use rustc_span::symbol::sym;

declare_lint! {
/// The `forbidden_lint_groups` lint detects violations of
/// `forbid` applied to a lint group. Due to a bug in the compiler,
/// these used to be overlooked entirely. They now generate a warning.
///
/// ### Example
///
/// ```rust
/// #![forbid(warnings)]
/// #![deny(bad_style)]
///
/// fn main() {}
/// ```
///
/// {{produces}}
///
/// ### Recommended fix
///
/// If your crate is using `#![forbid(warnings)]`,
/// we recommend that you change to `#![deny(warnings)]`.
///
/// ### Explanation
///
/// Due to a compiler bug, applying `forbid` to lint groups
/// previously had no effect. The bug is now fixed but instead of
/// enforcing `forbid` we issue this future-compatibility warning
/// to avoid breaking existing crates.
pub FORBIDDEN_LINT_GROUPS,
Warn,
"applying forbid to lint-groups",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #81670 <https://github.com/rust-lang/rust/issues/81670>",
edition: None,
};
}

declare_lint! {
/// The `ill_formed_attribute_input` lint detects ill-formed attribute
/// inputs that were previously accepted and used in practice.
Expand Down Expand Up @@ -2884,6 +2922,7 @@ declare_lint_pass! {
/// Does nothing as a lint pass, but registers some `Lint`s
/// that are used by other parts of the compiler.
HardwiredLints => [
FORBIDDEN_LINT_GROUPS,
ILLEGAL_FLOATING_POINT_LITERAL_PATTERN,
ARITHMETIC_OVERFLOW,
UNCONDITIONAL_PANIC,
Expand Down
12 changes: 10 additions & 2 deletions compiler/rustc_middle/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_errors::{DiagnosticBuilder, DiagnosticId};
use rustc_hir::HirId;
use rustc_session::lint::{builtin, Level, Lint, LintId};
use rustc_session::lint::{
builtin::{self, FORBIDDEN_LINT_GROUPS},
Level, Lint, LintId,
};
use rustc_session::{DiagnosticMessageId, Session};
use rustc_span::hygiene::MacroKind;
use rustc_span::source_map::{DesugaringKind, ExpnKind, MultiSpan};
Expand Down Expand Up @@ -89,7 +92,12 @@ impl LintLevelSets {
// If we're about to issue a warning, check at the last minute for any
// directives against the warnings "lint". If, for example, there's an
// `allow(warnings)` in scope then we want to respect that instead.
if level == Level::Warn {
//
// We exempt `FORBIDDEN_LINT_GROUPS` from this because it specifically
// triggers in cases (like #80988) where you have `forbid(warnings)`,
// and so if we turned that into an error, it'd defeat the purpose of the
// future compatibility warning.
if level == Level::Warn && LintId::of(lint) != LintId::of(FORBIDDEN_LINT_GROUPS) {
let (warnings_level, warnings_src) =
self.get_lint_id_level(LintId::of(builtin::WARNINGS), idx, aux);
if let Some(configured_warning_level) = warnings_level {
Expand Down
13 changes: 13 additions & 0 deletions src/test/ui/lint/forbid-group-group-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Check what happens when we forbid a smaller group but
// then allow a superset of that group.

#![forbid(nonstandard_style)]

// FIXME: Arguably this should be an error, but the WARNINGS group is
// treated in a very special (and rather ad-hoc) way and
// it fails to trigger.
#[allow(warnings)]
fn main() {
let A: ();
//~^ ERROR should have a snake case name
}
15 changes: 15 additions & 0 deletions src/test/ui/lint/forbid-group-group-1.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: variable `A` should have a snake case name
--> $DIR/forbid-group-group-1.rs:11:9
|
LL | let A: ();
| ^ help: convert the identifier to snake case: `a`
|
note: the lint level is defined here
--> $DIR/forbid-group-group-1.rs:4:11
|
LL | #![forbid(nonstandard_style)]
| ^^^^^^^^^^^^^^^^^
= note: `#[forbid(non_snake_case)]` implied by `#[forbid(nonstandard_style)]`

error: aborting due to previous error

26 changes: 26 additions & 0 deletions src/test/ui/lint/forbid-group-group-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Check what happens when we forbid a bigger group but
// then deny a subset of that group.

#![forbid(warnings)]
#![deny(forbidden_lint_groups)]

#[allow(nonstandard_style)]
//~^ ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
//~| ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
//~| ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
//~| ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
//~| ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
//~| ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
//~| ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
//~| ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
//~| ERROR incompatible with previous
//~| WARNING previously accepted by the compiler
fn main() {}
Loading

0 comments on commit 54fa9ea

Please sign in to comment.