Skip to content

Refactor lint buffering to avoid requiring a giant enum #145747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 23, 2025

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Aug 22, 2025

Lint buffering currently relies on a giant enum BuiltinLintDiag containing all the lints that might potentially get buffered. In addition to being an unwieldy enum in a central crate, this also makes rustc_lint_defs a build bottleneck: it depends on various types from various crates (with a steady pressure to add more), and many crates depend on it.

Having all of these variants in a separate crate also prevents detecting when a variant becomes unused, which we can do with a dedicated type defined and used in the same crate.

Refactor this to use a dyn trait, to allow using LintDiagnostic types directly.

Because the existing BuiltinLintDiag requires some additional types in order to decorate some variants, which are only available later in rustc_lint, use an enum DecorateDiagCompat to handle both the dyn LintDiagnostic case and the BuiltinLintDiag case.


With the infrastructure in place, use it to migrate three of the enum variants to use LintDiagnostic directly, as a proof of concept and to demonstrate that the net result is a reduction in code size and a removal of a boilerplate-heavy layer of indirection.

Also remove an unused BuiltinLintDiag variant.

Lint buffering currently relies on a giant enum `BuiltinLintDiag`
containing all the lints that might potentially get buffered. In
addition to being an unwieldy enum in a central crate, this also makes
`rustc_lint_defs` a build bottleneck: it depends on various types from
various crates (with a steady pressure to add more), and many crates
depend on it.

Having all of these variants in a separate crate also prevents detecting
when a variant becomes unused, which we can do with a dedicated type
defined and used in the same crate.

Refactor this to use a dyn trait, to allow using `LintDiagnostic` types
directly.

This requires boxing, but all of this is already on the slow path
(emitting an error).

Because the existing `BuiltinLintDiag` requires some additional types in
order to decorate some variants, which are only available later in
`rustc_lint`, use an enum `DecorateDiagCompat` to handle both the `dyn
LintDiagnostic` case and the `BuiltinLintDiag` case.
This variant doesn't appear to have ever been used.

There's a matching message in `rustc_resolve`, that used to have a FIXME
for porting it to the new diagnostic infrastructure, but that message is
using `feature_err`, which doesn't use buffered lints. Thus, even when
that does get ported, it won't use `BuiltinLintDiag`.
@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2025

r? @jdonszelmann

rustbot has assigned @jdonszelmann.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

@fmease
Copy link
Member

fmease commented Aug 22, 2025

This requires boxing, but all of this is already on the slow path (emitting an error).

That's not factual, these buffered lints may not necessarily be emitted. Fn lint_level has the final say about that.

This change may therefore still be perf sensitive.

@fmease
Copy link
Member

fmease commented Aug 22, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Aug 22, 2025
Refactor lint buffering to avoid requiring a giant enum
@rust-bors

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 22, 2025
@rust-bors
Copy link

rust-bors bot commented Aug 22, 2025

