Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable more clippy lint #10150

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,34 @@ repository = "https://github.com/foundry-rs/foundry"
exclude = ["benches/", "tests/", "test-data/", "testdata/"]

[workspace.lints.clippy]
dbg-macro = "warn"
clear_with_drain = "warn"
cloned_instead_of_copied = "warn"
dbg_macro = "warn"
derive_partial_eq_without_eq = "warn"
explicit_into_iter_loop = "warn"
explicit_iter_loop = "warn"
flat_map_option = "warn"
from_iter_instead_of_collect = "warn"
implicit_clone = "warn"
imprecise_flops = "warn"
iter_on_empty_collections = "warn"
iter_with_drain = "warn"
iter_without_into_iter = "warn"
manual-string-new = "warn"
naive_bytecount = "warn"
needless_bitwise_bool = "warn"
needless_continue = "warn"
needless_for_each = "warn"
needless_pass_by_ref_mut = "warn"
nonstandard_macro_braces = "warn"
read_zero_byte_vec = "warn"
redundant_clone = "warn"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a fan of this, I'd rather enable an entire group than have to maintain a manual list of these, can we do something like clippy::all and clippy::whatever instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this pedantic = "warn"? Isn't this scope a bit too broad? Perhaps we can narrow the scope of this PR a little.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaniPopes could you pls chime in?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah that would be too much; I think all of these are fine except clippy::if_not_else, it's very common to have if !empty {} else {} which I feel like is more natural than the opposite

uninlined-format-args = "warn"
use-self = "warn"
redundant-clone = "warn"
octal-escapes = "allow"

# until <https://github.com/rust-lang/rust-clippy/issues/13885> is fixed
literal-string-with-formatting-args = "allow"
octal-escapes = "allow"
result_large_err = "allow"

[workspace.lints.rust]
Expand Down
6 changes: 3 additions & 3 deletions crates/anvil/core/src/eth/transaction/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -999,10 +999,10 @@ impl Decodable for TypedTransaction {
// Check byte after header
let ty = *h_decode_copy.first().ok_or(alloy_rlp::Error::Custom("empty slice"))?;

if ty != 0x7E {
Ok(TxEnvelope::decode(buf)?.into())
} else {
if ty == 0x7E {
Ok(Self::Deposit(DepositTransaction::decode_2718(buf)?))
} else {
Ok(TxEnvelope::decode(buf)?.into())
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions crates/anvil/src/eth/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,7 @@ impl EthApi {
node_info!("eth_signTransaction");

let from = request.from.map(Ok).unwrap_or_else(|| {
self.accounts()?.first().cloned().ok_or(BlockchainError::NoSignerAvailable)
self.accounts()?.first().copied().ok_or(BlockchainError::NoSignerAvailable)
})?;

let (nonce, _) = self.request_nonce(&request, from).await?;
Expand Down Expand Up @@ -986,7 +986,7 @@ impl EthApi {
node_info!("eth_sendTransaction");

let from = request.from.map(Ok).unwrap_or_else(|| {
self.accounts()?.first().cloned().ok_or(BlockchainError::NoSignerAvailable)
self.accounts()?.first().copied().ok_or(BlockchainError::NoSignerAvailable)
})?;
let (nonce, on_chain_nonce) = self.request_nonce(&request, from).await?;

Expand Down Expand Up @@ -2037,7 +2037,7 @@ impl EthApi {
};

let from = tx_req.from.map(Ok).unwrap_or_else(|| {
self.accounts()?.first().cloned().ok_or(BlockchainError::NoSignerAvailable)
self.accounts()?.first().copied().ok_or(BlockchainError::NoSignerAvailable)
})?;

// Get the nonce at the common block
Expand Down Expand Up @@ -2253,7 +2253,7 @@ impl EthApi {
}
}
}
block.transactions = BlockTransactions::Full(block_txs.to_vec());
block.transactions = BlockTransactions::Full(block_txs.clone());
blocks.push(block);
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/anvil/src/eth/backend/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ pub trait Db:

/// Deserialize and add all chain data to the backend storage
fn load_state(&mut self, state: SerializableState) -> DatabaseResult<bool> {
for (addr, account) in state.accounts.into_iter() {
for (addr, account) in state.accounts {
let old_account_nonce = DatabaseRef::basic_ref(self, addr)
.ok()
.and_then(|acc| acc.map(|acc| acc.nonce))
Expand All @@ -170,7 +170,7 @@ pub trait Db:
},
);

for (k, v) in account.storage.into_iter() {
for (k, v) in account.storage {
self.set_storage_at(addr, k, v)?;
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/anvil/src/eth/backend/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ impl<DB: Db + ?Sized, V: TransactionValidator> TransactionExecutor<'_, DB, V> {
let excess_blob_gas = if is_cancun { self.block_env.get_blob_excess_gas() } else { None };
let mut cumulative_blob_gas_used = if is_cancun { Some(0u64) } else { None };

for tx in self.into_iter() {
for tx in &mut self {
let tx = match tx {
TransactionExecutionOutcome::Executed(tx) => {
included.push(tx.transaction.clone());
Expand Down
8 changes: 4 additions & 4 deletions crates/anvil/src/eth/backend/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1413,10 +1413,10 @@ impl Backend {
gas_priority_fee: max_priority_fee_per_gas.map(U256::from),
max_fee_per_blob_gas: max_fee_per_blob_gas
.or_else(|| {
if !blob_hashes.is_empty() {
env.block.get_blob_gasprice()
} else {
if blob_hashes.is_empty() {
None
} else {
env.block.get_blob_gasprice()
}
})
.map(U256::from),
Expand Down Expand Up @@ -2594,7 +2594,7 @@ impl Backend {
.zip(storage_proofs)
.map(|(key, proof)| {
let storage_key: U256 = key.into();
let value = account.storage.get(&storage_key).cloned().unwrap_or_default();
let value = account.storage.get(&storage_key).copied().unwrap_or_default();
StorageProof { key: JsonStorageKey::Hash(key), value, proof }
})
.collect(),
Expand Down
2 changes: 1 addition & 1 deletion crates/anvil/src/eth/fees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ impl FeeHistoryService {
.filter_map(|p| {
let target_gas = (p * gas_used / 100f64) as u64;
let mut sum_gas = 0;
for (gas_used, effective_reward) in transactions.iter().cloned() {
for (gas_used, effective_reward) in transactions.iter().copied() {
sum_gas += gas_used;
if target_gas <= sum_gas {
return Some(effective_reward)
Expand Down
2 changes: 1 addition & 1 deletion crates/anvil/src/eth/otterscan/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ impl EthApi {
txs.iter().skip(page * page_size).take(page_size).cloned().collect(),
),
BlockTransactions::Hashes(txs) => BlockTransactions::Hashes(
txs.iter().skip(page * page_size).take(page_size).cloned().collect(),
txs.iter().skip(page * page_size).take(page_size).copied().collect(),
),
BlockTransactions::Uncle => unreachable!(),
};
Expand Down
2 changes: 1 addition & 1 deletion crates/anvil/src/eth/pool/transactions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ impl ReadyTransactions {
}
}

unlocked_tx.extend(to_remove.unlocks.iter().cloned())
unlocked_tx.extend(to_remove.unlocks.iter().copied())
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/anvil/src/eth/sign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub struct DevSigner {
impl DevSigner {
pub fn new(accounts: Vec<PrivateKeySigner>) -> Self {
let addresses = accounts.iter().map(|wallet| wallet.address()).collect::<Vec<_>>();
let accounts = addresses.iter().cloned().zip(accounts).collect();
let accounts = addresses.iter().copied().zip(accounts).collect();
Self { addresses, accounts }
}
}
Expand Down
8 changes: 4 additions & 4 deletions crates/anvil/tests/it/fork.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1466,12 +1466,12 @@ async fn test_reset_dev_account_nonce() {
async fn test_reset_updates_cache_path_when_rpc_url_not_provided() {
let config: NodeConfig = fork_config();

let (mut api, _handle) = spawn(config).await;
let (api, _handle) = spawn(config).await;
let info = api.anvil_node_info().await.unwrap();
let number = info.fork_config.fork_block_number.unwrap();
assert_eq!(number, BLOCK_NUMBER);

async fn get_block_from_cache_path(api: &mut EthApi) -> u64 {
async fn get_block_from_cache_path(api: &EthApi) -> u64 {
let db = api.backend.get_db().read().await;
let cache_path = db.maybe_inner().unwrap().cache().cache_path().unwrap();
cache_path
Expand All @@ -1485,7 +1485,7 @@ async fn test_reset_updates_cache_path_when_rpc_url_not_provided() {
.expect("must be valid number")
}

assert_eq!(BLOCK_NUMBER, get_block_from_cache_path(&mut api).await);
assert_eq!(BLOCK_NUMBER, get_block_from_cache_path(&api).await);

// Reset to older block without specifying a new rpc url
api.anvil_reset(Some(Forking {
Expand All @@ -1495,7 +1495,7 @@ async fn test_reset_updates_cache_path_when_rpc_url_not_provided() {
.await
.unwrap();

assert_eq!(BLOCK_NUMBER - 1_000_000, get_block_from_cache_path(&mut api).await);
assert_eq!(BLOCK_NUMBER - 1_000_000, get_block_from_cache_path(&api).await);
}

#[tokio::test(flavor = "multi_thread")]
Expand Down
6 changes: 3 additions & 3 deletions crates/cast/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,10 @@ pub async fn run_command(args: CastArgs) -> Result<()> {
print_tokens(&tokens);
}
CastSubcommand::AbiEncode { sig, packed, args } => {
if !packed {
sh_println!("{}", SimpleCast::abi_encode(&sig, &args)?)?
} else {
if packed {
sh_println!("{}", SimpleCast::abi_encode_packed(&sig, &args)?)?
} else {
sh_println!("{}", SimpleCast::abi_encode(&sig, &args)?)?
}
}
CastSubcommand::DecodeCalldata { sig, calldata } => {
Expand Down
2 changes: 1 addition & 1 deletion crates/cheatcodes/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ impl Cheatcode for envOr_12Call {
impl Cheatcode for envOr_13Call {
fn apply(&self, _state: &mut Cheatcodes) -> Result {
let Self { name, delim, defaultValue } = self;
let default = defaultValue.to_vec();
let default = defaultValue.clone();
env_array_default(name, delim, &default, &DynSolType::Bytes)
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/cheatcodes/src/evm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1180,7 +1180,7 @@ fn genesis_account(account: &Account) -> GenesisAccount {
}

/// Helper function to returns state diffs recorded for each changed account.
fn get_recorded_state_diffs(state: &mut Cheatcodes) -> BTreeMap<Address, AccountStateDiffs> {
fn get_recorded_state_diffs(state: &Cheatcodes) -> BTreeMap<Address, AccountStateDiffs> {
let mut state_diffs: BTreeMap<Address, AccountStateDiffs> = BTreeMap::default();
if let Some(records) = &state.recorded_account_diffs_stack {
records
Expand Down
18 changes: 9 additions & 9 deletions crates/cheatcodes/src/inspector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1555,10 +1555,10 @@ impl Inspector<&mut dyn DatabaseExt> for Cheatcodes {
},
};

if count != expected.count {
Some((expected, count))
} else {
if count == expected.count {
None
} else {
Some((expected, count))
}
})
.collect::<Vec<_>>();
Expand Down Expand Up @@ -1777,7 +1777,7 @@ impl Cheatcodes {
}

#[cold]
fn meter_gas_record(&mut self, interpreter: &mut Interpreter, ecx: Ecx) {
fn meter_gas_record(&mut self, interpreter: &Interpreter, ecx: Ecx) {
if matches!(interpreter.instruction_result, InstructionResult::Continue) {
self.gas_metering.gas_records.iter_mut().for_each(|record| {
if ecx.journaled_state.depth() == record.depth {
Expand All @@ -1798,7 +1798,7 @@ impl Cheatcodes {
}

#[cold]
fn meter_gas_end(&mut self, interpreter: &mut Interpreter) {
fn meter_gas_end(&mut self, interpreter: &Interpreter) {
// Remove recorded gas if we exit frame.
if will_exit(interpreter.instruction_result) {
self.gas_metering.paused_frames.pop();
Expand All @@ -1812,7 +1812,7 @@ impl Cheatcodes {
}

#[cold]
fn meter_gas_check(&mut self, interpreter: &mut Interpreter) {
fn meter_gas_check(&self, interpreter: &mut Interpreter) {
if will_exit(interpreter.instruction_result) {
// Reset gas if spent is less than refunded.
// This can happen if gas was paused / resumed or reset.
Expand All @@ -1833,7 +1833,7 @@ impl Cheatcodes {
/// cache) from mapped source address to the target address.
/// - generates arbitrary value and saves it in target address storage.
#[cold]
fn arbitrary_storage_end(&mut self, interpreter: &mut Interpreter, ecx: Ecx) {
fn arbitrary_storage_end(&mut self, interpreter: &Interpreter, ecx: Ecx) {
let (key, target_address) = if interpreter.current_opcode() == op::SLOAD {
(try_or_return!(interpreter.stack().peek(0)), interpreter.contract().target_address)
} else {
Expand Down Expand Up @@ -1867,7 +1867,7 @@ impl Cheatcodes {

/// Records storage slots reads and writes.
#[cold]
fn record_accesses(&mut self, interpreter: &mut Interpreter) {
fn record_accesses(&mut self, interpreter: &Interpreter) {
let Some(access) = &mut self.accesses else { return };
match interpreter.current_opcode() {
op::SLOAD => {
Expand All @@ -1883,7 +1883,7 @@ impl Cheatcodes {
}

#[cold]
fn record_state_diffs(&mut self, interpreter: &mut Interpreter, ecx: Ecx) {
fn record_state_diffs(&mut self, interpreter: &Interpreter, ecx: Ecx) {
let Some(account_accesses) = &mut self.recorded_account_diffs_stack else { return };
match interpreter.current_opcode() {
op::SELFDESTRUCT => {
Expand Down
6 changes: 3 additions & 3 deletions crates/cheatcodes/src/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -493,10 +493,10 @@ fn encode(values: Vec<DynSolValue>) -> Vec<u8> {
/// Canonicalize a json path key to always start from the root of the document.
/// Read more about json path syntax: <https://goessner.net/articles/JsonPath/>
pub(super) fn canonicalize_json_path(path: &str) -> Cow<'_, str> {
if !path.starts_with('$') {
format!("${path}").into()
} else {
if path.starts_with('$') {
path.into()
} else {
format!("${path}").into()
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/cheatcodes/src/script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ impl Wallets {

/// Locks inner Mutex and returns all signer addresses in the [MultiWallet].
pub fn signers(&self) -> Result<Vec<Address>> {
Ok(self.inner.lock().multi_wallet.signers()?.keys().cloned().collect())
Ok(self.inner.lock().multi_wallet.signers()?.keys().copied().collect())
}

/// Number of signers in the [MultiWallet].
Expand Down Expand Up @@ -251,7 +251,7 @@ fn broadcast(ccx: &mut CheatsCtxt, new_origin: Option<&Address>, single_call: bo
);
ensure!(ccx.state.broadcast.is_none(), "a broadcast is active already");

let mut new_origin = new_origin.cloned();
let mut new_origin = new_origin.copied();

if new_origin.is_none() {
let mut wallets = ccx.state.wallets().inner.lock();
Expand Down
12 changes: 6 additions & 6 deletions crates/cheatcodes/src/test/assert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,10 +456,10 @@ fn assert_true(condition: bool) -> Result<Vec<u8>, SimpleAssertionError> {
}

fn assert_false(condition: bool) -> Result<Vec<u8>, SimpleAssertionError> {
if !condition {
Ok(Default::default())
} else {
if condition {
Err(SimpleAssertionError)
} else {
Ok(Default::default())
}
}

Expand All @@ -472,10 +472,10 @@ fn assert_eq<'a, T: PartialEq>(left: &'a T, right: &'a T) -> ComparisonResult<'a
}

fn assert_not_eq<'a, T: PartialEq>(left: &'a T, right: &'a T) -> ComparisonResult<'a, T> {
if left != right {
Ok(Default::default())
} else {
if left == right {
Err(ComparisonAssertionError::Ne { left, right })
} else {
Ok(Default::default())
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/chisel/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1787,7 +1787,7 @@ mod tests {
T: AsRef<str> + std::fmt::Display + 'a,
I: IntoIterator<Item = &'a (T, DynSolType)> + 'a,
{
for (input, expected) in input.into_iter() {
for (input, expected) in input {
let input = input.as_ref();
let ty = get_type_ethabi(s, input, true);
assert_eq!(ty.as_ref(), Some(expected), "\n{input}");
Expand Down
4 changes: 2 additions & 2 deletions crates/chisel/src/session_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ pub struct IntermediateContract {
type IntermediateContracts = HashMap<String, IntermediateContract>;

/// Full compilation output for the [SessionSource]
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
pub struct GeneratedOutput {
/// The [IntermediateOutput] component
pub intermediate: IntermediateOutput,
Expand Down Expand Up @@ -438,7 +438,7 @@ impl SessionSource {
let Self { contract_name, global_code, top_level_code, run_code, config, .. } = self;

let script_import =
if !config.no_vm { "import {Script} from \"forge-std/Script.sol\";\n" } else { "" };
if config.no_vm { "" } else { "import {Script} from \"forge-std/Script.sol\";\n" };

format!(
r#"
Expand Down
Loading