Enable more clippy lints - #24322
Open
emilk wants to merge 40 commits into
Open
Conversation
github-actions
Bot
added
sql
emilk
commented
Aug 13, 2026
emilk
commented
Aug 13, 2026
emilk
commented
Aug 13, 2026
All of these are `allow` by default and currently have zero hits across the workspace (`--all-targets --all-features`), so they act purely as guards against future regressions: * Bug catchers: `same_functions_in_if_condition`, `self_only_used_in_recursion`, `unchecked_time_subtraction`, `expl_impl_clone_on_copy`, `into_iter_without_iter`, `iter_without_into_iter`, `unnecessary_safety_doc` * Performance: `large_stack_arrays`, `large_stack_frames`, `linkedlist`, `set_contains_or_insert`, `string_lit_chars_any` * Simplification / API hygiene: `empty_enums`, `fn_params_excessive_bools`, `iter_not_returning_iterator`, `non_std_lazy_statics`, `ptr_cast_constness`, `pub_without_shorthand`, `trait_duplication_in_bounds` Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `.peekable()` in the sqllogictest Postgres engine was never peeked, so it only added an extra layer of indirection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches `From` implementations that can panic, where `TryFrom` would be the honest signature. All three existing hits would need a breaking API change to fix, so they get `#[expect]` for now. Two of them (`Constraint`) panic on a protobuf message with an unset `constraint_mode`, i.e. on malformed input. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches exact float comparisons against constants, e.g. `x == 0.0`. The two existing hits in `value_transition!` really do want an exact comparison against `f32::MIN`/`MAX`, so they get `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches float literals that silently round, e.g. `let x: f32 = 0.1234567890123;`. The three existing hits are false positives: they spell out exact powers of two (2^64 and 2^64-2^41), which float `Display` renders with fewer digits, so the lint thinks precision was lost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`(a + b) / 2` overflows when `a + b` exceeds the type's range; `a.midpoint(b)` does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches strings that contain `{...}` but are never actually formatted.
This found two real bugs where the placeholder was silently printed
verbatim:
* `benchmarks/src/nlj.rs`: `"NLJ benchmark Q{query_id} failed…".to_string()`
* `parquet_advanced_index.rs`: `.expect("metadata for file not found: {filename}")`
The remaining hits are intentional: shell-style `${VAR:-default}`
placeholders, `{rows}`-style templates substituted with `str::replace`,
and braces inside expected struct output. Those get `#[expect]`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`&x as *const T` silently picks a pointer type; `std::ptr::from_ref(&x)` keeps the referent type explicit and cannot accidentally change it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches `// SAFETY:` comments that do not sit in front of anything unsafe, so that a `SAFETY:` comment reliably means "an unsafe block follows, and here is why it is sound". * Three comments documented an `unwrap` or a safe copy rather than unsafe code, so they lose the `SAFETY:` prefix. * Three sat in front of an `if` while the `unsafe` block was inside it, so they move next to the block they justify. * Two were prose false positives, where the lint matched "safety:" in the middle of a doc comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches large types passed by value, which forces a memcpy at every call. The single existing hit is `HyperLogLog::new_with_registers`, whose 16 KiB array is moved into the returned struct, so a reference would only add a copy. It gets `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches `-> Box<T>` where the caller gains nothing from the indirection. The single existing hit is a test helper that both takes and returns `Box<Expr>` so it can hand back the same allocation, so it gets `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`"foo".as_bytes()` goes through a UTF-8 str; `b"foo"` is already the byte literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`{:?}` on a `Path`/`PathBuf` prints it quoted and with escapes, which is
not what these user-facing messages want. `{}` on `.display()` is also
one less formatting layer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Option<Option<T>>` usually means a plain `Option` or a small enum would be clearer. All three existing hits use the nesting deliberately (unset vs. set-to-none, null list vs. null element, not-a-literal vs. literal-without-span), so they get `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A function that takes a single `Option<T>` only to `map` over it is easier to reuse if it takes `T` and the caller does the mapping. All five hits are fixed that way rather than suppressed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`#[ignore]` with no reason leaves the next reader guessing whether the test is broken, slow, or obsolete. The reasons for the six existing ones were already nearby, in a comment or in the test body, so they move into the attribute. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches code duplicated at the start or end of every branch of an `if`/`else`, which can be hoisted out. All eight hits are hoisted rather than suppressed. Three of them touch behaviour-sensitive code (the sort spill path, the StringView case conversion, and the array-map join probe), so the tests for the affected crates were run: 3495 tests, all passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Catches a binding whose type annotation forces a cast that a better annotation would remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`n <= i32::MAX as usize` restates what `i32::try_from(n).is_ok()` says directly, and the cast form is easy to get wrong for signed types. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`.filter_map(f).next()` is `.find_map(f)`, which stops at the first hit without building the intermediate adapter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Enforces uniform digit grouping in long literals, where an odd group is usually a typo. The three existing hits are deliberate: the grouping spells out the decimal scale, so `180_00000000` reads as 180 with scale 8. They get `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`opt.map(f).unwrap_or_default()` and `opt.filter(f).is_some()` are both `opt.is_some_and(f)`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Duration::from_secs(60)` says "60 seconds" where the code means one minute. `from_mins(1)` / `from_hours(..)` say it directly. Both constructors are stable since Rust 1.91, below our 1.94 MSRV. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A named lifetime that is used only once carries no information; `'_` makes it obvious that nothing is being tied together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Initializing fields in declaration order makes a struct literal easy to check against the definition, and makes it obvious when a field is missing. All hits are pure reorderings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`if cond { 1 } else { 0 }` is `T::from(cond)`, which cannot get the two
branches the wrong way round.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`T { ..Default::default() }` and `Self { ..self.clone() }` are just
`T::default()` and `self.clone()`.
Where clippy suggested a bare `Default::default()` the concrete type name
is kept, since it is what tells the reader what is being built.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`if !cond { panic!(msg) }` is `assert!(cond, msg)`, which states the
invariant instead of its negation.
One of clippy's rewrites produced a double negative
(`!...is_none()`); that one is written as `.is_some()` instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A `..` in a pattern that already binds every field does nothing today, but silently swallows any field added later. Removing it turns that into a compile error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`if let Some(true) = x` reads as a binding but is really an equality check; `x == Some(true)` (or `matches!`) says so. Clippy suggested one tuple comparison, `(is_valid, is_included) == (true, Some(true))`; that one is written as a plain `&&` instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`x.deref()` and `x.deref_mut()` are the operator spelled the long way; `&*x` / `&mut *x` is the idiomatic form and does not need `Deref` in scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit c7e25ff.
Addresses review feedback on the `equatable_if_let` commit: clippy suggested `matches!` in a few places where a plain equality check reads better. `predicate_bounds.rs` uses `.ok() == Some(false)` because `DataFusionError` does not implement `PartialEq`, so `== Ok(false)` does not compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit d82dc41.
This reverts commit 36abeb7.
This reverts commit 85c06c6.
* `test_parse_duration_with_overflow_check` uses `Duration::from_mins` for the `"…m"` input again, so the constructor mirrors the unit suffix in the string being parsed. That trips `duration_suboptimal_units`, so the test gets an `#[expect]` saying why. * Restore the `TODO`/`Issue` comments above the ignored `sort_with_mem_limit_2_cols_2` test, keeping a short `#[ignore]` reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24322 +/- ##
==========================================
- Coverage 81.14% 81.13% -0.01%
==========================================
Files 1112 1112
Lines 386933 386864 -69
Branches 386933 386864 -69
==========================================
- Hits 313967 313892 -75
+ Misses 54476 54475 -1
- Partials 18490 18497 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`ab12f5e4b` ("fix(ffi): preserve TableProvider DML overrides") landed on
main after this branch was measured and added a new
`[x].into_iter()`, which the `iter_on_single_items` lint enabled here
rejects. CI builds the merge commit, so it failed there but not locally.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
emilk
force-pushed
the
emilk/more-clippy-lints
branch
from
August 13, 2026 12:16
10b33a9 to
1755ad1
Compare
emilk
marked this pull request as ready for review
August 13, 2026 12:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Clippychecks in CI #18467Rationale for this change
More
allow-by-default clippy lints that simplify code, catch bugs, or improve performance.Every lint here was verified to be outside the default-warn groups, so none is a no-op.
What changes are included in this PR?
One commit per lint, including its fixes, so any single lint can be reverted on its own.
The first commit is the exception: 19 lints that had zero hits and need no code changes.
Two real bugs fell out of
literal_string_with_formatting_args, where a{placeholder}wasprinted verbatim instead of interpolated:
benchmarks/src/nlj.rs:"NLJ benchmark Q{query_id} failed…".to_string()parquet_advanced_index.rs:.expect("metadata for file not found: {filename}")fallible_impl_fromalso flagged thatFrom<protobuf::Constraint> for Constraintpanics on amessage with an unset
constraint_mode. Fixing that needs a breaking change toTryFrom, so itis only marked with
#[expect]here.Are these changes tested?
Covered by existing tests plus the clippy CI job. I also ran the extended test suite locally.
Are there any user-facing changes?
One non-breaking signature change:
format_human_displayand a few private helpers now takeTinstead of
Option<T>(clippy::single_option_map). No public API changes.