Replace `arc.clone()` with `Arc::clone` and reject the former in clippy (plus enforce clippy in tests) #3851

TheBlueMatt · 2025-06-12T15:45:16Z

`arc.clone()` leaves the code somewhat ambiguous to whether we're
doing an expensive deep-clone operation or if we're doing a
(relatively) cheap pointer-copy-and-atomic-increment operation.

More importantly, it leaves entirely unclear what the semantics of
the object we just created are - does updating it update the
original or leave it be?

`arc.clone()` leaves the code somewhat ambiguous to whether we're doing an expensive deep-clone operation or if we're doing a (relatively) cheap pointer-copy-and-atomic-increment operation. More importantly, it leaves entirely unclear what the semantics of the object we just created are - does updating it update the original or leave it be? Thus, here, we replace `arc.clone()` calls with `Arc::clone()` in `lightning-net-tokio/src/lib.rs`

`arc.clone()` leaves the code somewhat ambiguous to whether we're doing an expensive deep-clone operation or if we're doing a (relatively) cheap pointer-copy-and-atomic-increment operation. More importantly, it leaves entirely unclear what the semantics of the object we just created are - does updating it update the original or leave it be? Thus, here, we replace `arc.clone()` calls with `Arc::clone()` in `lightning-background-processor/src/lib.rs`

`arc.clone()` leaves the code somewhat ambiguous to whether we're doing an expensive deep-clone operation or if we're doing a (relatively) cheap pointer-copy-and-atomic-increment operation. More importantly, it leaves entirely unclear what the semantics of the object we just created are - does updating it update the original or leave it be? Thus, here we require `Arc::clone` via clippy enforcement in CI

In a coming commit we'll enable `clippy` linting in our test code. However, there's some things we do in tests (which we might reasonably do in non-test code in the future) that clippy is unhappy with, which we explicitly allow here.

In a coming commit we'll enable `clippy` linting in our test code. Here we prepare for this by addressing the lints we'll enforce in test codein the `lightning` crate.

ldk-reviews-bot · 2025-06-12T15:45:19Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

In a coming commit we'll enable `clippy` linting in our test code. Here we prepare for this by addressing the lints we'll enforce in test code in remaining crates.

`arc.clone()` leaves the code somewhat ambiguous to whether we're doing an expensive deep-clone operation or if we're doing a (relatively) cheap pointer-copy-and-atomic-increment operation. More importantly, it leaves entirely unclear what the semantics of the object we just created are - does updating it update the original or leave it be? Thus, here, we replace `arc.clone()` calls with `Arc::clone()` in `lightning-background-processor` tests.

`arc.clone()` leaves the code somewhat ambiguous to whether we're doing an expensive deep-clone operation or if we're doing a (relatively) cheap pointer-copy-and-atomic-increment operation. More importantly, it leaves entirely unclear what the semantics of the object we just created are - does updating it update the original or leave it be? Thus, here, we replace `arc.clone()` calls with `Arc::clone()` in remaining tests.

lightning/src/util/test_utils.rs

TheBlueMatt · 2025-06-12T23:07:24Z

Disabled the broken shellcheck lint:

