Skip to content

feat(forge): forge lint #10405

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

Merged
merged 128 commits into from
May 23, 2025
Merged

feat(forge): forge lint #10405

merged 128 commits into from
May 23, 2025

Conversation

0xrusowsky
Copy link
Contributor

@0xrusowsky 0xrusowsky commented Apr 28, 2025

this PR builds on top of the work that @0xKitsune did on #9590.

Closes #1970.

the proposed changes maintain most of the initial traits and structs + uses trait EarlyLintPass to adopt the clippy-like architecture that @DaniPopes requested.

relevant changes:

  • the Visit trait is implemented by the helper struct EarlyLintVisitor rather than the individual lints.
  • diagnostics aren't tracked by the linter anymore.

considerations:

  • i was unable to find any methods on the solar Session to get the warning/note counts (just errors), so for tests, i manually counted the occurrences from the buffer.
    since i wanted to minimize code duplication, i added an unnecessary boolean to the SolidityLinter which allows it write to the local buffer rather than stderr:

    /// Linter implementation to analyze Solidity source code responsible for identifying
    /// vulnerabilities gas optimizations, and best practices.
    #[derive(Debug, Clone, Default)]
    pub struct SolidityLinter {
        severity: Option<Vec<Severity>>,
        lints_included: Option<Vec<SolLint>>,
        lints_excluded: Option<Vec<SolLint>>,
        with_description: bool,
        // This field is only used for testing purposes, in production it will always be false.
        with_buffer_emitter: bool,
    }

    imo it should be fine, as the runtime overhead should be minimal and it would probably be optimized by the compiler:

     #[cfg(test)]
     pub(crate) fn with_buffer_emitter(mut self, with: bool) -> Self {
         self.with_buffer_emitter = with;
         self
     }
    
     // Helper function to ease testing, despite `fn lint` being the public API for the `Linter`
     pub(crate) fn lint_file(&self, file: &Path) -> Option<EmittedDiagnostics> {
         let mut sess = if self.with_buffer_emitter {
             Session::builder().with_buffer_emitter(ColorChoice::Never).build()
         } else {
             Session::builder().with_stderr_emitter().build()
         };
         // rest of the code
     }

    however, if this feels unacceptable, i could leave a clean SolidityLinter and implement a TestSolidityLinter with duplicate the logic

  • if the changes are validated i can add more lints and tests (so far i only added a minimal logic for incorrect-shift)

@DaniPopes
Copy link
Member

Unblocked by #10454

@0xrusowsky
Copy link
Contributor Author

0xrusowsky commented May 19, 2025

Unblocked by #10454

will merge with master later today, thanks for the heads up!

@grandizzy grandizzy added the T-feature Type: feature label May 21, 2025
grandizzy
grandizzy previously approved these changes May 22, 2025
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

lgtm, good to be sent from my side! @DaniPopes can you pls recheck before merge?

DaniPopes
DaniPopes previously approved these changes May 22, 2025
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

lgtm, pls address comment, merge master and merge

@0xrusowsky 0xrusowsky dismissed stale reviews from DaniPopes and grandizzy via c3526d1 May 23, 2025 02:23
@grandizzy grandizzy self-requested a review May 23, 2025 05:12
grandizzy
grandizzy previously approved these changes May 23, 2025
@grandizzy grandizzy self-requested a review May 23, 2025 05:33
@grandizzy grandizzy merged commit 96d107c into foundry-rs:master May 23, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from Ready For Review to Done in Foundry May 23, 2025
@zerosnacks
Copy link
Member

Amazing work @0xrusowsky & @0xKitsune!

@zerosnacks zerosnacks moved this from Done to Completed in Foundry May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

Add forge lint
6 participants