Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions crates/config/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,21 @@ 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<String>,
}

impl Default for LinterConfig {
fn default() -> Self {
Self {
lint_on_build: true,
severity: Vec::new(),
exclude_lints: Vec::new(),
ignore: Vec::new(),
mixed_case_exceptions: vec!["ERC".to_string()],
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion crates/forge/src/cmd/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())?
Expand Down
3 changes: 2 additions & 1 deletion crates/forge/src/cmd/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
6 changes: 5 additions & 1 deletion crates/forge/tests/cli/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,7 @@ severity = []
exclude_lints = []
ignore = []
lint_on_build = true
mixed_case_exceptions = ["ERC"]

[doc]
out = "docs"
Expand Down Expand Up @@ -1316,7 +1317,10 @@ exclude = []
"severity": [],
"exclude_lints": [],
"ignore": [],
"lint_on_build": true
"lint_on_build": true,
"mixed_case_exceptions": [
"ERC"
]
},
"doc": {
"out": "docs",
Expand Down
50 changes: 39 additions & 11 deletions crates/forge/tests/cli/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const OTHER_CONTRACT: &str = r#"
import { ContractWithLints } from "./ContractWithLints.sol";

contract OtherContractWithLints {
uint256 VARIABLE_MIXED_CASE_INFO;
function functionMIXEDCaseInfo() public {}
}
"#;

Expand All @@ -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#"
Expand Down Expand Up @@ -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


"#]]);
Expand All @@ -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![[""]]);
Expand All @@ -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


"#]]);
Expand All @@ -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#"
Expand Down Expand Up @@ -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![[
Expand Down Expand Up @@ -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()
};
});

Expand Down Expand Up @@ -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()
};
});

Expand All @@ -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()
};
});

Expand Down Expand Up @@ -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()
};
});

Expand Down
17 changes: 11 additions & 6 deletions crates/lint/src/linter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 {
Expand All @@ -80,7 +85,7 @@ impl<'s> LintContext<'s> {

/// Helper method to emit diagnostics easily from passes
pub fn emit<L: Lint>(&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;
}

Expand All @@ -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<L: Lint>(&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;
}

Expand Down
62 changes: 54 additions & 8 deletions crates/lint/src/sol/info/mixed_case.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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);
}
Expand All @@ -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())
}
Loading
Loading