☀️ Try build successful (CI)
Build commit: 553d20a (553d20a2d680d2600b660b351a0a2916ff3439db, parent: d127901d940d96209fd2ae8ff6769ad2788099fb)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (553d20a): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 3.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.1% [3.1%, 3.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (secondary 3.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.0% [3.0%, 3.0%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 470.79s -> 470.269s (-0.11%)
Artifact size: 378.31 MiB -> 378.13 MiB (-0.05%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 22, 2025
@jdonszelmann
Copy link
Contributor

hmm, looking good. I've run into this one a lot. I guess as you say we can migrate more of these in the near future.

@jdonszelmann
Copy link
Contributor

let's merge this before #137729 so that one is right from the start and can be simplified quite a bit :)

@jdonszelmann
Copy link
Contributor

yeah, looking good! @bors r+

@bors
Copy link
Collaborator

bors commented Aug 22, 2025

📌 Commit 52fadd8 has been approved by jdonszelmann

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2025
@joshtriplett
Copy link
Member Author

@bors rollup
(since it's performance-neutral)

bors added a commit that referenced this pull request Aug 23, 2025
Rollup of 28 pull requests

Successful merges:

 - #132087 (Fix overly restrictive lifetime in `core::panic::Location::file` return type)
 - #137396 (Recover `param: Ty = EXPR`)
 - #137457 (Fix host code appearing in Wasm binaries)
 - #142185 (Convert moves of references to copies in ReferencePropagation)
 - #144648 (Implementation: `#[feature(nonpoison_rwlock)]`)
 - #144897 (print raw lifetime idents with r#)
 - #145218 ([Debuginfo] improve enum value formatting in LLDB for better readability)
 - #145380 (Add codegen-llvm regression tests)
 - #145573 (Add an experimental unsafe(force_target_feature) attribute.)
 - #145597 (resolve: Remove `ScopeSet::Late`)
 - #145633 (Fix some typos in LocalKey documentation)
 - #145641 (On E0277, point at type that doesn't implement bound)
 - #145669 (rustdoc-search: GUI tests check for `//` in URL)
 - #145695 (Introduce ProjectionElem::try_map.)
 - #145710 (Fix the ABI parameter inconsistency issue in debug.rs for LoongArch64)
 - #145726 (Experiment: Reborrow trait)
 - #145731 (Make raw pointers work in type-based search)
 - #145736 (triagebot: Update style team reviewers)
 - #145738 (Uplift rustc_mir_transform::coverage::counters::union_find to rustc_data_structures.)
 - #145742 (rustdoc js: Even more typechecking improvments )
 - #145743 (doc: fix some typos in comment)
 - #145745 (tests: Ignore basic-stepping.rs on LoongArch)
 - #145747 (Refactor lint buffering to avoid requiring a giant enum)
 - #145751 (fix(lexer): Allow '-' in the frontmatter infostring continue set)
 - #145761 (Add aarch64_be-unknown-hermit target)
 - #145762 (convert strings to symbols in attr diagnostics)
 - #145763 (Ship LLVM tools for the correct target when cross-compiling)
 - #145765 (Revert suggestions for missing methods in tuples)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d3c9908 into rust-lang:master Aug 23, 2025
11 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Aug 23, 2025
rust-timer added a commit that referenced this pull request Aug 23, 2025
Rollup merge of #145747 - joshtriplett:builtin-diag-dyn, r=jdonszelmann

Refactor lint buffering to avoid requiring a giant enum

Lint buffering currently relies on a giant enum `BuiltinLintDiag` containing all the lints that might potentially get buffered. In addition to being an unwieldy enum in a central crate, this also makes `rustc_lint_defs` a build bottleneck: it depends on various types from various crates (with a steady pressure to add more), and many crates depend on it.

Having all of these variants in a separate crate also prevents detecting when a variant becomes unused, which we can do with a dedicated type defined and used in the same crate.

Refactor this to use a dyn trait, to allow using `LintDiagnostic` types directly.

Because the existing `BuiltinLintDiag` requires some additional types in order to decorate some variants, which are only available later in `rustc_lint`, use an enum `DecorateDiagCompat` to handle both the `dyn LintDiagnostic` case and the `BuiltinLintDiag` case.

---

With the infrastructure in place, use it to migrate three of the enum variants to use `LintDiagnostic` directly, as a proof of concept and to demonstrate that the net result is a reduction in code size and a removal of a boilerplate-heavy layer of indirection.

Also remove an unused `BuiltinLintDiag` variant.
@joshtriplett joshtriplett deleted the builtin-diag-dyn branch August 23, 2025 16:33
@fmease
Copy link
Member

fmease commented Aug 23, 2025

Thanks a lot, Josh! This made #137729 a lot simpler!

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 24, 2025
Rollup of 28 pull requests

Successful merges:

 - rust-lang/rust#132087 (Fix overly restrictive lifetime in `core::panic::Location::file` return type)
 - rust-lang/rust#137396 (Recover `param: Ty = EXPR`)
 - rust-lang/rust#137457 (Fix host code appearing in Wasm binaries)
 - rust-lang/rust#142185 (Convert moves of references to copies in ReferencePropagation)
 - rust-lang/rust#144648 (Implementation: `#[feature(nonpoison_rwlock)]`)
 - rust-lang/rust#144897 (print raw lifetime idents with r#)
 - rust-lang/rust#145218 ([Debuginfo] improve enum value formatting in LLDB for better readability)
 - rust-lang/rust#145380 (Add codegen-llvm regression tests)
 - rust-lang/rust#145573 (Add an experimental unsafe(force_target_feature) attribute.)
 - rust-lang/rust#145597 (resolve: Remove `ScopeSet::Late`)
 - rust-lang/rust#145633 (Fix some typos in LocalKey documentation)
 - rust-lang/rust#145641 (On E0277, point at type that doesn't implement bound)
 - rust-lang/rust#145669 (rustdoc-search: GUI tests check for `//` in URL)
 - rust-lang/rust#145695 (Introduce ProjectionElem::try_map.)
 - rust-lang/rust#145710 (Fix the ABI parameter inconsistency issue in debug.rs for LoongArch64)
 - rust-lang/rust#145726 (Experiment: Reborrow trait)
 - rust-lang/rust#145731 (Make raw pointers work in type-based search)
 - rust-lang/rust#145736 (triagebot: Update style team reviewers)
 - rust-lang/rust#145738 (Uplift rustc_mir_transform::coverage::counters::union_find to rustc_data_structures.)
 - rust-lang/rust#145742 (rustdoc js: Even more typechecking improvments )
 - rust-lang/rust#145743 (doc: fix some typos in comment)
 - rust-lang/rust#145745 (tests: Ignore basic-stepping.rs on LoongArch)
 - rust-lang/rust#145747 (Refactor lint buffering to avoid requiring a giant enum)
 - rust-lang/rust#145751 (fix(lexer): Allow '-' in the frontmatter infostring continue set)
 - rust-lang/rust#145761 (Add aarch64_be-unknown-hermit target)
 - rust-lang/rust#145762 (convert strings to symbols in attr diagnostics)
 - rust-lang/rust#145763 (Ship LLVM tools for the correct target when cross-compiling)
 - rust-lang/rust#145765 (Revert suggestions for missing methods in tuples)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants