Repository navigation
chore: cargo clippy fixes to prepare for the rustc upgrade: 1.98.1 -> 1.99.0 - #11734
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are mechanical, behavior-preserving, and consistently address the reported Rust 1.99 diagnostics.
Review effort: Balanced
Findings: None
What changed in this PR
Prepares the workspace for Rust 1.99 by resolving newly reported Clippy and deprecation diagnostics without changing runtime behavior.
Changes:
- Replaces deprecated atomic operations and legacy float-module imports.
- Standardizes macro delimiters and simplifies a single-element loop.
- Allows two known Clippy false positives in generated code.
| File | Description |
|---|---|
ci/scripts/rust-lint.sh |
Allows generated-code false positives. |
packages/pocket-ic/src/common/rest.rs |
Simplifies SNS feature handling. |
rs/consensus/chain_key/src/lib.rs |
Uses AtomicUsize::try_update. |
rs/embedders/src/wasmtime_embedder/host_memory.rs |
Uses AtomicUsize::try_update. |
rs/embedders/src/wasmtime_embedder/linker.rs |
Standardizes format! delimiters. |
rs/monitoring/metrics_proxy/src/proxy.rs |
Removes deprecated float-module import. |
rs/pocket_ic_server/src/main.rs |
Uses AtomicU64::try_update. |
rs/rust_canisters/tests/src/nan_canonicalized.rs |
Uses primitive float constants directly. |
rs/types/types/src/crypto/threshold_sig/errors/threshold_sign_error.rs |
Standardizes write! delimiters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
✅ No security or compliance issues detected. Reviewed everything up to b1b2b5c. Security OverviewDetected Code Changes
|
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
No canister behavioural changes.
… 1.99.0 Clippy 0.1.99 and rustc 1.99.0 report diagnostics that 1.98.1 does not: * `Atomic*::fetch_update` is deprecated in favour of its new name `try_update`, which is already stable in 1.98.1. * `use std::f32;` / `use std::f64;` made `f64::INFINITY` etc. resolve to the legacy module constants, which 1.99.0 fully deprecates. Dropping the imports makes them resolve to the primitive types' associated constants. * `nonstandard_macro_braces` and `single_element_loop` now catch a few more sites. * `double_must_use` and `redundant_field_names` now fire on macro-generated code: every `#[async_trait]` trait, and the thiserror (`#[from] source`) and derive-new derives. These are clippy false positives (rust-lang/rust-clippy#17547, rust-lang/rust-clippy#17525), so allow both lints in rust-lint.sh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`cargo clippy -p pocket-ic` also lints pocket-ic's workspace path dependencies, which include `#[async_trait]` traits, so the Windows lint needs the same two `--allow`s as rust-lint.sh. Also run the Windows jobs when rust-toolchain.toml changes, so a toolchain bump exercises them before it merges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8b643d4 to
b1b2b5c
Compare
Upgrade the rust toolchain from 1.98.1 to 1.99.0. See: https://blog.rust-lang.org/2026/10/01/Rust-1.99.0/. The `cargo clippy` fixes this needs are in the PR below this one in the stack (dfinity#11734). This PR is therefore the version bump in `rust-toolchain.toml` and `bazel/rust.MODULE.bazel`, plus the test data that the new codegen (LLVM 22 → 23) shifts. **`//rs/bitcoin/ckbtc/minter:ckbtc_minter_canbench_test`** was regenerated with `bazel run //rs/bitcoin/ckbtc/minter:ckbtc_minter_canbench_update`. All 7 benchmark totals stay within the noise threshold (+0.10% to +0.49%), but 7 small `::greedy` scopes moved beyond it: | status | name | calls | ins | ins Δ% | HI | HI Δ% | SMI | SMI Δ% | |--------|----------------------------------------------------|-------|--------|---------|----|--------|-----|---------| | + | `build_unsigned_transaction_6_10_btc::greedy` | 1 | 13.41K | +10.91% | 0 | 0.00% | 0 | 0.00% | | + | `build_unsigned_transaction_4_10m_sats::greedy` | 1 | 4.45K | +10.54% | 0 | 0.00% | 0 | 0.00% | | + | `build_unsigned_transaction_5_1_btc::greedy` | 1 | 4.45K | +10.54% | 0 | 0.00% | 0 | 0.00% | | + | `build_estimate_retrieve_btc_fee_1_50k_sats::greedy` | 1 | 4.74K | +9.67% | 0 | 0.00% | 0 | 0.00% | | + | `build_unsigned_transaction_1_50k_sats::greedy` | 1 | 4.74K | +9.67% | 0 | 0.00% | 0 | 0.00% | | + | `build_unsigned_transaction_2_100k_sats::greedy` | 1 | 4.79K | +9.54% | 0 | 0.00% | 0 | 0.00% | | + | `build_unsigned_transaction_3_1m_sats::greedy` | 1 | 3.28K | +8.82% | 0 | 0.00% | 0 | 0.00% | **`//rs/nns/governance:governance-canbench_test`** was regenerated with `bazel run //rs/nns/governance:governance-canbench_update`. One of its 28 benchmarks moved beyond the 5% noise threshold: | status | name | calls | ins | ins Δ% | HI | HI Δ% | SMI | SMI Δ% | |--------|----------------------------|-------|-------|---------|----|--------|-----|---------| | - | `neuron_metrics_calculation` | | 3.36M | -8.53% | 0 | 0.00% | 0 | 0.00% | **`//rs/registry/canister:registry-canbench_test`** was regenerated with `bazel run //rs/registry/canister:registry-canbench_update`. One of its 14 benchmarks moved beyond the noise threshold: | status | name | calls | ins | ins Δ% | HI | HI Δ% | SMI | SMI Δ% | |--------|-------------------------------------------------------------|-------|---------|---------|-----|--------|-----|---------| | - | `measure_routing_table_invariant_checks_shards_and_unsharded` | | 227.82M | -2.87% | 140 | 0.00% | 0 | 0.00% | **`//rs/ledger_suite/icrc1/ledger:canbench_u64_test`** and **`:canbench_u256_test`** were regenerated with `bazel run //rs/ledger_suite/icrc1/ledger:canbench_u64_update` and `:canbench_u256_update`. Both moved the same way. u64: | status | name | calls | ins | ins Δ% | HI | HI Δ% | SMI | SMI Δ% | |--------|-----------------------------------------------|-------|---------|---------|-----|--------|-----|---------| | + | `bench_icrc1_transfers::icrc103_get_allowances` | 1 | 5.35M | +17.25% | 0 | 0.00% | 0 | 0.00% | | + | `bench_icrc1_transfers::icrc2_approve` | 1 | 17.78B | +3.15% | 28 | 0.00% | 128 | 0.00% | | + | `bench_icrc1_transfers::icrc2_transfer_from` | 1 | 19.81B | +2.93% | 3 | 0.00% | 0 | 0.00% | | + | `bench_icrc1_transfers` | | 50.12B | +2.26% | 264 | 0.00% | 256 | 0.00% | | - | `bench_icrc1_transfers::upgrade` | 1 | 497.85M | -5.66% | 202 | 0.00% | 128 | 0.00% | | - | `bench_icrc1_transfers::post_upgrade` | 1 | 347.07M | -8.19% | 73 | 0.00% | 0 | 0.00% | u256: | status | name | calls | ins | ins Δ% | HI | HI Δ% | SMI | SMI Δ% | |--------|-----------------------------------------------|-------|---------|---------|-----|--------|-----|---------| | + | `bench_icrc1_transfers::icrc103_get_allowances` | 1 | 6.24M | +14.34% | 0 | 0.00% | 0 | 0.00% | | + | `bench_icrc1_transfers::icrc2_approve` | 1 | 18.35B | +3.23% | 28 | 0.00% | 128 | 0.00% | | + | `bench_icrc1_transfers::icrc2_transfer_from` | 1 | 20.27B | +3.02% | 3 | 0.00% | 0 | 0.00% | | + | `bench_icrc1_transfers` | | 51.70B | +2.36% | 262 | 0.00% | 256 | 0.00% | | - | `bench_icrc1_transfers::upgrade` | 1 | 492.28M | -6.75% | 200 | 0.00% | 128 | 0.00% | | - | `bench_icrc1_transfers::post_upgrade` | 1 | 341.50M | -9.70% | 71 | 0.00% | 0 | 0.00% | **`//rs/recovery/subnet_splitting:integration_test`** has no `_update` target. Its expected values are hardcoded in the test, so all ten were re-pinned by hand from one 1.99.0 reference run, in that run's source/destination orientation. As with 1.98.1 (dfinity#11534), what changed is the *partition*, not the workload: * Every total is identical: canisters 20, ingress messages 39, remote 10, local 28, http outcalls 10, heartbeats 694. Total instructions moved by 0.013% and total state size by 0.26%. * The split-finder solves a symmetric MILP over load samples derived from measured execution, and that is enough to flip a near-tied optimum to a different partition. * Both partitions balance instructions ~50/50, which the test also checks. | metric | 1.98.1 | 1.99.0 | | --- | --- | --- | | `states_sizes_bytes` | 3693264 / 5539880 | 5016967 / 4240197 | | `canisters_installed` | 8 / 12 | 11 / 9 | | `ingress_messages_executed` | 15 / 24 | 21 / 18 | | `remote_subnet_messages_executed_lower_bound` | 4 / 6 | 5 / 5 | | `local_subnet_messages_executed_upper_bound` | 11 / 17 | 15 / 13 | | `http_outcalls_executed` | 4 / 6 | 6 / 4 | | `heartbeats_and_global_timers_executed` | 368 / 326 | 341 / 353 | The test is deterministic: 5/5 uncached runs on 1.99.0 give byte-identical estimates. As noted in dfinity#11534, it pins the exact solution of a degenerate optimisation, so it will keep flipping on unrelated changes too. Asserting on the totals and on the ~50/50 instruction balance would make it robust. The other three canbench suites (ckETH minter, ICP ledger, SNS governance) stay within their noise thresholds. CI passes on all targets (`CI_ALL_BAZEL_TARGETS`), including the Windows PocketIC jobs and Build Determinism. No repin is needed. The `Cargo.Bazel.*.lock` digests hash the `cargo`/`rustc` of rules_rust's own host tools (pinned at 1.98.0 by rules_rust 0.74.0), not the version selected by `rust.toolchain`, and no third-party crate changed. rules_rust 0.74.0 ships no checksums for 1.99.0 (nor for the current 1.98.1), so the toolchain is downloaded unverified, exactly as 1.98.1 is today. Also checked, since tests won't necessarily catch these: * **`RangeInclusive`** ([rust#155114](rust-lang/rust#155114)): an exhausted range now reports different `start()`/`end()`. No code in the repo observes a range after iterating it. * **Wasm**: the `wasm32-unknown-unknown` target features are identical on both toolchains. Canister-like modules built with the canister flags use no new Wasm proposal, and they pass validation configured like the IC's. * **LLVM bitcode**: rustc 1.99.0 emits LLVM 23 bitcode, which the hermetic LLVM 22 `lld` cannot read. This is harmless today: canisters (`-C linker-plugin-lto`) link with rustc's bundled `rust-lld`, and native links carry no bitcode. Until the hermetic toolchain reaches LLVM 23, though, don't route wasm links through it or add `-C linker-plugin-lto` to native targets. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
cargo clippyfixes to prepare for upgrading torustc-1.99.0.
Clippy 0.1.99 and rustc 1.99.0 report diagnostics at 124 sites that 1.98.1 does not, in 67
source files plus 5 tonic-generated ones. 107 of these sites are clippy false positives in
macro-generated code; the other 17 are fixed in the code:
double_must_use#[must_use]determination. Clippy now sees that thePin<Box<dyn Future>>an#[async_trait]method returns is already#[must_use], and flags the#[must_use]that async-trait adds to it. That hits every one of the 61#[async_trait]traits in the workspace, plus 5 tonic-generated ones (141 diagnostics, one per async method)--allow(see below)redundant_field_namesFromimpl for#[from] sourcefields (16) andderive_new::new(25, all inic_boundary)--allow(see below)deprecated(rustc)Atomic*::fetch_updatein favour of its new nametry_update(3). rust#146882 fully deprecates the legacystd::f32/std::f64module constants, whichuse std::f32;/use std::f64;madef32::NANetc. resolve to (10)fetch_update→try_update; dropuse std::f64;frommetrics_proxyanduse std::f32;/use std::f64;fromnan_canonicalized(see below)nonstandard_macro_bracesnurserytostyle, soclippy::allnow enables itformat! {..}/write! {..}→format!(..)/write!(..)single_element_loopfor (flag, s) in [(sns, "sns")] { if flag.is_some() { .. } }→if sns.is_some() { .. }Three of these deserve a word:
double_must_useandredundant_field_namesare allowed globally rather than at each site,both in
ci/scripts/rust-lint.shand in the second clippy gate, the PocketIC Windows lint(
.github/workflows/pocket-ic-tests-windows.yml). Itscargo clippy -p pocket-icalso lintspocket-ic's workspace path dependencies, which include
#[async_trait]traits. The lints flag codethat the macros generate, not code anyone wrote. Per-site
#[allow]s would touch 64 files acrossmost teams: the 59 with flagged code plus the 5 that include tonic-generated code. Every new
#[async_trait]trait,#[from] sourcefield or#[derive(new)]struct would then need one too.(#17547, merged after 1.99 branched, so
it ships in 1.100). The second is still open as
#17525.
suppress the lints in their output, but both require syn 3, which this repo deliberately avoids
(
thiserror = "=2.0.18", feat: bump ic-wasm and drop syn 3 #11016). derive-new has had no release since 0.7.0.uninlined_format_args.The Windows workflow's path filter now also matches
rust-toolchain.toml, so a toolchain bump(like the PR stacked on this one) runs the Windows lint and tests before it merges, rather than
first in the next daily run on master.
use std::f32;/use std::f64;bring the module into scope. For names that exist in thatmodule it shadows the primitive type. So
f64::INFINITYresolved to the deprecatedstd::f64::INFINITYrather than the associated constant onf64(same value), whilef64::lnand the like fell through to the primitive type. Without the imports every
f32::…/f64::…pathin those two files resolves to the primitive type.
try_updateisfetch_updateunder its new name: same arguments, same result. It has beenstable since 1.95, so it also builds on the currently pinned toolchain.
Because this PR lands before the toolchain bump, it has to stay clean on 1.98.1 too. It does not
name any lint that is new in 1.99: both allowed lints exist in Clippy 0.1.98, so there is no
unknown_lintsissue. It also uses no API that 1.98.1 lacks.The toolchain upgrade itself is the PR stacked on top of this one.
Testing
./ci/scripts/rust-lint.shexits 0 both on the currently pinned 1.98.1 and on 1.99.0.cargo check --all-targets --all-featuresof the 7 touched cratesbazel buildof the 8 directly affected targetsrun was stopped in favour of CI, which passes on all targets (
CI_ALL_BAZEL_TARGETS). The 3 localfailures are environmental:
golden_state_swap_upgrade_twicecannot fetch its state over SSH fromthe dev environment, and
upgrade_canisters_with_golden_nns_stateneedsNNS_CANISTER_UPGRADE_SEQUENCE🤖 Generated with Claude Code