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

lint: warn on default_trait_access #1536

Merged
merged 2 commits into from
Jul 26, 2024
Merged

lint: warn on default_trait_access #1536

merged 2 commits into from
Jul 26, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 24, 2024

PR Type

enhancement, bug fix


Description

  • Allowed clippy::default_trait_access lint in the consensus module.
  • Replaced Default::default() with specific type defaults in multiple modules (evm.rs, block_header.rs, inmemory_permanent.rs, inmemory_temporary.rs).
  • Added HashMap import in evm.rs.
  • Changed return value from Default::default() to None in read_slot method of inmemory_permanent.rs.
  • Added default_trait_access to Clippy lints in Cargo.toml.

Changes walkthrough 📝

Relevant files
Enhancement
mod.rs
Allow `default_trait_access` lint in consensus module       

src/eth/consensus/mod.rs

  • Added clippy::default_trait_access to allowed lints.
+1/-1     
evm.rs
Use specific type defaults in EVM executor                             

src/eth/executor/evm.rs

  • Replaced Default::default() with specific type defaults.
  • Added HashMap import.
  • +6/-5     
    block_header.rs
    Use specific type defaults in block header                             

    src/eth/primitives/block_header.rs

  • Replaced Default::default() with specific type defaults.
  • Added EthersBytes import.
  • +5/-4     
    inmemory_permanent.rs
    Use specific type defaults in in-memory permanent storage

    src/eth/storage/inmemory/inmemory_permanent.rs

  • Replaced Default::default() with specific type defaults.
  • Changed return value from Default::default() to None in read_slot.
  • +4/-3     
    inmemory_temporary.rs
    Use specific type defaults in in-memory temporary storage

    src/eth/storage/inmemory/inmemory_temporary.rs

    • Replaced Default::default() with specific type defaults.
    +1/-1     
    rocks_state.rs
    Use `Mutex::default()` in RocksDB storage state                   

    src/eth/storage/rocks/rocks_state.rs

    • Replaced Default::default() with Mutex::default().
    +1/-1     
    Configuration changes
    Cargo.toml
    Add `default_trait_access` to Clippy lints                             

    Cargo.toml

    • Added default_trait_access to Clippy lints.
    +1/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Smell
    The reset method in RevmSession is performing multiple default initializations. Consider using self.storage_changes.clear() and self.metrics.reset() if applicable, to avoid unnecessary allocations.

    Initialization Logging
    Ensure that the initialization of InMemoryPermanentStorage logs all relevant configurations, especially the block_number.

    Error Handling
    In the read_slot method, ensure that any errors encountered while reading the slot are logged with the original error in a field called reason.

    Copy link

    github-actions bot commented Jul 24, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use AtomicU64::new(0) instead of AtomicU64::default() for explicit initialization

    Instead of using AtomicU64::default(), consider using AtomicU64::new(0) to
    explicitly initialize the atomic value to zero.

    src/eth/storage/inmemory/inmemory_permanent.rs [124]

    -block_number: AtomicU64::default(),
    +block_number: AtomicU64::new(0),
     
    Suggestion importance[1-10]: 8

    Why: Using AtomicU64::new(0) makes the initialization value explicit, which improves code clarity and readability.

    8
    Use HashMap::new() instead of HashMap::default() for better readability

    Instead of using HashMap::default(), consider using HashMap::new() for better
    readability and to explicitly indicate the creation of a new HashMap.

    src/eth/executor/evm.rs [204]

    -storage_changes: HashMap::default(),
    +storage_changes: HashMap::new(),
     
    Suggestion importance[1-10]: 7

    Why: Using HashMap::new() is more explicit and improves readability, but it doesn't change the functionality of the code.

    7
    Use EvmInput::new() instead of EvmInput::default() for better readability

    Instead of using EvmInput::default(), consider using EvmInput::new() if such a
    constructor exists, to make the code more explicit and readable.

    src/eth/executor/evm.rs [203]

    -input: EvmInput::default(),
    +input: EvmInput::new(),
     
    Suggestion importance[1-10]: 5

    Why: This suggestion is only valid if EvmInput::new() exists. If it doesn't, the suggestion is incorrect. The current code is already clear.

    5
    Use H256::zero() instead of H256::default() for better readability

    Instead of using H256::default(), consider using H256::zero() if such a method
    exists, to make the code more explicit and readable.

    src/eth/primitives/block_header.rs [164]

    -mix_hash: Some(H256::default()),
    +mix_hash: Some(H256::zero()),
     
    Suggestion importance[1-10]: 5

    Why: This suggestion is only valid if H256::zero() exists. If it doesn't, the suggestion is incorrect. The current code is already clear.

    5

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 24, 2024 14:59
    @dinhani-cw dinhani-cw merged commit 61026bd into main Jul 26, 2024
    33 checks passed
    @dinhani-cw dinhani-cw deleted the default branch July 26, 2024 06:12
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants