Skip to content

Commit

Permalink
fix: Fix hard static analysis errors (#14)
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
popzxc authored Aug 30, 2024
1 parent 234ad78 commit ce50752
Show file tree
Hide file tree
Showing 13 changed files with 50 additions and 32 deletions.
15 changes: 12 additions & 3 deletions crates/zkEVM-assembly/src/assembly/linking/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,22 @@ impl<const N: usize, E: VmEncodingMode<N>> Linker<N, E> {
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()));
}

Expand Down
12 changes: 12 additions & 0 deletions crates/zk_evm/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
2 changes: 1 addition & 1 deletion crates/zk_evm/src/opcodes/execution/far_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
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,
Expand Down
10 changes: 5 additions & 5 deletions crates/zk_evm/src/opcodes/execution/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
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());
}

Expand Down Expand Up @@ -298,7 +298,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
vm_state.local_state.monotonic_cycle_counter,
PrimitiveValue::empty(),
dst0_mem_location,
&self,
self,
);

(PubdataCost(0), 0)
Expand Down Expand Up @@ -366,7 +366,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
vm_state.local_state.monotonic_cycle_counter,
result,
dst0_mem_location,
&self,
self,
);

let extra_pubdata_cost = precompile_aux_data.extra_pubdata_cost;
Expand Down Expand Up @@ -419,7 +419,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
offset: 0,
memory_page: output_memory_page.0,
start: 0,
length: preimage_len_in_bytes as u32,
length: preimage_len_in_bytes,
};

(
Expand All @@ -436,7 +436,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
vm_state.local_state.monotonic_cycle_counter,
dst_0_value,
dst0_mem_location,
&self,
self,
);

(pubdata_cost, refund)
Expand Down
2 changes: 1 addition & 1 deletion crates/zk_evm/src/opcodes/execution/near_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
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;
Expand Down
2 changes: 1 addition & 1 deletion crates/zk_evm/src/opcodes/execution/sub.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
vm_state.local_state.monotonic_cycle_counter,
result,
dst0_mem_location,
&self,
self,
);
}
}
4 changes: 2 additions & 2 deletions crates/zk_evm/src/opcodes/execution/uma.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
vm_state.local_state.monotonic_cycle_counter,
result,
dst0_mem_location,
&self,
self,
);

if increment_offset {
Expand Down Expand Up @@ -423,7 +423,7 @@ impl<const N: usize, E: VmEncodingMode<N>> DecodedOpcode<N, E> {
vm_state.local_state.monotonic_cycle_counter,
result,
dst0_mem_location,
&self,
self,
);
}
} else {
Expand Down
6 changes: 2 additions & 4 deletions crates/zk_evm/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ lazy_static! {
}

pub fn contract_bytecode_to_words(code: &[[u8; 32]]) -> Vec<U256> {
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)
Expand Down Expand Up @@ -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<M: Memory> {
_marker: std::marker::PhantomData<M>,
}
Expand Down
12 changes: 5 additions & 7 deletions crates/zk_evm/src/vm_state/cycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(())
Expand Down
2 changes: 1 addition & 1 deletion crates/zk_evm/src/vm_state/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 1 addition & 3 deletions crates/zk_evm/src/vm_state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -101,7 +99,7 @@ impl<const N: usize, E: VmEncodingMode<N>> VmLocalState<N, E> {
}

pub fn timestamp_for_code_or_src_read(&self) -> Timestamp {
Timestamp(self.timestamp + 0)
Timestamp(self.timestamp)
}

pub fn callstack_is_full(&self) -> bool {
Expand Down
8 changes: 6 additions & 2 deletions crates/zkevm_test_harness/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#![recursion_limit = "32"]
#![allow(dropping_references)]
#![feature(allocator_api)]
#![feature(array_chunks)]
#![feature(stmt_expr_attributes)]
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit ce50752

Please sign in to comment.