clippy: uninlined_format_args by steviez · Pull Request #6854 · anza-xyz/agave

steviez · 2025-07-06T23:05:32Z

Problem

Work towards #6850 - it appears that using variables inline in format! is now required. Sample error output:

error: variables can be used directly in the `format!` string
 --> version/build.rs:8:17
  |
8 |                 println!("cargo:rustc-env=AGAVE_GIT_COMMIT_HASH={}", trimmed_hash);
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
  = note: `-D clippy::uninlined-format-args` implied by `-D warnings`
  = help: to override `-D warnings` add `#[allow(clippy::uninlined_format_args)]`
help: change this to
  |
8 -                 println!("cargo:rustc-env=AGAVE_GIT_COMMIT_HASH={}", trimmed_hash);
8 +                 println!("cargo:rustc-env=AGAVE_GIT_COMMIT_HASH={trimmed_hash}");
  |

Summary of Changes

The steps to create this PR:

Setting version in rust-toolchain.toml to 1.88.0
Run cargo clippy --fix --allow-dirty
Manually revert several changes that were not uninlined_format_args complaints
Add all changed files to first commit
Run ./cargo nightly fmt to cleanup strings modified in 1st commit
Run cargo clippy --fix --tests --allow-dirty
Run cargo nightly fmt to cleanup strings modified in 3rd commit

mergify · 2025-07-06T23:06:05Z

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

mergify · 2025-07-06T23:36:16Z

If this PR represents a change to the public RPC API:

Make sure it includes a complementary update to rpc-client/ (example)
Open a follow-up PR to update the JavaScript client @solana/kit (example)

Thank you for keeping the RPC clients in sync with the server API @steviez.

steviez

Build is currently failing since fmt differs; however, I think the change it wants to adopt is kind of ridiculous. See here:
https://buildkite.com/anza/agave/builds/26034#0197e23a-a436-4805-ad12-6f69c1e9500f

Note the misalignment of the parentheses for trace!; might play around with it for a little but also might cave and just let it go with the misalignment:

