diff --git a/crates/config/src/lint.rs b/crates/config/src/lint.rs index 1c5c6fdb02b2c..472bf3dd79266 100644 --- a/crates/config/src/lint.rs +++ b/crates/config/src/lint.rs @@ -25,7 +25,13 @@ pub struct LinterConfig { /// /// Defaults to true. Set to false to disable automatic linting during builds. pub lint_on_build: bool, + + /// Configurable patterns that should be excluded when performing `mixedCase` lint checks. + /// + /// Default's to ["ERC"] to allow common names like `rescueERC20` or `ERC721TokenReceiver`. + pub mixed_case_exceptions: Vec, } + impl Default for LinterConfig { fn default() -> Self { Self { @@ -33,6 +39,7 @@ impl Default for LinterConfig { severity: Vec::new(), exclude_lints: Vec::new(), ignore: Vec::new(), + mixed_case_exceptions: vec!["ERC".to_string()], } } } diff --git a/crates/forge/src/cmd/build.rs b/crates/forge/src/cmd/build.rs index d3726c42bc11e..2d5918c931f12 100644 --- a/crates/forge/src/cmd/build.rs +++ b/crates/forge/src/cmd/build.rs @@ -146,7 +146,8 @@ impl BuildArgs { .filter_map(|s| forge_lint::sol::SolLint::try_from(s.as_str()).ok()) .collect(), ) - }); + }) + .with_mixed_case_exceptions(&config.lint.mixed_case_exceptions); // Expand ignore globs and canonicalize from the get go let ignored = expand_globs(&config.root, config.lint.ignore.iter())? diff --git a/crates/forge/src/cmd/lint.rs b/crates/forge/src/cmd/lint.rs index 4b3bf58893a69..40177370edac9 100644 --- a/crates/forge/src/cmd/lint.rs +++ b/crates/forge/src/cmd/lint.rs @@ -109,7 +109,8 @@ impl LintArgs { .with_description(true) .with_lints(include) .without_lints(exclude) - .with_severity(if severity.is_empty() { None } else { Some(severity) }); + .with_severity(if severity.is_empty() { None } else { Some(severity) }) + .with_mixed_case_exceptions(&config.lint.mixed_case_exceptions); let sess = linter.init(); diff --git a/crates/forge/tests/cli/config.rs b/crates/forge/tests/cli/config.rs index e2e55265f6924..389acf5456e55 100644 --- a/crates/forge/tests/cli/config.rs +++ b/crates/forge/tests/cli/config.rs @@ -1088,6 +1088,7 @@ severity = [] exclude_lints = [] ignore = [] lint_on_build = true +mixed_case_exceptions = ["ERC"] [doc] out = "docs" @@ -1316,7 +1317,10 @@ exclude = [] "severity": [], "exclude_lints": [], "ignore": [], - "lint_on_build": true + "lint_on_build": true, + "mixed_case_exceptions": [ + "ERC" + ] }, "doc": { "out": "docs", diff --git a/crates/forge/tests/cli/lint.rs b/crates/forge/tests/cli/lint.rs index 112a800cbbf85..ecbc02ddc518d 100644 --- a/crates/forge/tests/cli/lint.rs +++ b/crates/forge/tests/cli/lint.rs @@ -33,7 +33,7 @@ const OTHER_CONTRACT: &str = r#" import { ContractWithLints } from "./ContractWithLints.sol"; contract OtherContractWithLints { - uint256 VARIABLE_MIXED_CASE_INFO; + function functionMIXEDCaseInfo() public {} } "#; @@ -60,6 +60,7 @@ forgetest!(can_use_config, |prj, cmd| { exclude_lints: vec!["incorrect-shift".into()], ignore: vec![], lint_on_build: true, + ..Default::default() }; }); cmd.arg("lint").assert_success().stderr_eq(str![[r#" @@ -87,16 +88,17 @@ forgetest!(can_use_config_ignore, |prj, cmd| { exclude_lints: vec![], ignore: vec!["src/ContractWithLints.sol".into()], lint_on_build: true, + ..Default::default() }; }); cmd.arg("lint").assert_success().stderr_eq(str![[r#" -note[mixed-case-variable]: mutable variables should use mixedCase - [FILE]:9:17 +note[mixed-case-function]: function names should use mixedCase + [FILE]:9:18 | -9 | uint256 VARIABLE_MIXED_CASE_INFO; - | ------------------------ +9 | function functionMIXEDCaseInfo() public {} + | --------------------- | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function "#]]); @@ -108,6 +110,25 @@ note[mixed-case-variable]: mutable variables should use mixedCase exclude_lints: vec![], ignore: vec!["src/ContractWithLints.sol".into(), "src/OtherContract.sol".into()], lint_on_build: true, + ..Default::default() + }; + }); + cmd.arg("lint").assert_success().stderr_eq(str![[""]]); +}); + +forgetest!(can_use_config_mixed_case_exception, |prj, cmd| { + prj.wipe_contracts(); + prj.add_source("ContractWithLints", CONTRACT).unwrap(); + prj.add_source("OtherContract", OTHER_CONTRACT).unwrap(); + + // Check config for `ignore` + prj.update_config(|config| { + config.lint = LinterConfig { + severity: vec![], + exclude_lints: vec![], + ignore: vec!["src/ContractWithLints.sol".into()], + lint_on_build: true, + mixed_case_exceptions: vec!["MIXED".to_string()], }; }); cmd.arg("lint").assert_success().stderr_eq(str![[""]]); @@ -125,16 +146,17 @@ forgetest!(can_override_config_severity, |prj, cmd| { exclude_lints: vec![], ignore: vec!["src/ContractWithLints.sol".into()], lint_on_build: true, + ..Default::default() }; }); cmd.arg("lint").args(["--severity", "info"]).assert_success().stderr_eq(str![[r#" -note[mixed-case-variable]: mutable variables should use mixedCase - [FILE]:9:17 +note[mixed-case-function]: function names should use mixedCase + [FILE]:9:18 | -9 | uint256 VARIABLE_MIXED_CASE_INFO; - | ------------------------ +9 | function functionMIXEDCaseInfo() public {} + | --------------------- | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function "#]]); @@ -152,6 +174,7 @@ forgetest!(can_override_config_path, |prj, cmd| { exclude_lints: vec!["incorrect-shift".into()], ignore: vec!["src/ContractWithLints.sol".into()], lint_on_build: true, + ..Default::default() }; }); cmd.arg("lint").arg("src/ContractWithLints.sol").assert_success().stderr_eq(str![[r#" @@ -179,6 +202,7 @@ forgetest!(can_override_config_lint, |prj, cmd| { exclude_lints: vec!["incorrect-shift".into()], ignore: vec![], lint_on_build: true, + ..Default::default() }; }); cmd.arg("lint").args(["--only-lint", "incorrect-shift"]).assert_success().stderr_eq(str![[ @@ -207,6 +231,7 @@ forgetest!(build_runs_linter_by_default, |prj, cmd| { exclude_lints: vec!["incorrect-shift".into()], ignore: vec![], lint_on_build: true, + ..Default::default() }; }); @@ -270,6 +295,7 @@ forgetest!(build_respects_quiet_flag_for_linting, |prj, cmd| { exclude_lints: vec!["incorrect-shift".into()], ignore: vec![], lint_on_build: true, + ..Default::default() }; }); @@ -288,6 +314,7 @@ forgetest!(build_with_json_uses_json_linter_output, |prj, cmd| { exclude_lints: vec!["incorrect-shift".into()], ignore: vec![], lint_on_build: true, + ..Default::default() }; }); @@ -316,6 +343,7 @@ forgetest!(build_respects_lint_on_build_false, |prj, cmd| { exclude_lints: vec!["incorrect-shift".into()], ignore: vec![], lint_on_build: false, + ..Default::default() }; }); diff --git a/crates/lint/src/linter/mod.rs b/crates/lint/src/linter/mod.rs index d2622d50b640a..552bb88008b8c 100644 --- a/crates/lint/src/linter/mod.rs +++ b/crates/lint/src/linter/mod.rs @@ -33,7 +33,7 @@ use crate::inline_config::InlineConfig; /// # Note: /// /// - For `early_lint` and `late_lint`, the `ParsingContext` should have the sources pre-loaded. -pub trait Linter: Send + Sync + Clone { +pub trait Linter: Send + Sync { type Language: Language; type Lint: Lint; @@ -52,18 +52,23 @@ pub trait Lint { pub struct LintContext<'s> { sess: &'s Session, with_description: bool, - pub inline_config: InlineConfig, + pub config: LinterConfig<'s>, active_lints: Vec<&'static str>, } +pub struct LinterConfig<'s> { + pub inline: InlineConfig, + pub mixed_case_exceptions: &'s [String], +} + impl<'s> LintContext<'s> { pub fn new( sess: &'s Session, with_description: bool, - config: InlineConfig, + config: LinterConfig<'s>, active_lints: Vec<&'static str>, ) -> Self { - Self { sess, with_description, inline_config: config, active_lints } + Self { sess, with_description, config, active_lints } } pub fn session(&self) -> &'s Session { @@ -80,7 +85,7 @@ impl<'s> LintContext<'s> { /// Helper method to emit diagnostics easily from passes pub fn emit(&self, lint: &'static L, span: Span) { - if self.inline_config.is_disabled(span, lint.id()) || !self.is_lint_enabled(lint.id()) { + if self.config.inline.is_disabled(span, lint.id()) || !self.is_lint_enabled(lint.id()) { return; } @@ -101,7 +106,7 @@ impl<'s> LintContext<'s> { /// For Diff snippets, if no span is provided, it will use the lint's span. /// If unable to get code from the span, it will fall back to a Block snippet. pub fn emit_with_fix(&self, lint: &'static L, span: Span, snippet: Snippet) { - if self.inline_config.is_disabled(span, lint.id()) || !self.is_lint_enabled(lint.id()) { + if self.config.inline.is_disabled(span, lint.id()) || !self.is_lint_enabled(lint.id()) { return; } diff --git a/crates/lint/src/sol/info/mixed_case.rs b/crates/lint/src/sol/info/mixed_case.rs index 4bceb257dbd2c..9063e08ce097c 100644 --- a/crates/lint/src/sol/info/mixed_case.rs +++ b/crates/lint/src/sol/info/mixed_case.rs @@ -1,9 +1,9 @@ use super::{MixedCaseFunction, MixedCaseVariable}; use crate::{ linter::{EarlyLintPass, LintContext}, - sol::{Severity, SolLint}, + sol::{Severity, SolLint, info::screaming_snake_case::is_screaming_snake_case}, }; -use solar_ast::{ItemFunction, VariableDefinition}; +use solar_ast::{FunctionHeader, ItemFunction, VariableDefinition, Visibility}; declare_forge_lint!( MIXED_CASE_FUNCTION, @@ -15,7 +15,8 @@ declare_forge_lint!( impl<'ast> EarlyLintPass<'ast> for MixedCaseFunction { fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { if let Some(name) = func.header.name - && !is_mixed_case(name.as_str(), true) + && !is_mixed_case(name.as_str(), true, ctx.config.mixed_case_exceptions) + && !is_constant_getter(&func.header) { ctx.emit(&MIXED_CASE_FUNCTION, name.span); } @@ -37,28 +38,73 @@ impl<'ast> EarlyLintPass<'ast> for MixedCaseVariable { ) { if var.mutability.is_none() && let Some(name) = var.name - && !is_mixed_case(name.as_str(), false) + && !is_mixed_case(name.as_str(), false, ctx.config.mixed_case_exceptions) { ctx.emit(&MIXED_CASE_VARIABLE, name.span); } } } -/// Check if a string is mixedCase +/// Checks if a string is mixedCase. /// /// To avoid false positives like `fn increment()` or `uint256 counter`, /// lowercase strings are treated as mixedCase. -pub fn is_mixed_case(s: &str, is_fn: bool) -> bool { +pub fn is_mixed_case(s: &str, is_fn: bool, allowed_patterns: &[String]) -> bool { if s.len() <= 1 { return true; } - // Remove leading/trailing underscores like `heck` does - if s.trim_matches('_') == format!("{}", heck::AsLowerCamelCase(s)).as_str() { + // Remove leading/trailing underscores like `heck` does. + if check_lower_mixed_case(s.trim_matches('_')) { return true; } + // Ignore user-defined infixes. + for pattern in allowed_patterns { + if let Some(pos) = s.find(pattern.as_str()) + && check_lower_mixed_case(&s[..pos]) + && check_upper_mixed_case_post_pattern(&s[pos + pattern.len()..]) + { + return true; + } + } + // Ignore `fn test*`, `fn invariant_*`, and `fn statefulFuzz*` patterns, as they usually contain // (allowed) underscores. is_fn && (s.starts_with("test") || s.starts_with("invariant_") || s.starts_with("statefulFuzz")) } + +fn check_lower_mixed_case(s: &str) -> bool { + s == heck::AsLowerCamelCase(s).to_string().as_str() +} + +fn check_upper_mixed_case_post_pattern(s: &str) -> bool { + // Find the index of the first character that is not a numeric digit. + let Some(split_idx) = s.find(|c: char| !c.is_numeric()) else { + return true; + }; + + // Validate the characters preceding the initial numbers have the correct format. + let trimmed = &s[split_idx..]; + if let Some(c) = trimmed.chars().next() + && !c.is_alphabetic() + { + return false; + } + trimmed == heck::AsUpperCamelCase(trimmed).to_string().as_str() +} + +/// Checks if a function getter is a valid constant getter with a heuristic: +/// * name is `SCREAMING_SNAKE_CASE` +/// * external view visibility and mutability. +/// * zero parameters. +/// * exactly one return value. +/// * return value is an elementary type +fn is_constant_getter(header: &FunctionHeader<'_>) -> bool { + header.visibility().is_some_and(|v| matches!(v, Visibility::External)) + && header.state_mutability().is_view() + && header.parameters.is_empty() + && header.returns().len() == 1 + && header.returns().first().is_some_and(|ret| ret.ty.kind.is_elementary()) + && is_screaming_snake_case(header.name.unwrap().as_str()) +} diff --git a/crates/lint/src/sol/mod.rs b/crates/lint/src/sol/mod.rs index 4ebb90715fc67..56d4883f3a570 100644 --- a/crates/lint/src/sol/mod.rs +++ b/crates/lint/src/sol/mod.rs @@ -2,6 +2,7 @@ use crate::{ inline_config::{InlineConfig, InlineConfigItem}, linter::{ EarlyLintPass, EarlyLintVisitor, LateLintPass, LateLintVisitor, Lint, LintContext, Linter, + LinterConfig, }, }; use foundry_common::comments::Comments; @@ -45,25 +46,27 @@ static ALL_REGISTERED_LINTS: LazyLock> = LazyLock::new(|| { /// Linter implementation to analyze Solidity source code responsible for identifying /// vulnerabilities gas optimizations, and best practices. -#[derive(Debug, Clone)] -pub struct SolidityLinter { +#[derive(Debug)] +pub struct SolidityLinter<'a> { path_config: ProjectPathsConfig, severity: Option>, lints_included: Option>, lints_excluded: Option>, with_description: bool, with_json_emitter: bool, + mixed_case_exceptions: &'a [String], } -impl SolidityLinter { +impl<'a> SolidityLinter<'a> { pub fn new(path_config: ProjectPathsConfig) -> Self { Self { path_config, + with_description: true, severity: None, lints_included: None, lints_excluded: None, - with_description: true, with_json_emitter: false, + mixed_case_exceptions: &[], } } @@ -92,6 +95,15 @@ impl SolidityLinter { self } + pub fn with_mixed_case_exceptions(mut self, exceptions: &'a [String]) -> Self { + self.mixed_case_exceptions = exceptions; + self + } + + fn config(&self, inline: InlineConfig) -> LinterConfig<'_> { + LinterConfig { inline, mixed_case_exceptions: self.mixed_case_exceptions } + } + fn include_lint(&self, lint: SolLint) -> bool { self.severity.as_ref().is_none_or(|sev| sev.contains(&lint.severity())) && self.lints_included.as_ref().is_none_or(|incl| incl.contains(&lint)) @@ -99,7 +111,7 @@ impl SolidityLinter { } fn process_source_ast<'ast>( - &self, + &'ast self, sess: &'ast Session, ast: &'ast ast::SourceUnit<'ast>, file: &SourceFile, @@ -139,7 +151,7 @@ impl SolidityLinter { let inline_config = parse_inline_config(sess, &comments, InlineConfigSource::Ast(ast)); // Initialize and run the early lint visitor - let ctx = LintContext::new(sess, self.with_description, inline_config, lints); + let ctx = LintContext::new(sess, self.with_description, self.config(inline_config), lints); let mut early_visitor = EarlyLintVisitor::new(&ctx, &mut passes); _ = early_visitor.visit_source_unit(ast); early_visitor.post_source_unit(ast); @@ -191,7 +203,7 @@ impl SolidityLinter { parse_inline_config(sess, &comments, InlineConfigSource::Hir((&gcx.hir, source_id))); // Run late lint visitor - let ctx = LintContext::new(sess, self.with_description, inline_config, lints); + let ctx = LintContext::new(sess, self.with_description, self.config(inline_config), lints); let mut late_visitor = LateLintVisitor::new(&ctx, &mut passes, &gcx.hir); // Visit this specific source @@ -201,7 +213,7 @@ impl SolidityLinter { } } -impl Linter for SolidityLinter { +impl<'a> Linter for SolidityLinter<'a> { type Language = SolcLanguage; type Lint = SolLint; diff --git a/crates/lint/testdata/MixedCase.sol b/crates/lint/testdata/MixedCase.sol index 75126cf8998e6..b2a4b5e15e46e 100644 --- a/crates/lint/testdata/MixedCase.sol +++ b/crates/lint/testdata/MixedCase.sol @@ -59,4 +59,17 @@ contract MixedCaseTest { function statefulFuzz_mixedcase_disabled() public {} function statefulFuzzMixedCaseDisabled() public {} function statefulFuzzmixedcasedisabled() public {} + + // ERC is, by default, an allowed infix + function rescueERC6909(address token, address to, uint256 tokenId, uint256 amount) public {} + function ERC20DoSomething() public {} + function ERC20_DoSomething() public {} // invalid because of the underscore + //~^NOTE: function names should use mixedCase + + // SCREAMING_SNAKE_CASE is allowed for functions that are most likely constant getters + function MAX_NUMBER() external view returns (uint256) {} + function HAS_PARAMS(address addr) external view returns (uint256) {} //~NOTE: function names should use mixedCase + function HAS_NO_RETURN() external view {} //~NOTE: function names should use mixedCase + function HAS_MORE_THAN_ONE_RETURN() external view returns (uint256, uint256) {} //~NOTE: function names should use mixedCase + function NOT_ELEMENTARY_RETURN() external view returns (uint256[]) {} //~NOTE: function names should use mixedCase } diff --git a/crates/lint/testdata/MixedCase.stderr b/crates/lint/testdata/MixedCase.stderr index 0976dced8716c..58e3f2f669559 100644 --- a/crates/lint/testdata/MixedCase.stderr +++ b/crates/lint/testdata/MixedCase.stderr @@ -94,3 +94,43 @@ note[mixed-case-function]: function names should use mixedCase | = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function +note[mixed-case-function]: function names should use mixedCase + --> ROOT/testdata/MixedCase.sol:LL:CC + | +66 | function ERC20_DoSomething() public {} // invalid because of the underscore + | ----------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function + +note[mixed-case-function]: function names should use mixedCase + --> ROOT/testdata/MixedCase.sol:LL:CC + | +71 | function HAS_PARAMS(address addr) external view returns (uint256) {} + | ---------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function + +note[mixed-case-function]: function names should use mixedCase + --> ROOT/testdata/MixedCase.sol:LL:CC + | +72 | function HAS_NO_RETURN() external view {} + | ------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function + +note[mixed-case-function]: function names should use mixedCase + --> ROOT/testdata/MixedCase.sol:LL:CC + | +73 | function HAS_MORE_THAN_ONE_RETURN() external view returns (uint256, uint256) {} + | ------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function + +note[mixed-case-function]: function names should use mixedCase + --> ROOT/testdata/MixedCase.sol:LL:CC + | +74 | function NOT_ELEMENTARY_RETURN() external view returns (uint256[]) {} + | --------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-function +