Skip to content

fix(clippy): allow unused_async on the synchronous engine impls - #14

Closed
LifeInTheWeeds wants to merge 1 commit into
mainfrom
fix/clippy-unused-async-on-sync-engines
Closed

LifeInTheWeeds wants to merge 1 commit into
mainfrom
fix/clippy-unused-async-on-sync-engines

Conversation

@LifeInTheWeeds

Copy link
Copy Markdown
Collaborator

Unblocks CI for #12 and #13. Two #[allow] attributes, no behaviour change.

Problem

CI runs cargo clippy --all-targets --all-features -- -D warnings against stable, with nothing pinning the toolchain. A newer stable clippy began firing unused_async on async trait-impl methods:

error: unused `async` for async trait impl function with no `.await` statements
  --> src/engine/sqlite/mod.rs:35:5
  --> src/engine/sqlite/mod.rs:77:5
  --> src/engine/sqlite/mod.rs:152:5
  --> src/engine/duckdb/mod.rs:43:5
  --> src/engine/duckdb/mod.rs:63:5
  --> src/engine/duckdb/mod.rs:100:5
error: could not compile `plenum` (lib) due to 6 previous errors

Those 6 are exactly the 3 DatabaseEngine methods (validate_connection, introspect, execute) in the 2 engines with synchronous drivers:

Engine async fn .await Trips lint
postgres 37 92 no
mysql 33 94 no
sqlite 25 22 yes
duckdb 26 29 yes

Why #[allow] rather than removing async

DatabaseEngine declares these methods as returning futures, so every impl must match that signature whether or not its body awaits. Satisfying the lint would mean hand-rolling impl Future bodies purely to appease it — strictly worse code for no benefit.

The attributes sit on the two impl blocks with a comment explaining why, matching existing house style (the tree already carries allow(clippy::future_not_send), allow(clippy::too_many_arguments), allow(clippy::struct_excessive_bools) and others).

This is drift, not regression

Clippy passed on main in July against this same code. Nothing in the source changed; the toolchain moved underneath it. My PRs are simply the first CI runs on this repo in two months, so they surfaced it.

The root-cause fix is pinning the clippy toolchain in .github/workflows/ci.yml rather than chasing lints as they appear. That's your call and deliberately out of scope here — happy to send it as a follow-up if you want it.

Merge order

main does not currently compile against duckdb >= 1.10505.0, so clippy aborts on that error before reaching these lints. This PR cannot go green until the non_exhaustive wildcard fix (first commit of #12) lands. It is branched from main to stay single-purpose.

Suggested order:

  1. fix(duckdb): unbreak the build, make --engine duckdb reachable, and green up clippy #12 — unbreaks the build, then enables --engine duckdb
  2. fix(clippy): allow unused_async on the synchronous engine impls #14 (this one) — turns clippy green
  3. fix(mcp): return -32601 for unknown methods, and implement the spec-required ping #13 — the MCP -32601 / ping spec fix

I'm happy to rebase any of the three once the first lands.

🤖 Generated with Claude Code

https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR

CI runs `cargo clippy --all-targets --all-features -- -D warnings` against the
`stable` toolchain with nothing pinning it. A newer stable clippy began firing
`unused_async` on async trait-impl methods, producing 6 errors:

    src/engine/sqlite/mod.rs:35, 77, 152
    src/engine/duckdb/mod.rs:43, 63, 100

That is exactly the 3 `DatabaseEngine` methods (`validate_connection`,
`introspect`, `execute`) in the 2 engines whose drivers are synchronous. The
`postgres` and `mysql` impls are unaffected because they genuinely `.await`.

The `async` cannot be dropped: `DatabaseEngine` declares these methods as
returning futures, so every impl must match that signature regardless of
whether its body awaits. Satisfying the lint would mean hand-rolling
`impl Future` bodies purely to appease it, which is strictly worse code.

`#[allow(clippy::unused_async)]` on the two impl blocks, with a comment saying
why. This matches existing house style -- the tree already carries
`allow(clippy::future_not_send)`, `allow(clippy::too_many_arguments)` and
several others.

Note this is drift, not regression: clippy passed on main in July against this
same code. Pinning the clippy toolchain in CI would address the cause rather
than the symptom, but that is a maintainer call and out of scope here.

Ordering: main does not currently compile against duckdb >= 1.10505.0, so
clippy aborts on that error before it reaches these lints. This branch
therefore cannot go green until the `non_exhaustive` wildcard fix (first commit
of #12) lands. It is branched from main to stay single-purpose.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR
@LifeInTheWeeds

Copy link
Copy Markdown
Collaborator Author

Superseded by #12 — closing.

Splitting this out was the wrong call in practice. main doesn't compile against duckdb >= 1.10505.0, so clippy aborted on E0004 before ever reaching these lints, which meant this PR could never go green standalone. It's now the third commit of #12, where CI can actually verify it — and does: all 5 jobs pass.

The fix also changed while it was here, which is the other reason it belongs where it can be tested. Two earlier attempts failed:

  1. #[allow(clippy::unused_async)] on the impl block — no effect
  2. the same attribute on each function — also no effect

The real cause is that clippy 0.1.98 (Rust 1.98, 2026-09-01) split unused_async_trait_impl out of unused_async as a separate lint. The error text says so — "unused async for async trait impl function" — and I read past it twice. The landed fix allows both names.

No code is lost; the branch fix/clippy-unused-async-on-sync-engines still exists if you want the isolated diff.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant