Skip to content

Commit

Permalink
Auto merge of #125116 - blyxyas:ignore-allowed-lints-final, r=<try>
Browse files Browse the repository at this point in the history
(Big performance change) Do not run lints that cannot emit

Before this lint, adding a lint was a difficult matter because it always had some overhead involved. This was because all lints would run, no matter their default level, or if the user had `#![allow]`ed them. This PR changes that. This change would improve both the Rust lint infrastructure and Clippy, but Clippy will see the most benefit, as it has about 900 registered lints (and growing!)

So yeah, with this little patch we filter all lints pre-linting, and remove any lint that is either:
- Manually `#![allow]`ed in the whole crate,
- Allowed in the command line, or
- Not manually enabled with `#[warn]` or similar, and its default level is `Allow`

I open this PR to receive some feedback, mainly related to performance. We have lots of `Lock`s, `with_lock` and similar functions (also lots of cloning), so the filtering performance is not the best.

In an older iteration, instead of doing this in the parsing phase, we developed a visitor with the same function but without so many locks, would reverting to that change help? I'm not sure tbh.
  • Loading branch information
bors committed May 14, 2024
2 parents c45e831 + 7606f89 commit cc1d40f
Show file tree
Hide file tree
Showing 45 changed files with 247 additions and 49 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4342,6 +4342,7 @@ dependencies = [
"rustc_hir",
"rustc_hir_pretty",
"rustc_index",
"rustc_lint_defs",
"rustc_macros",
"rustc_query_system",
"rustc_serialize",
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/attr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ impl AttrItem {
self.args.span().map_or(self.path.span, |args_span| self.path.span.to(args_span))
}

fn meta_item_list(&self) -> Option<ThinVec<NestedMetaItem>> {
pub fn meta_item_list(&self) -> Option<ThinVec<NestedMetaItem>> {
match &self.args {
AttrArgs::Delimited(args) if args.delim == Delimiter::Parenthesis => {
MetaItemKind::list_from_tokens(args.tokens.clone())
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ use rustc_trait_selection::traits::{self, misc::type_allowed_to_implement_copy};

use crate::nonstandard_style::{method_context, MethodLateContext};

use std::default::Default;
use std::fmt::Write;

// hardwired lints from rustc_lint_defs
Expand Down Expand Up @@ -462,6 +463,7 @@ declare_lint! {
report_in_external_macro
}

#[derive(Default)]
pub struct MissingDoc;

impl_lint_pass!(MissingDoc => [MISSING_DOCS]);
Expand Down Expand Up @@ -903,8 +905,8 @@ pub struct DeprecatedAttr {

impl_lint_pass!(DeprecatedAttr => []);

impl DeprecatedAttr {
pub fn new() -> DeprecatedAttr {
impl Default for DeprecatedAttr {
fn default() -> Self {
DeprecatedAttr { depr_attrs: deprecated_attributes() }
}
}
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_lint/src/early.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,9 @@ impl LintPass for RuntimeCombinedEarlyLintPass<'_> {
fn name(&self) -> &'static str {
panic!()
}
fn get_lints(&self) -> crate::LintVec {
panic!()
}
}

macro_rules! impl_early_lint_pass {
Expand Down
41 changes: 36 additions & 5 deletions compiler/rustc_lint/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,9 @@ impl LintPass for RuntimeCombinedLateLintPass<'_, '_> {
fn name(&self) -> &'static str {
panic!()
}
fn get_lints(&self) -> crate::LintVec {
panic!()
}
}

macro_rules! impl_late_lint_pass {
Expand Down Expand Up @@ -360,13 +363,41 @@ pub fn late_lint_mod<'tcx, T: LateLintPass<'tcx> + 'tcx>(
// Note: `passes` is often empty. In that case, it's faster to run
// `builtin_lints` directly rather than bundling it up into the
// `RuntimeCombinedLateLintPass`.
let late_module_passes = &unerased_lint_store(tcx.sess).late_module_passes;
if late_module_passes.is_empty() {
let store = unerased_lint_store(tcx.sess);

if store.late_module_passes.is_empty() {
late_lint_mod_inner(tcx, module_def_id, context, builtin_lints);
} else {
let mut passes: Vec<_> = late_module_passes.iter().map(|mk_pass| (mk_pass)(tcx)).collect();
passes.push(Box::new(builtin_lints));
let pass = RuntimeCombinedLateLintPass { passes: &mut passes[..] };
let passes: Vec<_> =
store.late_module_passes.iter().map(|mk_pass| (mk_pass)(tcx)).collect();

// Filter unused lints
let (lints_to_emit, lints_allowed) = &**tcx.lints_that_can_emit(());
// let lints_to_emit = &lints_that_can_emit.0;
// let lints_allowed = &lints_that_can_emit.1;

// Now, we'll filtered passes in a way that discards any lint that won't trigger.
// If any lint is a given pass is detected to be emitted, we will keep that pass.
// Otherwise, we don't
let mut filtered_passes: Vec<Box<dyn LateLintPass<'tcx>>> = passes
.into_iter()
.filter(|pass| {
let pass = LintPass::get_lints(pass);
pass.iter().any(|&lint| {
let lint_name = &lint.name.to_lowercase()
// Doing some calculations here to account for those separators
[lint.name.find("::").unwrap_or(lint.name.len() - 2) + 2..]
.to_string();
lints_to_emit.contains(&lint_name)
|| (!lints_allowed.contains(&lint_name)
&& lint.default_level != crate::Level::Allow)
})
})
.collect();

filtered_passes.push(Box::new(builtin_lints));

let pass = RuntimeCombinedLateLintPass { passes: &mut filtered_passes[..] };
late_lint_mod_inner(tcx, module_def_id, context, pass);
}
}
Expand Down
77 changes: 74 additions & 3 deletions compiler/rustc_lint/src/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ use crate::{
};
use rustc_ast as ast;
use rustc_ast_pretty::pprust;
use rustc_data_structures::fx::FxIndexMap;
use rustc_data_structures::{
fx::FxIndexMap,
sync::{join, Lock, Lrc},
};
use rustc_errors::{Diag, DiagMessage, LintDiagnostic, MultiSpan};
use rustc_feature::{Features, GateIssue};
use rustc_hir as hir;
Expand Down Expand Up @@ -151,6 +154,19 @@ fn lint_expectations(tcx: TyCtxt<'_>, (): ()) -> Vec<(LintExpectationId, LintExp
builder.provider.expectations
}

/// Walk the whole crate collecting nodes where lint levels change
/// (e.g. `#[allow]` attributes), and joins that list with the warn-by-default
/// (and not allowed in the crate) and CLI lints. The returned value is a tuple
/// of 1. The lints that will emit (or at least, should run), and 2.
/// The lints that are allowed at the crate level and will not emit.
pub fn lints_that_can_emit(tcx: TyCtxt<'_>, (): ()) -> Lrc<(Vec<String>, Vec<String>)> {
let mut visitor = LintLevelMinimum::new(tcx);
visitor.process_opts();
visitor.lint_level_minimums();

Lrc::new((visitor.lints_to_emit.into_inner(), visitor.lints_allowed.into_inner()))
}

#[instrument(level = "trace", skip(tcx), ret)]
fn shallow_lint_levels_on(tcx: TyCtxt<'_>, owner: hir::OwnerId) -> ShallowLintLevelMap {
let store = unerased_lint_store(tcx.sess);
Expand Down Expand Up @@ -253,7 +269,7 @@ impl LintLevelsProvider for LintLevelQueryMap<'_> {
}
}

struct QueryMapExpectationsWrapper<'tcx> {
pub(crate) struct QueryMapExpectationsWrapper<'tcx> {
tcx: TyCtxt<'tcx>,
/// HirId of the currently investigated element.
cur: HirId,
Expand Down Expand Up @@ -450,6 +466,60 @@ impl<'tcx> Visitor<'tcx> for LintLevelsBuilder<'_, QueryMapExpectationsWrapper<'
}
}

/// Visitor with the only function of visiting every item-like in a crate and
/// computing the highest level that every lint gets put to.
///
/// E.g., if a crate has a global #![allow(lint)] attribute, but a single item
/// uses #[warn(lint)], this visitor will set that lint level as `Warn`
struct LintLevelMinimum<'tcx> {
tcx: TyCtxt<'tcx>,
/// The actual list of detected lints.
lints_to_emit: Lock<Vec<String>>,
lints_allowed: Lock<Vec<String>>,
}

impl<'tcx> LintLevelMinimum<'tcx> {
pub fn new(tcx: TyCtxt<'tcx>) -> Self {
Self {
tcx,
// That magic number is the current number of lints + some more for possible future lints
lints_to_emit: Lock::new(Vec::with_capacity(230)),
lints_allowed: Lock::new(Vec::with_capacity(100)),
}
}

fn process_opts(&mut self) {
for (lint, level) in &self.tcx.sess.opts.lint_opts {
if *level == Level::Allow {
self.lints_allowed.with_lock(|lints_allowed| lints_allowed.push(lint.to_string()));
} else {
self.lints_to_emit.with_lock(|lints_to_emit| lints_to_emit.push(lint.to_string()));
}
}
}

fn lint_level_minimums(&mut self) {
join(
|| {
self.tcx.sess.psess.lints_that_can_emit.with_lock(|vec| {
for lint_symbol in vec {
self.lints_to_emit
.with_lock(|lints_to_emit| lints_to_emit.push(lint_symbol.to_string()));
}
});
},
|| {
self.tcx.sess.psess.lints_allowed.with_lock(|vec| {
for lint_symbol in vec {
self.lints_allowed
.with_lock(|lints_allowed| lints_allowed.push(lint_symbol.to_string()));
}
});
},
);
}
}

pub struct LintLevelsBuilder<'s, P> {
sess: &'s Session,
features: &'s Features,
Expand Down Expand Up @@ -1133,7 +1203,8 @@ impl<'s, P: LintLevelsProvider> LintLevelsBuilder<'s, P> {
}

pub(crate) fn provide(providers: &mut Providers) {
*providers = Providers { shallow_lint_levels_on, lint_expectations, ..*providers };
*providers =
Providers { shallow_lint_levels_on, lint_expectations, lints_that_can_emit, ..*providers };
}

pub fn parse_lint_and_tool_name(lint_name: &str) -> (Option<Symbol>, &str) {
Expand Down
23 changes: 12 additions & 11 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#![feature(rustdoc_internals)]
#![feature(array_windows)]
#![feature(box_patterns)]
#![feature(box_into_inner)]
#![feature(control_flow_enum)]
#![feature(extract_if)]
#![feature(if_let_guard)]
Expand Down Expand Up @@ -156,15 +157,15 @@ early_lint_methods!(
[
pub BuiltinCombinedEarlyLintPass,
[
UnusedParens: UnusedParens::new(),
UnusedParens: UnusedParens::default(),
UnusedBraces: UnusedBraces,
UnusedImportBraces: UnusedImportBraces,
UnsafeCode: UnsafeCode,
SpecialModuleName: SpecialModuleName,
AnonymousParameters: AnonymousParameters,
EllipsisInclusiveRangePatterns: EllipsisInclusiveRangePatterns::default(),
NonCamelCaseTypes: NonCamelCaseTypes,
DeprecatedAttr: DeprecatedAttr::new(),
DeprecatedAttr: DeprecatedAttr::default(),
WhileTrue: WhileTrue,
NonAsciiIdents: NonAsciiIdents,
HiddenUnicodeCodepoints: HiddenUnicodeCodepoints,
Expand Down Expand Up @@ -545,23 +546,23 @@ fn register_builtins(store: &mut LintStore) {
}

fn register_internals(store: &mut LintStore) {
store.register_lints(&LintPassImpl::get_lints());
store.register_lints(&LintPassImpl::default().get_lints());
store.register_early_pass(|| Box::new(LintPassImpl));
store.register_lints(&DefaultHashTypes::get_lints());
store.register_lints(&DefaultHashTypes::default().get_lints());
store.register_late_mod_pass(|_| Box::new(DefaultHashTypes));
store.register_lints(&QueryStability::get_lints());
store.register_lints(&QueryStability::default().get_lints());
store.register_late_mod_pass(|_| Box::new(QueryStability));
store.register_lints(&ExistingDocKeyword::get_lints());
store.register_lints(&ExistingDocKeyword::default().get_lints());
store.register_late_mod_pass(|_| Box::new(ExistingDocKeyword));
store.register_lints(&TyTyKind::get_lints());
store.register_lints(&TyTyKind::default().get_lints());
store.register_late_mod_pass(|_| Box::new(TyTyKind));
store.register_lints(&Diagnostics::get_lints());
store.register_lints(&Diagnostics::default().get_lints());
store.register_late_mod_pass(|_| Box::new(Diagnostics));
store.register_lints(&BadOptAccess::get_lints());
store.register_lints(&BadOptAccess::default().get_lints());
store.register_late_mod_pass(|_| Box::new(BadOptAccess));
store.register_lints(&PassByValue::get_lints());
store.register_lints(&PassByValue::default().get_lints());
store.register_late_mod_pass(|_| Box::new(PassByValue));
store.register_lints(&SpanUseEqCtxt::get_lints());
store.register_lints(&SpanUseEqCtxt::default().get_lints());
store.register_late_mod_pass(|_| Box::new(SpanUseEqCtxt));
// FIXME(davidtwco): deliberately do not include `UNTRANSLATABLE_DIAGNOSTIC` and
// `DIAGNOSTIC_OUTSIDE_OF_IMPL` here because `-Wrustc::internal` is provided to every crate and
Expand Down
10 changes: 8 additions & 2 deletions compiler/rustc_lint/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ macro_rules! declare_combined_late_lint_pass {

$v fn get_lints() -> $crate::LintVec {
let mut lints = Vec::new();
$(lints.extend_from_slice(&$pass::get_lints());)*
$(lints.extend_from_slice(&$pass::default().get_lints());)*
lints
}
}
Expand All @@ -122,6 +122,9 @@ macro_rules! declare_combined_late_lint_pass {
fn name(&self) -> &'static str {
panic!()
}
fn get_lints(&self) -> LintVec {
panic!()
}
}
)
}
Expand Down Expand Up @@ -218,7 +221,7 @@ macro_rules! declare_combined_early_lint_pass {

$v fn get_lints() -> $crate::LintVec {
let mut lints = Vec::new();
$(lints.extend_from_slice(&$pass::get_lints());)*
$(lints.extend_from_slice(&$pass::default().get_lints());)*
lints
}
}
Expand All @@ -232,6 +235,9 @@ macro_rules! declare_combined_early_lint_pass {
fn name(&self) -> &'static str {
panic!()
}
fn get_lints(&self) -> LintVec {
panic!()
}
}
)
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ declare_lint! {
"detects ambiguous wide pointer comparisons"
}

#[derive(Copy, Clone)]
#[derive(Copy, Clone, Default)]
pub struct TypeLimits {
/// Id of the last visited negated expression
negated_expr_id: Option<hir::HirId>,
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_lint/src/unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1018,8 +1018,8 @@ pub struct UnusedParens {
parens_in_cast_in_lt: Vec<ast::NodeId>,
}

impl UnusedParens {
pub fn new() -> Self {
impl Default for UnusedParens {
fn default() -> Self {
Self { with_self_ty_parens: false, parens_in_cast_in_lt: Vec::new() }
}
}
Expand Down
18 changes: 14 additions & 4 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,7 @@ pub type LintVec = Vec<&'static Lint>;

pub trait LintPass {
fn name(&self) -> &'static str;
fn get_lints(&self) -> LintVec;
}

/// Implements `LintPass for $ty` with the given list of `Lint` statics.
Expand All @@ -878,9 +879,7 @@ macro_rules! impl_lint_pass {
($ty:ty => [$($lint:expr),* $(,)?]) => {
impl $crate::LintPass for $ty {
fn name(&self) -> &'static str { stringify!($ty) }
}
impl $ty {
pub fn get_lints() -> $crate::LintVec { vec![$($lint),*] }
fn get_lints(&self) -> $crate::LintVec { vec![$($lint),*] }
}
};
}
Expand All @@ -890,7 +889,18 @@ macro_rules! impl_lint_pass {
#[macro_export]
macro_rules! declare_lint_pass {
($(#[$m:meta])* $name:ident => [$($lint:expr),* $(,)?]) => {
$(#[$m])* #[derive(Copy, Clone)] pub struct $name;
$(#[$m])* #[derive(Copy, Clone, Default)] pub struct $name;
$crate::impl_lint_pass!($name => [$($lint),*]);
};
}

#[allow(rustc::lint_pass_impl_without_macro)]
impl<P: LintPass + ?Sized> LintPass for Box<P> {
fn name(&self) -> &'static str {
(**self).name()
}

fn get_lints(&self) -> LintVec {
(**self).get_lints()
}
}
1 change: 1 addition & 0 deletions compiler/rustc_middle/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ rustc_graphviz = { path = "../rustc_graphviz" }
rustc_hir = { path = "../rustc_hir" }
rustc_hir_pretty = { path = "../rustc_hir_pretty" }
rustc_index = { path = "../rustc_index" }
rustc_lint_defs = { path = "../rustc_lint_defs" }
rustc_macros = { path = "../rustc_macros" }
rustc_query_system = { path = "../rustc_query_system" }
rustc_serialize = { path = "../rustc_serialize" }
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ impl ShallowLintLevelMap {
/// This lint level is not usable for diagnostics, it needs to be corrected by
/// `reveal_actual_level` beforehand.
#[instrument(level = "trace", skip(self, tcx), ret)]
fn probe_for_lint_level(
pub fn probe_for_lint_level(
&self,
tcx: TyCtxt<'_>,
id: LintId,
Expand Down
Loading

0 comments on commit cc1d40f

Please sign in to comment.