Skip to content

Cargo: Enable most pedantic Clippy lints + some from nursery & restriction - #23095

Open
paldepind wants to merge 62 commits into
mainfrom
rust/more-lint
Open

paldepind wants to merge 62 commits into
mainfrom
rust/more-lint

Conversation

@paldepind

@paldepind paldepind commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

This PR enables most lints in Clippy's pedantic group, similar to what we used to have for the C/C++ BMN setup. Compared to that I've disable a few more rules, since they provided false positives or results of dubious value.

All the lint errors have been fixed using Clippy's automatic --fix option or by Copilot. I've vetted each rule that required changes and gone through all the changes. Combined with #23053 we've now fixed 207 lint errors! Of course many of these are very tiny improvements, but almost all of them makes the code more consistent, idiomatic, performant, clear/explicit, robust, etc. So I think this is overall a really nice improvement.

I did a commit for each lint rule that was addressed. This makes it easy to see why something was changed, but make per-commit review rather tedious. The changes here are rather simple and of a if-it-still-compiles-it-still-works kind. So I think a very light review of the changes is sufficient. I've started DCA for Rust, Ruby and Unified.

paldepind and others added 11 commits November 11, 2026 11:48
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Active critical build and compilation failures remain, including unresolved Bazel dependencies and an invalid String collection, along with a breaking public API change.

5 open issues
What changed in this PR

This PR enables most Clippy pedantic lints across the Rust workspace, applies the resulting idiomatic refactors, and replaces direct lazy_static usage with std::sync::LazyLock.

Changes:

  • Enables workspace-wide Clippy pedantic lints with targeted exceptions.
  • Applies refactors across extractors, Yeast, generators, tests, and supporting utilities.
  • Updates Cargo and Bazel dependency metadata.
