feat(forge): Forge Lint

[0xKitsune]

ref #1970

This PR introduces forge lint, which implements a static analyzer built on top of solar and is capable of detecting known issues, vulnerabilities, informational warnings and gas optimizations. This is a first pass at forge lint and I am opening this draft in order to receive early feedback on the design and general direction.

The core component of this design centers around the declare_lints! macro which allows you to specify the pattern along with the severity, name and description.

declare_lints!(
    (DivideBeforeMultiply, Severity::Med, "divide-before-multiply", "Multiplication should occur before division to avoid loss of precision."),
    (VariableCamelCase, Severity::Info, "variable-camel-case", "Variables should follow `camelCase` naming conventions unless they are constants or immutables."),
    (AsmKeccak256, Severity::Gas, "asm-keccak256", "Hashing via keccak256 can be done with inline assembly to save `x` gas."),
);

Once the pattern is defined, you can implement the Visit trait to match on any instances of the pattern.

use solar_ast::{
    ast::{Expr, ExprKind},
    visit::Visit,
};

use crate::AsmKeccak256;

impl<'ast> Visit<'ast> for AsmKeccak256 {
    fn visit_expr(&mut self, expr: &'ast Expr<'ast>) {
        if let ExprKind::Call(expr, _) = &expr.kind {
            if let ExprKind::Ident(ident) = &expr.kind {
                if ident.name.as_str() == "keccak256" {
                    self.items.push(expr.span);
                }
            }
        }
        self.walk_expr(expr);
    }
}

There is still quite a bit of work to do but if this direction makes sense, I am happy to continue adding the rest of the features as well as additional patterns. I can also port over any of the patterns from solstat, sstan or start working on patterns in other static analysis tools like slither as well.

[gakonst]

Fantastic. Really excited for this. Anything we can learn from slither here as well?

[0xKitsune]

Hello just bumping this, let me know if you would like me to continue with this direction or if you would rather iterate on the current approach before moving forward with this feature.

[DaniPopes]

I would rather have each lint emit the diagnostic as soon as the conditions are met. The approach should be the same as in clippy since the diagnostic APIs are identical. See also https://rustc-dev-guide.rust-lang.org/diagnostics.html

Here's the rustc implementation https://github.com/rust-lang/rust/blob/2620eb42d72d24baa1ca1056a769862b92c85f7f/compiler/rustc_lint/src/early.rs#L29
There is a separate trait from visitor to provide a context argument through which to emit the lints and also potentially get other information about the program

[0xKitsune]

Sounds good, thanks for the clarity, Ill update the PR accordingly.

[jenpaff]

hey @0xKitsune ! Will you be able to push this PR over the finish based on the feedback provided? alternatively, please flag and we can take it over if you cannot invest any more time

[0xKitsune]

Hey @jenpaff, thanks for bumping this and sorry for the long delay. This got buried with other priorities on my side, but I can update the PR this week.

I can certainly help push forge lint forward as I have time but if significant work is needed in subsequent PRs and someone else has the interest/more time to take this on, feel free to move forward without me.

Ill get this PR updated this week!

[jenpaff]

Hey @jenpaff, thanks for bumping this and sorry for the long delay. This got buried with other priorities on my side, but I can update the PR this week.

I can certainly help push forge lint forward as I have time but if significant work is needed in subsequent PRs and someone else has the interest/more time to take this on, feel free to move forward without me.

Ill get this PR updated this week!

awesome, feel free to flag if you need any support from us.

[0xKitsune]

Hey @DaniPopes, I have a few quick questions for clarification. This one is a bit outside my usual wheelhouse and I want to make sure I understand the recommended approach before updating the PR.

The approach should be the same as in clippy since the diagnostic APIs are identical.

IIUC clippy lints generally adhere to the following implementation details (using PubUse as an example):

• Use declare_clippy_lint! to declare metadata
• Use declare_lint_pass!
• Implement EarlyLintPass (or LateLintPass if using the HIR)

I am under the impression that we can not use rustc directly for EarlyLintPass, declare_lint_pass! , etc. and we will need to implement a version of these for Foundry. Here is my current mental model, let me know if this is in the right direction or if there is a better approach that you would recommend.

