From a0a997bfd06977e89737a5705001b103f8bfcdc1 Mon Sep 17 00:00:00 2001 From: Arthur Welf Date: Mon, 13 Oct 2025 23:33:36 +0200 Subject: [PATCH 1/4] Fix clippy::double_parens warnings in unexpected_cycle_recovery and unexpected_cycle_initial macros --- components/salsa-macro-rules/src/unexpected_cycle_recovery.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/salsa-macro-rules/src/unexpected_cycle_recovery.rs b/components/salsa-macro-rules/src/unexpected_cycle_recovery.rs index 8d56d54f3..4df525e22 100644 --- a/components/salsa-macro-rules/src/unexpected_cycle_recovery.rs +++ b/components/salsa-macro-rules/src/unexpected_cycle_recovery.rs @@ -5,7 +5,7 @@ macro_rules! unexpected_cycle_recovery { ($db:ident, $value:ident, $count:ident, $($other_inputs:ident),*) => {{ std::mem::drop($db); - std::mem::drop(($($other_inputs,)*)); + $(std::mem::drop($other_inputs);)* panic!("cannot recover from cycle") }}; } @@ -14,7 +14,7 @@ macro_rules! unexpected_cycle_recovery { macro_rules! unexpected_cycle_initial { ($db:ident, $($other_inputs:ident),*) => {{ std::mem::drop($db); - std::mem::drop(($($other_inputs,)*)); + $(std::mem::drop($other_inputs);)* panic!("no cycle initial value") }}; } From 25200fcfa148014dc327d62a30f2800444060e94 Mon Sep 17 00:00:00 2001 From: Arthur Welf Date: Tue, 14 Oct 2025 21:47:21 +0200 Subject: [PATCH 2/4] Add tests to verify problematic behavior --- tests/clippy_double_parens_regression.rs | 49 ++++++++++++++++++++++++ tests/verify_no_double_parens.rs | 37 ++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 tests/clippy_double_parens_regression.rs create mode 100644 tests/verify_no_double_parens.rs diff --git a/tests/clippy_double_parens_regression.rs b/tests/clippy_double_parens_regression.rs new file mode 100644 index 000000000..cbfd54294 --- /dev/null +++ b/tests/clippy_double_parens_regression.rs @@ -0,0 +1,49 @@ +//! Regression test for clippy::double_parens warnings in cycle macros. +//! +//! This test ensures that tracked functions with no additional inputs (beyond `db`) +//! don't trigger clippy::double_parens warnings when using cycle_initial or cycle_fn. +//! +//! Before the fix in components/salsa-macro-rules/src/unexpected_cycle_recovery.rs, +//! these macros would generate `std::mem::drop(())` which triggered the warning. +//! +//! Run `cargo test --test verify_no_double_parens` to verify the fix. +//! +//! See: https://github.com/salsa-rs/salsa/issues/1004 + +// This tracked function has no additional inputs beyond `db`. +// With the old code, this would trigger clippy::double_parens warnings in the +// generated `unexpected_cycle_initial` and `unexpected_cycle_recovery` macros. +#[salsa::tracked] +fn simple_tracked_query(_db: &dyn salsa::Database) -> u32 { + 100 +} + +// Tracked function with cycle recovery and no additional inputs. +// The cycle_initial and cycle_fn functions also have no additional inputs beyond `db`, +// which would trigger the clippy warning with the old code. +#[salsa::tracked(cycle_fn=cycle_recover, cycle_initial=initial)] +fn query_with_cycle_support(_db: &dyn salsa::Database) -> u32 { + 200 +} + +fn initial(_db: &dyn salsa::Database) -> u32 { + 0 +} + +fn cycle_recover( + _db: &dyn salsa::Database, + value: &u32, + _count: u32, +) -> salsa::CycleRecoveryAction { + // Just return the value to avoid actual cycling in this test + salsa::CycleRecoveryAction::Fallback(*value) +} + +#[test_log::test] +fn test_no_clippy_warnings_for_no_input_functions() { + let db = salsa::DatabaseImpl::default(); + + // These functions should compile without clippy::double_parens warnings + assert_eq!(simple_tracked_query(&db), 100); + assert_eq!(query_with_cycle_support(&db), 200); +} diff --git a/tests/verify_no_double_parens.rs b/tests/verify_no_double_parens.rs new file mode 100644 index 000000000..0d82f482c --- /dev/null +++ b/tests/verify_no_double_parens.rs @@ -0,0 +1,37 @@ +//! Compile-time verification test for clippy::double_parens fix. +//! +//! This test verifies that the expanded macros don't contain the problematic +//! std::mem::drop(()) pattern that triggers clippy warnings. + +use std::process::Command; + +#[test] +fn verify_expanded_code_has_no_double_parens() { + let output = Command::new("cargo") + .args(["expand", "--test", "clippy_double_parens_regression"]) + .output() + .expect("Failed to execute cargo expand"); + + let expanded = String::from_utf8_lossy(&output.stdout); + + // Filter out lines that are comments or documentation to avoid false positives + let code_lines: Vec<&str> = expanded + .lines() + .filter(|line| { + let trimmed = line.trim(); + !trimmed.starts_with("//") && !trimmed.starts_with("///") + }) + .collect(); + + let code_only = code_lines.join("\n"); + + // Check for the problematic pattern in the code (ignore comments) + if code_only.contains("std::mem::drop(())") { + panic!( + "Found std::mem::drop(()) in expanded code.\n\ + This pattern triggers clippy::double_parens warnings.\n\ + The macro should use $(std::mem::drop($other_inputs);)* \ + instead of std::mem::drop(($($other_inputs,)*))" + ); + } +} From d5275aea672269b11c108a56c46f1eff6448eaaf Mon Sep 17 00:00:00 2001 From: Arthur Welf Date: Tue, 14 Oct 2025 22:27:24 +0200 Subject: [PATCH 3/4] Add inventory gate to tracked functions test --- tests/clippy_double_parens_regression.rs | 1 + tests/verify_no_double_parens.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/clippy_double_parens_regression.rs b/tests/clippy_double_parens_regression.rs index cbfd54294..8e2b58234 100644 --- a/tests/clippy_double_parens_regression.rs +++ b/tests/clippy_double_parens_regression.rs @@ -9,6 +9,7 @@ //! Run `cargo test --test verify_no_double_parens` to verify the fix. //! //! See: https://github.com/salsa-rs/salsa/issues/1004 +#![cfg(feature = "inventory")] // This tracked function has no additional inputs beyond `db`. // With the old code, this would trigger clippy::double_parens warnings in the diff --git a/tests/verify_no_double_parens.rs b/tests/verify_no_double_parens.rs index 0d82f482c..332bd3605 100644 --- a/tests/verify_no_double_parens.rs +++ b/tests/verify_no_double_parens.rs @@ -2,6 +2,7 @@ //! //! This test verifies that the expanded macros don't contain the problematic //! std::mem::drop(()) pattern that triggers clippy warnings. +#![cfg(feature = "inventory")] use std::process::Command; From c280c5ea7b2bb001f0fe9575f0e10a72eb9968b2 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 15 Oct 2025 09:47:38 +0200 Subject: [PATCH 4/4] Make the test a warnings test --- tests/verify_no_double_parens.rs | 38 ------------------- .../double_parens.rs} | 3 -- tests/warnings/main.rs | 1 + 3 files changed, 1 insertion(+), 41 deletions(-) delete mode 100644 tests/verify_no_double_parens.rs rename tests/{clippy_double_parens_regression.rs => warnings/double_parens.rs} (94%) diff --git a/tests/verify_no_double_parens.rs b/tests/verify_no_double_parens.rs deleted file mode 100644 index 332bd3605..000000000 --- a/tests/verify_no_double_parens.rs +++ /dev/null @@ -1,38 +0,0 @@ -//! Compile-time verification test for clippy::double_parens fix. -//! -//! This test verifies that the expanded macros don't contain the problematic -//! std::mem::drop(()) pattern that triggers clippy warnings. -#![cfg(feature = "inventory")] - -use std::process::Command; - -#[test] -fn verify_expanded_code_has_no_double_parens() { - let output = Command::new("cargo") - .args(["expand", "--test", "clippy_double_parens_regression"]) - .output() - .expect("Failed to execute cargo expand"); - - let expanded = String::from_utf8_lossy(&output.stdout); - - // Filter out lines that are comments or documentation to avoid false positives - let code_lines: Vec<&str> = expanded - .lines() - .filter(|line| { - let trimmed = line.trim(); - !trimmed.starts_with("//") && !trimmed.starts_with("///") - }) - .collect(); - - let code_only = code_lines.join("\n"); - - // Check for the problematic pattern in the code (ignore comments) - if code_only.contains("std::mem::drop(())") { - panic!( - "Found std::mem::drop(()) in expanded code.\n\ - This pattern triggers clippy::double_parens warnings.\n\ - The macro should use $(std::mem::drop($other_inputs);)* \ - instead of std::mem::drop(($($other_inputs,)*))" - ); - } -} diff --git a/tests/clippy_double_parens_regression.rs b/tests/warnings/double_parens.rs similarity index 94% rename from tests/clippy_double_parens_regression.rs rename to tests/warnings/double_parens.rs index 8e2b58234..21f8239f2 100644 --- a/tests/clippy_double_parens_regression.rs +++ b/tests/warnings/double_parens.rs @@ -6,10 +6,7 @@ //! Before the fix in components/salsa-macro-rules/src/unexpected_cycle_recovery.rs, //! these macros would generate `std::mem::drop(())` which triggered the warning. //! -//! Run `cargo test --test verify_no_double_parens` to verify the fix. -//! //! See: https://github.com/salsa-rs/salsa/issues/1004 -#![cfg(feature = "inventory")] // This tracked function has no additional inputs beyond `db`. // With the old code, this would trigger clippy::double_parens warnings in the diff --git a/tests/warnings/main.rs b/tests/warnings/main.rs index 77438e538..7802a1638 100644 --- a/tests/warnings/main.rs +++ b/tests/warnings/main.rs @@ -2,6 +2,7 @@ #![deny(warnings)] +mod double_parens; mod needless_borrow; mod needless_lifetimes; mod unused_variable_db;