File Description
unified/​swift-syntax-rs/​src/​main.rs Simplifies source input selection.
unified/​swift-syntax-rs/​src/​lib.rs Updates test string handling and lint allowances.
unified/​extractor/​tests/​corpus_tests.rs Simplifies environment parsing and assertions.
unified/​extractor/​src/​languages/​swift/​swift.rs Adds a lint allowance and simplifies iteration.
unified/​extractor/​Cargo.toml Removes the direct lazy_static dependency.
shared/​yeast/​tests/​test.rs Simplifies helper APIs and raw string literals.
shared/​yeast/​src/​range.rs Adds must_use annotations.
shared/​yeast/​src/​query.rs Simplifies pattern matching and copying.
shared/​yeast/​src/​lib.rs Applies API, formatting, and ownership refactors.
shared/​yeast/​src/​dump.rs Updates documentation and lint allowances.
shared/​yeast/​src/​captures.rs Simplifies option handling.
shared/​yeast/​src/​bin/​node_types_yaml.rs Simplifies input selection.
shared/​yeast-schema/​src/​node_types_yaml.rs Refactors field conversion and test literals.
shared/​yeast-macros/​src/​template_parse.rs Avoids unnecessary group moves.
shared/​yeast-macros/​src/​lower.rs Passes token streams by reference and adds a lint allowance.
shared/​yeast-macros/​src/​ast.rs Simplifies a test raw string literal.
shared/​tree-sitter-extractor/​tests/​multiple_languages.rs Updates renamed test fixture fields.
shared/​tree-sitter-extractor/​tests/​integration_test.rs Updates renamed test fixture fields.
shared/​tree-sitter-extractor/​tests/​common/​mod.rs Renames the source archive fixture field.
shared/​tree-sitter-extractor/​src/​trap.rs Simplifies formatting and writes output incrementally.
shared/​tree-sitter-extractor/​src/​options.rs Simplifies error formatting.
shared/​tree-sitter-extractor/​src/​node_types.rs Refactors ownership and string construction.
shared/​tree-sitter-extractor/​src/​generator/​ql.rs Simplifies generated output formatting.
shared/​tree-sitter-extractor/​src/​generator/​ql_gen.rs Simplifies matching and adds lint allowances.
shared/​tree-sitter-extractor/​src/​generator/​mod.rs Simplifies generated name construction and borrowing.
shared/​tree-sitter-extractor/​src/​file_paths.rs Simplifies path handling and formatting.
shared/​tree-sitter-extractor/​src/​extractor/​mod.rs Simplifies verbosity handling and control flow.
shared/​tree-sitter-extractor/​src/​extractor/​driver.rs Adds a lint allowance and improves path formatting.
shared/​tree-sitter-extractor/​src/​diagnostics.rs Simplifies string cloning and defaults.
shared/​tree-sitter-extractor/​Cargo.toml Removes the direct lazy_static dependency.
rust/​extractor/​src/​util.rs Changes option comparison inputs to borrowed values.
rust/​extractor/​src/​translate/​base.rs Makes statement-style control flow explicit.
rust/​extractor/​src/​rust_analyzer.rs Moves local constants near their uses.
rust/​extractor/​src/​main.rs Simplifies option handling and adds a lint allowance.
rust/​extractor/​src/​diagnostics.rs Simplifies default string construction.
rust/​extractor/​src/​crate_graph.rs Simplifies iterator filtering and comparison.
rust/​extractor/​src/​configuration.rs Adapts callers to the updated option comparator.
rust/​extractor/​src/​config/​deserialize.rs Avoids an unnecessary string clone.
rust/​extractor/​src/​config.rs Simplifies the separator literal.
rust/​ast-generator/​src/​main.rs Simplifies grouped matches and borrowing.
rust/​ast-generator/​patches/​rust-analyzer.patch Applies equivalent Clippy-driven changes to the patched generator.
ruby/​extractor/​src/​main.rs Passes extractor options by reference.
ruby/​extractor/​src/​extractor.rs Replaces lazy_static with LazyLock and simplifies ownership.
ruby/​extractor/​Cargo.toml Removes the direct lazy_static dependency.
ql/​buramu/​src/​main.rs Replaces lazy_static with LazyLock.
ql/​buramu/​Cargo.toml Removes the direct lazy_static dependency.
MODULE.bazel Updates exported Rust workspace dependencies.
misc/​bazel/​3rdparty/​rust_workspace_deps/​lazy_static/​BUILD.bazel Removes the obsolete dependency alias.
misc/​bazel/​3rdparty/​rust_workspace_deps/​lazy_static-1.5.1/​BUILD.bazel Removes the obsolete versioned alias.
misc/​bazel/​3rdparty/​rust_workspace_deps/​crates.bzl Updates generated dependency mappings.
misc/​bazel/​3rdparty/​rust_workspace_deps/​BUILD.bazel Removes obsolete lazy_static aliases.
cpp/​extractor/​bmn/​src/​package_database.rs Improves path error formatting.
cpp/​extractor/​bmn/​src/​main.rs Improves executable path error formatting.
cpp/​extractor/​bmn/​src/​gnu_compiler_default_include_finder.rs Updates compiler error formatting and tests.
cpp/​extractor/​bmn/​src/​file_filter.rs Improves path error formatting.
cpp/​extractor/​bmn/​src/​dependency_installation.rs Improves command failure formatting.
cpp/​extractor/​bmn/​src/​compiler.rs Improves compiler error formatting.
clippy.toml Adds documentation identifier configuration.
Cargo.toml Enables workspace-wide pedantic Clippy lints and exceptions.
Cargo.lock Removes direct lazy_static dependency entries from workspace packages.

Review effort: Lite (auto)

Comment thread MODULE.bazel Resolved
Comment thread shared/yeast/src/lib.rs Resolved
Comment thread clippy.toml Resolved
paldepind and others added 11 commits November 11, 2026 12:28
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e4429304-eb0c-4b7d-9675-11e1eabfd0b8
@paldepind
paldepind requested a review from a team as a code owner November 11, 2026 11:44

@asgerf asgerf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in unified LGTM

