From ab4567866ebf76981c8357ca0dcb0c8cc910e6b1 Mon Sep 17 00:00:00 2001 From: DaughterOfMars Date: Fri, 22 Nov 2024 08:48:22 -0500 Subject: [PATCH] chore(*): Add msim clippy check and fix clippy lints (#4180) * chore(*): Add msim clippy check and fix clippy lints * Update config patch * fix script * 1.81 fixes and remove copyright * review --- .github/workflows/_rust_lints.yml | 5 +++ Cargo.toml | 2 - crates/iota-benchmark/tests/simtest.rs | 10 ++--- crates/iota-config/src/local_ip_utils.rs | 11 ++++- crates/iota-core/src/authority.rs | 6 +-- crates/iota-core/src/checkpoints/mod.rs | 42 +++++++------------ crates/iota-core/src/consensus_handler.rs | 2 +- .../tests/object_deletion_tests.rs | 21 ++++------ .../tests/protocol_version_tests.rs | 16 ++++--- crates/iota-framework/src/lib.rs | 2 +- crates/iota-node/src/handle.rs | 4 +- crates/iota-protocol-config/src/lib.rs | 4 +- crates/iota-simulator/src/lib.rs | 2 +- crates/iota-storage/tests/key_value_tests.rs | 9 ++-- crates/iota-swarm/src/memory/container-sim.rs | 6 ++- .../iota-types/src/iota_system_state/mod.rs | 2 +- scripts/simtest/clippy.sh | 20 +++++++++ scripts/simtest/config-patch | 27 ++++++------ 18 files changed, 104 insertions(+), 87 deletions(-) create mode 100755 scripts/simtest/clippy.sh diff --git a/.github/workflows/_rust_lints.yml b/.github/workflows/_rust_lints.yml index f63dfb5cab0..e42b1b1eb76 100644 --- a/.github/workflows/_rust_lints.yml +++ b/.github/workflows/_rust_lints.yml @@ -62,3 +62,8 @@ jobs: # See '.cargo/config' for list of enabled/disabled clippy lints - name: Check Clippy Lints run: cargo ci-clippy + + - name: Check Clippy Lints with msim + run: | + git clean -fd + ./scripts/simtest/clippy.sh diff --git a/Cargo.toml b/Cargo.toml index 1a8a277a70d..55279866086 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -461,5 +461,3 @@ transaction-fuzzer = { path = "crates/transaction-fuzzer" } typed-store = { path = "crates/typed-store" } typed-store-derive = { path = "crates/typed-store-derive" } typed-store-error = { path = "crates/typed-store-error" } - -[patch.crates-io] diff --git a/crates/iota-benchmark/tests/simtest.rs b/crates/iota-benchmark/tests/simtest.rs index e9e9d3b68c6..e3a161bdd33 100644 --- a/crates/iota-benchmark/tests/simtest.rs +++ b/crates/iota-benchmark/tests/simtest.rs @@ -569,7 +569,7 @@ mod test { .collect() }) .unwrap_or_else(|_| vec![]); - assert!(checkpoint_files.len() > 0); + assert!(!checkpoint_files.is_empty()); let bytes = std::fs::read(checkpoint_files.first().unwrap()).unwrap(); let _checkpoint: CheckpointData = @@ -792,11 +792,7 @@ mod test { return false; // don't fail ineligible nodes } let mut rng = thread_rng(); - if rng.gen_range(0.0..1.0) < probability { - true - } else { - false - } + rng.gen_range(0.0..1.0) < probability } async fn build_test_cluster( @@ -901,7 +897,7 @@ mod test { let bank = BenchmarkBank::new(proxy.clone(), primary_coin); let system_state_observer = { let mut system_state_observer = SystemStateObserver::new(proxy.clone()); - if let Ok(_) = system_state_observer.state.changed().await { + if system_state_observer.state.changed().await.is_ok() { info!( "Got the new state (reference gas price and/or protocol config) from system state object" ); diff --git a/crates/iota-config/src/local_ip_utils.rs b/crates/iota-config/src/local_ip_utils.rs index 0675eeb4354..406177a3e7d 100644 --- a/crates/iota-config/src/local_ip_utils.rs +++ b/crates/iota-config/src/local_ip_utils.rs @@ -18,13 +18,20 @@ pub struct SimAddressManager { } #[cfg(msim)] -impl SimAddressManager { - pub fn new() -> Self { +impl Default for SimAddressManager { + fn default() -> Self { Self { next_ip_offset: AtomicI16::new(1), next_port: AtomicI16::new(9000), } } +} + +#[cfg(msim)] +impl SimAddressManager { + pub fn new() -> Self { + Self::default() + } pub fn get_next_ip(&self) -> String { let offset = self diff --git a/crates/iota-core/src/authority.rs b/crates/iota-core/src/authority.rs index ea62b573629..bc3ad09453c 100644 --- a/crates/iota-core/src/authority.rs +++ b/crates/iota-core/src/authority.rs @@ -1284,7 +1284,7 @@ impl AuthorityState { let digest = *certificate.digest(); fail_point_if!("correlated-crash-process-certificate", || { - if iota_simulator::random::deterministic_probability_once(&digest, 0.01) { + if iota_simulator::random::deterministic_probability_once(digest, 0.01) { iota_simulator::task::kill_current_node(None); } }); @@ -4185,7 +4185,7 @@ impl AuthorityState { #[cfg(msim)] let extra_packages = framework_injection::get_extra_packages(self.name); #[cfg(msim)] - let system_packages = system_packages.map(|p| p).chain(extra_packages.iter()); + let system_packages = system_packages.chain(&extra_packages); for system_package in system_packages { let modules = system_package.modules().to_vec(); @@ -5105,7 +5105,7 @@ pub mod framework_injection { } pub fn get_extra_packages(name: AuthorityName) -> Vec { - let built_in = BTreeSet::from_iter(BuiltInFramework::all_package_ids().into_iter()); + let built_in = BTreeSet::from_iter(BuiltInFramework::all_package_ids()); let extra: Vec = OVERRIDE.with(|cfg| { cfg.borrow() .keys() diff --git a/crates/iota-core/src/checkpoints/mod.rs b/crates/iota-core/src/checkpoints/mod.rs index 1277b5d4c51..3c819e3c8c6 100644 --- a/crates/iota-core/src/checkpoints/mod.rs +++ b/crates/iota-core/src/checkpoints/mod.rs @@ -1698,18 +1698,12 @@ impl CheckpointBuilder { let ccps = root_txs .iter() .filter_map(|tx| { - if let Some(tx) = tx { - if matches!( + tx.as_ref().filter(|tx| { + matches!( tx.transaction_data().kind(), TransactionKind::ConsensusCommitPrologueV1(_) - ) { - Some(tx) - } else { - None - } - } else { - None - } + ) + }) }) .collect::>(); @@ -1724,22 +1718,20 @@ impl CheckpointBuilder { .multi_get_transaction_blocks( &sorted .iter() - .map(|tx| tx.transaction_digest().clone()) + .map(|tx| *tx.transaction_digest()) .collect::>(), ) .unwrap(); - if ccps.len() == 0 { + if ccps.is_empty() { // If there is no consensus commit prologue transaction in the roots, then there // should be no consensus commit prologue transaction in the // checkpoint. - for tx in txs.iter() { - if let Some(tx) = tx { - assert!(!matches!( - tx.transaction_data().kind(), - TransactionKind::ConsensusCommitPrologueV1(_) - )); - } + for tx in txs.iter().flatten() { + assert!(!matches!( + tx.transaction_data().kind(), + TransactionKind::ConsensusCommitPrologueV1(_) + )); } } else { // If there is one consensus commit prologue, it must be the first one in the @@ -1751,13 +1743,11 @@ impl CheckpointBuilder { assert_eq!(ccps[0].digest(), txs[0].as_ref().unwrap().digest()); - for tx in txs.iter().skip(1) { - if let Some(tx) = tx { - assert!(!matches!( - tx.transaction_data().kind(), - TransactionKind::ConsensusCommitPrologueV1(_) - )); - } + for tx in txs.iter().skip(1).flatten() { + assert!(!matches!( + tx.transaction_data().kind(), + TransactionKind::ConsensusCommitPrologueV1(_) + )); } } } diff --git a/crates/iota-core/src/consensus_handler.rs b/crates/iota-core/src/consensus_handler.rs index eb785977989..e3146be27b4 100644 --- a/crates/iota-core/src/consensus_handler.rs +++ b/crates/iota-core/src/consensus_handler.rs @@ -403,7 +403,7 @@ impl ConsensusHandler { fail_point_if!("correlated-crash-after-consensus-commit-boundary", || { let key = [commit_sub_dag_index, self.epoch_store.epoch()]; - if iota_simulator::random::deterministic_probability(&key, 0.01) { + if iota_simulator::random::deterministic_probability(key, 0.01) { iota_simulator::task::kill_current_node(None); } }); diff --git a/crates/iota-e2e-tests/tests/object_deletion_tests.rs b/crates/iota-e2e-tests/tests/object_deletion_tests.rs index e3f116ac41d..6aab16a39d1 100644 --- a/crates/iota-e2e-tests/tests/object_deletion_tests.rs +++ b/crates/iota-e2e-tests/tests/object_deletion_tests.rs @@ -30,10 +30,9 @@ mod sim_only_tests { // root object. let (package_id, object_id) = publish_package_and_create_parent_object(&test_cluster).await; let child_id = create_owned_child(&test_cluster, package_id).await; - let wrap_child_txn_digest = wrap_child(&test_cluster, package_id, object_id, child_id) + let wrap_child_txn_digest = *wrap_child(&test_cluster, package_id, object_id, child_id) .await - .transaction_digest() - .clone(); + .transaction_digest(); fullnode .with_async(|node| async { @@ -46,7 +45,7 @@ mod sim_only_tests { .unwrap(); // Wait until the above checkpoint is pruned. - let _ = timeout( + timeout( Duration::from_secs(60), wait_until_checkpoint_pruned(node, checkpoint), ) @@ -80,14 +79,12 @@ mod sim_only_tests { // Next, we unwrap and delete the child object, as well as delete the root // object. let unwrap_delete_txn_digest = - unwrap_and_delete_child(&test_cluster, package_id, object_id) + *unwrap_and_delete_child(&test_cluster, package_id, object_id) .await - .transaction_digest() - .clone(); - let delete_root_obj_txn_digest = delete_object(&test_cluster, package_id, object_id) + .transaction_digest(); + let delete_root_obj_txn_digest = *delete_object(&test_cluster, package_id, object_id) .await - .transaction_digest() - .clone(); + .transaction_digest(); fullnode .with_async(|node| async { @@ -105,7 +102,7 @@ mod sim_only_tests { .await .unwrap(); - let _ = timeout( + timeout( Duration::from_secs(60), wait_until_checkpoint_pruned(node, std::cmp::max(checkpoint1, checkpoint2)), ) @@ -269,7 +266,7 @@ mod sim_only_tests { if let Some(seq) = node .state() .epoch_store_for_testing() - .get_transaction_checkpoint(&digest) + .get_transaction_checkpoint(digest) .unwrap() { return seq; diff --git a/crates/iota-e2e-tests/tests/protocol_version_tests.rs b/crates/iota-e2e-tests/tests/protocol_version_tests.rs index 38369359e08..4d34b9a39bc 100644 --- a/crates/iota-e2e-tests/tests/protocol_version_tests.rs +++ b/crates/iota-e2e-tests/tests/protocol_version_tests.rs @@ -129,7 +129,7 @@ mod sim_only_tests { .build() .await; - let validator = test_cluster.get_validator_pubkeys()[0].clone(); + let validator = test_cluster.get_validator_pubkeys()[0]; test_cluster.stop_node(&validator); assert_eq!( @@ -482,7 +482,7 @@ mod sim_only_tests { } async fn create_obj(cluster: &TestCluster) -> ObjectRef { - execute_creating(cluster, { + *execute_creating(cluster, { let mut builder = ProgrammableTransactionBuilder::new(); builder .move_call( @@ -500,11 +500,10 @@ mod sim_only_tests { .await .first() .unwrap() - .clone() } async fn wrap_obj(cluster: &TestCluster, obj: ObjectRef) -> ObjectRef { - execute_creating(cluster, { + *execute_creating(cluster, { let mut builder = ProgrammableTransactionBuilder::new(); builder .move_call( @@ -521,7 +520,6 @@ mod sim_only_tests { .await .first() .unwrap() - .clone() } async fn transfer_obj( @@ -569,9 +567,9 @@ mod sim_only_tests { .unwrap(); let results = response.results.unwrap(); - let return_ = &results.first().unwrap().return_values.first().unwrap().0; + let return_val = &results.first().unwrap().return_values.first().unwrap().0; - bcs::from_bytes(&return_).unwrap() + bcs::from_bytes(return_val).unwrap() } async fn execute_creating( @@ -610,11 +608,11 @@ mod sim_only_tests { } async fn expect_upgrade_failed(cluster: &TestCluster) { - monitor_version_change(&cluster, START /* expected proto version */).await; + monitor_version_change(cluster, START /* expected proto version */).await; } async fn expect_upgrade_succeeded(cluster: &TestCluster) { - monitor_version_change(&cluster, FINISH /* expected proto version */).await; + monitor_version_change(cluster, FINISH /* expected proto version */).await; } async fn get_framework_upgrade_versions( diff --git a/crates/iota-framework/src/lib.rs b/crates/iota-framework/src/lib.rs index d31cc29102d..1fb3d8da931 100644 --- a/crates/iota-framework/src/lib.rs +++ b/crates/iota-framework/src/lib.rs @@ -106,7 +106,7 @@ macro_rules! define_system_packages { pub struct BuiltInFramework; impl BuiltInFramework { - pub fn iter_system_packages() -> impl Iterator { + pub fn iter_system_packages<'a>() -> impl Iterator { // All system packages in the current build should be registered here, and this // is the only place we need to worry about if any of them changes. // TODO: Is it possible to derive dependencies from the bytecode instead of diff --git a/crates/iota-node/src/handle.rs b/crates/iota-node/src/handle.rs index 5371127680e..729b281cbec 100644 --- a/crates/iota-node/src/handle.rs +++ b/crates/iota-node/src/handle.rs @@ -135,7 +135,9 @@ impl Drop for IotaNodeHandle { fn drop(&mut self) { if self.shutdown_on_drop { let node_id = self.inner().sim_state.sim_node.id(); - iota_simulator::runtime::Handle::try_current().map(|h| h.delete_node(node_id)); + if let Some(h) = iota_simulator::runtime::Handle::try_current() { + h.delete_node(node_id); + } } } } diff --git a/crates/iota-protocol-config/src/lib.rs b/crates/iota-protocol-config/src/lib.rs index d13df4dfaab..fe3b91a845e 100644 --- a/crates/iota-protocol-config/src/lib.rs +++ b/crates/iota-protocol-config/src/lib.rs @@ -1053,12 +1053,12 @@ impl ProtocolConfig { } #[cfg(not(msim))] -static POISON_VERSION_METHODS: AtomicBool = AtomicBool::new(false); +static POISON_VERSION_METHODS: AtomicBool = const { AtomicBool::new(false) }; // Use a thread local in sim tests for test isolation. #[cfg(msim)] thread_local! { - static POISON_VERSION_METHODS: AtomicBool = AtomicBool::new(false); + static POISON_VERSION_METHODS: AtomicBool = const { AtomicBool::new(false) }; } // Instantiations for each protocol version. diff --git a/crates/iota-simulator/src/lib.rs b/crates/iota-simulator/src/lib.rs index 7bafac71cb5..815e9244a93 100644 --- a/crates/iota-simulator/src/lib.rs +++ b/crates/iota-simulator/src/lib.rs @@ -87,7 +87,7 @@ pub mod configs { env_configs: impl IntoIterator, ) -> SimConfig { let mut env_configs = HashMap::<&'static str, SimConfig>::from_iter(env_configs); - if let Some(env) = std::env::var("IOTA_SIM_CONFIG").ok() { + if let Ok(env) = std::env::var("IOTA_SIM_CONFIG") { if let Some(cfg) = env_configs.remove(env.as_str()) { info!("Using test config for IOTA_SIM_CONFIG={}", env); cfg diff --git a/crates/iota-storage/tests/key_value_tests.rs b/crates/iota-storage/tests/key_value_tests.rs index d6a78c0a868..dcab8d1be28 100644 --- a/crates/iota-storage/tests/key_value_tests.rs +++ b/crates/iota-storage/tests/key_value_tests.rs @@ -439,10 +439,9 @@ mod simtests { use super::*; - async fn svc( - State(state): State>>>>, - request: Request, - ) -> Response { + type Storage = HashMap>; + + async fn svc(State(state): State>>, request: Request) -> Response { let path = request.uri().path().to_string(); let key = path.trim_start_matches('/'); let value = state.lock().unwrap().get(key).cloned(); @@ -456,7 +455,7 @@ mod simtests { } } - async fn test_server(data: Arc>>>) { + async fn test_server(data: Arc>) { let handle = iota_simulator::runtime::Handle::current(); let builder = handle.create_node(); let (startup_sender, mut startup_receiver) = tokio::sync::watch::channel(false); diff --git a/crates/iota-swarm/src/memory/container-sim.rs b/crates/iota-swarm/src/memory/container-sim.rs index 7d637f3ccab..fb960014ea7 100644 --- a/crates/iota-swarm/src/memory/container-sim.rs +++ b/crates/iota-swarm/src/memory/container-sim.rs @@ -34,7 +34,9 @@ impl Drop for Container { fn drop(&mut self) { if let Some(handle) = self.handle.take() { tracing::info!("shutting down {}", handle.node_id); - iota_simulator::runtime::Handle::try_current().map(|h| h.delete_node(handle.node_id)); + if let Some(h) = iota_simulator::runtime::Handle::try_current() { + h.delete_node(handle.node_id); + } } } } @@ -57,7 +59,7 @@ impl Container { let startup_sender = Arc::new(startup_sender); let node = builder .ip(ip) - .name(&format!("{:?}", config.authority_public_key().concise())) + .name(format!("{:?}", config.authority_public_key().concise())) .init(move || { info!("Node restarted"); let config = config.clone(); diff --git a/crates/iota-types/src/iota_system_state/mod.rs b/crates/iota-types/src/iota_system_state/mod.rs index 5e5634a53d5..18f4fec702d 100644 --- a/crates/iota-types/src/iota_system_state/mod.rs +++ b/crates/iota-types/src/iota_system_state/mod.rs @@ -438,7 +438,7 @@ pub mod advance_epoch_result_injection { thread_local! { /// Override the result of advance_epoch in the range [start, end). - static OVERRIDE: RefCell> = RefCell::new(None); + static OVERRIDE: RefCell> = const { RefCell::new(None) }; } /// Override the result of advance_epoch transaction if new epoch is in the diff --git a/scripts/simtest/clippy.sh b/scripts/simtest/clippy.sh new file mode 100755 index 00000000000..951e372c211 --- /dev/null +++ b/scripts/simtest/clippy.sh @@ -0,0 +1,20 @@ +#!/bin/bash -e +# Copyright (c) 2024 IOTA Stiftung +# SPDX-License-Identifier: Apache-2.0 + +# verify that git repo is clean +if [[ -n $(git status -s) ]]; then + echo "Working directory is not clean. Please commit all changes before running this script." + exit 1 +fi + +# apply git patch +git apply ./scripts/simtest/config-patch + +root_dir=$(git rev-parse --show-toplevel) +export SIMTEST_STATIC_INIT_MOVE=$root_dir"/examples/move/basics" + +cargo ci-clippy + +# remove the patch +git checkout . diff --git a/scripts/simtest/config-patch b/scripts/simtest/config-patch index fb90ecb83fd..c186afaf26a 100644 --- a/scripts/simtest/config-patch +++ b/scripts/simtest/config-patch @@ -1,21 +1,24 @@ -diff --git a/.cargo/config b/.cargo/config -index ec2c459490..55985cbe9f 100644 ---- a/.cargo/config -+++ b/.cargo/config -@@ -25,4 +25,4 @@ move-clippy = [ - ] - +diff --git a/.cargo/config.toml b/.cargo/config.toml +index cc5620ecfe..d2ad8f1dac 100644 +--- a/.cargo/config.toml ++++ b/.cargo/config.toml +@@ -6,7 +6,7 @@ ci-udeps = "udeps --all-targets --backend=depinfo" + ci-udeps-external = "udeps --all-targets --manifest-path external-crates/move/Cargo.toml --backend=depinfo" + [build] -rustflags = ["-C", "force-frame-pointers=yes", "-C", "force-unwind-tables=yes"] +rustflags = ["-C", "force-frame-pointers=yes", "-C", "force-unwind-tables=yes", "--cfg", "msim"] + + # 64 bit MSVC, override default 1M stack with 8M stack + [target.x86_64-pc-windows-msvc] diff --git a/Cargo.toml b/Cargo.toml -index c0829bc1b6..4007f97d66 100644 +index a2371ff16f..57a1fedaa5 100644 --- a/Cargo.toml +++ b/Cargo.toml -@@ -682,5 +682,7 @@ field_names = "0.2.0" - semver = "1.0.16" - spinners = "4.1.0" - include_dir = "0.7.3" +@@ -461,3 +461,7 @@ transaction-fuzzer = { path = "crates/transaction-fuzzer" } + typed-store = { path = "crates/typed-store" } + typed-store-derive = { path = "crates/typed-store-derive" } + typed-store-error = { path = "crates/typed-store-error" } + +[patch.crates-io] +tokio = { git = "https://github.com/iotaledger/iota-sim.git", rev = "f16ef50ba7d874fe1f0960f248f6c651a634d6a5" }