Auto merge of #158239 - nnethercote:rework-lint-pass-running, r=Urgau

Rework lint pass running



Some cleanups relating to the running of lint passes.

r? @Urgau

Author: bors

compiler/rustc_hir_analysis/src/check/region.rs

@@ -148,7 +148,7 @@ fn resolve_block<'tcx>(
             if !terminating
                 && !visitor
                     .tcx
-                    .lints_that_dont_need_to_run(())
+                    .skippable_lints(())
                     .contains(&lint::LintId::of(lint::builtin::TAIL_EXPR_DROP_ORDER))
             {
                 // If this temporary scope will be changing once the codebase adopts Rust 2024,

compiler/rustc_interface/src/passes.rs

@@ -101,7 +101,6 @@ fn pre_expansion_lint<'a>(
                 lint_store,
                 registered_tools,
                 None,
-                rustc_lint::BuiltinCombinedPreExpansionLintPass::new(),
                 check_node,
             );
         },
@@ -479,7 +478,6 @@ fn early_lint_checks(tcx: TyCtxt<'_>, (): ()) {
         lint_store,
         tcx.registered_tools(()),
         Some(lint_buffer),
-        rustc_lint::BuiltinCombinedEarlyLintPass::new(),
         EarlyCheckNode::CrateRoot(&*krate, &*krate.attrs),
     )
 }

compiler/rustc_lint/src/context.rs

@@ -40,7 +40,8 @@ use self::TargetLint::*;
 use crate::levels::LintLevelsBuilder;
 use crate::passes::{EarlyLintPassObject, LateLintPassObject};
 
-type EarlyLintPassFactory = Box<dyn Fn() -> EarlyLintPassObject + sync::DynSend + sync::DynSync>;
+pub(crate) type EarlyLintPassFactory =
+    Box<dyn Fn() -> EarlyLintPassObject + sync::DynSend + sync::DynSync>;
 type LateLintPassFactory =
     Box<dyn for<'tcx> Fn(TyCtxt<'tcx>) -> LateLintPassObject<'tcx> + sync::DynSend + sync::DynSync>;
 

compiler/rustc_lint/src/early.rs

@@ -16,7 +16,7 @@ use rustc_span::{Ident, Span};
 use tracing::debug;
 
 use crate::DiagAndSess;
-use crate::context::{EarlyContext, LintContext, LintStore};
+use crate::context::{EarlyContext, EarlyLintPassFactory, LintContext, LintStore};
 use crate::passes::{EarlyLintPass, EarlyLintPassObject};
 
 pub(super) mod diagnostics;
@@ -316,7 +316,6 @@ pub fn check_ast_node<'a>(
     lint_store: &LintStore,
     registered_tools: &RegisteredTools,
     lint_buffer: Option<LintBuffer>,
-    builtin_lints: impl EarlyLintPass + 'static,
     check_node: EarlyCheckNode<'a>,
 ) {
     let context = EarlyContext::new(
@@ -328,35 +327,20 @@ pub fn check_ast_node<'a>(
         lint_buffer.unwrap_or_default(),
     );
 
-    // Note: `passes` is often empty. In that case, it's faster to run
-    // `builtin_lints` directly rather than bundling it up into the
-    // `RuntimeCombinedEarlyLintPass`.
-    let passes =
-        if pre_expansion { &lint_store.pre_expansion_passes } else { &lint_store.early_passes };
-    if passes.is_empty() {
-        check_ast_node_inner(sess, check_node, context, builtin_lints);
+    let context = if pre_expansion {
+        let builtin_lints = crate::BuiltinCombinedPreExpansionLintPass::new();
+        let passes = &lint_store.pre_expansion_passes;
+        run_passes(check_node, context, builtin_lints, passes)
     } else {
-        let mut passes: Vec<_> = passes.iter().map(|mk_pass| (mk_pass)()).collect();
-        passes.push(Box::new(builtin_lints));
-        let pass = RuntimeCombinedEarlyLintPass { passes };
-        check_ast_node_inner(sess, check_node, context, pass);
-    }
-}
-
-fn check_ast_node_inner<'a, T: EarlyLintPass>(
-    sess: &Session,
-    check_node: EarlyCheckNode<'a>,
-    context: EarlyContext<'_>,
-    pass: T,
-) {
-    let mut cx = EarlyContextAndPass { context, pass };
-
-    cx.with_lint_attrs(check_node.id(), check_node.attrs(), |cx| check_node.check(cx));
+        let builtin_lints = crate::BuiltinCombinedEarlyLintPass::new();
+        let passes = &lint_store.early_passes;
+        run_passes(check_node, context, builtin_lints, passes)
+    };
 
     // All of the buffered lints should have been emitted at this point.
     // If not, that means that we somehow buffered a lint for a node id
     // that was not lint-checked (perhaps it doesn't exist?). This is a bug.