• Implement declare_forge_lint! to declare lints and relevant metadata:

  declare_forge_lint! {
      /// Multiplication should occur before division to avoid loss of precision.
      pub DIVIDE_BEFORE_MULTIPLY,
      Warn,
      "Multiplication should occur before division to avoid loss of precision"
  }

• Implement LintPass trait and declare_lint_pass! macro.

  declare_lint_pass!(DivideBeforeMultiply => [DIVIDE_BEFORE_MULTIPLY]);

• Implement EarlyLintPass with check_* functions for AST nodes. Then this can be implemented for each lint.

  impl EarlyLintPass for DivideBeforeMultiply {
      fn check_expr(&self, cx: &LintContext, expr: &Expr<'_>) {
       if let ExprKind::Binary(left_expr, BinOp { kind: BinOpKind::Mul, .. }, _) = &expr.kind {
          if contains_division(left_expr) {
                 cx.emit(
                     &DIVIDE_BEFORE_MULTIPLY,
                      expr.span,
                     "multiplication should occur before division to avoid loss of precision",
                 );
           }
       }
    }
  }

• Diagnostics can be abstracted through a LintContext struct or something like clippys span_lint_and_then() for simplicity

  impl<'sess> LintContext<'sess> {
      pub fn emit(&self, lint: &Lint, span: Span, msg: &str) {
          match lint.level {
              LintLevel::Allow => {}
              LintLevel::Warn => self.sess.dcx().span_warn(span, msg),
              LintLevel::Deny => self.sess.dcx().span_err(span, msg),
          }
      }
  }

• Define EarlyContextAndPass (similar to rustc) and implement the visitor trait

  pub struct EarlyContextAndPass<'sess, T: EarlyLintPass> {
      context: &LintContext<'sess>,
      pass: T,
  }
  
  impl<'ast, 'sess, T: EarlyLintPass> Visit<'ast> for EarlyContextAndPass<'sess, T> {
      type BreakValue = ();
  
      fn visit_expr(&mut self, expr: &'ast Expr<'_>) -> ControlFlow<()> {
          self.pass.check_expr(&self.context, expr);
          self.walk_expr(expr);
          ControlFlow::Continue(())
      }
  
    // --snip--
  }

• Update the SolidityLinter to orchestrate linting

  pub struct SolidityLinter<'sess> {
      passes: Vec<Box<dyn EarlyLintPass + 'sess>>,
      sess: &'sess Session,
  }
  
  impl<'sess> SolidityLinter<'sess> {
      pub fn new(sess: &'sess Session) -> Self {
          Self { passes: vec![], sess }
      }
  
      pub fn register_pass<P: EarlyLintPass + 'sess>(&mut self, pass: P) {
          self.passes.push(Box::new(pass));
      }
  
      pub fn run<'ast>(&mut self, ast: &'ast SourceUnit<'ast>) {
          for pass in &mut self.passes {
              let context = LintContext { sess: self.sess };
              let mut cx = EarlyContextAndPass { context, pass: &**pass };
              cx.visit_source_unit(ast);
          }
      }
  }

• Finally a quick example for running the linter

  let ast = parser.parse_file().map_err(|e| e.emit())?;
  let mut linter = SolidityLinter::new(&sess);
  linter.register_pass(DivideBeforeMultiply);
  linter.run(&ast);

Let me know if this is in the right direction or feel free to clarify anything that I may be misunderstanding/overlooking.

[0xKitsune]

Hey @jenpaff @DaniPopes just bumping this. Let me know if you have any thoughts on the approach above.

Happy to get this PR over the line, just would like clarity on the approach. If you'd prefer to take it in a different direction or have someone else with more bandwidth handle it, no worries just let me know!

[DaniPopes]

Sorry I missed this, yes the approach looks right!

[jenpaff]

Hey @jenpaff @DaniPopes just bumping this. Let me know if you have any thoughts on the approach above.

Happy to get this PR over the line, just would like clarity on the approach. If you'd prefer to take it in a different direction or have someone else with more bandwidth handle it, no worries just let me know!

sg ! We can add this to the next release, let us know if you need anymore support

[DaniPopes]

Hey @0xKitsune, @0xrusowsky will be taking over in #10405 on top of this PR, thanks for your work!