$ git diff-tree -U1 211871bdcb 0dcea98439
diff --git a/ci/check-lint.sh b/ci/check-lint.sh
index 33aef91335..cd013dfd17 100755
--- a/ci/check-lint.sh
+++ b/ci/check-lint.sh
@@ -5,2 +5,3 @@ set -x
 CLIPPY() {
+	# shellcheck disable=SC2086
 	RUSTFLAGS='-D warnings' cargo clippy $1 -- $2 \

tnull

LGTM

tnull · 2025-06-13T07:29:18Z

ci/check-lint.sh

-	-A clippy::io_other_error `# to be removed once we hit MSRV 1.74`
+
+CLIPPY() {
+	# shellcheck disable=SC2086


Not sure if we shouldn't just fix it instead of disabling the check, but not going to block this because of it.

The lack of quotes is deliberate, we need to pass multiple things to clippy via one argument.

tnull

Ah, wait, rustfmt is unhappy.

Clippy points out that "format specifiers have no effect on `format_args!()`", i.e. that our attempt to fix the log context width to 55 chars doesn't do anything because its applied to the result of `format_args`ing the context. This appears to have broken when we optimized out a previous `format`, which we revert here.

Now that we've fixed many of the lints clippy cares about for our test code, switch to enforcing the lints in `--tests` as well. We disable a few lints that require substantial number of changes to our existing tests, sadly, though they'd probably be good changes to make at some point.

TheBlueMatt · 2025-06-13T12:11:42Z

Fixed.

TheBlueMatt · 2025-06-13T12:12:15Z

Can probably be landed with one ACK, the diff is just:

$ git diff-tree -U1 0dcea98439 07590e4d5a
diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs
index b1864dbe77..b26794a30c 100644
--- a/lightning/src/util/test_utils.rs
+++ b/lightning/src/util/test_utils.rs
@@ -1437,6 +1437,4 @@ impl Logger for TestLogger {
 	fn log(&self, record: Record) {
-		let context = format!(
-			"{} {} [{}:{}]",
-			self.id, record.level, record.module_path, record.line
-		);
+		let context =
+			format!("{} {} [{}:{}]", self.id, record.level, record.module_path, record.line);
 		let s = format!("{:<55} {}", context, record.args);

TheBlueMatt added 5 commits June 12, 2025 13:25

Address clippy lints in lightning test code

3a8863f

In a coming commit we'll enable `clippy` linting in our test code. Here we prepare for this by addressing the lints we'll enforce in test codein the `lightning` crate.

TheBlueMatt changed the title ~~Replace arc.clone() with Arc::clone and reject the former in clippy~~ Replace arc.clone() with Arc::clone and reject the former in clippy (plus enforce clippy in tests) Jun 12, 2025

TheBlueMatt added 3 commits June 12, 2025 15:48

Address clippy lints in remaining tests

f46be2c

In a coming commit we'll enable `clippy` linting in our test code. Here we prepare for this by addressing the lints we'll enforce in test code in remaining crates.

TheBlueMatt force-pushed the 2025-06-no-arc-clones branch from 6c06771 to 211871b Compare June 12, 2025 15:48

ldk-reviews-bot requested a review from jkczyz June 12, 2025 15:55

tnull previously approved these changes Jun 12, 2025

View reviewed changes

tnull reviewed Jun 12, 2025

View reviewed changes

lightning/src/util/test_utils.rs Show resolved Hide resolved

wpaulino previously approved these changes Jun 12, 2025

View reviewed changes

TheBlueMatt mentioned this pull request Jun 12, 2025

Trivial #3752 followups #3848

Open

TheBlueMatt dismissed stale reviews from wpaulino and tnull via 0dcea98 June 12, 2025 23:07

TheBlueMatt force-pushed the 2025-06-no-arc-clones branch from 211871b to 0dcea98 Compare June 12, 2025 23:07

TheBlueMatt requested review from wpaulino and tnull and removed request for jkczyz June 12, 2025 23:07

wpaulino previously approved these changes Jun 12, 2025

View reviewed changes

tnull previously approved these changes Jun 13, 2025

View reviewed changes

tnull reviewed Jun 13, 2025

View reviewed changes

tnull mentioned this pull request Jun 13, 2025

fuzz: Add LSPS message decoder fuzzing #3849

Open

TheBlueMatt added 2 commits June 13, 2025 12:11

TheBlueMatt dismissed tnull’s stale review via 07590e4 June 13, 2025 12:11

TheBlueMatt dismissed wpaulino’s stale review via 07590e4 June 13, 2025 12:11

TheBlueMatt force-pushed the 2025-06-no-arc-clones branch from 0dcea98 to 07590e4 Compare June 13, 2025 12:11

TheBlueMatt requested a review from tnull June 13, 2025 12:11

wpaulino approved these changes Jun 13, 2025

View reviewed changes

wpaulino merged commit ed16b5f into lightningdevkit:main Jun 13, 2025
27 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace `arc.clone()` with `Arc::clone` and reject the former in clippy (plus enforce clippy in tests) #3851

Replace `arc.clone()` with `Arc::clone` and reject the former in clippy (plus enforce clippy in tests) #3851

TheBlueMatt commented Jun 12, 2025

Uh oh!

ldk-reviews-bot commented Jun 12, 2025 •

edited

Loading

Uh oh!

Uh oh!

TheBlueMatt commented Jun 12, 2025

Uh oh!

tnull left a comment

Uh oh!

tnull Jun 13, 2025

Uh oh!

TheBlueMatt Jun 13, 2025

Uh oh!

tnull left a comment

Uh oh!

TheBlueMatt commented Jun 13, 2025

Uh oh!

TheBlueMatt commented Jun 13, 2025

Uh oh!

Uh oh!

Uh oh!

Replace arc.clone() with Arc::clone and reject the former in clippy (plus enforce clippy in tests) #3851

Replace arc.clone() with Arc::clone and reject the former in clippy (plus enforce clippy in tests) #3851

Conversation

TheBlueMatt commented Jun 12, 2025

Uh oh!

ldk-reviews-bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jun 12, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jun 13, 2025

Uh oh!

TheBlueMatt commented Jun 13, 2025

Uh oh!

Uh oh!

Uh oh!

Replace `arc.clone()` with `Arc::clone` and reject the former in clippy (plus enforce clippy in tests) #3851

Replace `arc.clone()` with `Arc::clone` and reject the former in clippy (plus enforce clippy in tests) #3851

ldk-reviews-bot commented Jun 12, 2025 •

edited

Loading