steviez · 2025-07-06T23:10:22Z

        debug!(
-            "Loaded Programs Cache Stats -- Hits: {}, Misses: {}, Evictions: {}, Reloads: {}, Insertions: {}, Lost-Insertions: {}, Replacements: {}, One-Hit-Wonders: {}, Prunes-Orphan: {}, Prunes-Environment: {}, Empty: {}, Water-Level: {}",
-            hits, misses, evictions, reloads, insertions, lost_insertions, replacements, one_hit_wonders, prunes_orphan, prunes_environment, empty_entries, water_level
+            "Loaded Programs Cache Stats -- Hits: {hits}, Misses: {misses}, Evictions: {evictions}, Reloads: {reloads}, Insertions: {insertions}, Lost-Insertions: {lost_insertions}, Replacements: {replacements}, One-Hit-Wonders: {one_hit_wonders}, Prunes-Orphan: {prunes_orphan}, Prunes-Environment: {prunes_environment}, Empty: {empty_entries}, Water-Level: {water_level}"


Not going to touch it as part of this PR, but this string is clearly too long and could be causing issues with cargo fmt for the whole file. Just calling out

steviez · 2025-07-06T23:11:05Z

                    debug!(
-                                "Too many requests: server responded with {:?}, {} retries left, pausing for {:?}",
-                                response, too_many_requests_retries, duration
+                                "Too many requests: server responded with {response:?}, {too_many_requests_retries} retries left, pausing for {duration:?}"


Another malformed line that we should clean up after this PR

steviez · 2025-07-06T23:12:02Z

                                    max_streams_per_interval: {max_streams_per_throttling_interval}, read_interval_streams: {streams_read_in_throttle_interval} \
-                                    throttle_duration: {throttle_duration:?}",
-                                    peer_type, total_stake);
+                                    throttle_duration: {throttle_duration:?}");


Another case: the indentation on these subsequent line looks off

steviez · 2025-07-06T23:39:21Z

-            error!("Failed to deploy the program: {}", e);
-            format!("Failed to deploy the program: {}", e)
+            error!("Failed to deploy the program: {e}");
+            format!("Failed to deploy the program: {e}")


For future - could probably create the string once, print it in error log and then return to avoid the double format!()

steviez · 2025-07-06T23:39:36Z

-            error!("Failed to fetch the program: {}", e);
-            format!("Failed to fetch the program: {}", e)
+            error!("Failed to fetch the program: {e}");
+            format!("Failed to fetch the program: {e}")


Same as above - can seemingly get this down to a single format!()

LucasSte

I skimmed through the files.

steviez · 2025-07-09T05:50:11Z

+                    let diff = new_account.lamports() - old_account.lamports();
+                    trace!("store_account_and_update_capitalization: increased: {pubkey} {diff}");


fmt was doing some weird stuff with the formatting here. Renaming increased ==> diff (and decreased ==> diff below) avoided it. Typically, I would prefer not to rename variables in a PR like this, but if you look at the 3rd commit, fmt was making a mess of it

codecov-commenter · 2025-07-09T06:45:41Z

Codecov Report

Attention: Patch coverage is 51.49606% with 308 lines in your changes missing coverage. Please review.

Project coverage is 83.2%. Comparing base (833b05b) to head (6d1998b).

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #6854    +/-   ##
========================================
  Coverage    83.2%    83.2%            
========================================
  Files         852      852            
  Lines      376951   376839   -112     
========================================
- Hits       313888   313866    -22     
+ Misses      63063    62973    -90

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

brooksprumo

Let no good deed go unpunished!

I made it up until cli/. IMO this PR is too large. I worry that it'll constantly require handling merge conflicts. That or it just gets stamped without someone doing a full review.

Maybe it is split up into smaller chunks with a few crates at a time?

brooksprumo · 2025-07-09T12:56:51Z

            info!(
-                "total_accounts_created: {} total_accounts_closed: {} tx_sent_count: {} loop_count: {} balance(s): {:?}",
-                total_accounts_created, total_accounts_closed, tx_sent_count, count, balances
+                "total_accounts_created: {total_accounts_created} total_accounts_closed: {total_accounts_closed} tx_sent_count: {tx_sent_count} loop_count: {count} balance(s): {balances:?}"


This line is significantly longer than 100 characters. Granted it was longer than 100 characters before too...

brooksprumo · 2025-07-09T12:57:34Z

                info!(
-                    "total_accounts_closed: {} tx_sent_count: {} loop_count: {} balance(s): {:?}",
-                    total_accounts_closed, tx_sent_count, count, balances
+                    "total_accounts_closed: {total_accounts_closed} tx_sent_count: {tx_sent_count} loop_count: {count} balance(s): {balances:?}"


Longer than 100 characters now, where as it wasn't before.

brooksprumo · 2025-07-09T12:58:31Z

        #[allow(clippy::stable_sort_primitive)]
        alive_roots.sort();
-        info!("{}: accounts_index alive_roots: {:?}", label, alive_roots,);
+        info!("{label}: accounts_index alive_roots: {alive_roots:?}",);


nit: not caused by you, but the trailing comma.

Suggested change

info!("{label}: accounts_index alive_roots: {alive_roots:?}",);

info!("{label}: accounts_index alive_roots: {alive_roots:?}");

brooksprumo · 2025-07-09T12:59:14Z

            if result.is_err() {
                // for vm.max_map_count, error is: {code: 12, kind: Other, message: "Cannot allocate memory"}
-                info!("memory map error: {:?}. This may be because vm.max_map_count is not set correctly.", result);
+                info!("memory map error: {result:?}. This may be because vm.max_map_count is not set correctly.");


Longer than 100 characters. Old was too.

brooksprumo · 2025-07-09T12:59:29Z

-        entry_size,
-        limit_size,
-    );
+    trace!("checked_total_size_sum: {total_size} + {entry_size} < {limit_size}",);


trailing comma.

brooksprumo · 2025-07-09T13:00:15Z

        info!(
-            "Funding keypair balance: {} max_fee: {} lamports_per_account: {} extra: {} total: {}",
-            funding_key_balance, max_fee, lamports_per_account, extra, total
+            "Funding keypair balance: {funding_key_balance} max_fee: {max_fee} lamports_per_account: {lamports_per_account} extra: {extra} total: {total}"


Longer than 100 characters.

brooksprumo · 2025-07-09T13:00:51Z

                        warn!(
-                            "Too many failed transfers... {} remaining, {} verified, {} failures",
-                            remaining_count, verified_txs, failed_verify
+                            "Too many failed transfers... {remaining_count} remaining, {verified_txs} verified, {failed_verify} failures"


Longer than 100 characters.

brooksprumo · 2025-07-09T13:00:55Z

                            info!(
-                                "Verifying transfers... {} remaining, {} verified, {} failures",
-                                remaining_count, verified_txs, failed_verify
+                                "Verifying transfers... {remaining_count} remaining, {verified_txs} verified, {failed_verify} failures"


Longer than 100 characters.

brooksprumo · 2025-07-09T13:01:10Z

            info!(
-                "Verifying transfers... {} remaining, {} verified, {} failures",
-                remaining_count, verified_txs, failed_verify
+                "Verifying transfers... {remaining_count} remaining, {verified_txs} verified, {failed_verify} failures"


Longer than 100 characters.

steviez · 2025-07-10T04:05:19Z

Let no good deed go unpunished!

As is tradition

I made it up until cli/. IMO this PR is too large. I worry that it'll constantly require handling merge conflicts. That or it just gets stamped without someone doing a full review.

I did review this one somewhat myself and was hoping I'd find another brave soul who could slog through it, but given that you found some other >100 character offenders, maybe we can break it up

Maybe it is split up into smaller chunks with a few crates at a time?

Appreciate you helping me pump my commit numbers 😆

steviez · 2025-07-10T05:35:37Z

Going to close this out in favor of smaller PR's

I made it up until cli/. IMO this PR is too large.

Sorry about any wasted efforts here Brooks. And similar sentiment for you @LucasSte; I broke the SVM owned stuff into separate PR (#6910) to hopefully avoid requiring multiple re-reviews from you & team

steviez requested a review from a team as a code owner July 6, 2025 23:05

steviez requested a review from a team as a code owner July 6, 2025 23:35

steviez commented Jul 7, 2025

View reviewed changes

LucasSte previously approved these changes Jul 7, 2025

View reviewed changes

steviez added 4 commits July 9, 2025 00:39

Run cargo clippy --tests --fix with Rust 1.88.0

ea0ec1a

./cargo nightly fmt

826c54d

./cargo nightly fmt doing some weird stuff

6371cc9

Rename variables to prevent fmt from mangling the formatting

6d1998b

steviez dismissed LucasSte’s stale review via 6d1998b July 9, 2025 05:47

steviez force-pushed the rust_v1.88_clippy branch from 98e4d25 to 6d1998b Compare July 9, 2025 05:47

steviez commented Jul 9, 2025

View reviewed changes

steviez requested a review from brooksprumo July 9, 2025 06:47

brooksprumo reviewed Jul 9, 2025

View reviewed changes

steviez mentioned this pull request Jul 10, 2025

accounts-db: Run cargo fmt with format_strings = true #6904

Merged

steviez marked this pull request as draft July 10, 2025 04:05

This was referenced Jul 10, 2025

clippy: uninlined_format_args (ledger-tool) #6909

Merged

Resolve Rust 1.88 clippy string lints and format strings (svm + misc) #6910

Merged

steviez closed this Jul 10, 2025

steviez deleted the rust_v1.88_clippy branch July 10, 2025 05:35

This was referenced Jul 22, 2025

thread-manager: Resolve Rust 1.88 clippy lints and format strings #7088

Merged

cli: Resolve Rust 1.88 clippy lints and format strings #7089

Merged

This was referenced Aug 9, 2025

runtime: Resolve Rust 1.88 clippy lints and format strings #7434

Merged

bench-tps: Resolve Rust 1.88 clippy lints and format strings #7435

Merged

steviez mentioned this pull request Aug 18, 2025

validator: Resolve Rust 1.88 clippy lints and format strings #7582

Merged

steviez mentioned this pull request Sep 2, 2025

Finish clippy uninlined_format_args cleanup #7825

Merged

		let diff = new_account.lamports() - old_account.lamports();
		trace!("store_account_and_update_capitalization: increased: {pubkey} {diff}");

	info!("{label}: accounts_index alive_roots: {alive_roots:?}",);
	info!("{label}: accounts_index alive_roots: {alive_roots:?}");

Conversation

steviez commented Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

mergify Bot commented Jul 6, 2025

Uh oh!

mergify Bot commented Jul 6, 2025

Uh oh!

steviez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucasSte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 9, 2025

Codecov Report

Uh oh!

brooksprumo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steviez commented Jul 10, 2025

Uh oh!

steviez commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

steviez commented Jul 6, 2025 •

edited

Loading

brooksprumo left a comment •

edited

Loading