Clippy 1.67 (#3916)

## Proposed Changes

Clippy 1.67.0 put us on blast for the size of some of our errors, most of them written by me ( 👀 ). This PR shrinks the size of `BeaconChainError` by dropping some extraneous info and boxing an inner error which should only occur infrequently anyway.

For the `AttestationSlashInfo` and `BlockSlashInfo` I opted to ignore the lint as they are always used in a `Result<A, Info>` where `A` is a similar size. This means they don't bloat the size of the `Result`, so it's a bit annoying for Clippy to report this as an issue.

I also chose to ignore `clippy::uninlined-format-args` because I think the benefit-to-churn ratio is too low. E.g. sometimes we have long identifiers in `format!` args and IMO the non-inlined form is easier to read:

```rust
// I prefer this...
format!(
    "{} did {} to {}",
    REALLY_LONG_CONSTANT_NAME,
    ANOTHER_REALLY_LONG_CONSTANT_NAME,
    regular_long_identifier_name
);
  
// To this
format!("{REALLY_LONG_CONSTANT_NAME} did {ANOTHER_REALLY_LONG_CONSTANT_NAME} to {regular_long_identifier_name}");
```

I tried generating an automatic diff with `cargo clippy --fix` but it came out at:

```
250 files changed, 1209 insertions(+), 1469 deletions(-)
```

Which seems like a bad idea when we'd have to back-merge it to `capella` and `eip4844` 😱
This commit is contained in:
Michael Sproul 2023-01-27 09:48:42 +00:00
parent e8d1dd4e7c
commit 0866b739d0
9 changed files with 26 additions and 21 deletions

View File

@ -164,7 +164,8 @@ lint:
-A clippy::from-over-into \ -A clippy::from-over-into \
-A clippy::upper-case-acronyms \ -A clippy::upper-case-acronyms \
-A clippy::vec-init-then-push \ -A clippy::vec-init-then-push \
-A clippy::question-mark -A clippy::question-mark \
-A clippy::uninlined-format-args
nightly-lint: nightly-lint:
cp .github/custom/clippy.toml . cp .github/custom/clippy.toml .

View File

@ -27,6 +27,11 @@
//! ▼ //! ▼
//! impl VerifiedAttestation //! impl VerifiedAttestation
//! ``` //! ```
// Ignore this lint for `AttestationSlashInfo` which is of comparable size to the non-error types it
// is returned alongside.
#![allow(clippy::result_large_err)]
mod batch; mod batch;
use crate::{ use crate::{

View File

@ -975,7 +975,9 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
.ok_or(Error::ExecutionLayerMissing)? .ok_or(Error::ExecutionLayerMissing)?
.get_payload_by_block_hash(exec_block_hash) .get_payload_by_block_hash(exec_block_hash)
.await .await
.map_err(|e| Error::ExecutionLayerErrorPayloadReconstruction(exec_block_hash, e))? .map_err(|e| {
Error::ExecutionLayerErrorPayloadReconstruction(exec_block_hash, Box::new(e))
})?
.ok_or(Error::BlockHashMissingFromExecutionLayer(exec_block_hash))?; .ok_or(Error::BlockHashMissingFromExecutionLayer(exec_block_hash))?;
// Verify payload integrity. // Verify payload integrity.
@ -992,8 +994,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
return Err(Error::InconsistentPayloadReconstructed { return Err(Error::InconsistentPayloadReconstructed {
slot: blinded_block.slot(), slot: blinded_block.slot(),
exec_block_hash, exec_block_hash,
canonical_payload_root: execution_payload_header.tree_hash_root(),
reconstructed_payload_root: header_from_payload.tree_hash_root(),
canonical_transactions_root: execution_payload_header.transactions_root, canonical_transactions_root: execution_payload_header.transactions_root,
reconstructed_transactions_root: header_from_payload.transactions_root, reconstructed_transactions_root: header_from_payload.transactions_root,
}); });

View File

@ -42,6 +42,11 @@
//! END //! END
//! //!
//! ``` //! ```
// Ignore this lint for `BlockSlashInfo` which is of comparable size to the non-error types it is
// returned alongside.
#![allow(clippy::result_large_err)]
use crate::eth1_finalization_cache::Eth1FinalizationData; use crate::eth1_finalization_cache::Eth1FinalizationData;
use crate::execution_payload::{ use crate::execution_payload::{
is_optimistic_candidate_block, validate_execution_payload_for_gossip, validate_merge_block, is_optimistic_candidate_block, validate_execution_payload_for_gossip, validate_merge_block,

View File

@ -141,13 +141,11 @@ pub enum BeaconChainError {
BuilderMissing, BuilderMissing,
ExecutionLayerMissing, ExecutionLayerMissing,
BlockVariantLacksExecutionPayload(Hash256), BlockVariantLacksExecutionPayload(Hash256),
ExecutionLayerErrorPayloadReconstruction(ExecutionBlockHash, execution_layer::Error), ExecutionLayerErrorPayloadReconstruction(ExecutionBlockHash, Box<execution_layer::Error>),
BlockHashMissingFromExecutionLayer(ExecutionBlockHash), BlockHashMissingFromExecutionLayer(ExecutionBlockHash),
InconsistentPayloadReconstructed { InconsistentPayloadReconstructed {
slot: Slot, slot: Slot,
exec_block_hash: ExecutionBlockHash, exec_block_hash: ExecutionBlockHash,
canonical_payload_root: Hash256,
reconstructed_payload_root: Hash256,
canonical_transactions_root: Hash256, canonical_transactions_root: Hash256,
reconstructed_transactions_root: Hash256, reconstructed_transactions_root: Hash256,
}, },

View File

@ -43,7 +43,7 @@ impl ForkChoiceSignalTx {
/// ///
/// Return an error if the provided `slot` is strictly less than any previously provided slot. /// Return an error if the provided `slot` is strictly less than any previously provided slot.
pub fn notify_fork_choice_complete(&self, slot: Slot) -> Result<(), BeaconChainError> { pub fn notify_fork_choice_complete(&self, slot: Slot) -> Result<(), BeaconChainError> {
let &(ref lock, ref condvar) = &*self.pair; let (lock, condvar) = &*self.pair;
let mut current_slot = lock.lock(); let mut current_slot = lock.lock();
@ -72,7 +72,7 @@ impl Default for ForkChoiceSignalTx {
impl ForkChoiceSignalRx { impl ForkChoiceSignalRx {
pub fn wait_for_fork_choice(&self, slot: Slot, timeout: Duration) -> ForkChoiceWaitResult { pub fn wait_for_fork_choice(&self, slot: Slot, timeout: Duration) -> ForkChoiceWaitResult {
let &(ref lock, ref condvar) = &*self.pair; let (lock, condvar) = &*self.pair;
let mut current_slot = lock.lock(); let mut current_slot = lock.lock();

View File

@ -115,11 +115,7 @@ impl Comparison {
let mut children = vec![]; let mut children = vec![];
for i in 0..std::cmp::max(a.len(), b.len()) { for i in 0..std::cmp::max(a.len(), b.len()) {
children.push(FieldComparison::new( children.push(FieldComparison::new(format!("{i}"), &a.get(i), &b.get(i)));
format!("{:}", i),
&a.get(i),
&b.get(i),
));
} }
Self::parent(field_name, a == b, children) Self::parent(field_name, a == b, children)
@ -164,8 +160,8 @@ impl FieldComparison {
Self { Self {
field_name, field_name,
equal: a == b, equal: a == b,
a: format!("{:?}", a), a: format!("{a:?}"),
b: format!("{:?}", b), b: format!("{b:?}"),
} }
} }

View File

@ -32,7 +32,7 @@ pub fn compare_fields_derive(input: TokenStream) -> TokenStream {
_ => panic!("compare_fields_derive only supports named struct fields."), _ => panic!("compare_fields_derive only supports named struct fields."),
}; };
let field_name = format!("{:}", ident_a); let field_name = ident_a.to_string();
let ident_b = ident_a.clone(); let ident_b = ident_a.clone();
let quote = if is_slice(field) { let quote = if is_slice(field) {

View File

@ -9,9 +9,9 @@ use crate::VerifySignatures;
use safe_arith::SafeArith; use safe_arith::SafeArith;
use types::consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR}; use types::consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR};
pub fn process_operations<'a, T: EthSpec, Payload: ExecPayload<T>>( pub fn process_operations<T: EthSpec, Payload: ExecPayload<T>>(
state: &mut BeaconState<T>, state: &mut BeaconState<T>,
block_body: BeaconBlockBodyRef<'a, T, Payload>, block_body: BeaconBlockBodyRef<T, Payload>,
verify_signatures: VerifySignatures, verify_signatures: VerifySignatures,
ctxt: &mut ConsensusContext<T>, ctxt: &mut ConsensusContext<T>,
spec: &ChainSpec, spec: &ChainSpec,
@ -232,9 +232,9 @@ pub fn process_attester_slashings<T: EthSpec>(
} }
/// Wrapper function to handle calling the correct version of `process_attestations` based on /// Wrapper function to handle calling the correct version of `process_attestations` based on
/// the fork. /// the fork.
pub fn process_attestations<'a, T: EthSpec, Payload: ExecPayload<T>>( pub fn process_attestations<T: EthSpec, Payload: ExecPayload<T>>(
state: &mut BeaconState<T>, state: &mut BeaconState<T>,
block_body: BeaconBlockBodyRef<'a, T, Payload>, block_body: BeaconBlockBodyRef<T, Payload>,
verify_signatures: VerifySignatures, verify_signatures: VerifySignatures,
ctxt: &mut ConsensusContext<T>, ctxt: &mut ConsensusContext<T>,
spec: &ChainSpec, spec: &ChainSpec,