From ce50752e8c277537c40e3e16cfd6bc6f7ab8e700 Mon Sep 17 00:00:00 2001 From: Igor Aleksanov Date: Fri, 30 Aug 2024 14:34:50 +0400 Subject: [PATCH] fix: Fix hard static analysis errors (#14) - Fixes all the "red" errors (deny by default) observable in the monorepo. - Fixes/silences _some_ of the "yellow" errors (there are just too many for a single PR). The ultimate goal is to enable clippy in CI. --- crates/zkEVM-assembly/src/assembly/linking/mod.rs | 15 ++++++++++++--- crates/zk_evm/src/lib.rs | 12 ++++++++++++ crates/zk_evm/src/opcodes/execution/far_call.rs | 2 +- crates/zk_evm/src/opcodes/execution/log.rs | 10 +++++----- crates/zk_evm/src/opcodes/execution/near_call.rs | 2 +- crates/zk_evm/src/opcodes/execution/sub.rs | 2 +- crates/zk_evm/src/opcodes/execution/uma.rs | 4 ++-- crates/zk_evm/src/utils.rs | 6 ++---- crates/zk_evm/src/vm_state/cycle.rs | 12 +++++------- crates/zk_evm/src/vm_state/helpers.rs | 2 +- crates/zk_evm/src/vm_state/mod.rs | 4 +--- crates/zkevm_test_harness/src/lib.rs | 8 ++++++-- .../individual_circuits/storage_sort_dedup.rs | 3 +-- 13 files changed, 50 insertions(+), 32 deletions(-) diff --git a/crates/zkEVM-assembly/src/assembly/linking/mod.rs b/crates/zkEVM-assembly/src/assembly/linking/mod.rs index a4899c5..9ebbec9 100644 --- a/crates/zkEVM-assembly/src/assembly/linking/mod.rs +++ b/crates/zkEVM-assembly/src/assembly/linking/mod.rs @@ -289,13 +289,22 @@ impl> Linker { let all_constant_labels_to_offset = constant_labels_to_offset.keys().cloned().collect(); let all_globals_labels_to_offset = globals_labels_to_offset.keys().cloned().collect(); - for el in all_function_labels_to_pc.intersection(&all_constant_labels_to_offset) { + if let Some(el) = all_function_labels_to_pc + .intersection(&all_constant_labels_to_offset) + .next() + { return Err(AssemblyParseError::DuplicateLabel(el.clone())); } - for el in all_function_labels_to_pc.intersection(&all_globals_labels_to_offset) { + if let Some(el) = all_function_labels_to_pc + .intersection(&all_globals_labels_to_offset) + .next() + { return Err(AssemblyParseError::DuplicateLabel(el.clone())); } - for el in all_constant_labels_to_offset.intersection(&all_globals_labels_to_offset) { + if let Some(el) = all_constant_labels_to_offset + .intersection(&all_globals_labels_to_offset) + .next() + { return Err(AssemblyParseError::DuplicateLabel(el.clone())); } diff --git a/crates/zk_evm/src/lib.rs b/crates/zk_evm/src/lib.rs index e8e38bd..ffc42a2 100644 --- a/crates/zk_evm/src/lib.rs +++ b/crates/zk_evm/src/lib.rs @@ -1,3 +1,15 @@ +#![allow( + clippy::bool_comparison, // Local preference. + clippy::bool_assert_comparison, // Local preference. + clippy::match_like_matches_macro, // Doesn't always look better. + clippy::let_and_return, // Worsens readability. + clippy::collapsible_else_if, // Local preference. + clippy::collapsible_if, // Local preference. + clippy::collapsible_match, // Local preference. + clippy::assign_op_pattern, // Local preference. + clippy::single_match, // Local preference. +)] + pub mod block_properties; pub mod errors; pub mod flags; diff --git a/crates/zk_evm/src/opcodes/execution/far_call.rs b/crates/zk_evm/src/opcodes/execution/far_call.rs index 9d55d23..b974ff7 100644 --- a/crates/zk_evm/src/opcodes/execution/far_call.rs +++ b/crates/zk_evm/src/opcodes/execution/far_call.rs @@ -624,7 +624,7 @@ impl> DecodedOpcode { code_page: mapped_code_page, sp: E::PcOrImm::from_u64_clipped(INITIAL_SP_ON_FAR_CALL), pc: E::PcOrImm::from_u64_clipped(0u64), - exception_handler_location: exception_handler_location, + exception_handler_location, ergs_remaining: passed_ergs, this_shard_id: new_this_shard_id, caller_shard_id, diff --git a/crates/zk_evm/src/opcodes/execution/log.rs b/crates/zk_evm/src/opcodes/execution/log.rs index 8da8a84..4774dcf 100644 --- a/crates/zk_evm/src/opcodes/execution/log.rs +++ b/crates/zk_evm/src/opcodes/execution/log.rs @@ -239,7 +239,7 @@ impl> DecodedOpcode { assert_eq!(pubdata_cost.0, 0); assert_eq!(ergs_refund, 0); } else { - assert!(abs(pubdata_cost.0) <= MAX_PUBDATA_COST_PER_QUERY as i32); + assert!(abs(pubdata_cost.0) <= MAX_PUBDATA_COST_PER_QUERY); assert!(ergs_refund <= LogOpcode::StorageWrite.ergs_price()); } @@ -298,7 +298,7 @@ impl> DecodedOpcode { vm_state.local_state.monotonic_cycle_counter, PrimitiveValue::empty(), dst0_mem_location, - &self, + self, ); (PubdataCost(0), 0) @@ -366,7 +366,7 @@ impl> DecodedOpcode { vm_state.local_state.monotonic_cycle_counter, result, dst0_mem_location, - &self, + self, ); let extra_pubdata_cost = precompile_aux_data.extra_pubdata_cost; @@ -419,7 +419,7 @@ impl> DecodedOpcode { offset: 0, memory_page: output_memory_page.0, start: 0, - length: preimage_len_in_bytes as u32, + length: preimage_len_in_bytes, }; ( @@ -436,7 +436,7 @@ impl> DecodedOpcode { vm_state.local_state.monotonic_cycle_counter, dst_0_value, dst0_mem_location, - &self, + self, ); (pubdata_cost, refund) diff --git a/crates/zk_evm/src/opcodes/execution/near_call.rs b/crates/zk_evm/src/opcodes/execution/near_call.rs index cc5aae4..19c0abf 100644 --- a/crates/zk_evm/src/opcodes/execution/near_call.rs +++ b/crates/zk_evm/src/opcodes/execution/near_call.rs @@ -67,7 +67,7 @@ impl> DecodedOpcode { let current_stack = vm_state.local_state.callstack.get_current_stack(); // we only need to change a PC and formally start a new context - let mut new_stack = current_stack.clone(); + let mut new_stack = *current_stack; new_stack.pc = dst; new_stack.exception_handler_location = exception_handler_location; new_stack.ergs_remaining = passed_ergs; diff --git a/crates/zk_evm/src/opcodes/execution/sub.rs b/crates/zk_evm/src/opcodes/execution/sub.rs index 24851cd..a7d6cdb 100644 --- a/crates/zk_evm/src/opcodes/execution/sub.rs +++ b/crates/zk_evm/src/opcodes/execution/sub.rs @@ -50,7 +50,7 @@ impl> DecodedOpcode { vm_state.local_state.monotonic_cycle_counter, result, dst0_mem_location, - &self, + self, ); } } diff --git a/crates/zk_evm/src/opcodes/execution/uma.rs b/crates/zk_evm/src/opcodes/execution/uma.rs index 06545d8..1d52f01 100644 --- a/crates/zk_evm/src/opcodes/execution/uma.rs +++ b/crates/zk_evm/src/opcodes/execution/uma.rs @@ -338,7 +338,7 @@ impl> DecodedOpcode { vm_state.local_state.monotonic_cycle_counter, result, dst0_mem_location, - &self, + self, ); if increment_offset { @@ -423,7 +423,7 @@ impl> DecodedOpcode { vm_state.local_state.monotonic_cycle_counter, result, dst0_mem_location, - &self, + self, ); } } else { diff --git a/crates/zk_evm/src/utils.rs b/crates/zk_evm/src/utils.rs index f750ef7..0179b79 100644 --- a/crates/zk_evm/src/utils.rs +++ b/crates/zk_evm/src/utils.rs @@ -10,9 +10,7 @@ lazy_static! { } pub fn contract_bytecode_to_words(code: &[[u8; 32]]) -> Vec { - code.into_iter() - .map(|el| U256::from_big_endian(el)) - .collect() + code.iter().map(|el| U256::from_big_endian(el)).collect() // for code each 8 byte sequence is somehow encoded integer, // or full 32 byte word is an integer constant (also encoded with some endianess) @@ -47,7 +45,7 @@ pub fn u256_to_address_unchecked(integer: &U256) -> crate::Address { crate::Address::from_slice(&buffer[12..32]) } -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Default, Clone, Copy)] pub struct GenericNoopTracer { _marker: std::marker::PhantomData, } diff --git a/crates/zk_evm/src/vm_state/cycle.rs b/crates/zk_evm/src/vm_state/cycle.rs index e7cee2d..f42d97f 100644 --- a/crates/zk_evm/src/vm_state/cycle.rs +++ b/crates/zk_evm/src/vm_state/cycle.rs @@ -80,7 +80,7 @@ pub fn read_and_decode< // global generic tracing if DT::CALL_BEFORE_DECODING { let local_state = VmLocalStateData { - vm_local_state: &local_state, + vm_local_state: local_state, }; tracer.before_decoding(local_state, memory); @@ -121,7 +121,6 @@ pub fn read_and_decode< // then for our BE machine we should consider that "first" bytes, that will be // our integer's "highest" bytes, so to follow bytearray-like enumeration we // have to use inverse order here - let u256_word = u256_word; E::integer_representaiton_from_u256(u256_word, sub_pc) } else { // use a saved one @@ -180,8 +179,7 @@ pub fn read_and_decode< } // now try to get ergs price (unmodified for hard cases), that will also allow us to catch invalid opcode - let mut ergs_cost = - zkevm_opcode_defs::OPCODES_PRICES[opcode_raw_variant_idx.into_usize()] as u32; + let mut ergs_cost: u32 = zkevm_opcode_defs::OPCODES_PRICES[opcode_raw_variant_idx.into_usize()]; if skip_cycle { // we have already paid for it ergs_cost = 0; @@ -319,7 +317,7 @@ impl< let (after_masking_decoded, delayed_changes, skip_cycle) = read_and_decode( &self.local_state, - &mut self.memory, + &self.memory, &mut self.witness_tracer, tracer, ); @@ -421,7 +419,7 @@ impl< new_pc, }; - tracer.before_execution(local_state, data, &mut self.memory); + tracer.before_execution(local_state, data, &self.memory); } let is_kernel_mode = self @@ -481,7 +479,7 @@ impl< dst0_mem_location, }; - tracer.after_execution(local_state, data, &mut self.memory); + tracer.after_execution(local_state, data, &self.memory); } Ok(()) diff --git a/crates/zk_evm/src/vm_state/helpers.rs b/crates/zk_evm/src/vm_state/helpers.rs index 4a16644..33357bd 100644 --- a/crates/zk_evm/src/vm_state/helpers.rs +++ b/crates/zk_evm/src/vm_state/helpers.rs @@ -81,7 +81,7 @@ impl< pub fn read_code(&mut self, monotonic_cycle_counter: u32, key: MemoryKey) -> MemoryQuery { read_code( - &mut self.memory, + &self.memory, &mut self.witness_tracer, monotonic_cycle_counter, key, diff --git a/crates/zk_evm/src/vm_state/mod.rs b/crates/zk_evm/src/vm_state/mod.rs index be2095d..b699d3c 100644 --- a/crates/zk_evm/src/vm_state/mod.rs +++ b/crates/zk_evm/src/vm_state/mod.rs @@ -23,8 +23,6 @@ pub const SUPPORTED_ISA_VERSION: ISAVersion = ISAVersion(2); const _: () = if SUPPORTED_ISA_VERSION.0 != zkevm_opcode_defs::DEFAULT_ISA_VERSION.0 { panic!() -} else { - () }; use crate::zkevm_opcode_defs::{STARTING_BASE_PAGE, STARTING_TIMESTAMP}; @@ -101,7 +99,7 @@ impl> VmLocalState { } pub fn timestamp_for_code_or_src_read(&self) -> Timestamp { - Timestamp(self.timestamp + 0) + Timestamp(self.timestamp) } pub fn callstack_is_full(&self) -> bool { diff --git a/crates/zkevm_test_harness/src/lib.rs b/crates/zkevm_test_harness/src/lib.rs index 234c8ab..c3bd543 100644 --- a/crates/zkevm_test_harness/src/lib.rs +++ b/crates/zkevm_test_harness/src/lib.rs @@ -1,5 +1,4 @@ #![recursion_limit = "32"] -#![allow(dropping_references)] #![feature(allocator_api)] #![feature(array_chunks)] #![feature(stmt_expr_attributes)] @@ -9,7 +8,12 @@ #![feature(associated_type_defaults)] #![feature(bigint_helper_methods)] #![allow(unused_imports)] -#![allow(clippy::drop_ref)] +#![allow( + dropping_references, + clippy::needless_borrow, + clippy::needless_borrows_for_generic_args, + clippy::needless_range_loop +)] use crate::boojum::field::goldilocks::GoldilocksField; use crate::boojum::implementations::poseidon2::Poseidon2Goldilocks; diff --git a/crates/zkevm_test_harness/src/witness/individual_circuits/storage_sort_dedup.rs b/crates/zkevm_test_harness/src/witness/individual_circuits/storage_sort_dedup.rs index b0618d8..e0ea703 100644 --- a/crates/zkevm_test_harness/src/witness/individual_circuits/storage_sort_dedup.rs +++ b/crates/zkevm_test_harness/src/witness/individual_circuits/storage_sort_dedup.rs @@ -364,8 +364,7 @@ pub(crate) fn compute_storage_dedup_and_sort< } else { // read if new_this_cell_current_depth == 0 { - new_this_cell_has_explicit_read_and_rollback_depth_zero = - true || new_this_cell_has_explicit_read_and_rollback_depth_zero; + new_this_cell_has_explicit_read_and_rollback_depth_zero = true; } new_this_cell_current_value = item.raw_query.read_value; }