From b01c759f0c5371a2e59e271c64d6db10de9182bd Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Tue, 8 Jul 2025 20:08:01 -0400 Subject: [PATCH 01/22] feat: add `unwrapped_modifier_logic` gas lint --- crates/lint/src/sol/gas/mod.rs | 8 +- .../src/sol/gas/unwrapped_modifier_logic.rs | 77 +++++++ crates/lint/testdata/ModifierLogic.sol | 200 ++++++++++++++++++ crates/lint/testdata/ModifierLogic.stderr | 152 +++++++++++++ 4 files changed, 436 insertions(+), 1 deletion(-) create mode 100644 crates/lint/src/sol/gas/unwrapped_modifier_logic.rs create mode 100644 crates/lint/testdata/ModifierLogic.sol create mode 100644 crates/lint/testdata/ModifierLogic.stderr diff --git a/crates/lint/src/sol/gas/mod.rs b/crates/lint/src/sol/gas/mod.rs index 9664f3baf6dcc..0e295eb3dfa69 100644 --- a/crates/lint/src/sol/gas/mod.rs +++ b/crates/lint/src/sol/gas/mod.rs @@ -3,4 +3,10 @@ use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; mod keccak; use keccak::ASM_KECCAK256; -register_lints!((AsmKeccak256, late, (ASM_KECCAK256))); +mod unwrapped_modifier_logic; +use unwrapped_modifier_logic::UNWRAPPED_MODIFIER_LOGIC; + +register_lints!( + (AsmKeccak256, late, (ASM_KECCAK256)), + (ModifierLogic, early, (UNWRAPPED_MODIFIER_LOGIC)) +); diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs new file mode 100644 index 0000000000000..ee7737b259ba8 --- /dev/null +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -0,0 +1,77 @@ +use super::ModifierLogic; +use crate::{ + linter::{EarlyLintPass, LintContext}, + sol::{Severity, SolLint}, +}; +use solar_ast::{ExprKind, FunctionKind, ItemFunction, Stmt, StmtKind}; + +declare_forge_lint!( + UNWRAPPED_MODIFIER_LOGIC, + Severity::Gas, + "unwrapped-modifier-logic", + "modifier logic should be wrapped to avoid code duplication and reduce codesize" +); + +impl<'ast> EarlyLintPass<'ast> for ModifierLogic { + fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { + // Only check modifiers + if func.kind != FunctionKind::Modifier { + return; + } + + // Skip if modifier has no body or the body is empty + let body = match &func.body { + Some(body) if !body.is_empty() => body, + _ => return, + }; + + // Emit lint if the modifier contains unwrapped logic + if contains_unwrapped_logic(body) + && let Some(name) = func.header.name + { + ctx.emit(&UNWRAPPED_MODIFIER_LOGIC, name.span); + } + } +} + +/// Returns true if the modifier body contains any logic other than: +/// - The placeholder `_;` +/// - Calls to internal/private/public functions via direct identifier +fn contains_unwrapped_logic(stmts: &[Stmt<'_>]) -> bool { + stmts.iter().any(|stmt| !is_permitted_statement(stmt)) +} + +/// Returns true if the statement is allowed in a modifier without triggering the lint +fn is_permitted_statement(stmt: &Stmt<'_>) -> bool { + match &stmt.kind { + StmtKind::Placeholder => true, + StmtKind::Expr(expr) => is_permitted_expression(expr), + _ => false, + } +} + +/// Returns true if the expression is a call to a function via direct identifier +/// (i.e., not a built-in like require/assert/revert, and not a member/external call) +fn is_permitted_expression(expr: &solar_ast::Expr<'_>) -> bool { + match &expr.kind { + // Only allow function calls. + ExprKind::Call(func_expr, _) => match &func_expr.kind { + // Allow direct calls to user-defined functions (by identifier) + ExprKind::Ident(ident) => { + // Disallow calls to built-in control flow functions like require/assert/revert + !matches!(ident.name.as_str(), "require" | "assert" | "revert") + } + + // Disallow member calls (e.g., object.method()), which could be external or library + // calls + // TODO: enable library calls + ExprKind::Member(_, _) => false, + + // Disallow all other forms of function expressions (e.g., function pointers, etc.) + _ => false, + }, + + // Disallow all other expression types (not a function call) + _ => false, + } +} diff --git a/crates/lint/testdata/ModifierLogic.sol b/crates/lint/testdata/ModifierLogic.sol new file mode 100644 index 0000000000000..2826b939bd101 --- /dev/null +++ b/crates/lint/testdata/ModifierLogic.sol @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +/** + * @title ModifierLogicTest + * @notice Test cases for the unwrapped-modifier-logic lint + * @dev This lint helps optimize gas by preventing modifier code duplication. + * Solidity inlines modifier code at each usage point instead of using jumps, + * so any logic in modifiers gets duplicated, increasing deployment costs. + */ +contract ModifierLogicTest { + mapping(address => bool) public isOwner; + + // Good patterns: Only call internal/private/public methods + + modifier empty() { + _; + } + + modifier onlyOwnerPublic() { + checkOwnerPublic(msg.sender); + _; + } + + modifier onlyOwnerPrivate() { + checkOwnerPrivate(msg.sender); + _; + } + + modifier onlyOwnerInternal() { + checkOwnerInternal(msg.sender); + _; + } + + modifier ownerOwnerPublicPrivateInternal(address owner0, address owner1, address owner2) { + checkOwnerPublic(owner0); + checkOwnerPrivate(owner1); + checkOwnerInternal(owner2); + _; + } + + modifier singleInternalWithParam(address sender) { + checkOwnerInternal(sender); + _; + } + + modifier multipleInternalWithParam(address owner0, address owner1, address owner2) { + checkOwnerPublic(owner0); + checkOwnerPrivate(owner1); + checkOwnerInternal(owner2); + _; + } + + function checkOwnerPublic(address sender) public view { + require(isOwner[sender], "Not owner"); + } + + function checkOwnerPrivate(address sender) private view { + require(isOwner[sender], "Not owner"); + } + + function checkOwnerInternal(address sender) internal view { + require(isOwner[sender], "Not owner"); + } + + // Bad patterns: Any logic that is not just a call to an internal/private/public method + + // 1. require + modifier onlyOwnerRequire() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + require(isOwner[msg.sender], "Not owner"); + _; + } + + // 2. require with param + modifier onlyOwnerRequireWithParam(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + require(isOwner[sender], "Not owner"); + _; + } + + // 3. assert + modifier onlyOwnerAssert() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + assert(isOwner[msg.sender]); + _; + } + + // 4. assert with param + modifier onlyOwnerAssertWithParam(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + assert(isOwner[sender]); + _; + } + + // 5. conditional revert + modifier onlyOwnerConditionalRevert() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + if (!isOwner[msg.sender]) { + revert("Not owner"); + } + _; + } + + // 6. conditional revert with param + modifier onlyOwnerConditionalRevertWithParam(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + if (!isOwner[sender]) { + revert("Not owner"); + } + _; + } + + // 7. assignment + modifier setOwner(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + isOwner[sender] = true; + _; + } + + // 8. assignment with param + modifier setOwnerWithParam(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + isOwner[sender] = true; + _; + } + + // 9. combination: require + internal call + modifier requireAndInternal(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + require(isOwner[sender], "Not owner"); + checkOwnerInternal(sender); + _; + } + + // 10. combination: assignment + internal call + modifier assignAndInternal(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + isOwner[sender] = true; + checkOwnerInternal(sender); + _; + } + + // 11. combination: require + assignment + modifier requireAndAssign(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + require(isOwner[sender], "Not owner"); + isOwner[sender] = false; + _; + } + + // 12. combination: require + public call + modifier requireAndPublic(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + require(isOwner[sender], "Not owner"); + checkOwnerPublic(sender); + _; + } + + // 13. combination: assignment + public call + modifier assignAndPublic(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + isOwner[sender] = true; + checkOwnerPublic(sender); + _; + } + + // 14. combination: require + assignment + internal call + modifier requireAssignInternal(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + require(isOwner[sender], "Not owner"); + isOwner[sender] = false; + checkOwnerInternal(sender); + _; + } + + // 15. combination: require + assignment + public call + modifier requireAssignPublic(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + require(isOwner[sender], "Not owner"); + isOwner[sender] = false; + checkOwnerPublic(sender); + _; + } + + // 16. inline assembly + modifier withAssembly(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + assembly { + let x := sender + } + _; + } + + // 17. event emission + event DidSomething(address who); + + modifier emitEvent(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + emit DidSomething(sender); + _; + } + + // 18. inline revert string + modifier inlineRevert(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + if (sender == address(0)) revert("Zero address"); + _; + } + + // 19. combination: event + require + internal call + modifier eventRequireInternal(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + emit DidSomething(sender); + require(isOwner[sender], "Not owner"); + checkOwnerInternal(sender); + _; + } +} \ No newline at end of file diff --git a/crates/lint/testdata/ModifierLogic.stderr b/crates/lint/testdata/ModifierLogic.stderr new file mode 100644 index 0000000000000..988c45ca81c79 --- /dev/null +++ b/crates/lint/testdata/ModifierLogic.stderr @@ -0,0 +1,152 @@ +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +69 | modifier onlyOwnerRequire() { + | ---------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +75 | modifier onlyOwnerRequireWithParam(address sender) { + | ------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +81 | modifier onlyOwnerAssert() { + | --------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +87 | modifier onlyOwnerAssertWithParam(address sender) { + | ------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +93 | modifier onlyOwnerConditionalRevert() { + | -------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +101 | modifier onlyOwnerConditionalRevertWithParam(address sender) { + | ----------------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +109 | modifier setOwner(address sender) { + | -------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +115 | modifier setOwnerWithParam(address sender) { + | ----------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +121 | modifier requireAndInternal(address sender) { + | ------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +128 | modifier assignAndInternal(address sender) { + | ----------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +135 | modifier requireAndAssign(address sender) { + | ---------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +142 | modifier requireAndPublic(address sender) { + | ---------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +149 | modifier assignAndPublic(address sender) { + | --------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +156 | modifier requireAssignInternal(address sender) { + | --------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +164 | modifier requireAssignPublic(address sender) { + | ------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +172 | modifier withAssembly(address sender) { + | ------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +182 | modifier emitEvent(address sender) { + | --------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +188 | modifier inlineRevert(address sender) { + | ------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize + --> ROOT/testdata/ModifierLogic.sol:LL:CC + | +194 | modifier eventRequireInternal(address sender) { + | -------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + From 7e9d6c67a16cf893a32607700e2c58c04b0eecd3 Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 9 Jul 2025 00:22:44 -0400 Subject: [PATCH 02/22] cleanup --- .../src/sol/gas/unwrapped_modifier_logic.rs | 58 ++++++++----------- ...erLogic.sol => UnwrappedModifierLogic.sol} | 4 +- ...c.stderr => UnwrappedModifierLogic.stderr} | 38 ++++++------ 3 files changed, 45 insertions(+), 55 deletions(-) rename crates/lint/testdata/{ModifierLogic.sol => UnwrappedModifierLogic.sol} (98%) rename crates/lint/testdata/{ModifierLogic.stderr => UnwrappedModifierLogic.stderr} (85%) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index ee7737b259ba8..117b5e5839cd7 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -1,9 +1,9 @@ -use super::ModifierLogic; +use super::UnwrappedModifierLogic; use crate::{ linter::{EarlyLintPass, LintContext}, sol::{Severity, SolLint}, }; -use solar_ast::{ExprKind, FunctionKind, ItemFunction, Stmt, StmtKind}; +use solar_ast::{ExprKind, ItemFunction, Stmt, StmtKind}; declare_forge_lint!( UNWRAPPED_MODIFIER_LOGIC, @@ -12,21 +12,21 @@ declare_forge_lint!( "modifier logic should be wrapped to avoid code duplication and reduce codesize" ); -impl<'ast> EarlyLintPass<'ast> for ModifierLogic { +impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { - // Only check modifiers - if func.kind != FunctionKind::Modifier { + // If not a modifier, skip. + if !func.kind.is_modifier() { return; } - // Skip if modifier has no body or the body is empty + // If modifier has no contents, skip. let body = match &func.body { - Some(body) if !body.is_empty() => body, + Some(body) => body, _ => return, }; - // Emit lint if the modifier contains unwrapped logic - if contains_unwrapped_logic(body) + // If body contains unwrapped logic, emit. + if body.iter().any(|stmt| !is_valid_stmt(stmt)) && let Some(name) = func.header.name { ctx.emit(&UNWRAPPED_MODIFIER_LOGIC, name.span); @@ -34,44 +34,34 @@ impl<'ast> EarlyLintPass<'ast> for ModifierLogic { } } -/// Returns true if the modifier body contains any logic other than: -/// - The placeholder `_;` -/// - Calls to internal/private/public functions via direct identifier -fn contains_unwrapped_logic(stmts: &[Stmt<'_>]) -> bool { - stmts.iter().any(|stmt| !is_permitted_statement(stmt)) -} - -/// Returns true if the statement is allowed in a modifier without triggering the lint -fn is_permitted_statement(stmt: &Stmt<'_>) -> bool { +fn is_valid_stmt(stmt: &Stmt<'_>) -> bool { match &stmt.kind { + // If the statement is an expression, emit if not valid. + StmtKind::Expr(expr) => is_valid_expr(expr), + + // If the statement is a placeholder, skip. StmtKind::Placeholder => true, - StmtKind::Expr(expr) => is_permitted_expression(expr), + + // Disallow all other statements. _ => false, } } -/// Returns true if the expression is a call to a function via direct identifier -/// (i.e., not a built-in like require/assert/revert, and not a member/external call) -fn is_permitted_expression(expr: &solar_ast::Expr<'_>) -> bool { +fn is_valid_expr(expr: &solar_ast::Expr<'_>) -> bool { match &expr.kind { - // Only allow function calls. + // If the expression is a function call... ExprKind::Call(func_expr, _) => match &func_expr.kind { - // Allow direct calls to user-defined functions (by identifier) - ExprKind::Ident(ident) => { - // Disallow calls to built-in control flow functions like require/assert/revert - !matches!(ident.name.as_str(), "require" | "assert" | "revert") - } + // If the expression is a built-in control flow function, emit. + ExprKind::Ident(ident) => !matches!(ident.name.as_str(), "require" | "assert"), - // Disallow member calls (e.g., object.method()), which could be external or library - // calls - // TODO: enable library calls - ExprKind::Member(_, _) => false, + // If the expression is a member call, emit. + ExprKind::Member(_, _) => false, // TODO: enable library calls - // Disallow all other forms of function expressions (e.g., function pointers, etc.) + // Disallow all other expressions. _ => false, }, - // Disallow all other expression types (not a function call) + // Disallow all other expressions. _ => false, } } diff --git a/crates/lint/testdata/ModifierLogic.sol b/crates/lint/testdata/UnwrappedModifierLogic.sol similarity index 98% rename from crates/lint/testdata/ModifierLogic.sol rename to crates/lint/testdata/UnwrappedModifierLogic.sol index 2826b939bd101..5462d744cf172 100644 --- a/crates/lint/testdata/ModifierLogic.sol +++ b/crates/lint/testdata/UnwrappedModifierLogic.sol @@ -2,13 +2,13 @@ pragma solidity ^0.8.0; /** - * @title ModifierLogicTest + * @title UnwrappedModifierLogicTest * @notice Test cases for the unwrapped-modifier-logic lint * @dev This lint helps optimize gas by preventing modifier code duplication. * Solidity inlines modifier code at each usage point instead of using jumps, * so any logic in modifiers gets duplicated, increasing deployment costs. */ -contract ModifierLogicTest { +contract UnwrappedModifierLogicTest { mapping(address => bool) public isOwner; // Good patterns: Only call internal/private/public methods diff --git a/crates/lint/testdata/ModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr similarity index 85% rename from crates/lint/testdata/ModifierLogic.stderr rename to crates/lint/testdata/UnwrappedModifierLogic.stderr index 988c45ca81c79..bb05275d4683b 100644 --- a/crates/lint/testdata/ModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -1,5 +1,5 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 69 | modifier onlyOwnerRequire() { | ---------------- @@ -7,7 +7,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 75 | modifier onlyOwnerRequireWithParam(address sender) { | ------------------------- @@ -15,7 +15,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 81 | modifier onlyOwnerAssert() { | --------------- @@ -23,7 +23,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 87 | modifier onlyOwnerAssertWithParam(address sender) { | ------------------------ @@ -31,7 +31,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 93 | modifier onlyOwnerConditionalRevert() { | -------------------------- @@ -39,7 +39,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 101 | modifier onlyOwnerConditionalRevertWithParam(address sender) { | ----------------------------------- @@ -47,7 +47,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 109 | modifier setOwner(address sender) { | -------- @@ -55,7 +55,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 115 | modifier setOwnerWithParam(address sender) { | ----------------- @@ -63,7 +63,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 121 | modifier requireAndInternal(address sender) { | ------------------ @@ -71,7 +71,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 128 | modifier assignAndInternal(address sender) { | ----------------- @@ -79,7 +79,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 135 | modifier requireAndAssign(address sender) { | ---------------- @@ -87,7 +87,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 142 | modifier requireAndPublic(address sender) { | ---------------- @@ -95,7 +95,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 149 | modifier assignAndPublic(address sender) { | --------------- @@ -103,7 +103,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 156 | modifier requireAssignInternal(address sender) { | --------------------- @@ -111,7 +111,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 164 | modifier requireAssignPublic(address sender) { | ------------------- @@ -119,7 +119,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 172 | modifier withAssembly(address sender) { | ------------ @@ -127,7 +127,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 182 | modifier emitEvent(address sender) { | --------- @@ -135,7 +135,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 188 | modifier inlineRevert(address sender) { | ------------ @@ -143,7 +143,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/ModifierLogic.sol:LL:CC + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 194 | modifier eventRequireInternal(address sender) { | -------------------- From 7cea74a8d070d6642cd922776e84be348fecc15b Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 9 Jul 2025 00:54:03 -0400 Subject: [PATCH 03/22] cleanup test --- .../lint/testdata/UnwrappedModifierLogic.sol | 190 ++++++------------ .../testdata/UnwrappedModifierLogic.stderr | 122 ++--------- 2 files changed, 69 insertions(+), 243 deletions(-) diff --git a/crates/lint/testdata/UnwrappedModifierLogic.sol b/crates/lint/testdata/UnwrappedModifierLogic.sol index 5462d744cf172..8a6bc344c0c2f 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.sol +++ b/crates/lint/testdata/UnwrappedModifierLogic.sol @@ -11,190 +11,112 @@ pragma solidity ^0.8.0; contract UnwrappedModifierLogicTest { mapping(address => bool) public isOwner; - // Good patterns: Only call internal/private/public methods + // Helpers - modifier empty() { - _; - } - - modifier onlyOwnerPublic() { - checkOwnerPublic(msg.sender); - _; - } - - modifier onlyOwnerPrivate() { - checkOwnerPrivate(msg.sender); - _; - } - - modifier onlyOwnerInternal() { - checkOwnerInternal(msg.sender); - _; - } - - modifier ownerOwnerPublicPrivateInternal(address owner0, address owner1, address owner2) { - checkOwnerPublic(owner0); - checkOwnerPrivate(owner1); - checkOwnerInternal(owner2); - _; - } - - modifier singleInternalWithParam(address sender) { - checkOwnerInternal(sender); - _; - } - - modifier multipleInternalWithParam(address owner0, address owner1, address owner2) { - checkOwnerPublic(owner0); - checkOwnerPrivate(owner1); - checkOwnerInternal(owner2); - _; - } - - function checkOwnerPublic(address sender) public view { + function checkPublic(address sender) public { require(isOwner[sender], "Not owner"); } - function checkOwnerPrivate(address sender) private view { + function checkPrivate(address sender) private { require(isOwner[sender], "Not owner"); } - function checkOwnerInternal(address sender) internal view { + function checkInternal(address sender) internal { require(isOwner[sender], "Not owner"); } - // Bad patterns: Any logic that is not just a call to an internal/private/public method - - // 1. require - modifier onlyOwnerRequire() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - require(isOwner[msg.sender], "Not owner"); - _; - } - - // 2. require with param - modifier onlyOwnerRequireWithParam(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - require(isOwner[sender], "Not owner"); - _; - } - - // 3. assert - modifier onlyOwnerAssert() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - assert(isOwner[msg.sender]); - _; - } - - // 4. assert with param - modifier onlyOwnerAssertWithParam(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - assert(isOwner[sender]); - _; - } - - // 5. conditional revert - modifier onlyOwnerConditionalRevert() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - if (!isOwner[msg.sender]) { - revert("Not owner"); - } - _; - } + // Good patterns - // 6. conditional revert with param - modifier onlyOwnerConditionalRevertWithParam(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - if (!isOwner[sender]) { - revert("Not owner"); - } + modifier empty() { _; } - // 7. assignment - modifier setOwner(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - isOwner[sender] = true; + modifier publicFn() { + checkPublic(msg.sender); _; } - // 8. assignment with param - modifier setOwnerWithParam(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - isOwner[sender] = true; + modifier privateFn() { + checkPrivate(msg.sender); _; } - // 9. combination: require + internal call - modifier requireAndInternal(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - require(isOwner[sender], "Not owner"); - checkOwnerInternal(sender); + modifier internalFn() { + checkInternal(msg.sender); _; } - // 10. combination: assignment + internal call - modifier assignAndInternal(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - isOwner[sender] = true; - checkOwnerInternal(sender); + modifier publicPrivateInternal(address owner0, address owner1, address owner2) { + checkPublic(owner0); + checkPrivate(owner1); + checkInternal(owner2); _; } - // 11. combination: require + assignment - modifier requireAndAssign(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - require(isOwner[sender], "Not owner"); - isOwner[sender] = false; - _; - } + // Bad patterns - // 12. combination: require + public call - modifier requireAndPublic(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - require(isOwner[sender], "Not owner"); - checkOwnerPublic(sender); + modifier requireBuiltIn() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + checkPublic(msg.sender); + require(isOwner[msg.sender], "Not owner"); + checkPrivate(msg.sender); _; + checkInternal(msg.sender); } - // 13. combination: assignment + public call - modifier assignAndPublic(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - isOwner[sender] = true; - checkOwnerPublic(sender); + modifier assertBuiltIn() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + checkPublic(msg.sender); + assert(isOwner[msg.sender]); + checkPrivate(msg.sender); _; + checkInternal(msg.sender); } - // 14. combination: require + assignment + internal call - modifier requireAssignInternal(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - require(isOwner[sender], "Not owner"); - isOwner[sender] = false; - checkOwnerInternal(sender); + modifier conditionalRevert() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + checkPublic(msg.sender); + if (!isOwner[msg.sender]) { + revert("Not owner"); + } + checkPrivate(msg.sender); _; + checkInternal(msg.sender); } - // 15. combination: require + assignment + public call - modifier requireAssignPublic(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - require(isOwner[sender], "Not owner"); - isOwner[sender] = false; - checkOwnerPublic(sender); + modifier assign(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + checkPublic(sender); + bool _isOwner = true; + checkPrivate(sender); + isOwner[sender] = _isOwner; _; + checkInternal(sender); } - // 16. inline assembly - modifier withAssembly(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier assemblyBlock(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + checkPublic(sender); assembly { let x := sender } + checkPrivate(sender); _; + checkInternal(sender); } - // 17. event emission - event DidSomething(address who); - - modifier emitEvent(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - emit DidSomething(sender); + modifier uncheckedBlock(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + checkPublic(sender); + unchecked { + sender; + } + checkPrivate(sender); _; + checkInternal(sender); } - // 18. inline revert string - modifier inlineRevert(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize - if (sender == address(0)) revert("Zero address"); - _; - } + event DidSomething(address who); - // 19. combination: event + require + internal call - modifier eventRequireInternal(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier emitEvent(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + checkPublic(sender); emit DidSomething(sender); - require(isOwner[sender], "Not owner"); - checkOwnerInternal(sender); + checkPrivate(sender); _; + checkInternal(sender); } } \ No newline at end of file diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr index bb05275d4683b..62764aebf8a0b 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -1,152 +1,56 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -69 | modifier onlyOwnerRequire() { - | ---------------- +58 | modifier requireBuiltIn() { + | -------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -75 | modifier onlyOwnerRequireWithParam(address sender) { - | ------------------------- +66 | modifier assertBuiltIn() { + | ------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -81 | modifier onlyOwnerAssert() { - | --------------- +74 | modifier conditionalRevert() { + | ----------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -87 | modifier onlyOwnerAssertWithParam(address sender) { - | ------------------------ +84 | modifier assign(address sender) { + | ------ | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -93 | modifier onlyOwnerConditionalRevert() { - | -------------------------- +93 | modifier assemblyBlock(address sender) { + | ------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -101 | modifier onlyOwnerConditionalRevertWithParam(address sender) { - | ----------------------------------- +103 | modifier uncheckedBlock(address sender) { + | -------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -109 | modifier setOwner(address sender) { - | -------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -115 | modifier setOwnerWithParam(address sender) { - | ----------------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -121 | modifier requireAndInternal(address sender) { - | ------------------ - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -128 | modifier assignAndInternal(address sender) { - | ----------------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -135 | modifier requireAndAssign(address sender) { - | ---------------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -142 | modifier requireAndPublic(address sender) { - | ---------------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -149 | modifier assignAndPublic(address sender) { - | --------------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -156 | modifier requireAssignInternal(address sender) { - | --------------------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -164 | modifier requireAssignPublic(address sender) { - | ------------------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -172 | modifier withAssembly(address sender) { - | ------------ - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -182 | modifier emitEvent(address sender) { +115 | modifier emitEvent(address sender) { | --------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -188 | modifier inlineRevert(address sender) { - | ------------ - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -194 | modifier eventRequireInternal(address sender) { - | -------------------- - | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic - From 8480ae1d90b4947a7fda9dfd35414b3d622827c6 Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 9 Jul 2025 07:35:31 -0400 Subject: [PATCH 04/22] nits --- .../src/sol/gas/unwrapped_modifier_logic.rs | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index 117b5e5839cd7..9897a5a69bf6b 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -20,10 +20,7 @@ impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { } // If modifier has no contents, skip. - let body = match &func.body { - Some(body) => body, - _ => return, - }; + let Some(body) = &func.body else { return }; // If body contains unwrapped logic, emit. if body.iter().any(|stmt| !is_valid_stmt(stmt)) @@ -47,21 +44,16 @@ fn is_valid_stmt(stmt: &Stmt<'_>) -> bool { } } +// TODO: Support library member calls like `Lib.foo` (throws false positives). fn is_valid_expr(expr: &solar_ast::Expr<'_>) -> bool { - match &expr.kind { - // If the expression is a function call... - ExprKind::Call(func_expr, _) => match &func_expr.kind { - // If the expression is a built-in control flow function, emit. - ExprKind::Ident(ident) => !matches!(ident.name.as_str(), "require" | "assert"), - - // If the expression is a member call, emit. - ExprKind::Member(_, _) => false, // TODO: enable library calls - - // Disallow all other expressions. - _ => false, - }, - - // Disallow all other expressions. - _ => false, + // If the expression is a call, continue. + if let ExprKind::Call(func_expr, _) = &expr.kind + && let ExprKind::Ident(ident) = &func_expr.kind + { + // If the call is a built-in control flow function, emit. + return !matches!(ident.name.as_str(), "require" | "assert"); } + + // Disallow all other expressions. + false } From d7a6829bdc319c6598ec7acebfab58097b55fd79 Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Tue, 15 Jul 2025 14:42:40 -0400 Subject: [PATCH 05/22] nit: shorten warning --- .../lint/src/sol/gas/unwrapped_modifier_logic.rs | 2 +- crates/lint/testdata/UnwrappedModifierLogic.sol | 14 +++++++------- crates/lint/testdata/UnwrappedModifierLogic.stderr | 14 +++++++------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index 9897a5a69bf6b..ec96f45ea2399 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -9,7 +9,7 @@ declare_forge_lint!( UNWRAPPED_MODIFIER_LOGIC, Severity::Gas, "unwrapped-modifier-logic", - "modifier logic should be wrapped to avoid code duplication and reduce codesize" + "wrap modifier logic to reduce code size" ); impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { diff --git a/crates/lint/testdata/UnwrappedModifierLogic.sol b/crates/lint/testdata/UnwrappedModifierLogic.sol index 8a6bc344c0c2f..8f7dfc689295a 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.sol +++ b/crates/lint/testdata/UnwrappedModifierLogic.sol @@ -55,7 +55,7 @@ contract UnwrappedModifierLogicTest { // Bad patterns - modifier requireBuiltIn() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier requireBuiltIn() { //~NOTE: wrap modifier logic to reduce code size checkPublic(msg.sender); require(isOwner[msg.sender], "Not owner"); checkPrivate(msg.sender); @@ -63,7 +63,7 @@ contract UnwrappedModifierLogicTest { checkInternal(msg.sender); } - modifier assertBuiltIn() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier assertBuiltIn() { //~NOTE: wrap modifier logic to reduce code size checkPublic(msg.sender); assert(isOwner[msg.sender]); checkPrivate(msg.sender); @@ -71,7 +71,7 @@ contract UnwrappedModifierLogicTest { checkInternal(msg.sender); } - modifier conditionalRevert() { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier conditionalRevert() { //~NOTE: wrap modifier logic to reduce code size checkPublic(msg.sender); if (!isOwner[msg.sender]) { revert("Not owner"); @@ -81,7 +81,7 @@ contract UnwrappedModifierLogicTest { checkInternal(msg.sender); } - modifier assign(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier assign(address sender) { //~NOTE: wrap modifier logic to reduce code size checkPublic(sender); bool _isOwner = true; checkPrivate(sender); @@ -90,7 +90,7 @@ contract UnwrappedModifierLogicTest { checkInternal(sender); } - modifier assemblyBlock(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier assemblyBlock(address sender) { //~NOTE: wrap modifier logic to reduce code size checkPublic(sender); assembly { let x := sender @@ -100,7 +100,7 @@ contract UnwrappedModifierLogicTest { checkInternal(sender); } - modifier uncheckedBlock(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier uncheckedBlock(address sender) { //~NOTE: wrap modifier logic to reduce code size checkPublic(sender); unchecked { sender; @@ -112,7 +112,7 @@ contract UnwrappedModifierLogicTest { event DidSomething(address who); - modifier emitEvent(address sender) { //~NOTE: modifier logic should be wrapped to avoid code duplication and reduce codesize + modifier emitEvent(address sender) { //~NOTE: wrap modifier logic to reduce code size checkPublic(sender); emit DidSomething(sender); checkPrivate(sender); diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr index 62764aebf8a0b..3ec35fa9304dc 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -1,4 +1,4 @@ -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 58 | modifier requireBuiltIn() { @@ -6,7 +6,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 66 | modifier assertBuiltIn() { @@ -14,7 +14,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 74 | modifier conditionalRevert() { @@ -22,7 +22,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 84 | modifier assign(address sender) { @@ -30,7 +30,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 93 | modifier assemblyBlock(address sender) { @@ -38,7 +38,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 103 | modifier uncheckedBlock(address sender) { @@ -46,7 +46,7 @@ note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code d | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic -note[unwrapped-modifier-logic]: modifier logic should be wrapped to avoid code duplication and reduce codesize +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | 115 | modifier emitEvent(address sender) { From 37680b31eff8bd868e59d050de9152454e7b7e12 Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 16 Jul 2025 15:41:42 -0400 Subject: [PATCH 06/22] fix: rebase error --- crates/lint/src/sol/gas/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/lint/src/sol/gas/mod.rs b/crates/lint/src/sol/gas/mod.rs index 0e295eb3dfa69..7b879d98ce8fc 100644 --- a/crates/lint/src/sol/gas/mod.rs +++ b/crates/lint/src/sol/gas/mod.rs @@ -8,5 +8,5 @@ use unwrapped_modifier_logic::UNWRAPPED_MODIFIER_LOGIC; register_lints!( (AsmKeccak256, late, (ASM_KECCAK256)), - (ModifierLogic, early, (UNWRAPPED_MODIFIER_LOGIC)) + (UnwrappedModifierLogic, early, (UNWRAPPED_MODIFIER_LOGIC)) ); From 9f0a28b08894a6d831c606abc1768f077f811b73 Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 16 Jul 2025 18:17:23 -0400 Subject: [PATCH 07/22] suggest fix --- .../src/sol/gas/unwrapped_modifier_logic.rs | 110 ++++++++++++- .../lint/testdata/UnwrappedModifierLogic.sol | 118 +++++++------- .../testdata/UnwrappedModifierLogic.stderr | 146 ++++++++++++++++-- 3 files changed, 303 insertions(+), 71 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index ec96f45ea2399..930eb3f9f71ce 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -1,6 +1,6 @@ use super::UnwrappedModifierLogic; use crate::{ - linter::{EarlyLintPass, LintContext}, + linter::{EarlyLintPass, LintContext, Snippet}, sol::{Severity, SolLint}, }; use solar_ast::{ExprKind, ItemFunction, Stmt, StmtKind}; @@ -14,21 +14,117 @@ declare_forge_lint!( impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { - // If not a modifier, skip. - if !func.kind.is_modifier() { + // Skip non-modifiers and empty modifiers. + if !func.kind.is_modifier() || func.body.is_none() { return; } - // If modifier has no contents, skip. - let Some(body) = &func.body else { return }; + // Get the body of the modifier. + let body = func.body.as_ref().unwrap(); - // If body contains unwrapped logic, emit. - if body.iter().any(|stmt| !is_valid_stmt(stmt)) + // Find the placeholder statement position. + let placeholder_idx = body.iter().position(|s| matches!(s.kind, StmtKind::Placeholder)); + + // Split statements before and after placeholder. + let (before, after) = if let Some(idx) = placeholder_idx { + (&body[..idx], &body[idx + 1..]) + } else { + (&body[..], &[][..]) + }; + + // Check if statements need wrapping. + let needs_lint = |stmts: &[Stmt<'_>]| { + let valid_count = stmts.iter().filter(|s| is_valid_stmt(s)).count(); + let has_invalid = stmts.iter().any(|s| !is_valid_stmt(s)); + has_invalid || valid_count > 1 + }; + + // Emit lint if either section needs wrapping. + if (needs_lint(before) || needs_lint(after)) && let Some(name) = func.header.name { + self.emit_lint_with_fix(ctx, &name, &func.header.parameters, body, before, after); + } + } +} + +impl UnwrappedModifierLogic { + fn emit_lint_with_fix<'a>( + &self, + ctx: &LintContext<'_>, + name: &solar_ast::Ident, + params: &solar_ast::ParameterList<'_>, + full_body: &'a [Stmt<'a>], + before: &'a [Stmt<'a>], + after: &'a [Stmt<'a>], + ) { + if let Some(snippet) = + self.generate_fix(name.name.as_str(), params, full_body, before, after) + { + ctx.emit_with_fix(&UNWRAPPED_MODIFIER_LOGIC, name.span, snippet); + } else { ctx.emit(&UNWRAPPED_MODIFIER_LOGIC, name.span); } } + + fn generate_fix<'a>( + &self, + modifier_name: &str, + params: &solar_ast::ParameterList<'_>, + full_body: &'a [Stmt<'a>], + before: &'a [Stmt<'a>], + after: &'a [Stmt<'a>], + ) -> Option { + // Build parameter list for function calls. + let param_list = format!( + "({})", + params + .vars + .iter() + .filter_map(|v| v.name.as_ref()) + .map(|n| n.name.to_string()) + .collect::>() + .join(", ") + ); + + // Check which sections need wrapping. + let wrap_before = !before.is_empty() && needs_wrapping(before); + let wrap_after = !after.is_empty() && needs_wrapping(after); + + // Generate replacement based on what needs wrapping. + let replacement = match (wrap_before, wrap_after) { + // If both sections need wrapping, wrap both appending `Before` and `After` to the + // modifier name. + (true, true) => format!( + "_{}Before{};\n_;\n_{}After{};", + modifier_name, param_list, modifier_name, param_list + ), + + // If only one before section needs wrapping, wrap that section. + (true, false) => { + format!("_{}{};\n_;", modifier_name, param_list) + } + + // If only one after section needs wrapping, wrap that section. + (false, true) => format!("_;\n_{}{};", modifier_name, param_list), + + // If no sections need wrapping, return None. + (false, false) => return None, + }; + + // Return the replacement snippet. + Some(Snippet::Diff { + desc: Some("wrap modifier logic to reduce code size"), + span: Some(full_body.first()?.span.to(full_body.last()?.span)), + add: replacement, + }) + } +} + +fn needs_wrapping(stmts: &[Stmt<'_>]) -> bool { + let valid_count = stmts.iter().filter(|s| is_valid_stmt(s)).count(); + let has_invalid = stmts.iter().any(|s| !is_valid_stmt(s)); + has_invalid || valid_count > 1 } fn is_valid_stmt(stmt: &Stmt<'_>) -> bool { diff --git a/crates/lint/testdata/UnwrappedModifierLogic.sol b/crates/lint/testdata/UnwrappedModifierLogic.sol index 8f7dfc689295a..e233110bfaa34 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.sol +++ b/crates/lint/testdata/UnwrappedModifierLogic.sol @@ -9,114 +9,128 @@ pragma solidity ^0.8.0; * so any logic in modifiers gets duplicated, increasing deployment costs. */ contract UnwrappedModifierLogicTest { - mapping(address => bool) public isOwner; - // Helpers - function checkPublic(address sender) public { - require(isOwner[sender], "Not owner"); - } + event DidSomething(address who); + mapping(address => bool) isOwner; + mapping(address => mapping(bytes32 => bool)) hasRole; - function checkPrivate(address sender) private { - require(isOwner[sender], "Not owner"); - } + /// ----------------------------------------------------------------------- + /// Good patterns (only 1 valid statement before or after placeholder) + /// ----------------------------------------------------------------------- + + function checkPublic(address sender) public {} + function checkPrivate(address sender) private {} + function checkInternal(address sender) internal {} - function checkInternal(address sender) internal { - require(isOwner[sender], "Not owner"); + modifier onlyOwnerPublic() { + checkPublic(msg.sender); + _; } - // Good patterns + modifier onlyOwnerPrivate() { + checkPrivate(msg.sender); + _; + } - modifier empty() { + modifier onlyOwnerInternal() { + checkInternal(msg.sender); _; } - modifier publicFn() { + modifier onlyOwnerBeforeAfter() { checkPublic(msg.sender); _; + checkPrivate(msg.sender); } - modifier privateFn() { + /// ----------------------------------------------------------------------- + /// Bad patterns (multiple valid statements before or after placeholder) + /// ----------------------------------------------------------------------- + + // Bad because there are multiple valid function calls before the placeholder + modifier multipleBeforePlaceholder() { //~NOTE: wrap modifier logic to reduce code size + checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() checkPrivate(msg.sender); + checkInternal(msg.sender); _; } - modifier internalFn() { - checkInternal(msg.sender); + // Bad because there are multiple valid function calls after the placeholder + modifier multipleAfterPlaceholder() { //~NOTE: wrap modifier logic to reduce code size _; + checkPublic(msg.sender); // These should become _multipleAfterPlaceholder() + checkPrivate(msg.sender); + checkInternal(msg.sender); } - modifier publicPrivateInternal(address owner0, address owner1, address owner2) { - checkPublic(owner0); - checkPrivate(owner1); - checkInternal(owner2); + // Bad because there are multiple valid statements both before and after + modifier multipleBeforeAfterPlaceholder(address sender) { //~NOTE: wrap modifier logic to reduce code size + checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) + checkPrivate(sender); _; + checkInternal(sender); // These should become _multipleBeforeAfterPlaceholderAfter(sender) + checkPublic(sender); } - // Bad patterns + /// ----------------------------------------------------------------------- + /// Bad patterns (uses built-in control flow) + /// ----------------------------------------------------------------------- - modifier requireBuiltIn() { //~NOTE: wrap modifier logic to reduce code size - checkPublic(msg.sender); - require(isOwner[msg.sender], "Not owner"); - checkPrivate(msg.sender); + // Bad because `require` built-in is used. + modifier onlyOwner() { //~NOTE: wrap modifier logic to reduce code size + require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); _; - checkInternal(msg.sender); } - modifier assertBuiltIn() { //~NOTE: wrap modifier logic to reduce code size - checkPublic(msg.sender); - assert(isOwner[msg.sender]); - checkPrivate(msg.sender); + // Bad because `if/revert` is used. + modifier onlyRole(bytes32 role) { //~NOTE: wrap modifier logic to reduce code size + if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); _; - checkInternal(msg.sender); } - modifier conditionalRevert() { //~NOTE: wrap modifier logic to reduce code size - checkPublic(msg.sender); - if (!isOwner[msg.sender]) { - revert("Not owner"); - } - checkPrivate(msg.sender); + // Bad because `assert` built-in is used. + modifier onlyRoleOrOpenRole(bytes32 role) { //~NOTE: wrap modifier logic to reduce code size + assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); _; - checkInternal(msg.sender); } + // Bad because `assert` built-in is used (ensures we can parse multiple params). + modifier onlyRoleOrAdmin(bytes32 role, address admin) { //~NOTE: wrap modifier logic to reduce code size + assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); + _; + } + + /// ----------------------------------------------------------------------- + /// Bad patterns (other invalid expressions and statements) + /// ----------------------------------------------------------------------- + + // Only call expressions are allowed (public/private/internal functions). modifier assign(address sender) { //~NOTE: wrap modifier logic to reduce code size - checkPublic(sender); bool _isOwner = true; - checkPrivate(sender); isOwner[sender] = _isOwner; _; - checkInternal(sender); } + // Only call expressions are allowed (public/private/internal functions). modifier assemblyBlock(address sender) { //~NOTE: wrap modifier logic to reduce code size - checkPublic(sender); assembly { let x := sender } - checkPrivate(sender); _; - checkInternal(sender); } + // Only call expressions are allowed (public/private/internal functions). modifier uncheckedBlock(address sender) { //~NOTE: wrap modifier logic to reduce code size - checkPublic(sender); unchecked { sender; } - checkPrivate(sender); _; - checkInternal(sender); } - event DidSomething(address who); - + // Only call expressions are allowed (public/private/internal functions). modifier emitEvent(address sender) { //~NOTE: wrap modifier logic to reduce code size - checkPublic(sender); emit DidSomething(sender); - checkPrivate(sender); _; - checkInternal(sender); } } \ No newline at end of file diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr index 3ec35fa9304dc..cce95338f0a4d 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -1,56 +1,178 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -58 | modifier requireBuiltIn() { - | -------------- +52 | modifier multipleBeforePlaceholder() { + | ------------------------- | + = note: wrap modifier logic to reduce code size + + - checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() + - checkPrivate(msg.sender); + - checkInternal(msg.sender); + - _; + + _multipleBeforePlaceholder(); + + _; + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -66 | modifier assertBuiltIn() { - | ------------- +60 | modifier multipleAfterPlaceholder() { + | ------------------------ | + = note: wrap modifier logic to reduce code size + + - _; + - checkPublic(msg.sender); // These should become _multipleAfterPlaceholder() + - checkPrivate(msg.sender); + - checkInternal(msg.sender); + + _; + + _multipleAfterPlaceholder(); + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -74 | modifier conditionalRevert() { - | ----------------- +68 | modifier multipleBeforeAfterPlaceholder(address sender) { + | ------------------------------ | + = note: wrap modifier logic to reduce code size + + - checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) + - checkPrivate(sender); + - _; + - checkInternal(sender); // These should become _multipleBeforeAfterPlaceholderAfter(sender) + - checkPublic(sender); + + _multipleBeforeAfterPlaceholderBefore(sender); + + _; + + _multipleBeforeAfterPlaceholderAfter(sender); + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -84 | modifier assign(address sender) { - | ------ +81 | modifier onlyOwner() { + | --------- | + = note: wrap modifier logic to reduce code size + + - require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); + - _; + + _onlyOwner(); + + _; + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -93 | modifier assemblyBlock(address sender) { - | ------------- +87 | modifier onlyRole(bytes32 role) { + | -------- | + = note: wrap modifier logic to reduce code size + + - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); + - _; + + _onlyRole(role); + + _; + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +93 | modifier onlyRoleOrOpenRole(bytes32 role) { + | ------------------ + | + = note: wrap modifier logic to reduce code size + + - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); + - _; + + _onlyRoleOrOpenRole(role); + + _; + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +99 | modifier onlyRoleOrAdmin(bytes32 role, address admin) { + | --------------- + | + = note: wrap modifier logic to reduce code size + + - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); + - _; + + _onlyRoleOrAdmin(role, admin); + + _; + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +109 | modifier assign(address sender) { + | ------ + | + = note: wrap modifier logic to reduce code size + + - bool _isOwner = true; + - isOwner[sender] = _isOwner; + - _; + + _assign(sender); + + _; + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +116 | modifier assemblyBlock(address sender) { + | ------------- + | + = note: wrap modifier logic to reduce code size + + - assembly { + - let x := sender + - } + - _; + + _assemblyBlock(sender); + + _; + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -103 | modifier uncheckedBlock(address sender) { +124 | modifier uncheckedBlock(address sender) { | -------------- | + = note: wrap modifier logic to reduce code size + + - unchecked { + - sender; + - } + - _; + + _uncheckedBlock(sender); + + _; + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -115 | modifier emitEvent(address sender) { +132 | modifier emitEvent(address sender) { | --------- | + = note: wrap modifier logic to reduce code size + + - emit DidSomething(sender); + - _; + + _emitEvent(sender); + + _; + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic From 9009636408736af8f1e81854d18661582555c9c2 Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 16 Jul 2025 19:05:25 -0400 Subject: [PATCH 08/22] cleanup --- crates/lint/src/linter/mod.rs | 2 +- crates/lint/src/sol/gas/unwrapped_modifier_logic.rs | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/lint/src/linter/mod.rs b/crates/lint/src/linter/mod.rs index f6770038eecf3..dcd86d16e2c6a 100644 --- a/crates/lint/src/linter/mod.rs +++ b/crates/lint/src/linter/mod.rs @@ -26,7 +26,7 @@ use crate::inline_config::InlineConfig; /// /// # Required Methods /// -/// - `init`: Creates a new solar `Session` with the appropiate linter configuration. +/// - `init`: Creates a new solar `Session` with the appropriate linter configuration. /// - `early_lint`: Scans the source files (using the AST) emitting a diagnostic for lints found. /// - `late_lint`: Scans the source files (using the HIR) emitting a diagnostic for lints found. pub trait Linter: Send + Sync + Clone { diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index 930eb3f9f71ce..91db29aefcd06 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -96,17 +96,16 @@ impl UnwrappedModifierLogic { // If both sections need wrapping, wrap both appending `Before` and `After` to the // modifier name. (true, true) => format!( - "_{}Before{};\n_;\n_{}After{};", - modifier_name, param_list, modifier_name, param_list + "_{modifier_name}Before{param_list};\n_;\n_{modifier_name}After{param_list};" ), // If only one before section needs wrapping, wrap that section. (true, false) => { - format!("_{}{};\n_;", modifier_name, param_list) + format!("_{modifier_name}{param_list};\n_;") } // If only one after section needs wrapping, wrap that section. - (false, true) => format!("_;\n_{}{};", modifier_name, param_list), + (false, true) => format!("_;\n_{modifier_name}{param_list};"), // If no sections need wrapping, return None. (false, false) => return None, From d9f0d06a1ff97d91ce589bb6e6aaca061fd45a58 Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Thu, 17 Jul 2025 00:27:51 -0400 Subject: [PATCH 09/22] parse indents + restructure --- .../src/sol/gas/unwrapped_modifier_logic.rs | 200 ++++++++---------- .../testdata/UnwrappedModifierLogic.stderr | 68 +++--- 2 files changed, 128 insertions(+), 140 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index 91db29aefcd06..b717557192f19 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -14,141 +14,129 @@ declare_forge_lint!( impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { - // Skip non-modifiers and empty modifiers. - if !func.kind.is_modifier() || func.body.is_none() { + // Only check modifiers with a body and name. + if !func.kind.is_modifier() || func.body.is_none() || func.header.name.is_none() { return; } - // Get the body of the modifier. - let body = func.body.as_ref().unwrap(); + let name = func.header.name.unwrap(); + let stmts = &func.body.as_ref().unwrap().stmts[..]; - // Find the placeholder statement position. - let placeholder_idx = body.iter().position(|s| matches!(s.kind, StmtKind::Placeholder)); + // Split statements into before and after the placeholder `_`. + let (before, after) = stmts + .iter() + .position(|s| matches!(s.kind, StmtKind::Placeholder)) + .map_or((stmts, &[][..]), |idx| (&stmts[..idx], &stmts[idx + 1..])); - // Split statements before and after placeholder. - let (before, after) = if let Some(idx) = placeholder_idx { - (&body[..idx], &body[idx + 1..]) - } else { - (&body[..], &[][..]) - }; - - // Check if statements need wrapping. - let needs_lint = |stmts: &[Stmt<'_>]| { - let valid_count = stmts.iter().filter(|s| is_valid_stmt(s)).count(); - let has_invalid = stmts.iter().any(|s| !is_valid_stmt(s)); - has_invalid || valid_count > 1 - }; - - // Emit lint if either section needs wrapping. - if (needs_lint(before) || needs_lint(after)) - && let Some(name) = func.header.name + // Generate a fix snippet if the modifier logic should be wrapped. + if let Some(snippet) = + self.get_snippet(ctx, &name, &func.header.parameters, stmts, before, after) { - self.emit_lint_with_fix(ctx, &name, &func.header.parameters, body, before, after); + ctx.emit_with_fix(&UNWRAPPED_MODIFIER_LOGIC, name.span, snippet); } } } impl UnwrappedModifierLogic { - fn emit_lint_with_fix<'a>( + // TODO: Support library member calls like `Lib.foo` (throws false positives). + fn is_valid_expr(&self, expr: &solar_ast::Expr<'_>) -> bool { + if let ExprKind::Call(func_expr, _) = &expr.kind + && let ExprKind::Ident(ident) = &func_expr.kind + { + return !matches!(ident.name.as_str(), "require" | "assert"); + } + false + } + + fn is_valid_stmt(&self, stmt: &Stmt<'_>) -> bool { + match &stmt.kind { + StmtKind::Expr(expr) => self.is_valid_expr(expr), + StmtKind::Placeholder => true, + _ => false, + } + } + + fn check_stmts(&self, stmts: &[Stmt<'_>]) -> bool { + let mut total_valid = 0; + for stmt in stmts { + if !self.is_valid_stmt(stmt) { + return true; + } + if let StmtKind::Expr(expr) = &stmt.kind + && self.is_valid_expr(expr) + { + total_valid += 1; + } + } + total_valid > 1 + } + + fn get_indent_and_span( &self, ctx: &LintContext<'_>, - name: &solar_ast::Ident, - params: &solar_ast::ParameterList<'_>, - full_body: &'a [Stmt<'a>], - before: &'a [Stmt<'a>], - after: &'a [Stmt<'a>], - ) { - if let Some(snippet) = - self.generate_fix(name.name.as_str(), params, full_body, before, after) - { - ctx.emit_with_fix(&UNWRAPPED_MODIFIER_LOGIC, name.span, snippet); - } else { - ctx.emit(&UNWRAPPED_MODIFIER_LOGIC, name.span); + full_body: &[Stmt<'_>], + ) -> (String, Option) { + let (first, last) = match (full_body.first(), full_body.last()) { + (Some(f), Some(l)) => (f, l), + _ => return (" ".to_string(), None), + }; + + let source_map = ctx.session().source_map(); + let loc_info = source_map.lookup_char_pos(first.span.lo()); + let line_start = first.span.lo() - solar_interface::BytePos(loc_info.col.to_usize() as u32); + + match source_map.span_to_snippet(solar_ast::Span::new(line_start, first.span.lo())) { + Ok(indent) => (indent, Some(solar_ast::Span::new(line_start, last.span.hi()))), + Err(_) => (" ".to_string(), None), } } - fn generate_fix<'a>( + fn get_snippet<'a>( &self, - modifier_name: &str, + ctx: &LintContext<'_>, + name: &solar_ast::Ident, params: &solar_ast::ParameterList<'_>, full_body: &'a [Stmt<'a>], before: &'a [Stmt<'a>], after: &'a [Stmt<'a>], ) -> Option { - // Build parameter list for function calls. - let param_list = format!( - "({})", - params - .vars - .iter() - .filter_map(|v| v.name.as_ref()) - .map(|n| n.name.to_string()) - .collect::>() - .join(", ") - ); - - // Check which sections need wrapping. - let wrap_before = !before.is_empty() && needs_wrapping(before); - let wrap_after = !after.is_empty() && needs_wrapping(after); - - // Generate replacement based on what needs wrapping. - let replacement = match (wrap_before, wrap_after) { - // If both sections need wrapping, wrap both appending `Before` and `After` to the - // modifier name. - (true, true) => format!( - "_{modifier_name}Before{param_list};\n_;\n_{modifier_name}After{param_list};" - ), + // Check if before/after blocks should be wrapped. + let wrap_before = !before.is_empty() && self.check_stmts(before); + let wrap_after = !after.is_empty() && self.check_stmts(after); - // If only one before section needs wrapping, wrap that section. - (true, false) => { - format!("_{modifier_name}{param_list};\n_;") - } + if !wrap_before && !wrap_after { + return None; + } - // If only one after section needs wrapping, wrap that section. - (false, true) => format!("_;\n_{modifier_name}{param_list};"), + // Get modifier name. + let modifier_name = name.name.as_str(); - // If no sections need wrapping, return None. - (false, false) => return None, + // Get modifier parameters. + let param_list = params + .vars + .iter() + .filter_map(|v| v.name.as_ref()) + .map(|n| n.name.to_string()) + .collect::>() + .join(", "); + + // Get indentation and span. + let (indent, span) = self.get_indent_and_span(ctx, full_body); + + // Generate replacement code. + let fix = match (wrap_before, wrap_after) { + (true, true) => format!( + "{indent}_{modifier_name}Before({param_list});\n{indent}_;\n{indent}_{modifier_name}After({param_list});" + ), + (true, false) => format!("{indent}_{modifier_name}({param_list});\n{indent}_;"), + (false, true) => format!("{indent}_;\n{indent}_{modifier_name}({param_list});"), + (false, false) => unreachable!(), }; - // Return the replacement snippet. Some(Snippet::Diff { desc: Some("wrap modifier logic to reduce code size"), - span: Some(full_body.first()?.span.to(full_body.last()?.span)), - add: replacement, + span, + add: fix, }) } } - -fn needs_wrapping(stmts: &[Stmt<'_>]) -> bool { - let valid_count = stmts.iter().filter(|s| is_valid_stmt(s)).count(); - let has_invalid = stmts.iter().any(|s| !is_valid_stmt(s)); - has_invalid || valid_count > 1 -} - -fn is_valid_stmt(stmt: &Stmt<'_>) -> bool { - match &stmt.kind { - // If the statement is an expression, emit if not valid. - StmtKind::Expr(expr) => is_valid_expr(expr), - - // If the statement is a placeholder, skip. - StmtKind::Placeholder => true, - - // Disallow all other statements. - _ => false, - } -} - -// TODO: Support library member calls like `Lib.foo` (throws false positives). -fn is_valid_expr(expr: &solar_ast::Expr<'_>) -> bool { - // If the expression is a call, continue. - if let ExprKind::Call(func_expr, _) = &expr.kind - && let ExprKind::Ident(ident) = &func_expr.kind - { - // If the call is a built-in control flow function, emit. - return !matches!(ident.name.as_str(), "require" | "assert"); - } - - // Disallow all other expressions. - false -} diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr index cce95338f0a4d..ddb0adda9b6c2 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -6,12 +6,12 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() + - checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() - checkPrivate(msg.sender); - checkInternal(msg.sender); - _; - + _multipleBeforePlaceholder(); - + _; + + _multipleBeforePlaceholder(); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -23,12 +23,12 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - _; + - _; - checkPublic(msg.sender); // These should become _multipleAfterPlaceholder() - checkPrivate(msg.sender); - checkInternal(msg.sender); - + _; - + _multipleAfterPlaceholder(); + + _; + + _multipleAfterPlaceholder(); = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -40,14 +40,14 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) + - checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) - checkPrivate(sender); - _; - checkInternal(sender); // These should become _multipleBeforeAfterPlaceholderAfter(sender) - checkPublic(sender); - + _multipleBeforeAfterPlaceholderBefore(sender); - + _; - + _multipleBeforeAfterPlaceholderAfter(sender); + + _multipleBeforeAfterPlaceholderBefore(sender); + + _; + + _multipleBeforeAfterPlaceholderAfter(sender); = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -59,10 +59,10 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); + - require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); - _; - + _onlyOwner(); - + _; + + _onlyOwner(); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -74,10 +74,10 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); + - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); - _; - + _onlyRole(role); - + _; + + _onlyRole(role); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -89,10 +89,10 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); + - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); - _; - + _onlyRoleOrOpenRole(role); - + _; + + _onlyRoleOrOpenRole(role); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -104,10 +104,10 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); + - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); - _; - + _onlyRoleOrAdmin(role, admin); - + _; + + _onlyRoleOrAdmin(role, admin); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -119,11 +119,11 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - bool _isOwner = true; + - bool _isOwner = true; - isOwner[sender] = _isOwner; - _; - + _assign(sender); - + _; + + _assign(sender); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -135,12 +135,12 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - assembly { + - assembly { - let x := sender - } - _; - + _assemblyBlock(sender); - + _; + + _assemblyBlock(sender); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -152,12 +152,12 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - unchecked { + - unchecked { - sender; - } - _; - + _uncheckedBlock(sender); - + _; + + _uncheckedBlock(sender); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -169,10 +169,10 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - emit DidSomething(sender); + - emit DidSomething(sender); - _; - + _emitEvent(sender); - + _; + + _emitEvent(sender); + + _; = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic From 48fed743dfc3dba2575620b958bb8dd0964aa724 Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Thu, 17 Jul 2025 14:52:21 -0400 Subject: [PATCH 10/22] improve suggestion --- .../src/sol/gas/unwrapped_modifier_logic.rs | 123 +++++++++++++----- .../testdata/UnwrappedModifierLogic.stderr | 103 +++++++++++++++ 2 files changed, 195 insertions(+), 31 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index b717557192f19..471db10a6b60d 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -4,6 +4,7 @@ use crate::{ sol::{Severity, SolLint}, }; use solar_ast::{ExprKind, ItemFunction, Stmt, StmtKind}; +use solar_interface::BytePos; declare_forge_lint!( UNWRAPPED_MODIFIER_LOGIC, @@ -29,9 +30,7 @@ impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { .map_or((stmts, &[][..]), |idx| (&stmts[..idx], &stmts[idx + 1..])); // Generate a fix snippet if the modifier logic should be wrapped. - if let Some(snippet) = - self.get_snippet(ctx, &name, &func.header.parameters, stmts, before, after) - { + if let Some(snippet) = self.get_snippet(ctx, func, stmts, before, after) { ctx.emit_with_fix(&UNWRAPPED_MODIFIER_LOGIC, name.span, snippet); } } @@ -74,69 +73,131 @@ impl UnwrappedModifierLogic { fn get_indent_and_span( &self, ctx: &LintContext<'_>, + func: &ItemFunction<'_>, full_body: &[Stmt<'_>], - ) -> (String, Option) { - let (first, last) = match (full_body.first(), full_body.last()) { + ) -> (String, String, solar_ast::Span) { + let (first, _) = match (full_body.first(), full_body.last()) { (Some(f), Some(l)) => (f, l), - _ => return (" ".to_string(), None), + _ => { + let name_span = func.header.name.unwrap().span; + return (" ".to_string(), " ".to_string(), name_span); + } }; let source_map = ctx.session().source_map(); let loc_info = source_map.lookup_char_pos(first.span.lo()); - let line_start = first.span.lo() - solar_interface::BytePos(loc_info.col.to_usize() as u32); - - match source_map.span_to_snippet(solar_ast::Span::new(line_start, first.span.lo())) { - Ok(indent) => (indent, Some(solar_ast::Span::new(line_start, last.span.hi()))), - Err(_) => (" ".to_string(), None), - } + let line_start = first.span.lo() - BytePos(loc_info.col.to_usize() as u32); + + let body_indent = source_map + .span_to_snippet(solar_ast::Span::new(line_start, first.span.lo())) + .unwrap_or_else(|_| " ".to_string()); + + // Get modifier indent + let name_span = func.header.name.unwrap().span; + let pos = source_map.lookup_char_pos(name_span.lo()); + let mod_line_start = name_span.lo() - BytePos(pos.col.to_usize() as u32); + + let mod_indent = source_map + .span_to_snippet(solar_ast::Span::new(mod_line_start, name_span.lo())) + .ok() + .and_then(|s| s.rfind("modifier").map(|p| mod_line_start + BytePos(p as u32))) + .and_then(|start| { + source_map.span_to_snippet(solar_ast::Span::new(mod_line_start, start)).ok() + }) + .unwrap_or_else(|| " ".to_string()); + + // Get full function span + let start = name_span.lo() + - BytePos(source_map.lookup_char_pos(name_span.lo()).col.to_usize() as u32); + let span = func + .body + .as_ref() + .map(|b| solar_ast::Span::new(start, b.span.hi())) + .unwrap_or(name_span); + + (body_indent, mod_indent, span) } fn get_snippet<'a>( &self, ctx: &LintContext<'_>, - name: &solar_ast::Ident, - params: &solar_ast::ParameterList<'_>, + func: &ItemFunction<'_>, full_body: &'a [Stmt<'a>], before: &'a [Stmt<'a>], after: &'a [Stmt<'a>], ) -> Option { - // Check if before/after blocks should be wrapped. let wrap_before = !before.is_empty() && self.check_stmts(before); let wrap_after = !after.is_empty() && self.check_stmts(after); - if !wrap_before && !wrap_after { + if !(wrap_before || wrap_after) { return None; } - // Get modifier name. - let modifier_name = name.name.as_str(); + let header_name = func.header.name.unwrap(); + let modifier_name = header_name.name.as_str(); + let params = &func.header.parameters; - // Get modifier parameters. let param_list = params .vars .iter() - .filter_map(|v| v.name.as_ref()) - .map(|n| n.name.to_string()) + .filter_map(|v| v.name.as_ref().map(|n| n.name.to_string())) + .collect::>() + .join(", "); + + let param_decls = params + .vars + .iter() + .map(|v| { + let name = v.name.as_ref().map(|n| n.name.as_str()).unwrap_or(""); + let ty = ctx + .span_to_snippet(v.ty.span) + .unwrap_or_else(|| "/* unknown type */".to_string()); + format!("{ty} {name}") + }) .collect::>() .join(", "); - // Get indentation and span. - let (indent, span) = self.get_indent_and_span(ctx, full_body); + let (body_indent, mod_indent, span) = self.get_indent_and_span(ctx, func, full_body); - // Generate replacement code. - let fix = match (wrap_before, wrap_after) { + let body = match (wrap_before, wrap_after) { (true, true) => format!( - "{indent}_{modifier_name}Before({param_list});\n{indent}_;\n{indent}_{modifier_name}After({param_list});" + "{body_indent}_{modifier_name}Before({param_list});\n{body_indent}_;\n{body_indent}_{modifier_name}After({param_list});" ), - (true, false) => format!("{indent}_{modifier_name}({param_list});\n{indent}_;"), - (false, true) => format!("{indent}_;\n{indent}_{modifier_name}({param_list});"), - (false, false) => unreachable!(), + (true, false) => { + format!("{body_indent}_{modifier_name}({param_list});\n{body_indent}_;") + } + (false, true) => { + format!("{body_indent}_;\n{body_indent}_{modifier_name}({param_list});") + } + _ => unreachable!(), + }; + + let mut replacement = format!( + "{mod_indent}modifier {modifier_name}({param_decls}) {{\n{body}\n{mod_indent}}}" + ); + + let build_func = |stmts: &[Stmt<'_>], suffix: &str| { + let body_stmts = stmts + .iter() + .filter_map(|s| ctx.span_to_snippet(s.span)) + .map(|code| format!("\n{body_indent}{code}")) + .collect::(); + format!( + "\n\n{mod_indent}function _{modifier_name}{suffix}({param_decls}) internal {{{body_stmts}\n{mod_indent}}}" + ) }; + if wrap_before { + replacement.push_str(&build_func(before, if wrap_after { "Before" } else { "" })); + } + if wrap_after { + replacement.push_str(&build_func(after, if wrap_before { "After" } else { "" })); + } + Some(Snippet::Diff { desc: Some("wrap modifier logic to reduce code size"), - span, - add: fix, + span: Some(span), + add: replacement, }) } } diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr index ddb0adda9b6c2..fba9b80753397 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -6,12 +6,22 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier multipleBeforePlaceholder() { - checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() - checkPrivate(msg.sender); - checkInternal(msg.sender); - _; + - } + + modifier multipleBeforePlaceholder() { + _multipleBeforePlaceholder(); + _; + + } + + + + function _multipleBeforePlaceholder() internal { + + checkPublic(msg.sender); + + checkPrivate(msg.sender); + + checkInternal(msg.sender); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -23,12 +33,22 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier multipleAfterPlaceholder() { - _; - checkPublic(msg.sender); // These should become _multipleAfterPlaceholder() - checkPrivate(msg.sender); - checkInternal(msg.sender); + - } + + modifier multipleAfterPlaceholder() { + _; + _multipleAfterPlaceholder(); + + } + + + + function _multipleAfterPlaceholder() internal { + + checkPublic(msg.sender); + + checkPrivate(msg.sender); + + checkInternal(msg.sender); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -40,14 +60,28 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier multipleBeforeAfterPlaceholder(address sender) { - checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) - checkPrivate(sender); - _; - checkInternal(sender); // These should become _multipleBeforeAfterPlaceholderAfter(sender) - checkPublic(sender); + - } + + modifier multipleBeforeAfterPlaceholder(address sender) { + _multipleBeforeAfterPlaceholderBefore(sender); + _; + _multipleBeforeAfterPlaceholderAfter(sender); + + } + + + + function _multipleBeforeAfterPlaceholderBefore(address sender) internal { + + checkPublic(sender); + + checkPrivate(sender); + + } + + + + function _multipleBeforeAfterPlaceholderAfter(address sender) internal { + + checkInternal(sender); + + checkPublic(sender); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -59,10 +93,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier onlyOwner() { - require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); - _; + - } + + modifier onlyOwner() { + _onlyOwner(); + _; + + } + + + + function _onlyOwner() internal { + + require(isOwner[msg.sender], "Not owner"); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -74,10 +116,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier onlyRole(bytes32 role) { - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); - _; + - } + + modifier onlyRole(bytes32 role) { + _onlyRole(role); + _; + + } + + + + function _onlyRole(bytes32 role) internal { + + if(!hasRole[msg.sender][role]) revert("Not authorized"); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -89,10 +139,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier onlyRoleOrOpenRole(bytes32 role) { - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); - _; + - } + + modifier onlyRoleOrOpenRole(bytes32 role) { + _onlyRoleOrOpenRole(role); + _; + + } + + + + function _onlyRoleOrOpenRole(bytes32 role) internal { + + assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -104,10 +162,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier onlyRoleOrAdmin(bytes32 role, address admin) { - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); - _; + - } + + modifier onlyRoleOrAdmin(bytes32 role, address admin) { + _onlyRoleOrAdmin(role, admin); + _; + + } + + + + function _onlyRoleOrAdmin(bytes32 role, address admin) internal { + + assert(hasRole[msg.sender][role] || msg.sender == admin); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -119,11 +185,20 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier assign(address sender) { - bool _isOwner = true; - isOwner[sender] = _isOwner; - _; + - } + + modifier assign(address sender) { + _assign(sender); + _; + + } + + + + function _assign(address sender) internal { + + bool _isOwner = true; + + isOwner[sender] = _isOwner; + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -135,12 +210,22 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier assemblyBlock(address sender) { - assembly { - let x := sender - } - _; + - } + + modifier assemblyBlock(address sender) { + _assemblyBlock(sender); + _; + + } + + + + function _assemblyBlock(address sender) internal { + + assembly { + + let x := sender + + } + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -152,12 +237,22 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier uncheckedBlock(address sender) { - unchecked { - sender; - } - _; + - } + + modifier uncheckedBlock(address sender) { + _uncheckedBlock(sender); + _; + + } + + + + function _uncheckedBlock(address sender) internal { + + unchecked { + + sender; + + } + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -169,10 +264,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size + - modifier emitEvent(address sender) { - emit DidSomething(sender); - _; + - } + + modifier emitEvent(address sender) { + _emitEvent(sender); + _; + + } + + + + function _emitEvent(address sender) internal { + + emit DidSomething(sender); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic From 380359f7524cae9a8e142cbd323d07d1616f7df8 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Sat, 19 Jul 2025 08:14:53 +0200 Subject: [PATCH 11/22] chore: normalize ind of the code to be removed --- crates/lint/src/linter/mod.rs | 39 ++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/crates/lint/src/linter/mod.rs b/crates/lint/src/linter/mod.rs index dcd86d16e2c6a..39db11538e8bb 100644 --- a/crates/lint/src/linter/mod.rs +++ b/crates/lint/src/linter/mod.rs @@ -133,32 +133,37 @@ pub enum Snippet { impl Snippet { pub fn to_note(self, ctx: &LintContext<'_>) -> Vec<(DiagMsg, Style)> { - let mut output = Vec::new(); - match self.desc() { - Some(desc) => { - output.push((DiagMsg::from(desc), Style::NoStyle)); - output.push((DiagMsg::from("\n\n"), Style::NoStyle)); - } - None => output.push((DiagMsg::from(" \n"), Style::NoStyle)), - } + let mut output = if let Some(desc) = self.desc() { + vec![(DiagMsg::from(desc), Style::NoStyle), (DiagMsg::from("\n\n"), Style::NoStyle)] + } else { + vec![(DiagMsg::from(" \n"), Style::NoStyle)] + }; + match self { Self::Diff { span, add, .. } => { - // Get the original code from the span if provided + // Get the original code from the span if provided and normalize its indentation if let Some(span) = span && let Some(rmv) = ctx.span_to_snippet(span) { - for line in rmv.lines() { - output.push((DiagMsg::from(format!("- {line}\n")), Style::Removal)); + let mut lines = rmv.lines().peekable(); + if let Some(first) = lines.peek() { + let ind = first.len() - first.trim_start().len(); + output.extend(lines.map(|line| { + ( + DiagMsg::from(format!("- {}\n", line.get(ind..).unwrap_or(""))), + Style::Removal, + ) + })); } } - for line in add.lines() { - output.push((DiagMsg::from(format!("+ {line}\n")), Style::Addition)); - } + output.extend( + add.lines().map(|line| (DiagMsg::from(format!("+ {line}\n")), Style::Addition)), + ); } Self::Block { code, .. } => { - for line in code.lines() { - output.push((DiagMsg::from(format!("- {line}\n")), Style::NoStyle)); - } + output.extend( + code.lines().map(|line| (DiagMsg::from(format!("- {line}\n")), Style::NoStyle)), + ); } } output.push((DiagMsg::from("\n"), Style::NoStyle)); From ea05d4be4b4f4ff7721ea8d7543f2220cbcade7e Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Wed, 23 Jul 2025 15:35:13 +0200 Subject: [PATCH 12/22] improve devex + docs --- crates/lint/src/linter/mod.rs | 96 +++++++++++++++++++++++++++-------- 1 file changed, 75 insertions(+), 21 deletions(-) diff --git a/crates/lint/src/linter/mod.rs b/crates/lint/src/linter/mod.rs index 39db11538e8bb..8030b66aafebf 100644 --- a/crates/lint/src/linter/mod.rs +++ b/crates/lint/src/linter/mod.rs @@ -89,13 +89,13 @@ impl<'s> LintContext<'s> { // Convert the snippet to ensure we have the appropriate type let snippet = match snippet { - Snippet::Diff { desc, span: diff_span, add } => { + Snippet::Diff { desc, span: diff_span, add, trim_code } => { // Use the provided span or fall back to the lint span let target_span = diff_span.unwrap_or(span); // Check if we can get the original code if self.span_to_snippet(target_span).is_some() { - Snippet::Diff { desc, span: Some(target_span), add } + Snippet::Diff { desc, span: Some(target_span), add, trim_code } } else { // Fall back to Block if we can't get the original code Snippet::Block { desc, code: add } @@ -118,17 +118,56 @@ impl<'s> LintContext<'s> { diag.emit(); } + /// Gets the "raw" source code (snippet) of the given span. pub fn span_to_snippet(&self, span: Span) -> Option { self.sess.source_map().span_to_snippet(span).ok() } + + /// Gets the number of leading whitespaces (indentation) of the line where the span begins. + pub fn get_ind_for_span(&self, span: Span) -> usize { + if span.is_dummy() { + return 0; + } + + // Get the line text and compute the indentation prior to the span's position. + let loc = self.sess.source_map().lookup_char_pos(span.lo()); + if let Some(line_text) = loc.file.get_line(loc.line) { + let col_offset = loc.col.to_usize(); + if col_offset <= line_text.len() { + let prev_text = &line_text[..col_offset]; + return prev_text.len() - prev_text.trim().len(); + } + } + + 0 + } } #[derive(Debug, Clone, Eq, PartialEq)] pub enum Snippet { - /// Represents a code block. Can have an optional description. - Block { desc: Option<&'static str>, code: String }, - /// Represents a code diff. Can have an optional description and a span for the code to remove. - Diff { desc: Option<&'static str>, span: Option, add: String }, + /// A standalone block of code. Used for showing examples without suggesting a fix. + Block { + /// An optional description displayed above the code block. + desc: Option<&'static str>, + /// The source code to display. Multi-line strings should include newlines. + code: String, + }, + + /// A proposed code change, displayed as a diff. Used to suggest replacements, showing the code + /// to be removed (from `span`) and the code to be added (from `add`). + Diff { + /// An optional description displayed above the diff. + desc: Option<&'static str>, + /// The `Span` of the source code to be removed. Note that, if uninformed, + /// `fn emit_with_fix()` falls back to the lint span. + span: Option, + /// The fix. + add: String, + /// If `true`, the leading whitespaces of the first line will be trimed from the whole code + /// block. Applies to both, the added and removed code. This is useful for aligning + /// the indentation of multi-line replacements. + trim_code: bool, + }, } impl Snippet { @@ -140,29 +179,31 @@ impl Snippet { }; match self { - Self::Diff { span, add, .. } => { + Self::Diff { span, add, trim_code: trim, .. } => { // Get the original code from the span if provided and normalize its indentation if let Some(span) = span && let Some(rmv) = ctx.span_to_snippet(span) { - let mut lines = rmv.lines().peekable(); - if let Some(first) = lines.peek() { - let ind = first.len() - first.trim_start().len(); - output.extend(lines.map(|line| { - ( - DiagMsg::from(format!("- {}\n", line.get(ind..).unwrap_or(""))), - Style::Removal, - ) - })); - } + let ind = ctx.get_ind_for_span(span); + output.extend(rmv.lines().map(|line| { + let content = if trim { Self::trim_start_limited(line, ind) } else { line }; + (DiagMsg::from(format!("- {content}\n")), Style::Removal) + })); + output.extend(add.lines().map(|line| { + let content = if trim { Self::trim_start_limited(line, ind) } else { line }; + (DiagMsg::from(format!("+ {content}\n")), Style::Addition) + })); + } else { + // Should never happen, but fall back to `Self::Block` behavior. + output.extend( + add.lines() + .map(|line| (DiagMsg::from(format!("{line}\n")), Style::NoStyle)), + ); } - output.extend( - add.lines().map(|line| (DiagMsg::from(format!("+ {line}\n")), Style::Addition)), - ); } Self::Block { code, .. } => { output.extend( - code.lines().map(|line| (DiagMsg::from(format!("- {line}\n")), Style::NoStyle)), + code.lines().map(|line| (DiagMsg::from(format!("{line}\n")), Style::NoStyle)), ); } } @@ -176,4 +217,17 @@ impl Snippet { Self::Block { desc, .. } => *desc, } } + + fn trim_start_limited(s: &str, max_chars: usize) -> &str { + let (mut chars, mut byte_offset) = (0, 0); + for c in s.chars() { + if chars >= max_chars || !c.is_whitespace() { + break; + } + chars += 1; + byte_offset += c.len_utf8(); + } + + &s[byte_offset..] + } } From 9f5557f702170eae99c1dcd3e645fc8bfa1a307d Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Wed, 23 Jul 2025 16:03:51 +0200 Subject: [PATCH 13/22] feat: simplify + use built-in indentation alignment --- .../src/sol/gas/unwrapped_modifier_logic.rs | 74 +---- .../testdata/UnwrappedModifierLogic.stderr | 288 +++++++++--------- 2 files changed, 157 insertions(+), 205 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index 471db10a6b60d..83a22c9628bc8 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -3,8 +3,7 @@ use crate::{ linter::{EarlyLintPass, LintContext, Snippet}, sol::{Severity, SolLint}, }; -use solar_ast::{ExprKind, ItemFunction, Stmt, StmtKind}; -use solar_interface::BytePos; +use solar_ast::{ExprKind, ItemFunction, Span, Stmt, StmtKind}; declare_forge_lint!( UNWRAPPED_MODIFIER_LOGIC, @@ -16,21 +15,20 @@ declare_forge_lint!( impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { // Only check modifiers with a body and name. - if !func.kind.is_modifier() || func.body.is_none() || func.header.name.is_none() { - return; - } - - let name = func.header.name.unwrap(); - let stmts = &func.body.as_ref().unwrap().stmts[..]; + let (body, name) = match (func.body.as_ref(), func.header.name) { + (Some(body), Some(name)) if func.kind.is_modifier() => (body, name), + _ => return, + }; // Split statements into before and after the placeholder `_`. + let stmts = &body.stmts[..]; let (before, after) = stmts .iter() .position(|s| matches!(s.kind, StmtKind::Placeholder)) .map_or((stmts, &[][..]), |idx| (&stmts[..idx], &stmts[idx + 1..])); // Generate a fix snippet if the modifier logic should be wrapped. - if let Some(snippet) = self.get_snippet(ctx, func, stmts, before, after) { + if let Some(snippet) = self.get_snippet(ctx, func, before, after) { ctx.emit_with_fix(&UNWRAPPED_MODIFIER_LOGIC, name.span, snippet); } } @@ -70,59 +68,10 @@ impl UnwrappedModifierLogic { total_valid > 1 } - fn get_indent_and_span( - &self, - ctx: &LintContext<'_>, - func: &ItemFunction<'_>, - full_body: &[Stmt<'_>], - ) -> (String, String, solar_ast::Span) { - let (first, _) = match (full_body.first(), full_body.last()) { - (Some(f), Some(l)) => (f, l), - _ => { - let name_span = func.header.name.unwrap().span; - return (" ".to_string(), " ".to_string(), name_span); - } - }; - - let source_map = ctx.session().source_map(); - let loc_info = source_map.lookup_char_pos(first.span.lo()); - let line_start = first.span.lo() - BytePos(loc_info.col.to_usize() as u32); - - let body_indent = source_map - .span_to_snippet(solar_ast::Span::new(line_start, first.span.lo())) - .unwrap_or_else(|_| " ".to_string()); - - // Get modifier indent - let name_span = func.header.name.unwrap().span; - let pos = source_map.lookup_char_pos(name_span.lo()); - let mod_line_start = name_span.lo() - BytePos(pos.col.to_usize() as u32); - - let mod_indent = source_map - .span_to_snippet(solar_ast::Span::new(mod_line_start, name_span.lo())) - .ok() - .and_then(|s| s.rfind("modifier").map(|p| mod_line_start + BytePos(p as u32))) - .and_then(|start| { - source_map.span_to_snippet(solar_ast::Span::new(mod_line_start, start)).ok() - }) - .unwrap_or_else(|| " ".to_string()); - - // Get full function span - let start = name_span.lo() - - BytePos(source_map.lookup_char_pos(name_span.lo()).col.to_usize() as u32); - let span = func - .body - .as_ref() - .map(|b| solar_ast::Span::new(start, b.span.hi())) - .unwrap_or(name_span); - - (body_indent, mod_indent, span) - } - fn get_snippet<'a>( &self, ctx: &LintContext<'_>, func: &ItemFunction<'_>, - full_body: &'a [Stmt<'a>], before: &'a [Stmt<'a>], after: &'a [Stmt<'a>], ) -> Option { @@ -157,8 +106,9 @@ impl UnwrappedModifierLogic { .collect::>() .join(", "); - let (body_indent, mod_indent, span) = self.get_indent_and_span(ctx, func, full_body); - + let body_indent = " ".repeat(ctx.get_ind_for_span( + before.first().or(after.first()).map(|stmt| stmt.span).unwrap_or(func.header.span), + )); let body = match (wrap_before, wrap_after) { (true, true) => format!( "{body_indent}_{modifier_name}Before({param_list});\n{body_indent}_;\n{body_indent}_{modifier_name}After({param_list});" @@ -172,6 +122,7 @@ impl UnwrappedModifierLogic { _ => unreachable!(), }; + let mod_indent = " ".repeat(ctx.get_ind_for_span(func.header.span)); let mut replacement = format!( "{mod_indent}modifier {modifier_name}({param_decls}) {{\n{body}\n{mod_indent}}}" ); @@ -196,8 +147,9 @@ impl UnwrappedModifierLogic { Some(Snippet::Diff { desc: Some("wrap modifier logic to reduce code size"), - span: Some(span), + span: Some(Span::new(func.header.span.lo(), func.body_span.hi())), add: replacement, + trim_code: true, }) } } diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr index fba9b80753397..be3717bc495d8 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -6,22 +6,22 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier multipleBeforePlaceholder() { - - checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() - - checkPrivate(msg.sender); - - checkInternal(msg.sender); - - _; - - } - + modifier multipleBeforePlaceholder() { - + _multipleBeforePlaceholder(); - + _; - + } + - modifier multipleBeforePlaceholder() { + - checkPublic(msg.sender); // These should become _multipleBeforePlaceholder() + - checkPrivate(msg.sender); + - checkInternal(msg.sender); + - _; + - } + + modifier multipleBeforePlaceholder() { + + _multipleBeforePlaceholder(); + + _; + + } + - + function _multipleBeforePlaceholder() internal { - + checkPublic(msg.sender); - + checkPrivate(msg.sender); - + checkInternal(msg.sender); - + } + + function _multipleBeforePlaceholder() internal { + + checkPublic(msg.sender); + + checkPrivate(msg.sender); + + checkInternal(msg.sender); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -33,22 +33,22 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier multipleAfterPlaceholder() { - - _; - - checkPublic(msg.sender); // These should become _multipleAfterPlaceholder() - - checkPrivate(msg.sender); - - checkInternal(msg.sender); - - } - + modifier multipleAfterPlaceholder() { - + _; - + _multipleAfterPlaceholder(); - + } + - modifier multipleAfterPlaceholder() { + - _; + - checkPublic(msg.sender); // These should become _multipleAfterPlaceholder() + - checkPrivate(msg.sender); + - checkInternal(msg.sender); + - } + + modifier multipleAfterPlaceholder() { + + _; + + _multipleAfterPlaceholder(); + + } + - + function _multipleAfterPlaceholder() internal { - + checkPublic(msg.sender); - + checkPrivate(msg.sender); - + checkInternal(msg.sender); - + } + + function _multipleAfterPlaceholder() internal { + + checkPublic(msg.sender); + + checkPrivate(msg.sender); + + checkInternal(msg.sender); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -60,28 +60,28 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier multipleBeforeAfterPlaceholder(address sender) { - - checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) - - checkPrivate(sender); - - _; - - checkInternal(sender); // These should become _multipleBeforeAfterPlaceholderAfter(sender) - - checkPublic(sender); - - } - + modifier multipleBeforeAfterPlaceholder(address sender) { - + _multipleBeforeAfterPlaceholderBefore(sender); - + _; - + _multipleBeforeAfterPlaceholderAfter(sender); - + } + - modifier multipleBeforeAfterPlaceholder(address sender) { + - checkPublic(sender); // These should become _multipleBeforeAfterPlaceholderBefore(sender) + - checkPrivate(sender); + - _; + - checkInternal(sender); // These should become _multipleBeforeAfterPlaceholderAfter(sender) + - checkPublic(sender); + - } + + modifier multipleBeforeAfterPlaceholder(address sender) { + + _multipleBeforeAfterPlaceholderBefore(sender); + + _; + + _multipleBeforeAfterPlaceholderAfter(sender); + + } + - + function _multipleBeforeAfterPlaceholderBefore(address sender) internal { - + checkPublic(sender); - + checkPrivate(sender); - + } + + function _multipleBeforeAfterPlaceholderBefore(address sender) internal { + + checkPublic(sender); + + checkPrivate(sender); + + } + - + function _multipleBeforeAfterPlaceholderAfter(address sender) internal { - + checkInternal(sender); - + checkPublic(sender); - + } + + function _multipleBeforeAfterPlaceholderAfter(address sender) internal { + + checkInternal(sender); + + checkPublic(sender); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -93,18 +93,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier onlyOwner() { - - require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); - - _; - - } - + modifier onlyOwner() { - + _onlyOwner(); - + _; - + } + - modifier onlyOwner() { + - require(isOwner[msg.sender], "Not owner"); // _onlyOwner(); + - _; + - } + + modifier onlyOwner() { + + _onlyOwner(); + + _; + + } + - + function _onlyOwner() internal { - + require(isOwner[msg.sender], "Not owner"); - + } + + function _onlyOwner() internal { + + require(isOwner[msg.sender], "Not owner"); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -116,18 +116,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier onlyRole(bytes32 role) { - - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); - - _; - - } - + modifier onlyRole(bytes32 role) { - + _onlyRole(role); - + _; - + } + - modifier onlyRole(bytes32 role) { + - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); + - _; + - } + + modifier onlyRole(bytes32 role) { + + _onlyRole(role); + + _; + + } + - + function _onlyRole(bytes32 role) internal { - + if(!hasRole[msg.sender][role]) revert("Not authorized"); - + } + + function _onlyRole(bytes32 role) internal { + + if(!hasRole[msg.sender][role]) revert("Not authorized"); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -139,18 +139,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier onlyRoleOrOpenRole(bytes32 role) { - - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); - - _; - - } - + modifier onlyRoleOrOpenRole(bytes32 role) { - + _onlyRoleOrOpenRole(role); - + _; - + } + - modifier onlyRoleOrOpenRole(bytes32 role) { + - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); + - _; + - } + + modifier onlyRoleOrOpenRole(bytes32 role) { + + _onlyRoleOrOpenRole(role); + + _; + + } + - + function _onlyRoleOrOpenRole(bytes32 role) internal { - + assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); - + } + + function _onlyRoleOrOpenRole(bytes32 role) internal { + + assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -162,18 +162,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier onlyRoleOrAdmin(bytes32 role, address admin) { - - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); - - _; - - } - + modifier onlyRoleOrAdmin(bytes32 role, address admin) { - + _onlyRoleOrAdmin(role, admin); - + _; - + } + - modifier onlyRoleOrAdmin(bytes32 role, address admin) { + - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); + - _; + - } + + modifier onlyRoleOrAdmin(bytes32 role, address admin) { + + _onlyRoleOrAdmin(role, admin); + + _; + + } + - + function _onlyRoleOrAdmin(bytes32 role, address admin) internal { - + assert(hasRole[msg.sender][role] || msg.sender == admin); - + } + + function _onlyRoleOrAdmin(bytes32 role, address admin) internal { + + assert(hasRole[msg.sender][role] || msg.sender == admin); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -185,20 +185,20 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier assign(address sender) { - - bool _isOwner = true; - - isOwner[sender] = _isOwner; - - _; - - } - + modifier assign(address sender) { - + _assign(sender); - + _; - + } + - modifier assign(address sender) { + - bool _isOwner = true; + - isOwner[sender] = _isOwner; + - _; + - } + + modifier assign(address sender) { + + _assign(sender); + + _; + + } + - + function _assign(address sender) internal { - + bool _isOwner = true; - + isOwner[sender] = _isOwner; - + } + + function _assign(address sender) internal { + + bool _isOwner = true; + + isOwner[sender] = _isOwner; + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -210,22 +210,22 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier assemblyBlock(address sender) { - - assembly { - - let x := sender - - } - - _; + - modifier assemblyBlock(address sender) { + - assembly { + - let x := sender - } - + modifier assemblyBlock(address sender) { - + _assemblyBlock(sender); - + _; - + } + - _; + - } + + modifier assemblyBlock(address sender) { + + _assemblyBlock(sender); + + _; + + } + - + function _assemblyBlock(address sender) internal { - + assembly { - + let x := sender - + } + + function _assemblyBlock(address sender) internal { + + assembly { + + let x := sender + } + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -237,22 +237,22 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier uncheckedBlock(address sender) { - - unchecked { - - sender; - - } - - _; + - modifier uncheckedBlock(address sender) { + - unchecked { + - sender; - } - + modifier uncheckedBlock(address sender) { - + _uncheckedBlock(sender); - + _; - + } + - _; + - } + + modifier uncheckedBlock(address sender) { + + _uncheckedBlock(sender); + + _; + + } + - + function _uncheckedBlock(address sender) internal { - + unchecked { - + sender; - + } + + function _uncheckedBlock(address sender) internal { + + unchecked { + + sender; + } + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic @@ -264,18 +264,18 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size | = note: wrap modifier logic to reduce code size - - modifier emitEvent(address sender) { - - emit DidSomething(sender); - - _; - - } - + modifier emitEvent(address sender) { - + _emitEvent(sender); - + _; - + } + - modifier emitEvent(address sender) { + - emit DidSomething(sender); + - _; + - } + + modifier emitEvent(address sender) { + + _emitEvent(sender); + + _; + + } + - + function _emitEvent(address sender) internal { - + emit DidSomething(sender); - + } + + function _emitEvent(address sender) internal { + + emit DidSomething(sender); + + } = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic From 394bb599765a5528f4462e5a9f811e46509a611f Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 23 Jul 2025 14:30:01 -0400 Subject: [PATCH 14/22] fix: false positive library member calls --- crates/lint/src/sol/gas/mod.rs | 2 +- .../src/sol/gas/unwrapped_modifier_logic.rs | 107 +++++++++++------- .../lint/testdata/UnwrappedModifierLogic.sol | 9 ++ .../testdata/UnwrappedModifierLogic.stderr | 102 ++++++++--------- 4 files changed, 125 insertions(+), 95 deletions(-) diff --git a/crates/lint/src/sol/gas/mod.rs b/crates/lint/src/sol/gas/mod.rs index 7b879d98ce8fc..463ec5d3d6a98 100644 --- a/crates/lint/src/sol/gas/mod.rs +++ b/crates/lint/src/sol/gas/mod.rs @@ -8,5 +8,5 @@ use unwrapped_modifier_logic::UNWRAPPED_MODIFIER_LOGIC; register_lints!( (AsmKeccak256, late, (ASM_KECCAK256)), - (UnwrappedModifierLogic, early, (UNWRAPPED_MODIFIER_LOGIC)) + (UnwrappedModifierLogic, late, (UNWRAPPED_MODIFIER_LOGIC)) ); diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index 83a22c9628bc8..c1897e58bbaf9 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -1,9 +1,10 @@ use super::UnwrappedModifierLogic; use crate::{ - linter::{EarlyLintPass, LintContext, Snippet}, + linter::{LateLintPass, LintContext, Snippet}, sol::{Severity, SolLint}, }; -use solar_ast::{ExprKind, ItemFunction, Span, Stmt, StmtKind}; +use solar_ast::Span; +use solar_sema::hir::{self, Res}; declare_forge_lint!( UNWRAPPED_MODIFIER_LOGIC, @@ -12,11 +13,16 @@ declare_forge_lint!( "wrap modifier logic to reduce code size" ); -impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { - fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { - // Only check modifiers with a body and name. - let (body, name) = match (func.body.as_ref(), func.header.name) { - (Some(body), Some(name)) if func.kind.is_modifier() => (body, name), +impl<'hir> LateLintPass<'hir> for UnwrappedModifierLogic { + fn check_function( + &mut self, + ctx: &LintContext<'_>, + hir: &'hir hir::Hir<'hir>, + func: &'hir hir::Function<'hir>, + ) { + // Only check modifiers with a body and a name + let (body, name) = match (func.kind, &func.body, func.name) { + (solar_ast::FunctionKind::Modifier, Some(body), Some(name)) => (body, name), _ => return, }; @@ -24,43 +30,52 @@ impl<'ast> EarlyLintPass<'ast> for UnwrappedModifierLogic { let stmts = &body.stmts[..]; let (before, after) = stmts .iter() - .position(|s| matches!(s.kind, StmtKind::Placeholder)) + .position(|s| matches!(s.kind, hir::StmtKind::Placeholder)) .map_or((stmts, &[][..]), |idx| (&stmts[..idx], &stmts[idx + 1..])); // Generate a fix snippet if the modifier logic should be wrapped. - if let Some(snippet) = self.get_snippet(ctx, func, before, after) { + if let Some(snippet) = self.get_snippet(ctx, hir, func, before, after) { ctx.emit_with_fix(&UNWRAPPED_MODIFIER_LOGIC, name.span, snippet); } } } impl UnwrappedModifierLogic { - // TODO: Support library member calls like `Lib.foo` (throws false positives). - fn is_valid_expr(&self, expr: &solar_ast::Expr<'_>) -> bool { - if let ExprKind::Call(func_expr, _) = &expr.kind - && let ExprKind::Ident(ident) = &func_expr.kind - { - return !matches!(ident.name.as_str(), "require" | "assert"); + fn is_valid_expr(&self, hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> bool { + match &expr.kind { + hir::ExprKind::Call(func_expr, _, _) => match &func_expr.kind { + hir::ExprKind::Ident(resolutions) => { + !resolutions.iter().any(|r| matches!(r, Res::Builtin(_))) + } + hir::ExprKind::Member(base, _) => { + if let hir::ExprKind::Ident(resolutions) = &base.kind { + resolutions.iter().any(|r| matches!(r, Res::Item(hir::ItemId::Contract(id)) if hir.contract(*id).kind == solar_ast::ContractKind::Library)) + } else { + false + } + } + _ => false, + }, + _ => false, } - false } - fn is_valid_stmt(&self, stmt: &Stmt<'_>) -> bool { + fn is_valid_stmt(&self, hir: &hir::Hir<'_>, stmt: &hir::Stmt<'_>) -> bool { match &stmt.kind { - StmtKind::Expr(expr) => self.is_valid_expr(expr), - StmtKind::Placeholder => true, + hir::StmtKind::Expr(expr) => self.is_valid_expr(hir, expr), + hir::StmtKind::Placeholder => true, _ => false, } } - fn check_stmts(&self, stmts: &[Stmt<'_>]) -> bool { + fn check_stmts(&self, hir: &hir::Hir<'_>, stmts: &[hir::Stmt<'_>]) -> bool { let mut total_valid = 0; for stmt in stmts { - if !self.is_valid_stmt(stmt) { + if !self.is_valid_stmt(hir, stmt) { return true; } - if let StmtKind::Expr(expr) = &stmt.kind - && self.is_valid_expr(expr) + if let hir::StmtKind::Expr(expr) = &stmt.kind + && self.is_valid_expr(hir, expr) { total_valid += 1; } @@ -71,35 +86,41 @@ impl UnwrappedModifierLogic { fn get_snippet<'a>( &self, ctx: &LintContext<'_>, - func: &ItemFunction<'_>, - before: &'a [Stmt<'a>], - after: &'a [Stmt<'a>], + hir: &hir::Hir<'_>, + func: &hir::Function<'_>, + before: &'a [hir::Stmt<'a>], + after: &'a [hir::Stmt<'a>], ) -> Option { - let wrap_before = !before.is_empty() && self.check_stmts(before); - let wrap_after = !after.is_empty() && self.check_stmts(after); + let wrap_before = !before.is_empty() && self.check_stmts(hir, before); + let wrap_after = !after.is_empty() && self.check_stmts(hir, after); if !(wrap_before || wrap_after) { return None; } - let header_name = func.header.name.unwrap(); - let modifier_name = header_name.name.as_str(); - let params = &func.header.parameters; + let binding = func.name.unwrap(); + let modifier_name = binding.name.as_str(); - let param_list = params - .vars + // Get parameter names from the function + let param_list = func + .parameters .iter() - .filter_map(|v| v.name.as_ref().map(|n| n.name.to_string())) + .filter_map(|&var_id| { + let var = hir.variable(var_id); + var.name.map(|n| n.name.to_string()) + }) .collect::>() .join(", "); - let param_decls = params - .vars + // Get parameter declarations with types + let param_decls = func + .parameters .iter() - .map(|v| { - let name = v.name.as_ref().map(|n| n.name.as_str()).unwrap_or(""); + .map(|&var_id| { + let var = hir.variable(var_id); + let name = var.name.as_ref().map(|n| n.name.as_str()).unwrap_or(""); let ty = ctx - .span_to_snippet(v.ty.span) + .span_to_snippet(var.ty.span) .unwrap_or_else(|| "/* unknown type */".to_string()); format!("{ty} {name}") }) @@ -107,7 +128,7 @@ impl UnwrappedModifierLogic { .join(", "); let body_indent = " ".repeat(ctx.get_ind_for_span( - before.first().or(after.first()).map(|stmt| stmt.span).unwrap_or(func.header.span), + before.first().or(after.first()).map(|stmt| stmt.span).unwrap_or(func.span), )); let body = match (wrap_before, wrap_after) { (true, true) => format!( @@ -122,12 +143,12 @@ impl UnwrappedModifierLogic { _ => unreachable!(), }; - let mod_indent = " ".repeat(ctx.get_ind_for_span(func.header.span)); + let mod_indent = " ".repeat(ctx.get_ind_for_span(func.span)); let mut replacement = format!( "{mod_indent}modifier {modifier_name}({param_decls}) {{\n{body}\n{mod_indent}}}" ); - let build_func = |stmts: &[Stmt<'_>], suffix: &str| { + let build_func = |stmts: &[hir::Stmt<'_>], suffix: &str| { let body_stmts = stmts .iter() .filter_map(|s| ctx.span_to_snippet(s.span)) @@ -147,7 +168,7 @@ impl UnwrappedModifierLogic { Some(Snippet::Diff { desc: Some("wrap modifier logic to reduce code size"), - span: Some(Span::new(func.header.span.lo(), func.body_span.hi())), + span: Some(Span::new(func.span.lo(), func.body_span.hi())), add: replacement, trim_code: true, }) diff --git a/crates/lint/testdata/UnwrappedModifierLogic.sol b/crates/lint/testdata/UnwrappedModifierLogic.sol index e233110bfaa34..7a937fabcb548 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.sol +++ b/crates/lint/testdata/UnwrappedModifierLogic.sol @@ -1,6 +1,10 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; +library Lib { + function onlyOwner(address sender) internal {} +} + /** * @title UnwrappedModifierLogicTest * @notice Test cases for the unwrapped-modifier-logic lint @@ -23,6 +27,11 @@ contract UnwrappedModifierLogicTest { function checkPrivate(address sender) private {} function checkInternal(address sender) internal {} + modifier onlyOwnerLibrary() { + Lib.onlyOwner(msg.sender); + _; + } + modifier onlyOwnerPublic() { checkPublic(msg.sender); _; diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr index be3717bc495d8..e0480f7e920cc 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -1,7 +1,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -52 | modifier multipleBeforePlaceholder() { +61 | modifier multipleBeforePlaceholder() { | ------------------------- | = note: wrap modifier logic to reduce code size @@ -28,7 +28,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -60 | modifier multipleAfterPlaceholder() { +69 | modifier multipleAfterPlaceholder() { | ------------------------ | = note: wrap modifier logic to reduce code size @@ -55,7 +55,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -68 | modifier multipleBeforeAfterPlaceholder(address sender) { +77 | modifier multipleBeforeAfterPlaceholder(address sender) { | ------------------------------ | = note: wrap modifier logic to reduce code size @@ -88,7 +88,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -81 | modifier onlyOwner() { +90 | modifier onlyOwner() { | --------- | = note: wrap modifier logic to reduce code size @@ -111,7 +111,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -87 | modifier onlyRole(bytes32 role) { +96 | modifier onlyRole(bytes32 role) { | -------- | = note: wrap modifier logic to reduce code size @@ -132,55 +132,55 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -93 | modifier onlyRoleOrOpenRole(bytes32 role) { - | ------------------ - | - = note: wrap modifier logic to reduce code size - - - modifier onlyRoleOrOpenRole(bytes32 role) { - - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); - - _; - - } - + modifier onlyRoleOrOpenRole(bytes32 role) { - + _onlyRoleOrOpenRole(role); - + _; - + } - + - + function _onlyRoleOrOpenRole(bytes32 role) internal { - + assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); - + } - - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +102 | modifier onlyRoleOrOpenRole(bytes32 role) { + | ------------------ + | + = note: wrap modifier logic to reduce code size + + - modifier onlyRoleOrOpenRole(bytes32 role) { + - assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); // _onlyRoleOrOpenRole(role); + - _; + - } + + modifier onlyRoleOrOpenRole(bytes32 role) { + + _onlyRoleOrOpenRole(role); + + _; + + } + + + + function _onlyRoleOrOpenRole(bytes32 role) internal { + + assert(hasRole[msg.sender][role] || hasRole[address(0)][role]); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -99 | modifier onlyRoleOrAdmin(bytes32 role, address admin) { - | --------------- - | - = note: wrap modifier logic to reduce code size - - - modifier onlyRoleOrAdmin(bytes32 role, address admin) { - - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); - - _; - - } - + modifier onlyRoleOrAdmin(bytes32 role, address admin) { - + _onlyRoleOrAdmin(role, admin); - + _; - + } - + - + function _onlyRoleOrAdmin(bytes32 role, address admin) internal { - + assert(hasRole[msg.sender][role] || msg.sender == admin); - + } - - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +108 | modifier onlyRoleOrAdmin(bytes32 role, address admin) { + | --------------- + | + = note: wrap modifier logic to reduce code size + + - modifier onlyRoleOrAdmin(bytes32 role, address admin) { + - assert(hasRole[msg.sender][role] || msg.sender == admin); // _onlyRoleOrAdmin(role, admin); + - _; + - } + + modifier onlyRoleOrAdmin(bytes32 role, address admin) { + + _onlyRoleOrAdmin(role, admin); + + _; + + } + + + + function _onlyRoleOrAdmin(bytes32 role, address admin) internal { + + assert(hasRole[msg.sender][role] || msg.sender == admin); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -109 | modifier assign(address sender) { +118 | modifier assign(address sender) { | ------ | = note: wrap modifier logic to reduce code size @@ -205,7 +205,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -116 | modifier assemblyBlock(address sender) { +125 | modifier assemblyBlock(address sender) { | ------------- | = note: wrap modifier logic to reduce code size @@ -232,7 +232,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -124 | modifier uncheckedBlock(address sender) { +133 | modifier uncheckedBlock(address sender) { | -------------- | = note: wrap modifier logic to reduce code size @@ -259,7 +259,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -132 | modifier emitEvent(address sender) { +141 | modifier emitEvent(address sender) { | --------- | = note: wrap modifier logic to reduce code size From b0d793d6ce2db655a0920a03627066556bdb8cdf Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 23 Jul 2025 14:31:55 -0400 Subject: [PATCH 15/22] nit --- crates/lint/src/sol/gas/unwrapped_modifier_logic.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index c1897e58bbaf9..bba67e485b2ff 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -69,7 +69,7 @@ impl UnwrappedModifierLogic { } fn check_stmts(&self, hir: &hir::Hir<'_>, stmts: &[hir::Stmt<'_>]) -> bool { - let mut total_valid = 0; + let mut has_valid = false; for stmt in stmts { if !self.is_valid_stmt(hir, stmt) { return true; @@ -77,10 +77,13 @@ impl UnwrappedModifierLogic { if let hir::StmtKind::Expr(expr) = &stmt.kind && self.is_valid_expr(hir, expr) { - total_valid += 1; + if has_valid { + return true; + } + has_valid = true; } } - total_valid > 1 + false } fn get_snippet<'a>( From e6c08dd60d95866b5aa3ffa0dd2a5f7734c29c1e Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Wed, 23 Jul 2025 19:10:48 -0400 Subject: [PATCH 16/22] test: external calls throw lint --- .../lint/testdata/UnwrappedModifierLogic.sol | 16 ++++ .../testdata/UnwrappedModifierLogic.stderr | 85 ++++++++++++------- 2 files changed, 70 insertions(+), 31 deletions(-) diff --git a/crates/lint/testdata/UnwrappedModifierLogic.sol b/crates/lint/testdata/UnwrappedModifierLogic.sol index 7a937fabcb548..acaa891fb0f26 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.sol +++ b/crates/lint/testdata/UnwrappedModifierLogic.sol @@ -5,6 +5,10 @@ library Lib { function onlyOwner(address sender) internal {} } +contract C { + function onlyOwner() public {} +} + /** * @title UnwrappedModifierLogicTest * @notice Test cases for the unwrapped-modifier-logic lint @@ -15,6 +19,8 @@ library Lib { contract UnwrappedModifierLogicTest { // Helpers + C immutable c; + event DidSomething(address who); mapping(address => bool) isOwner; mapping(address => mapping(bytes32 => bool)) hasRole; @@ -142,4 +148,14 @@ contract UnwrappedModifierLogicTest { emit DidSomething(sender); _; } + + /// ----------------------------------------------------------------------- + /// Bad patterns (contract calls) + /// ----------------------------------------------------------------------- + + // Bad because there's an external call. + modifier onlyOwnerContract(address sender) { //~NOTE: wrap modifier logic to reduce code size + c.onlyOwner(sender); + _; + } } \ No newline at end of file diff --git a/crates/lint/testdata/UnwrappedModifierLogic.stderr b/crates/lint/testdata/UnwrappedModifierLogic.stderr index e0480f7e920cc..9885759311cc7 100644 --- a/crates/lint/testdata/UnwrappedModifierLogic.stderr +++ b/crates/lint/testdata/UnwrappedModifierLogic.stderr @@ -1,7 +1,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -61 | modifier multipleBeforePlaceholder() { +67 | modifier multipleBeforePlaceholder() { | ------------------------- | = note: wrap modifier logic to reduce code size @@ -28,7 +28,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -69 | modifier multipleAfterPlaceholder() { +75 | modifier multipleAfterPlaceholder() { | ------------------------ | = note: wrap modifier logic to reduce code size @@ -55,7 +55,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -77 | modifier multipleBeforeAfterPlaceholder(address sender) { +83 | modifier multipleBeforeAfterPlaceholder(address sender) { | ------------------------------ | = note: wrap modifier logic to reduce code size @@ -88,7 +88,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -90 | modifier onlyOwner() { +96 | modifier onlyOwner() { | --------- | = note: wrap modifier logic to reduce code size @@ -109,32 +109,32 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size - --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC - | -96 | modifier onlyRole(bytes32 role) { - | -------- - | - = note: wrap modifier logic to reduce code size - - - modifier onlyRole(bytes32 role) { - - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); - - _; - - } - + modifier onlyRole(bytes32 role) { - + _onlyRole(role); - + _; - + } - + - + function _onlyRole(bytes32 role) internal { - + if(!hasRole[msg.sender][role]) revert("Not authorized"); - + } - - = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +102 | modifier onlyRole(bytes32 role) { + | -------- + | + = note: wrap modifier logic to reduce code size + + - modifier onlyRole(bytes32 role) { + - if(!hasRole[msg.sender][role]) revert("Not authorized"); // _onlyRole(role); + - _; + - } + + modifier onlyRole(bytes32 role) { + + _onlyRole(role); + + _; + + } + + + + function _onlyRole(bytes32 role) internal { + + if(!hasRole[msg.sender][role]) revert("Not authorized"); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -102 | modifier onlyRoleOrOpenRole(bytes32 role) { +108 | modifier onlyRoleOrOpenRole(bytes32 role) { | ------------------ | = note: wrap modifier logic to reduce code size @@ -157,7 +157,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -108 | modifier onlyRoleOrAdmin(bytes32 role, address admin) { +114 | modifier onlyRoleOrAdmin(bytes32 role, address admin) { | --------------- | = note: wrap modifier logic to reduce code size @@ -180,7 +180,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -118 | modifier assign(address sender) { +124 | modifier assign(address sender) { | ------ | = note: wrap modifier logic to reduce code size @@ -205,7 +205,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -125 | modifier assemblyBlock(address sender) { +131 | modifier assemblyBlock(address sender) { | ------------- | = note: wrap modifier logic to reduce code size @@ -232,7 +232,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -133 | modifier uncheckedBlock(address sender) { +139 | modifier uncheckedBlock(address sender) { | -------------- | = note: wrap modifier logic to reduce code size @@ -259,7 +259,7 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC | -141 | modifier emitEvent(address sender) { +147 | modifier emitEvent(address sender) { | --------- | = note: wrap modifier logic to reduce code size @@ -279,3 +279,26 @@ note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic +note[unwrapped-modifier-logic]: wrap modifier logic to reduce code size + --> ROOT/testdata/UnwrappedModifierLogic.sol:LL:CC + | +157 | modifier onlyOwnerContract(address sender) { + | ----------------- + | + = note: wrap modifier logic to reduce code size + + - modifier onlyOwnerContract(address sender) { + - c.onlyOwner(sender); + - _; + - } + + modifier onlyOwnerContract(address sender) { + + _onlyOwnerContract(sender); + + _; + + } + + + + function _onlyOwnerContract(address sender) internal { + + c.onlyOwner(sender); + + } + + = help: https://book.getfoundry.sh/reference/forge/forge-lint#unwrapped-modifier-logic + From 7949511f722c4f8f82a60e6905e01ba804582a60 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 24 Jul 2025 16:16:41 +0200 Subject: [PATCH 17/22] refactor: simplify + document fns --- .../src/sol/gas/unwrapped_modifier_logic.rs | 75 ++++++++++--------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index bba67e485b2ff..3ecd0e23cfc4a 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -3,7 +3,7 @@ use crate::{ linter::{LateLintPass, LintContext, Snippet}, sol::{Severity, SolLint}, }; -use solar_ast::Span; +use solar_ast::{self as ast, Span}; use solar_sema::hir::{self, Res}; declare_forge_lint!( @@ -22,7 +22,7 @@ impl<'hir> LateLintPass<'hir> for UnwrappedModifierLogic { ) { // Only check modifiers with a body and a name let (body, name) = match (func.kind, &func.body, func.name) { - (solar_ast::FunctionKind::Modifier, Some(body), Some(name)) => (body, name), + (ast::FunctionKind::Modifier, Some(body), Some(name)) => (body, name), _ => return, }; @@ -41,48 +41,49 @@ impl<'hir> LateLintPass<'hir> for UnwrappedModifierLogic { } impl UnwrappedModifierLogic { + /// Returns `true` if an expr is not a built-in ('require' or 'assert') call or a lib function. fn is_valid_expr(&self, hir: &hir::Hir<'_>, expr: &hir::Expr<'_>) -> bool { - match &expr.kind { - hir::ExprKind::Call(func_expr, _, _) => match &func_expr.kind { - hir::ExprKind::Ident(resolutions) => { - !resolutions.iter().any(|r| matches!(r, Res::Builtin(_))) - } - hir::ExprKind::Member(base, _) => { - if let hir::ExprKind::Ident(resolutions) = &base.kind { - resolutions.iter().any(|r| matches!(r, Res::Item(hir::ItemId::Contract(id)) if hir.contract(*id).kind == solar_ast::ContractKind::Library)) - } else { - false - } - } - _ => false, - }, - _ => false, - } - } + if let hir::ExprKind::Call(func_expr, _, _) = &expr.kind { + if let hir::ExprKind::Ident(resolutions) = &func_expr.kind { + return !resolutions.iter().any(|r| matches!(r, Res::Builtin(_))); + } - fn is_valid_stmt(&self, hir: &hir::Hir<'_>, stmt: &hir::Stmt<'_>) -> bool { - match &stmt.kind { - hir::StmtKind::Expr(expr) => self.is_valid_expr(hir, expr), - hir::StmtKind::Placeholder => true, - _ => false, + if let hir::ExprKind::Member(base, _) = &func_expr.kind + && let hir::ExprKind::Ident(resolutions) = &base.kind + { + return resolutions.iter().any(|r| { + matches!(r, Res::Item(hir::ItemId::Contract(id)) if hir.contract(*id).kind == ast::ContractKind::Library) + }); + } } + + false } - fn check_stmts(&self, hir: &hir::Hir<'_>, stmts: &[hir::Stmt<'_>]) -> bool { - let mut has_valid = false; + /// Checks if a block of statements is complex and should be wrapped in a helper function. + /// + /// This is true if the block contains: + /// 1. Any statement that is not a placeholder or a valid expression. + /// 2. More than one simple call expression. + fn stmts_require_wrapping(&self, hir: &hir::Hir<'_>, stmts: &[hir::Stmt<'_>]) -> bool { + let mut has_valid_stmt = false; for stmt in stmts { - if !self.is_valid_stmt(hir, stmt) { - return true; - } - if let hir::StmtKind::Expr(expr) = &stmt.kind - && self.is_valid_expr(hir, expr) - { - if has_valid { - return true; + match &stmt.kind { + hir::StmtKind::Placeholder => continue, + hir::StmtKind::Expr(expr) => { + if self.is_valid_expr(hir, expr) { + if has_valid_stmt { + return true; + } + has_valid_stmt = true; + } else { + return true; + } } - has_valid = true; + _ => return true, } } + false } @@ -94,8 +95,8 @@ impl UnwrappedModifierLogic { before: &'a [hir::Stmt<'a>], after: &'a [hir::Stmt<'a>], ) -> Option { - let wrap_before = !before.is_empty() && self.check_stmts(hir, before); - let wrap_after = !after.is_empty() && self.check_stmts(hir, after); + let wrap_before = !before.is_empty() && self.stmts_require_wrapping(hir, before); + let wrap_after = !after.is_empty() && self.stmts_require_wrapping(hir, after); if !(wrap_before || wrap_after) { return None; From a6a31830fe85c04333492fdfd6ca32cab6c03220 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 24 Jul 2025 16:21:28 +0200 Subject: [PATCH 18/22] style: clippy --- crates/lint/src/linter/mod.rs | 6 +++--- crates/lint/src/sol/gas/unwrapped_modifier_logic.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/lint/src/linter/mod.rs b/crates/lint/src/linter/mod.rs index 8030b66aafebf..8b30dd6660bd5 100644 --- a/crates/lint/src/linter/mod.rs +++ b/crates/lint/src/linter/mod.rs @@ -163,9 +163,9 @@ pub enum Snippet { span: Option, /// The fix. add: String, - /// If `true`, the leading whitespaces of the first line will be trimed from the whole code - /// block. Applies to both, the added and removed code. This is useful for aligning - /// the indentation of multi-line replacements. + /// If `true`, the leading whitespaces of the first line will be trimmed from the whole + /// code block. Applies to both, the added and removed code. This is useful for + /// aligning the indentation of multi-line replacements. trim_code: bool, }, } diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index 3ecd0e23cfc4a..c2603981cb24d 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -27,7 +27,7 @@ impl<'hir> LateLintPass<'hir> for UnwrappedModifierLogic { }; // Split statements into before and after the placeholder `_`. - let stmts = &body.stmts[..]; + let stmts = body.stmts[..].as_ref(); let (before, after) = stmts .iter() .position(|s| matches!(s.kind, hir::StmtKind::Placeholder)) From c876c5eb7d56a4f08976ac7b1d2ea80a8673c749 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 24 Jul 2025 16:28:47 +0200 Subject: [PATCH 19/22] nit --- crates/lint/src/sol/gas/unwrapped_modifier_logic.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs index c2603981cb24d..b9835db8b50ce 100644 --- a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs @@ -71,14 +71,10 @@ impl UnwrappedModifierLogic { match &stmt.kind { hir::StmtKind::Placeholder => continue, hir::StmtKind::Expr(expr) => { - if self.is_valid_expr(hir, expr) { - if has_valid_stmt { - return true; - } - has_valid_stmt = true; - } else { + if !self.is_valid_expr(hir, expr) || has_valid_stmt { return true; } + has_valid_stmt = true; } _ => return true, } From b9799e62c46b0f11a8aaf44beeb05f72bec33b26 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Thu, 24 Jul 2025 16:59:01 +0200 Subject: [PATCH 20/22] refactor: move to new `codesize` lint group --- crates/config/src/lint.rs | 8 ++++++-- crates/lint/src/sol/codesize/mod.rs | 6 ++++++ .../src/sol/{gas => codesize}/unwrapped_modifier_logic.rs | 0 crates/lint/src/sol/gas/mod.rs | 8 +------- crates/lint/src/sol/mod.rs | 8 ++++++-- 5 files changed, 19 insertions(+), 11 deletions(-) create mode 100644 crates/lint/src/sol/codesize/mod.rs rename crates/lint/src/sol/{gas => codesize}/unwrapped_modifier_logic.rs (100%) diff --git a/crates/config/src/lint.rs b/crates/config/src/lint.rs index fbb80c5d83cb8..7fdd7a18cf6f9 100644 --- a/crates/config/src/lint.rs +++ b/crates/config/src/lint.rs @@ -45,6 +45,7 @@ pub enum Severity { Low, Info, Gas, + CodeSize, } impl Severity { @@ -55,6 +56,7 @@ impl Severity { Self::Low => Paint::yellow(message).bold().to_string(), Self::Info => Paint::cyan(message).bold().to_string(), Self::Gas => Paint::green(message).bold().to_string(), + Self::CodeSize => Paint::green(message).bold().to_string(), } } } @@ -63,7 +65,7 @@ impl From for Level { fn from(severity: Severity) -> Self { match severity { Severity::High | Severity::Med | Severity::Low => Self::Warning, - Severity::Info | Severity::Gas => Self::Note, + Severity::Info | Severity::Gas | Severity::CodeSize => Self::Note, } } } @@ -76,6 +78,7 @@ impl fmt::Display for Severity { Self::Low => self.color("Low"), Self::Info => self.color("Info"), Self::Gas => self.color("Gas"), + Self::CodeSize => self.color("CodeSize"), }; write!(f, "{colored}") } @@ -102,8 +105,9 @@ impl FromStr for Severity { "low" => Ok(Self::Low), "info" => Ok(Self::Info), "gas" => Ok(Self::Gas), + "size" | "codesize" | "code-size" => Ok(Self::CodeSize), _ => Err(format!( - "unknown variant: found `{s}`, expected `one of `High`, `Med`, `Low`, `Info`, `Gas``" + "unknown variant: found `{s}`, expected `one of `High`, `Med`, `Low`, `Info`, `Gas`, CodeSize`" )), } } diff --git a/crates/lint/src/sol/codesize/mod.rs b/crates/lint/src/sol/codesize/mod.rs new file mode 100644 index 0000000000000..730b341fbc35a --- /dev/null +++ b/crates/lint/src/sol/codesize/mod.rs @@ -0,0 +1,6 @@ +use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; + +mod unwrapped_modifier_logic; +use unwrapped_modifier_logic::UNWRAPPED_MODIFIER_LOGIC; + +register_lints!((UnwrappedModifierLogic, late, (UNWRAPPED_MODIFIER_LOGIC))); diff --git a/crates/lint/src/sol/gas/unwrapped_modifier_logic.rs b/crates/lint/src/sol/codesize/unwrapped_modifier_logic.rs similarity index 100% rename from crates/lint/src/sol/gas/unwrapped_modifier_logic.rs rename to crates/lint/src/sol/codesize/unwrapped_modifier_logic.rs diff --git a/crates/lint/src/sol/gas/mod.rs b/crates/lint/src/sol/gas/mod.rs index 463ec5d3d6a98..a77f9be9d1e95 100644 --- a/crates/lint/src/sol/gas/mod.rs +++ b/crates/lint/src/sol/gas/mod.rs @@ -3,10 +3,4 @@ use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; mod keccak; use keccak::ASM_KECCAK256; -mod unwrapped_modifier_logic; -use unwrapped_modifier_logic::UNWRAPPED_MODIFIER_LOGIC; - -register_lints!( - (AsmKeccak256, late, (ASM_KECCAK256)), - (UnwrappedModifierLogic, late, (UNWRAPPED_MODIFIER_LOGIC)) -); +register_lints!((AsmKeccak256, late, (ASM_KECCAK256)),); diff --git a/crates/lint/src/sol/mod.rs b/crates/lint/src/sol/mod.rs index 01d04373c6ac1..4654f3d0f6715 100644 --- a/crates/lint/src/sol/mod.rs +++ b/crates/lint/src/sol/mod.rs @@ -27,6 +27,7 @@ use thiserror::Error; #[macro_use] pub mod macros; +pub mod codesize; pub mod gas; pub mod high; pub mod info; @@ -38,6 +39,7 @@ static ALL_REGISTERED_LINTS: LazyLock> = LazyLock::new(|| { lints.extend_from_slice(med::REGISTERED_LINTS); lints.extend_from_slice(info::REGISTERED_LINTS); lints.extend_from_slice(gas::REGISTERED_LINTS); + lints.extend_from_slice(codesize::REGISTERED_LINTS); lints.into_iter().map(|lint| lint.id()).collect() }); @@ -109,9 +111,10 @@ impl SolidityLinter { passes_and_lints.extend(med::create_early_lint_passes()); passes_and_lints.extend(info::create_early_lint_passes()); - // Do not apply gas-severity rules on tests and scripts + // Do not apply 'gas' and 'codesize' severity rules on tests and scripts if !self.path_config.is_test_or_script(path) { passes_and_lints.extend(gas::create_early_lint_passes()); + passes_and_lints.extend(codesize::create_early_lint_passes()); } // Filter passes based on linter config @@ -146,11 +149,12 @@ impl SolidityLinter { passes_and_lints.extend(med::create_late_lint_passes()); passes_and_lints.extend(info::create_late_lint_passes()); - // Do not apply gas-severity rules on tests and scripts + // Do not apply 'gas' and 'codesize' severity rules on tests and scripts if let FileName::Real(ref path) = file.name && !self.path_config.is_test_or_script(path) { passes_and_lints.extend(gas::create_late_lint_passes()); + passes_and_lints.extend(codesize::create_late_lint_passes()); } // Filter passes based on config From 1883e22bc36aec5738a86e2084e3f0bdcc08d69c Mon Sep 17 00:00:00 2001 From: 0xrusowsky <90208954+0xrusowsky@users.noreply.github.com> Date: Thu, 24 Jul 2025 17:28:02 +0200 Subject: [PATCH 21/22] fix: typo Co-authored-by: clandestine.eth <96172957+0xClandestine@users.noreply.github.com> --- crates/config/src/lint.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/config/src/lint.rs b/crates/config/src/lint.rs index 7fdd7a18cf6f9..1c5c6fdb02b2c 100644 --- a/crates/config/src/lint.rs +++ b/crates/config/src/lint.rs @@ -107,7 +107,7 @@ impl FromStr for Severity { "gas" => Ok(Self::Gas), "size" | "codesize" | "code-size" => Ok(Self::CodeSize), _ => Err(format!( - "unknown variant: found `{s}`, expected `one of `High`, `Med`, `Low`, `Info`, `Gas`, CodeSize`" + "unknown variant: found `{s}`, expected `one of `High`, `Med`, `Low`, `Info`, `Gas`, `CodeSize`" )), } } From 5dd172c93ce5e8981495087ac3e48f67c03683ad Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Fri, 25 Jul 2025 13:03:40 +0200 Subject: [PATCH 22/22] chore: code clarity + more docs --- crates/lint/src/linter/mod.rs | 37 +++++++------- .../sol/codesize/unwrapped_modifier_logic.rs | 48 ++++++++----------- 2 files changed, 37 insertions(+), 48 deletions(-) diff --git a/crates/lint/src/linter/mod.rs b/crates/lint/src/linter/mod.rs index c419e153dfa30..e1b66e2bcce4d 100644 --- a/crates/lint/src/linter/mod.rs +++ b/crates/lint/src/linter/mod.rs @@ -128,18 +128,16 @@ impl<'s> LintContext<'s> { } /// Gets the number of leading whitespaces (indentation) of the line where the span begins. - pub fn get_ind_for_span(&self, span: Span) -> usize { - if span.is_dummy() { - return 0; - } - - // Get the line text and compute the indentation prior to the span's position. - let loc = self.sess.source_map().lookup_char_pos(span.lo()); - if let Some(line_text) = loc.file.get_line(loc.line) { - let col_offset = loc.col.to_usize(); - if col_offset <= line_text.len() { - let prev_text = &line_text[..col_offset]; - return prev_text.len() - prev_text.trim().len(); + pub fn get_span_indentation(&self, span: Span) -> usize { + if !span.is_dummy() { + // Get the line text and compute the indentation prior to the span's position. + let loc = self.sess.source_map().lookup_char_pos(span.lo()); + if let Some(line_text) = loc.file.get_line(loc.line) { + let col_offset = loc.col.to_usize(); + if col_offset <= line_text.len() { + let prev_text = &line_text[..col_offset]; + return prev_text.len() - prev_text.trim().len(); + } } } @@ -188,15 +186,13 @@ impl Snippet { if let Some(span) = span && let Some(rmv) = ctx.span_to_snippet(span) { - let ind = ctx.get_ind_for_span(span); - output.extend(rmv.lines().map(|line| { - let content = if trim { Self::trim_start_limited(line, ind) } else { line }; - (DiagMsg::from(format!("- {content}\n")), Style::Removal) - })); - output.extend(add.lines().map(|line| { + let ind = ctx.get_span_indentation(span); + let diag_msg = |line: &str, prefix: &str, style: Style| { let content = if trim { Self::trim_start_limited(line, ind) } else { line }; - (DiagMsg::from(format!("+ {content}\n")), Style::Addition) - })); + (DiagMsg::from(format!("{prefix}{content}\n")), style) + }; + output.extend(rmv.lines().map(|line| diag_msg(line, "- ", Style::Removal))); + output.extend(add.lines().map(|line| diag_msg(line, "+ ", Style::Addition))); } else { // Should never happen, but fall back to `Self::Block` behavior. output.extend( @@ -222,6 +218,7 @@ impl Snippet { } } + /// Removes up to `max_chars` whitespaces from the start of the string. fn trim_start_limited(s: &str, max_chars: usize) -> &str { let (mut chars, mut byte_offset) = (0, 0); for c in s.chars() { diff --git a/crates/lint/src/sol/codesize/unwrapped_modifier_logic.rs b/crates/lint/src/sol/codesize/unwrapped_modifier_logic.rs index b9835db8b50ce..d7eedb168503a 100644 --- a/crates/lint/src/sol/codesize/unwrapped_modifier_logic.rs +++ b/crates/lint/src/sol/codesize/unwrapped_modifier_logic.rs @@ -100,34 +100,26 @@ impl UnwrappedModifierLogic { let binding = func.name.unwrap(); let modifier_name = binding.name.as_str(); + let mut param_list = vec![]; + let mut param_decls = vec![]; + + for var_id in func.parameters { + let var = hir.variable(*var_id); + let ty = ctx + .span_to_snippet(var.ty.span) + .unwrap_or_else(|| "/* unknown type */".to_string()); + + // solidity functions should always have named parameters + if let Some(ident) = var.name { + param_list.push(ident.to_string()); + param_decls.push(format!("{ty} {}", ident.to_string())); + } + } - // Get parameter names from the function - let param_list = func - .parameters - .iter() - .filter_map(|&var_id| { - let var = hir.variable(var_id); - var.name.map(|n| n.name.to_string()) - }) - .collect::>() - .join(", "); - - // Get parameter declarations with types - let param_decls = func - .parameters - .iter() - .map(|&var_id| { - let var = hir.variable(var_id); - let name = var.name.as_ref().map(|n| n.name.as_str()).unwrap_or(""); - let ty = ctx - .span_to_snippet(var.ty.span) - .unwrap_or_else(|| "/* unknown type */".to_string()); - format!("{ty} {name}") - }) - .collect::>() - .join(", "); - - let body_indent = " ".repeat(ctx.get_ind_for_span( + let param_list = param_list.join(", "); + let param_decls = param_decls.join(", "); + + let body_indent = " ".repeat(ctx.get_span_indentation( before.first().or(after.first()).map(|stmt| stmt.span).unwrap_or(func.span), )); let body = match (wrap_before, wrap_after) { @@ -143,7 +135,7 @@ impl UnwrappedModifierLogic { _ => unreachable!(), }; - let mod_indent = " ".repeat(ctx.get_ind_for_span(func.span)); + let mod_indent = " ".repeat(ctx.get_span_indentation(func.span)); let mut replacement = format!( "{mod_indent}modifier {modifier_name}({param_decls}) {{\n{body}\n{mod_indent}}}" );