Enable more lints by emilk · Pull Request #24066 · apache/datafusion

emilk · 2026-08-03T19:56:20Z

Which issue does this PR close?

Part of Stricter Clippy checks in CI #18467
Follow-up to chore: apply workspace lints to all crates #24076

Rationale for this change

There are a lot of lints we can enable to, which different abilities to:

Simplify the code
Write more efficient code
Catch bugs
etc

What changes are included in this PR?

One commit per new lint (maybe easiest to review commit by commit!).

I hope to add even more lints in later PRsq

Are these changes tested?

Covered by existing tests plus the clippy CI job.

Are there any user-facing changes?

No.

codecov-commenter · 2026-08-03T21:10:46Z

Codecov Report

❌ Patch coverage is 69.56522% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.04%. Comparing base (70c26a0) to head (101919d).

Files with missing lines	Patch %	Lines
datafusion/datasource-arrow/src/source.rs	50.00%	0 Missing and 4 partials ⚠️
datafusion/functions-nested/src/range.rs	50.00%	0 Missing and 2 partials ⚠️
benchmarks/src/clickbench.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24066      +/-   ##
==========================================
- Coverage   81.04%   81.04%   -0.01%     
==========================================
  Files        1105     1105              
  Lines      380163   380152      -11     
  Branches   380163   380152      -11     
==========================================
- Hits       308111   308101      -10     
- Misses      53834    53839       +5     
+ Partials    18218    18212       -6

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

2010YOUY01 · 2026-08-04T03:51:34Z

Thank you. I suggest including only the first two commits in this PR to make it easier to verify.

For any additional rules, I think we should first confirm that they are useful, so it would be better to add them in separate PRs.

emilk · 2026-08-04T07:27:16Z

I can do that - but I've tested out all these lints (and more) myself over the last couple of years in both https://github.com/rerun-io/rerun and https://github.com/emilk/egui and can vouch for their usefulness

emilk · 2026-08-04T07:31:28Z

Just the two first commits:

chore: apply workspace lints to all crates #24076

2010YOUY01 · 2026-08-04T08:35:02Z

I can do that - but I've tested out all these lints (and more) myself over the last couple of years in both https://github.com/rerun-io/rerun and https://github.com/emilk/egui and can vouch for their usefulness

Splitting them into separate PRs can make them easier to review. I suggest to do the following steps

chore: apply workspace lints to all crates #24076 LGTM, I'll merge it shortly
Open a separate PR for commit feat: enable a batch of non-default clippy lints workspace-wide
I will review the extra lint rule list later (that list is really helpful!), if there is no issue, we could do that in another PR.

## Which issue does this PR close? - Part of apache#18467 - Broken out of apache#24066 - Sibling PR: apache/arrow-rs#10533 ## Rationale for this change The workspace already has a `[workspace.lints]` table, but it was missing from three crates ## What changes are included in this PR? Inheriting the workspace lints in all crates, and fixing the resulting violations ## Are these changes tested? Yes, by existing tests and CI ## Are there any user-facing changes? No --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

All of these are already violation-free across the workspace, so this is pure future-proofing: they only fire on newly written code. They are a subset of the lint set used by https://github.com/emilk/egui, picked because they trigger no (or almost no) violations in DataFusion today. The single existing violation is a deliberate `mem::forget` in an FFI test helper, now marked with `#[expect]`. Part of apache#18467 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`HashTable::find_entry` takes `&mut self`; `find` does the same lookup through `&self`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Gating the `include_str!` docs on `cfg(doc)` means touching the included markdown no longer forces a rebuild of the crate for non-doc builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`SAFETY:` comments should sit next to the `unsafe` block they justify. The existing ones either documented safe code (reworded) or were attached to the enclosing `if` rather than the `unsafe` block (moved). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Enables the rustc lint groups and individual lints from https://github.com/emilk/egui/blob/main/Cargo.toml that DataFusion violates at most 10 times today: * `future_incompatible`, `nonstandard_style`, `rust_2018_idioms` (groups) * `rust_2021_prelude_collisions`, `semicolon_in_expressions_from_macros`, `unsafe_op_in_unsafe_fn`, `unused_extern_crates`, `unused_import_braces`, `unused_lifetimes` `elided_lifetimes_in_paths` is part of `rust_2018_idioms` but has ~800 violations, so it is explicitly allowed for now. `trivial_numeric_casts` (31 violations) and `unsafe_code` are left out entirely. The violations fixed here are vestigial `extern crate` items, redundant import braces, and two `rstest` helpers whose lifetime is unused after macro expansion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Enables the `rustdoc::all` group. Two of its lints have more than 10 violations today and are explicitly allowed for now: `missing_crate_level_docs` (18) and `unescaped_backticks` (28). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Removing it made `libc` an unused dependency (`cargo machete` CI failure): the crate has no path references, it is only linked so the `fesetround`/`fegetround` symbols declared in this module resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Keep the `LogicalPlanType::` prefix on the `CustomScan` match arm so it matches its sibling arms; drop the variant import instead. * Restore a `SAFETY:` comment on the second `unsafe` block in `cursor.rs`. * Drop `rustdoc::broken_intra_doc_links`, already covered by `rustdoc::all`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

