Don't run late lints that are disabled in the entire crate

[Manishearth]

Related: #59024

Rustc comes with a respectable number of lints, but clippy comes with a lot. Furthermore, Clippy has the concept of a "restriction" lint: a lint that is probably not useful to most users (and is not itself a judgement on "good rust style") but will be useful for codebases operating under certain constraints (for example, the lints that forbid unwrap/expect/panic/indexing).

The current linting infrastructure runs all lints at all times, only using lint levels to determine if a lint must be emitted. This basically means that most codebases running clippy are spending a lot of time running the code for lints they never see. This is true for rustc as well, though it has fewer Allow lints by default.

I proposed #59024 in the past so that lint code only gets run when the lint is enabled. There are some tricky architecture constraints around hotswapping the lint list whilst traversing the AST.

However, I don't think there's anything stopping us from doing this at a global level: it should not be hard or expensive to collect the list of enabled lints in the crate by the time late lint passes run, just collect all non-allow() lint attributes and combine them with the defaults, removing any that were manually disabled at the crate level.

Thoughts? Clippy is not super slow and this isn't a bottleneck, but as we add lints this becomes more of a problem, and it also prevents us from adding potentially computationally expensive restriction lints (one of the best things about restriction lints is that we can be more flexible around them!)

cc @rust-lang/clippy

[Mark-Simulacrum]

cc #74704

[flip1995]

I think the biggest challenge that we need to solve is that Lint<>LintPass is (or can be) a many-to-many relationship. One Lint can be emitted from multiple passes and one LintPass can emit multiple lints. AFAIK we don't do the former in Clippy, but we do a lot of the latter.

I think introducing a policy that allow-by-default lints must be in their own pass is not feasible and would worsen the dev experience of Clippy.

We do however have a list of all lints a lint pass can emit. So I think the best first step (and maybe this is already enough) would be to check if any of those lints are >=warn-by-default at some point and then run lint passes depending on that.

[xFrednet]

While working on #[expect] I noticed that a few rustc lints, like non_ascii_idents, already implement this behavior. (Playground). I like the idea, but found it confusing, that #[warn/deny/forbid()] attributes didn't work on items/statements/expressions. If this behavior is implemented, there should be some kind of warning, if a user tries to enable such a lint on non-crate levels. Maybe with a new lint, that clearly explains that this lint first has to be enabled on a crate level.

I think introducing a policy that allow-by-default lints must be in their own pass is not feasible and would worsen the dev experience of Clippy.

Agreed! What we could do, on the implementation level, is to first check if the lint is enabled, before we do most of the individual linting logic. Basically, how is_lint_allowed is used in rare cases right now. This might be annoying if we do it for all lints, but for restriction lints this could definitely make sense.

[Manishearth]

@flip1995 This is an optimization and doesn't have to be perfect, we can exclude passes which are all-allowed, as you said.

I think a policy to try and put allow-by-default lints into their own passes unless they are related is fine and is already roughly what Clippy does.

I like the idea, but found it confusing, that #[warn/deny/forbid()] attributes didn't work on items/statements/expressions. If this behavior is implemented, there should be some kind of warning, if a user tries to enable such a lint on non-crate levels. Maybe with a new lint, that clearly explains that this lint first has to be enabled on a crate level.

You've misunderstood the proposal, when I say "disabled in the entire crate", I mean "disabled in the entire crate and never enabled ever". This is for lints whose name is never mentioned in an allow attribute at all.

Agreed! What we could do, on the implementation level, is to first check if the lint is enabled, before we do most of the individual linting logic. Basically, how is_lint_allowed is used in rare cases right now. This might be annoying if we do it for all lints, but for restriction lints this could definitely make sense.

Nah, because that breaks the ability to enable the lint at the item level.

I was thinking that we could have rustc mutate the lint pass vector (or create a new one) based on this. Everything else works the same.

[RalfJung]

This has to be done very carefully -- while impl_lint_pass takes a list of lints that is somehow associated with the pass, it is currently entirely undocumented what that list is for, so we cannot expect that this list means "this pass will only emit the following lints (and can be skipped if those lints are all allowed)".

Also, it's not currently possible to have the same lint in that list for multiple passes (that will ICE saying something about a lint being registered twice), so for lints that are emitted from multiple different places, this list will necessarily be incomplete.

(I'm entirely talking about rustc lints here, I don't know much about Clippy.)