paldepind and others added 11 commits November 13, 2026 10:30
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
…ets`

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Enable additional lints. These have no results in our code but are
suggested in: https://microsoft.github.io/rust-guidelines/guidelines/universal/index.html#clippy-lints
@paldepind

Copy link
Copy Markdown
Contributor Author

I've enabled some more lints from the nursery and restriction group. These where suggested in https://microsoft.github.io/rust-guidelines/guidelines/universal/index.html#clippy-lints.

@paldepind paldepind changed the title Cargo: Enable most pedantic Clippy lints Cargo: Enable most pedantic Clippy lints + some from nursery & restriction Nov 13, 2026
paldepind and others added 6 commits November 13, 2026 11:47
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8f8a55e2-1e40-48cd-9120-9de95e969ee9

@tausbn tausbn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of these look good to me, but a bunch of them are (in my opinion) somewhat questionable.

Also, a meta-comment: perhaps in the future try to split this up so it isn't a single 56-commit PR. That way, the unobjectionable parts can be merged, while the more dubious lints can be discussed further. This was a somewhat exhausting PR to review.

for component in components {
match component {
std::path::Component::CurDir => continue,
std::path::Component::CurDir => {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit ambivalent about this one. Yes, for the current incarnation of the code (where there's nothing following the match) the continue is indeed superfluous, but if we were to add code after the match, this would no longer be true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could go both ways couldn't it? We might add code after the match that should apply to all cases and in that case the {} would be what we want.

I think the change makes things clearer: the control flow here is in fact simple normal evaluation and including continue gives the indication that that's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, let's keep it in.

Comment thread rust/ast-generator/src/main.rs Resolved
Comment thread shared/yeast/src/query.rs Outdated
match self {
QueryNode::Node { kind, .. } => Some(kind),
QueryNode::UnnamedNode { kind } => Some(kind),
QueryNode::Node { kind, .. } | QueryNode::UnnamedNode { kind } => Some(kind),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another one where I don't feel like the "improvement" outweighs the fact that it makes the code harder to read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now reverted and match_same_arms is disabled.


impl AstNode for ExtraNode {
fn kind(&self) -> &str {
fn kind(&self) -> &'static str {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the previous version was simpler. Less precise, sure, but simpler.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this one because the type makes it immediately clear that kind is not some dynamic thing that is constructed on the fly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is of course also readily apparent from the implementation, so I'm guessing the situation you have in mind is one where you're looking at some call to kind on an ExtraNode and you inspect the type to see that it's static. I think I would just be more confused by this (since kind is not in general static on AstNode itself).

To be honest, I'm maybe a 3/10 on the sliding scale, so if you prefer it, I'm fine with leaving it in. 🙂

Comment thread shared/yeast/tests/test.rs Resolved
// mustache is not very good at avoiding blank lines
// adopting the workaround from https://github.com/groue/GRMustache/issues/46#issuecomment-19498046
s.split("\n")
s.split('\n')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a marginal improvement at best. "Pedantic" is the right word for this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is very minor.

pub root_dir: PathBuf,
pub file_list: PathBuf,
pub source_archive_dir: PathBuf,
pub archive_dir: PathBuf,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not entirely clear to me why this rewrite is better. Is it because it's superfluous to have a field starting with source when the struct is called SourceArchive? In that case, shouldn't it go all the way to calling the field dir?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the lint to prevent struct where the field names repeat the struct name. But I also felt that just dir was too ambiguous >.<. Probably better to just revert this and disable the rule which I've done.

}
index += 1;

const CODING: [char; 12] = ['C', 'c', 'O', 'o', 'D', 'd', 'I', 'i', 'N', 'n', 'G', 'g'];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this ~20 lines up seems like a net loss for readability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it doesn't help much here. But some of the other instances the rule found where pretty good. Do you think we should add an allow, accept this, or disable the rule entirely?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case, probably a better solution would be to refactor the code (so that CODING can be right next to its code in that helper function), so I say let's just keep the change as-is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the best solution. I've done that in 2fa146b.

@paldepind

Copy link
Copy Markdown
Contributor Author

Also, a meta-comment: perhaps in the future try to split this up so it isn't a single 56-commit PR. That way, the unobjectionable parts can be merged, while the more dubious lints can be discussed further. This was a somewhat exhausting PR to review.

You're right. I didn't expect this to be as big going in and then I just kept going. But I should've stopped and split this across some PRs. Thanks for pushing through with the review 🙏

I've disabled two of the lints and fixed the things you spotted. Let me know if there's any of the comments where you feel further lints should be undone?

@tausbn tausbn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've addressed all of my comments. ❤️

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ QL-for-QL Ruby Rust Pull requests that update Rust code Unified

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants