Update code to remove lots of warnings from Rust 1.83 #15593

gregnazario · 2024-12-13T04:58:29Z

Description

There are a ton of warnings for mostly cosmetic things added to Rust 1.83. For anything that was complex, or required renaming, I added an allow.

Otherwise main things changed:

Features are required, so all names missing were added. Not much was looked into why the features existed.
Lifetimes are now elided '_ or sometimes named specifically 'a, each are approached correctly.
Now Rust checks ceil_div implementations and adds them accordingly
Lots of code was "unused", so dead_code annotation is added
Lots of unwrap_or, or other improvements about code that's unnecessary
Documentation is for some reason really aggressively formatted now. So, this fixes most of them. May be slightly off, but it should be good enough.
There are some checks with macros, those have been updated accordingly or ignored in the case of poem and the MoveVM cc @banool @georgemitenkov

How Has This Been Tested?

We're running the full testsuite on this PR to ensure it works.

Key Areas to Review

For each area, please review and ensure there is no behavior change. For the Move compiler, and storage, most of the changes are just removing <'a> and moving to '_ as per the compiler.

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-12-13T04:58:32Z

⏱️ 5h 31m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	1h 13m	🟥 🟥 🟥 🟥 🟥
execution-performance / test-target-determinator	42m	🟩 🟩 🟩 🟩 🟥 (+1 more)
rust-unit-tests	24m	🟥
rust-unit-tests	24m	🟥
rust-unit-tests	23m	🟥
test-target-determinator	23m	🟩 🟩 🟩 🟥
check	14m	🟩 🟩 🟩 🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
check-dynamic-deps	11m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-cargo-deny	9m	🟥 🟥 🟥 🟥 🟥 (+1 more)
fetch-last-released-docker-image-tag	6m	🟩 🟩 🟩 🟩
rust-doc-tests	6m	🟩

🚨 3 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg
test-target-determinator	7m	4m
execution-performance / test-target-determinator	7m	4m
execution-performance / single-node-performance	24m	18m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

banool

I reviewed the stuff I'm familiar with (faucet, CLI, indexer) pretty closely and skimmed the rest, looks good.

banool · 2024-12-13T19:35:32Z

crates/aptos-faucet/core/src/checkers/redis_ratelimit.rs

@@ -305,6 +305,7 @@ impl CheckerTrait for RedisRatelimitChecker {

    /// All we have to do here is decrement the counter if the request was a failure due
    /// to something wrong on our end.
+    #[allow(dependency_on_unit_never_type_fallback)]


Allowing this seems fine to me for now, the proper never type (!) isn't even stable yet, we'll revisit it later when it is.

banool · 2024-12-13T19:36:26Z

ecosystem/indexer-grpc/indexer-grpc-data-service/src/service.rs

-                } else {
-                    num_tasks
-                }
+                num_tasks.clamp(1, MAX_FETCH_TASKS_PER_REQUEST)


github-actions · 2024-12-13T21:11:03Z

✅ Forge suite `realistic_env_max_load` success on `ed657e0fdde1276de0c58c77e895a05d8d119fcb`

two traffics test: inner traffic : committed: 14883.87 txn/s, latency: 2667.78 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5659220
two traffics test : committed: 100.10 txn/s, latency: 1410.53 ms, (p50: 1400 ms, p70: 1400, p90: 1500 ms, p99: 1600 ms), latency samples: 1740
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.510, avg: 1.466", "ConsensusProposalToOrdered: max: 0.315, avg: 0.288", "ConsensusOrderedToCommit: max: 0.374, avg: 0.363", "ConsensusProposalToCommit: max: 0.659, avg: 0.651"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.55s no progress at version 31820 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.64s no progress at version 2177354 (avg 0.59s) [limit 16].
Test Ok

github-actions · 2024-12-13T21:11:29Z

✅ Forge suite `compat` success on `3c6e693a27339e73520f41030dce8fc9cd504967` ==> `ed657e0fdde1276de0c58c77e895a05d8d119fcb`

Compatibility test results for 3c6e693a27339e73520f41030dce8fc9cd504967 ==> ed657e0fdde1276de0c58c77e895a05d8d119fcb (PR)
1. Check liveness of validators at old version: 3c6e693a27339e73520f41030dce8fc9cd504967
compatibility::simple-validator-upgrade::liveness-check : committed: 15901.68 txn/s, latency: 2140.25 ms, (p50: 1800 ms, p70: 2000, p90: 2600 ms, p99: 12700 ms), latency samples: 548940
2. Upgrading first Validator to new version: ed657e0fdde1276de0c58c77e895a05d8d119fcb
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7451.05 txn/s, latency: 3846.94 ms, (p50: 4200 ms, p70: 4500, p90: 4600 ms, p99: 4800 ms), latency samples: 140480
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7448.16 txn/s, latency: 4329.97 ms, (p50: 4600 ms, p70: 4700, p90: 4700 ms, p99: 4800 ms), latency samples: 254260
3. Upgrading rest of first batch to new version: ed657e0fdde1276de0c58c77e895a05d8d119fcb
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6633.76 txn/s, latency: 4350.21 ms, (p50: 4700 ms, p70: 4900, p90: 5200 ms, p99: 5400 ms), latency samples: 132760
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7307.32 txn/s, latency: 4450.47 ms, (p50: 4800 ms, p70: 4900, p90: 5000 ms, p99: 5400 ms), latency samples: 247720
4. upgrading second batch to new version: ed657e0fdde1276de0c58c77e895a05d8d119fcb
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11641.43 txn/s, latency: 2372.28 ms, (p50: 2600 ms, p70: 2700, p90: 2800 ms, p99: 3000 ms), latency samples: 204920
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 12091.04 txn/s, latency: 2640.05 ms, (p50: 2700 ms, p70: 2800, p90: 2900 ms, p99: 3000 ms), latency samples: 392800
5. check swarm health
Compatibility test for 3c6e693a27339e73520f41030dce8fc9cd504967 ==> ed657e0fdde1276de0c58c77e895a05d8d119fcb passed
Test Ok