While working on #[expect] I noticed that a few rustc lints, like non_ascii_idents, already implement this behavior. (Playground). I like the idea, but found it confusing, that #[warn/deny/forbid()] attributes didn't work on items/statements/expressions.

That sounds like a bug. If you have an example where a lint cannot be enabled with function-level warn, please file an issue.

[Manishearth]

so we cannot expect that this list means "this pass will only emit the following lints (and can be skipped if those lints are all allowed)".

I would be very surprised if any rustc lint violated this. Clippy, a project with way more lints certainly doesn't.

so for lints that are emitted from multiple different places, this list will necessarily be incomplete.

yeah but those are builtins and emitted outside of lint passes entirely. Worst that can happen there is the optimization doesn't skip some lints.

[RalfJung]

I would be very surprised if any rustc lint violated this. Clippy, a project with way more lints certainly doesn't.

My PR #116098 violates this and I don't know how to fix it.

yeah but those are builtins and emitted outside of lint passes entirely. Worst that can happen there is the optimization doesn't skip some lints.

If the optimization says "all lints listed in this pass are allowed hence I will skip the pass", then once #116098 lands the optimization would skip the TypeLimits pass in some cases where that is wrong. I would not be surprised at all if the same is true for some other passes.

EDIT: Okay I did manage to fix it for that PR. But in general, a lint can be emitted from multiple passes, and currently we wouldn't be able to reflect that. We also have no testing whatsoeever in place which would ensure that a lint pass only emits the lints it declares. I think we start to rely on this property for optimizations, we need to have such testing to be sure this won't skip lints under subtle conditions.

[Manishearth]

I haven't gone through the individual commits but I still don't see why that situation would lead to a lint from one pass being emitted elsewhere.

Note that it's quite common for builtin lints to be emitted in different parts of the compiler, just that it's strange for that to happen with two lint passes.

I'm still not convinced that this isn't a very strange thing to do. Until you mentioned the possibility I had never thought that emitting a lint from a pass that doesn't declare it using the LintContext (not the tcx) would even work.

But introducing such a check seems reasonable, maybe?

[RalfJung]

In that PR, the lint invalid_nan_comparisons becomes a builtin lint (well there's two lists of "builtin" lints of course since otherwise things would be too easy to understand, I mean the one in rustc_lint_defs, not in rustc_lints) and is emitted from 2 places: the TypeLimits pass (to handle ==), and during MIR building (to handle match patterns). Originally I had added the lint to the list that contains all rustc_lint_defs::builtin lints, because that seems to be the thing to do. But then I couldn't also add it to the list for TypeLimits since that causes the lint to be registered twice. So now TypeLimits only declares two other lints. If those both get allowed and this optimization gets applied, TypeLimits gets skipped, and we are failing to report invalid_nan_comparisons.

[Alexendoo]

There's at least one case of lints being used across passes in clippy

[Manishearth]

It does seem to me that builtin lints should be special here since they're basically designed to be emitted from elsewhere. They do seem to be the main case this may ever happen.

Perhaps a certain flag when declaring lints that opts them out of this behavior entirely, used for builtin lints and the clippy special case. Such lints are allowed to be declared in other passes (as a secondary lint).

[Manishearth]

In general I feel that this is an antipattern and if people feel a strong need to do it we should have explicit opt outs.

[Alexendoo]

I'll have a go at implementing this

[Alexendoo]

Filtering out the passes works, but it's apparent that this wouldn't benefit rustc itself due to the combined passes - there's nothing to filter out

It may be beneficial to make the passes within the combined pass conditional. So as to not need two different kinds of filtering I think I'll first try moving clippy to its own combined passes

[Manishearth]

Rustc combines the passes in the lint registry, it still has separate passes iirc. We could just separate everything

[Alexendoo]

Yeah, moving (most of) the clippy lints into combined early/late passes didn't make as much difference as I thought it would, so that's next on my list of things to try

[Alexendoo]

Separating the combined pass + filtering the passes was a negative for rustc #116271 (comment)

[blyxyas]

Maybe CC

[Jarcho]

Yeah, moving (most of) the clippy lints into combined early/late passes didn't make as much difference as I thought it would, so that's next on my list of things to try

I've seen more than 10% wins when moving about two thirds of the passes into a combined pass (rustc's macros don't take constructor arguments). The measurements are from comparing the delta from cargo check using a release build.