-    for (id, lints) in cx.context.buffered.map {
+    for (id, lints) in context.buffered.map {
         if !lints.is_empty() {
             assert!(
                 sess.dcx().has_errors().is_some(),
@@ -367,3 +351,32 @@ fn check_ast_node_inner<'a, T: EarlyLintPass>(
         }
     }
 }
+
+fn run_passes<'a, 'ecx, T: EarlyLintPass + 'static>(
+    check_node: EarlyCheckNode<'a>,
+    context: EarlyContext<'ecx>,
+    builtin_lints: T,
+    passes: &[EarlyLintPassFactory],
+) -> EarlyContext<'ecx> {
+    // Note: `passes` is often empty. In that case, it's faster to run
+    // `builtin_lints` directly rather than bundling it up into the
+    // `RuntimeCombinedEarlyLintPass`.
+    if passes.is_empty() {
+        run_pass(check_node, context, builtin_lints)
+    } else {
+        let mut passes: Vec<_> = passes.iter().map(|mk_pass| mk_pass()).collect();
+        passes.push(Box::new(builtin_lints));
+        let pass = RuntimeCombinedEarlyLintPass { passes };
+        run_pass(check_node, context, pass)
+    }
+}
+
+fn run_pass<'a, 'ecx, T: EarlyLintPass>(
+    check_node: EarlyCheckNode<'a>,
+    context: EarlyContext<'ecx>,
+    pass: T,
+) -> EarlyContext<'ecx> {
+    let mut cx = EarlyContextAndPass { context, pass };
+    cx.with_lint_attrs(check_node.id(), check_node.attrs(), |cx| check_node.check(cx));
+    cx.context
+}

compiler/rustc_lint/src/if_let_rescope.rs