georgemitenkov

VM side is ok, interesting that same attributes with different arguments are considered the same by linter now.

vineethk

I've reviewed the move compilers, linter etc under third-party.

vineethk · 2024-12-17T20:16:55Z

third_party/move/evm/move-to-yul/src/abi_move_metadata.rs

@@ -11,7 +11,7 @@ use move_core_types::metadata::Metadata;
 use move_ethereum_abi::abi_move_type::{ABIMoveSignature, ABI_ETHER_MOVE_KEY};
 use std::{collections::BTreeMap, str};

-/// Generate Metadata for move signature
+// Generate Metadata for move signature


Perhaps we should keep the /// comment, but remove the empty lin below?

gregnazario requested review from alinush, rex1fernando, msmouse, lightmark, grao1991, JoshLind, zekun000, bchocho, gelash, zjma, mstraka100, banool, sasha8, ibalajiarun, georgemitenkov and vgao1996 as code owners December 13, 2024 04:58

grao1991 approved these changes Dec 13, 2024

View reviewed changes

msmouse approved these changes Dec 13, 2024

View reviewed changes

gregnazario requested review from heliuchuan, geekflyer, davidiw, movekevin, danielxiangzl and wrwg as code owners December 13, 2024 19:21

rustielin added CICD:build-images when this label is present github actions will start build+push rust images from the PR. CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR CICD:run-all-unit-tests Runs all unit tests labels Dec 13, 2024

banool approved these changes Dec 13, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

gregnazario added 21 commits December 13, 2024 15:35

storage updates

6e00fe2

statesync updates

c196adb

aptos move updates

eefb1db

crates updates

4e56e2b

mempool updates

9871f59

sdk updates

9a6a7a9

aptos node updates

ff63eda

config updates

5ec625a

consensus updates

604a4ad

grpc updates

c3495e6

execution updates

adc5d4f

secure updates

6c3f991

experimental updates

f695685

keyless updates

14ee4d8

network updates

ac8c6f9

peer monitoring updates

2740d1a

dependency updates

fc61af4

Fix evm hardhard move code

d456e94

remove manual pattern warning

c7748cf

simplify match statement

d5fcecc

fix thread dump

ed657e0

gregnazario force-pushed the update-to-latest-rust branch from 5df4772 to ed657e0 Compare December 13, 2024 20:45

This comment has been minimized.

Sign in to view

georgemitenkov approved these changes Dec 14, 2024

View reviewed changes

vineethk self-requested a review December 16, 2024 19:52

temp

b0d6beb

vineethk approved these changes Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update code to remove lots of warnings from Rust 1.83 #15593

Update code to remove lots of warnings from Rust 1.83 #15593

gregnazario commented Dec 13, 2024 •

edited

Loading

trunk-io bot commented Dec 13, 2024 •

edited

Loading

banool left a comment

banool Dec 13, 2024

banool Dec 13, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

georgemitenkov left a comment

vineethk left a comment

vineethk Dec 17, 2024

Update code to remove lots of warnings from Rust 1.83 #15593

Are you sure you want to change the base?

Update code to remove lots of warnings from Rust 1.83 #15593

Conversation

gregnazario commented Dec 13, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Dec 13, 2024 • edited Loading

banool left a comment

Choose a reason for hiding this comment

banool Dec 13, 2024

Choose a reason for hiding this comment

banool Dec 13, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 13, 2024

✅ Forge suite realistic_env_max_load success on ed657e0fdde1276de0c58c77e895a05d8d119fcb

github-actions bot commented Dec 13, 2024

✅ Forge suite compat success on 3c6e693a27339e73520f41030dce8fc9cd504967 ==> ed657e0fdde1276de0c58c77e895a05d8d119fcb

georgemitenkov left a comment

Choose a reason for hiding this comment

vineethk left a comment

Choose a reason for hiding this comment

vineethk Dec 17, 2024

Choose a reason for hiding this comment

gregnazario commented Dec 13, 2024 •

edited

Loading

trunk-io bot commented Dec 13, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `ed657e0fdde1276de0c58c77e895a05d8d119fcb`

✅ Forge suite `compat` success on `3c6e693a27339e73520f41030dce8fc9cd504967` ==> `ed657e0fdde1276de0c58c77e895a05d8d119fcb`