fix: false positives builtin functions

protofire · Aug 7, 2023 · 2a08c95 · 2a08c95
1 parent 3444491
commit 2a08c95
Show file tree

Hide file tree

Showing 8 changed files with 120 additions and 87 deletions.
diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js
@@ -29,7 +29,7 @@ module.exports = Object.freeze({
     'contract-name-camelcase': 'warn',
     'event-name-camelcase': 'warn',
     'func-name-mixedcase': 'warn',
-    'func-named-parameters': ['warn', 2],
+    'func-named-parameters': ['warn', 4],
     'func-param-name-mixedcase': 'warn',
     'immutable-vars-naming': [
       'warn',

diff --git a/conf/rulesets/solhint-recommended.js b/conf/rulesets/solhint-recommended.js
@@ -24,7 +24,6 @@ module.exports = Object.freeze({
     'contract-name-camelcase': 'warn',
     'event-name-camelcase': 'warn',
     'func-name-mixedcase': 'warn',
-    'func-named-parameters': ['warn', 2],
     'immutable-vars-naming': [
       'warn',
       {

diff --git a/docs/rules.md b/docs/rules.md
@@ -33,24 +33,24 @@ title:       "Rule Index of Solhint"
 
 ## Style Guide Rules
 
-| Rule Id                                                                              | Error                                                                                                   | Recommended  | Deprecated  |
-| ------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------- | ------------ | ----------- |
-| [const-name-snakecase](./rules/naming/const-name-snakecase.md)                       | Constant name must be in capitalized SNAKE_CASE. (Does not check IMMUTABLES, use immutable-vars-naming) | $~~~~~~~~$✔️ |             |
-| [contract-name-camelcase](./rules/naming/contract-name-camelcase.md)                 | Contract name must be in CamelCase.                                                                     | $~~~~~~~~$✔️ |             |
-| [event-name-camelcase](./rules/naming/event-name-camelcase.md)                       | Event name must be in CamelCase.                                                                        | $~~~~~~~~$✔️ |             |
-| [func-name-mixedcase](./rules/naming/func-name-mixedcase.md)                         | Function name must be in mixedCase.                                                                     | $~~~~~~~~$✔️ |             |
-| [func-named-parameters](./rules/naming/func-named-parameters.md)                     | Enforce function calls with Named Parameters when containing more than the configured qty               | $~~~~~~~~$✔️ |             |
-| [func-param-name-mixedcase](./rules/naming/func-param-name-mixedcase.md)             | Function param name must be in mixedCase.                                                               |              |             |
-| [immutable-vars-naming](./rules/naming/immutable-vars-naming.md)                     | Check Immutable variables. Capitalized SNAKE_CASE or mixedCase depending on configuration.              | $~~~~~~~~$✔️ |             |
-| [modifier-name-mixedcase](./rules/naming/modifier-name-mixedcase.md)                 | Modifier name must be in mixedCase.                                                                     |              |             |
-| [named-parameters-mapping](./rules/naming/named-parameters-mapping.md)               | Solidity v0.8.18 introduced named parameters on the mappings definition.                                |              |             |
-| [private-vars-leading-underscore](./rules/naming/private-vars-leading-underscore.md) | Private and internal names must start with a single underscore.                                         |              |             |
-| [use-forbidden-name](./rules/naming/use-forbidden-name.md)                           | Avoid to use letters 'I', 'l', 'O' as identifiers.                                                      | $~~~~~~~~$✔️ |             |
-| [var-name-mixedcase](./rules/naming/var-name-mixedcase.md)                           | Variable name must be in mixedCase. (Does not check IMMUTABLES, use immutable-vars-naming)              | $~~~~~~~~$✔️ |             |
-| [func-order](./rules/order/func-order.md)                                            | Function order is incorrect.                                                                            |              | $~~~~~~~$✔️ |
-| [imports-on-top](./rules/order/imports-on-top.md)                                    | Import statements must be on top.                                                                       | $~~~~~~~~$✔️ |             |
-| [ordering](./rules/order/ordering.md)                                                | Check order of elements in file and inside each contract, according to the style guide.                 |              |             |
-| [visibility-modifier-order](./rules/order/visibility-modifier-order.md)              | Visibility modifier must be first in list of modifiers.                                                 | $~~~~~~~~$✔️ |             |
+| Rule Id                                                                              | Error                                                                                                         | Recommended  | Deprecated  |
+| ------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------- | ------------ | ----------- |
+| [const-name-snakecase](./rules/naming/const-name-snakecase.md)                       | Constant name must be in capitalized SNAKE_CASE. (Does not check IMMUTABLES, use immutable-vars-naming)       | $~~~~~~~~$✔️ |             |
+| [contract-name-camelcase](./rules/naming/contract-name-camelcase.md)                 | Contract name must be in CamelCase.                                                                           | $~~~~~~~~$✔️ |             |
+| [event-name-camelcase](./rules/naming/event-name-camelcase.md)                       | Event name must be in CamelCase.                                                                              | $~~~~~~~~$✔️ |             |
+| [func-name-mixedcase](./rules/naming/func-name-mixedcase.md)                         | Function name must be in mixedCase.                                                                           | $~~~~~~~~$✔️ |             |
+| [func-named-parameters](./rules/naming/func-named-parameters.md)                     | Enforce named parameters for function calls with 4 or more arguments. This rule may have some false positives |              |             |
+| [func-param-name-mixedcase](./rules/naming/func-param-name-mixedcase.md)             | Function param name must be in mixedCase.                                                                     |              |             |
+| [immutable-vars-naming](./rules/naming/immutable-vars-naming.md)                     | Check Immutable variables. Capitalized SNAKE_CASE or mixedCase depending on configuration.                    | $~~~~~~~~$✔️ |             |
+| [modifier-name-mixedcase](./rules/naming/modifier-name-mixedcase.md)                 | Modifier name must be in mixedCase.                                                                           |              |             |
+| [named-parameters-mapping](./rules/naming/named-parameters-mapping.md)               | Solidity v0.8.18 introduced named parameters on the mappings definition.                                      |              |             |
+| [private-vars-leading-underscore](./rules/naming/private-vars-leading-underscore.md) | Private and internal names must start with a single underscore.                                               |              |             |
+| [use-forbidden-name](./rules/naming/use-forbidden-name.md)                           | Avoid to use letters 'I', 'l', 'O' as identifiers.                                                            | $~~~~~~~~$✔️ |             |
+| [var-name-mixedcase](./rules/naming/var-name-mixedcase.md)                           | Variable name must be in mixedCase. (Does not check IMMUTABLES, use immutable-vars-naming)                    | $~~~~~~~~$✔️ |             |
+| [func-order](./rules/order/func-order.md)                                            | Function order is incorrect.                                                                                  |              | $~~~~~~~$✔️ |
+| [imports-on-top](./rules/order/imports-on-top.md)                                    | Import statements must be on top.                                                                             | $~~~~~~~~$✔️ |             |
+| [ordering](./rules/order/ordering.md)                                                | Check order of elements in file and inside each contract, according to the style guide.                       |              |             |
+| [visibility-modifier-order](./rules/order/visibility-modifier-order.md)              | Visibility modifier must be first in list of modifiers.                                                       | $~~~~~~~~$✔️ |             |
         
 
 ## Security Rules

diff --git a/docs/rules/naming/func-named-parameters.md b/docs/rules/naming/func-named-parameters.md
@@ -5,29 +5,26 @@ title:       "func-named-parameters | Solhint"
 ---
 
 # func-named-parameters
-![Recommended Badge](https://img.shields.io/badge/-Recommended-brightgreen)
 ![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational)
 ![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)
-> The {"extends": "solhint:recommended"} property in a configuration file enables this rule.
-
 
 ## Description
-Enforce function calls with Named Parameters when containing more than the configured qty
+Enforce named parameters for function calls with 4 or more arguments. This rule may have some false positives
 
 ## Options
 This rule accepts an array of options:
 
-| Index | Description                                            | Default Value |
-| ----- | ------------------------------------------------------ | ------------- |
-| 0     | Rule severity. Must be one of "error", "warn", "off".  | warn          |
-| 1     | Maximum qty of unnamed parameters for a function call. | 2             |
+| Index | Description                                                                                              | Default Value |
+| ----- | -------------------------------------------------------------------------------------------------------- | ------------- |
+| 0     | Rule severity. Must be one of "error", "warn", "off".                                                    | warn          |
+| 1     | Minimum qty of unnamed parameters for a function call (to prevent false positives on builtin functions). | 4             |
 
 
 ### Example Config
 ```json
 {
   "rules": {
-    "func-named-parameters": ["warn",2]
+    "func-named-parameters": ["warn",4]
   }
 }
 ```
@@ -36,7 +33,7 @@ This rule accepts an array of options:
 ## Examples
 ### 👍 Examples of **correct** code for this rule
 
-#### Function call with two UNNAMED parameters (default is max 2)
+#### Function call with two UNNAMED parameters (default is 4)
 
 ```solidity
 functionName('0xA81705c8C247C413a19A244938ae7f4A0393944e', 1e18)
@@ -56,7 +53,7 @@ functionName({ sender: _senderAddress, amount: 1e18, token: _tokenAddress, recei
 
 ### 👎 Examples of **incorrect** code for this rule
 
-#### Function call with four UNNAMED parameters (default is max 2)
+#### Function call with four UNNAMED parameters (default 4)
 
 ```solidity
 functionName(_senderAddress, 1e18, _tokenAddress, _receiverAddress )

diff --git a/lib/rules/naming/func-named-parameters.js b/lib/rules/naming/func-named-parameters.js
@@ -2,30 +2,30 @@ const BaseChecker = require('../base-checker')
 const { severityDescription } = require('../../doc/utils')
 
 const DEFAULT_SEVERITY = 'warn'
-const DEFAULT_MAX_UNNAMED_ARGUMENTS = 2
+const DEFAULT_MIN_UNNAMED_ARGUMENTS = 4
 
 const ruleId = 'func-named-parameters'
 const meta = {
   type: 'naming',
 
   docs: {
-    description:
-      'Enforce function calls with Named Parameters when containing more than the configured qty',
+    description: `Enforce named parameters for function calls with ${DEFAULT_MIN_UNNAMED_ARGUMENTS} or more arguments. This rule may have some false positives`,
     category: 'Style Guide Rules',
     options: [
       {
         description: severityDescription,
         default: DEFAULT_SEVERITY,
       },
       {
-        description: 'Maximum qty of unnamed parameters for a function call.',
-        default: JSON.stringify(DEFAULT_MAX_UNNAMED_ARGUMENTS),
+        description:
+          'Minimum qty of unnamed parameters for a function call (to prevent false positives on builtin functions).',
+        default: JSON.stringify(DEFAULT_MIN_UNNAMED_ARGUMENTS),
       },
     ],
     examples: {
       good: [
         {
-          description: 'Function call with two UNNAMED parameters (default is max 2)',
+          description: 'Function call with two UNNAMED parameters (default is 4)',
           code: "functionName('0xA81705c8C247C413a19A244938ae7f4A0393944e', 1e18)",
         },
         {
@@ -39,25 +39,28 @@ const meta = {
       ],
       bad: [
         {
-          description: 'Function call with four UNNAMED parameters (default is max 2)',
+          description: 'Function call with four UNNAMED parameters (default 4)',
           code: 'functionName(_senderAddress, 1e18, _tokenAddress, _receiverAddress )',
         },
       ],
     },
   },
 
   isDefault: false,
-  recommended: true,
-  defaultSetup: [DEFAULT_SEVERITY, DEFAULT_MAX_UNNAMED_ARGUMENTS],
+  recommended: false,
+  defaultSetup: [DEFAULT_SEVERITY, DEFAULT_MIN_UNNAMED_ARGUMENTS],
 
   schema: { type: 'integer' },
 }
 
 class FunctionNamedParametersChecker extends BaseChecker {
   constructor(reporter, config) {
     super(reporter, ruleId, meta)
-    this.maxUnnamedArguments = config && config.getNumber(ruleId, DEFAULT_MAX_UNNAMED_ARGUMENTS)
-    if (!(this.maxUnnamedArguments >= 0)) this.maxUnnamedArguments = DEFAULT_MAX_UNNAMED_ARGUMENTS
+    this.maxUnnamedArguments =
+      (config && config.getNumber(ruleId, DEFAULT_MIN_UNNAMED_ARGUMENTS)) ||
+      DEFAULT_MIN_UNNAMED_ARGUMENTS
+    if (this.maxUnnamedArguments < DEFAULT_MIN_UNNAMED_ARGUMENTS)
+      this.maxUnnamedArguments = DEFAULT_MIN_UNNAMED_ARGUMENTS
   }
 
   FunctionCall(node) {
@@ -68,7 +71,7 @@ class FunctionNamedParametersChecker extends BaseChecker {
       if (qtyNamed === 0 && qtyArgs > this.maxUnnamedArguments) {
         this.error(
           node,
-          `Missing named parameters. Max unnamed parameters value is ${this.maxUnnamedArguments}`
+          `Named parameters missing. MIN unnamed argumenst is ${this.maxUnnamedArguments}`
         )
       }
     }

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "solhint",
-  "version": "3.5.1",
+  "version": "3.5.2",
   "description": "Solidity Code Linter",
   "main": "lib/index.js",
   "keywords": [

diff --git a/test/fixtures/naming/func-named-parameters.js b/test/fixtures/naming/func-named-parameters.js
@@ -1,53 +1,64 @@
 const FUNCTION_CALLS_ERRORS = {
-  maxUnnamed_0_1u: {
-    code: 'funcName(_sender);',
-    maxUnnamed: 0,
+  err1: {
+    code: 'funcName(_sender, amount, receiver, token1, token2, token3);',
+    minUnnamed: 5,
   },
 
-  maxUnnamed_1_3u: {
-    code: 'funcName(_sender, amount, receiver);',
-    maxUnnamed: 1,
+  err2: {
+    code: 'funcName(_sender, amount, receiver, token1, token2);',
+    minUnnamed: 4,
   },
 
-  maxUnnamed_2_3u: {
-    code: 'funcName(_sender, amount, receiver);',
-    maxUnnamed: 2,
-  },
-
-  maxUnnamed_3_4u: {
-    code: 'funcName(_sender, amount, receiver, token);',
-    maxUnnamed: 3,
+  err3: {
+    code: 'funcName(_sender, amount, receiver, token1, token2);',
+    minUnnamed: 0,
   },
 }
 
 const FUNCTION_CALLS_OK = {
-  maxUnnamed_0_0u: {
+  ok1: {
     code: 'funcName();',
-    maxUnnamed: 0,
+    minUnnamed: 0,
   },
 
-  maxUnnamed_0_0uB: {
-    code: 'funcName();',
-    maxUnnamed: 10,
+  ok2: {
+    code: 'address(0);',
+    minUnnamed: 10,
   },
 
-  maxUnnamed_1_0u: {
+  ok3: {
     code: 'funcName({ sender: _sender, amount: _amount, receiver: _receiver });',
-    maxUnnamed: 1,
+    minUnnamed: 1,
   },
 
-  maxUnnamed_1_1u: {
-    code: 'funcName(sender);',
-    maxUnnamed: 1,
+  ok4: {
+    code: 'assert(1 == 3);',
+    minUnnamed: 1,
   },
-  maxUnnamed_2_2u: {
-    code: 'funcName(sender, amount);',
-    maxUnnamed: 2,
+
+  ok5: {
+    code: 'bytes foo = abi.encodeWithSelector(hex"0102030405060708", uint16(0xff00));',
+    minUnnamed: 2,
   },
 
-  maxUnnamed3_0u: {
-    code: 'funcName({ sender: _sender, amount: _amount, receiver: _receiver });',
-    maxUnnamed: 3,
+  ok6: {
+    code: 'funcName({ sender: _sender, amount: _amount, receiver: _receiver, token1: _token1, token2: _token2 });',
+    minUnnamed: 5,
+  },
+
+  ok7: {
+    code: 'new BeaconProxy(address(0),abi.encodeWithSelector(bytes4(""),address(0),address(0),address(0)));',
+    minUnnamed: 2,
+  },
+
+  ok8: {
+    code: 'salt = keccak256(abi.encode(msg.sender, block.chainid, salt));',
+    minUnnamed: 0,
+  },
+
+  ok9: {
+    code: 'require(foobar != address(0), "foobar must a valid address");',
+    minUnnamed: 0,
   },
 }