@@ -269,7 +269,7 @@ impl_lint_pass!(
 impl<'tcx> LateLintPass<'tcx> for IfLetRescope {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
         if expr.span.edition().at_least_rust_2024()
-            || cx.tcx.lints_that_dont_need_to_run(()).contains(&LintId::of(IF_LET_RESCOPE))
+            || cx.tcx.skippable_lints(()).contains(&LintId::of(IF_LET_RESCOPE))
         {
             return;
         }

compiler/rustc_lint/src/late.rs

@@ -18,7 +18,7 @@ use rustc_span::Span;
 use tracing::debug;
 
 use crate::passes::LateLintPassObject;
-use crate::{LateContext, LateLintPass, LintId, LintStore};
+use crate::{LateContext, LateLintPass, LintStore, is_lint_pass_required};
 
 /// Extract the [`LintStore`] from [`Session`].
 ///
@@ -349,31 +349,26 @@ pub fn late_lint_mod<'tcx, T: LateLintPass<'tcx> + 'tcx>(
         only_module: true,
     };
 
+    let skippable_lints = tcx.skippable_lints(());
+
     // 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 store = unerased_lint_store(tcx.sess);
-
-    if store.late_module_passes.is_empty() {
-        // If all builtin lints can be skipped, there is no point in running `late_lint_mod_inner`
-        // at all. This happens often for dependencies built with `--cap-lints=allow`.
-        let dont_need_to_run = tcx.lints_that_dont_need_to_run(());
-        let can_skip_lints = builtin_lints
-            .get_lints()
-            .iter()
-            .all(|lint| dont_need_to_run.contains(&LintId::of(lint)));
-        if !can_skip_lints {
+    let mut passes: Vec<_> = unerased_lint_store(tcx.sess)
+        .late_module_passes
+        .iter()
+        .map(|mk_pass| mk_pass(tcx))
+        .filter(|pass| is_lint_pass_required(skippable_lints, &pass.get_lints()))
+        .collect();
+    let builtin_lints_must_run = is_lint_pass_required(skippable_lints, &builtin_lints.get_lints());
+    if passes.is_empty() {
+        if builtin_lints_must_run {
             late_lint_mod_inner(tcx, module_def_id, context, builtin_lints);
         }
     } else {
-        let builtin_lints = Box::new(builtin_lints) as Box<dyn LateLintPass<'tcx>>;
-        let passes = store
-            .late_module_passes
-            .iter()
-            .map(|mk_pass| (mk_pass)(tcx))
-            .chain(std::iter::once(builtin_lints))
-            .collect::<Vec<_>>();
-
+        if builtin_lints_must_run {
+            passes.push(Box::new(builtin_lints) as Box<dyn LateLintPass<'tcx>>);
+        }
         let pass = RuntimeCombinedLateLintPass { passes };
         late_lint_mod_inner(tcx, module_def_id, context, pass);
     }
@@ -404,18 +399,15 @@ fn late_lint_mod_inner<'tcx, T: LateLintPass<'tcx>>(
 }
 
 fn late_lint_crate<'tcx>(tcx: TyCtxt<'tcx>) {
-    let lints_that_dont_need_to_run = tcx.lints_that_dont_need_to_run(());
+    let skippable_lints = tcx.skippable_lints(());
 
     // Note: `passes` is often empty after filtering.
-    let mut passes: Vec<_> =
-        unerased_lint_store(tcx.sess).late_passes.iter().map(|mk_pass| (mk_pass)(tcx)).collect();
-    passes.retain(|pass| {
-        let lints = pass.get_lints();
-        // Lintless passes are always in
-        lints.is_empty() ||
-            // If the pass doesn't have a single needed lint, omit it
-            !lints.iter().all(|lint| lints_that_dont_need_to_run.contains(&LintId::of(lint)))
-    });
+    let passes: Vec<_> = unerased_lint_store(tcx.sess)
+        .late_passes
+        .iter()
+        .map(|mk_pass| mk_pass(tcx))
+        .filter(|pass| is_lint_pass_required(skippable_lints, &pass.get_lints()))
+        .collect();
     if passes.is_empty() {
         return;
     }

compiler/rustc_lint/src/levels.rs

@@ -114,11 +114,11 @@ impl LintLevelSets {
     }
 }
 
-fn lints_that_dont_need_to_run(tcx: TyCtxt<'_>, (): ()) -> UnordSet<LintId> {
+fn skippable_lints(tcx: TyCtxt<'_>, (): ()) -> UnordSet<LintId> {
     let store = unerased_lint_store(&tcx.sess);
     let root_map = tcx.shallow_lint_levels_on(hir::CRATE_OWNER_ID);
 
-    let mut dont_need_to_run: FxHashSet<LintId> = store
+    let mut skippable: FxHashSet<LintId> = store
         .get_lints()
         .into_iter()
         .filter(|lint| {
@@ -145,13 +145,13 @@ fn lints_that_dont_need_to_run(tcx: TyCtxt<'_>, (): ()) -> UnordSet<LintId> {
         for (_, specs) in map.specs.iter() {
             for (lint, level_spec) in specs.iter() {
                 if !level_spec.is_allow() {
-                    dont_need_to_run.remove(lint);
+                    skippable.remove(lint);
                 }
             }
         }
     }
 
-    dont_need_to_run.into()
+    skippable.into()
 }
 
 #[instrument(level = "trace", skip(tcx), ret)]
@@ -1035,7 +1035,7 @@ where
 }
 
 pub(crate) fn provide(providers: &mut Providers) {
-    *providers = Providers { shallow_lint_levels_on, lints_that_dont_need_to_run, ..*providers };
+    *providers = Providers { shallow_lint_levels_on, skippable_lints, ..*providers };
 }
 
 pub(crate) fn parse_lint_and_tool_name(lint_name: &str) -> (Option<Symbol>, &str) {

compiler/rustc_lint/src/lib.rs

@@ -117,6 +117,7 @@ use precedence::*;
 use ptr_nulls::*;
 use redundant_semicolon::*;
 use reference_casting::*;
+use rustc_data_structures::unord::UnordSet;
 use rustc_hir::def_id::LocalModDefId;
 use rustc_middle::query::Providers;
 use rustc_middle::ty::TyCtxt;
@@ -742,5 +743,21 @@ fn register_internals(store: &mut LintStore) {
     );
 }
 
+/// Is a pass (which contains `lints`) required to run? Maybe not, e.g. for dependencies built with
+/// `--cap-lints=allow`.
+///
+/// Note: this is a conservative estimate intended for optimization purposes. It might return
+/// `true` for a pass that need not run, but it will never return `false` for a pass that must run.
+pub fn is_lint_pass_required(skippable: &UnordSet<LintId>, lints: &LintVec) -> bool {
+    // A pass without any lints? Clippy sometimes does this, to collect things while traversing.
+    // Such a pass must always run.
+    if lints.is_empty() {
+        return true;
+    }
+
+    // Otherwise, the pass must run unless all lints within are skippable.
+    !lints.iter().all(|lint| skippable.contains(&LintId::of(lint)))
+}
+
 #[cfg(test)]
 mod tests;

compiler/rustc_middle/src/queries.rs

@@ -541,7 +541,7 @@ rustc_queries! {
         desc { "computing `#[expect]`ed lints in this crate" }
     }
 
-    query lints_that_dont_need_to_run(_: ()) -> &'tcx UnordSet<LintId> {
+    query skippable_lints(_: ()) -> &'tcx UnordSet<LintId> {
         arena_cache
         // This depends on the lint store, which includes internal lints when the
         // untracked `-Zunstable-options` flag is set.

compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs

@@ -187,7 +187,7 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<
         return;
     }
     if body.span.edition().at_least_rust_2024()
-        || tcx.lints_that_dont_need_to_run(()).contains(&lint::LintId::of(TAIL_EXPR_DROP_ORDER))
+        || tcx.skippable_lints(()).contains(&lint::LintId::of(TAIL_EXPR_DROP_ORDER))
     {
         return;
     }