Skip to content

Conversation

welf
Copy link

@welf welf commented Oct 13, 2025

Fixes #1004

In Nightly builds both cycle_initial and recover_from_cycle generate unnecessary parentheses warnings when using #[salsa::tracked] macro and the tracked function doesn't use any additional inputs:

= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#double_parens
= note: this error originates in the macro salsa::plumbing::unexpected_cycle_recovery which comes from the expansion of the attribute macro salsa::tracked (in Nightly builds, run with -Z macro-backtrace for more info)
= note: this error originates in the macro salsa::plumbing::unexpected_cycle_initial which comes from the expansion of the attribute macro salsa::tracked (in Nightly builds, run with -Z macro-backtrace for more info)

The Problem:

In components/salsa-macro-rules/src/unexpected_cycle_recovery.rs, both macros use this pattern: std::mem::drop(($($other_inputs,)*)); that creates a tuple containing all $other_inputs and drops them.

These macros are invoked from components/salsa-macro-rules/src/setup_tracked_fn.rs:305-316:

  • cycle_initial is called with: (db, $($input_id),*)
  • recover_from_cycle is called with: (db, value, count, $($input_id),*)

When $other_inputs is empty (function has no additional inputs beyond db), the expression ($($other_inputs,)*) expands to () (an empty tuple), resulting in: std::mem::drop(()) and it triggers clippy warnings.

The Fix:

Instead of creating a tuple and dropping it, we should repeat dropping each input individually:

  • instead of std::mem::drop(($($other_inputs,)*));
  • use $(std::mem::drop($other_inputs);)*

Copy link

netlify bot commented Oct 13, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit c280c5e
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/68ef51b057b3680008e116ff

Copy link

codspeed-hq bot commented Oct 13, 2025

CodSpeed Performance Report

Merging #1005 will degrade performances by 6.42%

Comparing welf:fix-clippy-double-parens-warnings (c280c5e) with master (8b0831f)

Summary

⚡ 1 improvement
❌ 1 regression
✅ 11 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
amortized[Input] 2.1 µs 2.3 µs -6.42%
amortized[SupertypeInput] 3 µs 2.9 µs +4.07%

@MichaReiser
Copy link
Contributor

MichaReiser commented Oct 14, 2025

Would you mind adding a test that triggers the problematic behavior (and wasn't fixed by #1001)

@welf
Copy link
Author

welf commented Oct 14, 2025

Would you mind adding a test that triggers the problematic behavior (and wasn't fixed by #1001)

I've encountered the problems in a project running on a nightly Rust after I've updated the toolchain to rustc 1.92.0-nightly (2300c2aef 2025-10-12). Nightly builds have more aggressive clippy lints and better diagnostics that can detect issues in macro-generated code but we don't see them in salsa when running on stable Rust.

So, I've created a basic regression test with tracked functions that have no additional inputs beyond db. These functions would trigger the problematic code path in the macros. The functions are located in tests/clippy_double_parens_regression.rs.

Then I've added a compile-time verification test in tests/verify_no_double_parens.rs that:

  • Runs cargo expand on the regression test
  • Parses the expanded macro output
  • Filters out comments to avoid false positives
  • Checks for the problematic std::mem::drop(()) pattern
  • Fails the test if the pattern is found in actual code

This test fails in the main branch and pass in this branch. Run cargo test --test verify_no_double_parens

P.S.: I don't know why adding tests affects performance benchmarks ;(

P.P.S.: Miri test fails because of running cargo expand external command by I have no other ideas how to test that macro expands to the problematic pattern:

image

@MichaReiser
Copy link
Contributor

MichaReiser commented Oct 15, 2025

I'm not sure if I'm doing something wrong but I don't get the double_parens warning on master when running

cargo +nightly clippy --test double_parens -- -W clippy::double_parens

where double_parens is the test you added

See #1006

@welf
Copy link
Author

welf commented Oct 15, 2025

I'm not sure if I'm doing something wrong but I don't get the double_parens warning on master when running

cargo +nightly clippy --test double_parens -- -W clippy::double_parens
image

rustc 1.92.0-nightly (4b94758d2 2025-10-13)
binary: rustc
commit-hash: 4b94758d2ba7d0ef71ccf5fde29ce4bc5d6fe2a4
commit-date: 2025-10-13
host: aarch64-apple-darwin
release: 1.92.0-nightly
LLVM version: 21.1.3

clippy 0.1.92 (4b94758d2b 2025-10-13)

@MichaReiser
Copy link
Contributor

Yeah not sure. But I'd prefer if we get #1006 to fail before fixing it.

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.

cycle_initial and recover_from_cycle macros generate clippy::double_parens warnings on #[salsa::tracked] macro use

2 participants