emilk · 2026-08-05T07:09:10Z

The first two commits have been merged to main, so I have rebased this PR now!

2010YOUY01

I have went over the extra rule list, and find most of them useful.

I suggest to:

Remove one rule (see below)
Remove non-Clippy lint changes (see comments)

Reference: https://rust-lang.github.io/rust-clippy/master/

Suggest not to enforce

unnecessary_safety_comment: now we informally use // safety: xxx comment for internal methods that require extra cares, this clippy rule forbids such convention and only allow such comment in Rust unsafes, I think this is not necessary.

…lints * Drop lints that clippy already enables by default (`char_lit_as_u8`, `empty_line_after_outer_attr`, `if_let_mutex`, `manual_ok_or`, `missing_enforced_import_renames`, `suspicious_command_arg_space`, `useless_transmute`). * Drop `unnecessary_safety_comment`: it conflicts with the existing convention of using `// SAFETY:` comments on safe-but-delicate internal methods. * Move the `[workspace.lints.rust]` and `[workspace.lints.rustdoc]` sets out to follow-up PRs to keep this one reviewable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

New code from `main` trips `clippy::pathbuf_init_then_push`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

2010YOUY01 · 2026-08-08T03:14:34Z

Thanks again, I have manually check the key changes, and let Codex checked code changes to ensure they're all related and safe.

I think we also need to remove mutex_integer clippy rule, it's overall controversial, and its own doc's # problem section has explained it:

After that this PR should be good to go

github-actions Bot added proto Related to proto crate functions Changes to functions implementation datasource Changes to the datasource crate ffi Changes to the ffi crate physical-plan Changes to the physical-plan crate labels Aug 3, 2026

emilk marked this pull request as ready for review August 3, 2026 19:59

emilk mentioned this pull request Aug 3, 2026

Rerun PRs rerun-io/opensource#2

Open

github-actions Bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate common Related to common crate optimizer Optimizer rules spark labels Aug 3, 2026

emilk mentioned this pull request Aug 4, 2026

chore: apply workspace lints to all crates #24076

Merged

emilk marked this pull request as draft August 4, 2026 07:31

emilk and others added 9 commits August 5, 2026 09:06

feat: enable clippy::manual_instant_elapsed

f585f00

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

feat: enable clippy::pathbuf_init_then_push

5ebd57d

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

feat: enable clippy::ref_option_ref

5c2e60b

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

feat: enable clippy::debug_assert_with_mut_call

2435d53

`HashTable::find_entry` takes `&mut self`; `find` does the same lookup through `&self`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

feat: enable clippy::str_split_at_newline

bc5dd6d

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

feat: enable clippy::flat_map_option

7649414

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

feat: enable clippy::verbose_file_reads

a7cc222

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

feat: enable clippy::doc_include_without_cfg

d87454f

Gating the `include_str!` docs on `cfg(doc)` means touching the included markdown no longer forces a rebuild of the crate for non-doc builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

emilk and others added 5 commits August 5, 2026 09:06

emilk force-pushed the emilk/workspace-lints branch from c159311 to d3e857a Compare August 5, 2026 07:06

emilk changed the title ~~chore: introduce a broader set of workspace clippy lints~~ Enable more lints Aug 5, 2026

emilk marked this pull request as ready for review August 5, 2026 07:08

2010YOUY01 reviewed Aug 6, 2026

View reviewed changes

Comment thread Cargo.toml

Comment thread Cargo.toml

Comment thread Cargo.toml Outdated

emilk and others added 3 commits August 6, 2026 12:15

Merge remote-tracking branch 'upstream/main' into emilk/workspace-lints

4ded53a

fix: use Path::join in get_query_path

101919d

New code from `main` trips `clippy::pathbuf_init_then_push`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions Bot removed optimizer Optimizer rules spark labels Aug 6, 2026

emilk requested a review from 2010YOUY01 August 6, 2026 10:41

Navigation Menu

Uh oh!

Enable more lints - #24066

Enable more lints#24066
emilk wants to merge 17 commits into
apache:mainfrom
emilk:emilk/workspace-lints

emilk commented Aug 3, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Aug 3, 2026 •

edited

Loading

Uh oh!

2010YOUY01 commented Aug 4, 2026

Uh oh!

emilk commented Aug 4, 2026

Uh oh!

emilk commented Aug 4, 2026

Uh oh!

2010YOUY01 commented Aug 4, 2026

Uh oh!

emilk commented Aug 5, 2026

Uh oh!

2010YOUY01 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

2010YOUY01 commented Aug 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

emilk commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

2010YOUY01 commented Aug 4, 2026

Uh oh!

emilk commented Aug 4, 2026

Uh oh!

emilk commented Aug 4, 2026

Uh oh!

2010YOUY01 commented Aug 4, 2026

Uh oh!

emilk commented Aug 5, 2026

Uh oh!

2010YOUY01 left a comment

Choose a reason for hiding this comment

Suggest not to enforce

Uh oh!

Uh oh!

Uh oh!

Uh oh!

2010YOUY01 commented Aug 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emilk commented Aug 3, 2026 •

edited

Loading

codecov-commenter commented Aug 3, 2026 •

edited

Loading