Merge pull request #25 from solhint-community/named-parameters-rule

Named parameters rule

docs/rules.md

@@ -6,20 +6,21 @@ title:       "Rule Index of Solhint"
 
 ## Best Practise Rules
 
-| Rule Id                                                            | Error                                                                                                                 | Recommended |
-| ------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- | ----------- |
-| [code-complexity](./rules/best-practises/code-complexity.md)       | Function has cyclomatic complexity "current" but allowed no more than maxcompl.                                       |             |
-| [function-max-lines](./rules/best-practises/function-max-lines.md) | Function body contains "count" lines but allowed no more than maxlines.                                               |             |
-| [max-line-length](./rules/best-practises/max-line-length.md)       | Line length must be no more than maxlen.                                                                              |             |
-| [max-states-count](./rules/best-practises/max-states-count.md)     | Contract has "some count" states declarations but allowed no more than maxstates.                                     | ✔️          |
-| [no-console](./rules/best-practises/no-console.md)                 | No console.log/logInt/logBytesX/logString/etc & No hardhat and forge-std console.sol import statements                | ✔️          |
-| [no-empty-blocks](./rules/best-practises/no-empty-blocks.md)       | Code block has zero statements inside. Some common exceptions apply.                                                  | ✔️          |
-| [no-global-import](./rules/best-practises/no-global-import.md)     | Import statement includes an entire file instead of selected symbols                                                  | ✔️          |
-| [no-unused-import](./rules/best-practises/no-unused-import.md)     | Reports a warning when an imported name is not used                                                                   | ✔️          |
-| [no-unused-vars](./rules/best-practises/no-unused-vars.md)         | Variable "name" is unused.                                                                                            | ✔️          |
-| [payable-fallback](./rules/best-practises/payable-fallback.md)     | When fallback is not payable you will not be able to receive ethers.                                                  | ✔️          |
-| [reason-string](./rules/best-practises/reason-string.md)           | Require or revert statement must have a reason string and check that each reason string is at most N characters long. | ✔️          |
-| [constructor-syntax](./rules/best-practises/constructor-syntax.md) | Constructors should use the new constructor keyword.                                                                  |             |
+| Rule Id                                                                  | Error                                                                                                                 | Recommended |
+| ------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- | ----------- |
+| [code-complexity](./rules/best-practises/code-complexity.md)             | Function has cyclomatic complexity "current" but allowed no more than maxcompl.                                       |             |
+| [function-max-lines](./rules/best-practises/function-max-lines.md)       | Function body contains "count" lines but allowed no more than maxlines.                                               |             |
+| [max-line-length](./rules/best-practises/max-line-length.md)             | Line length must be no more than maxlen.                                                                              |             |
+| [max-states-count](./rules/best-practises/max-states-count.md)           | Contract has "some count" states declarations but allowed no more than maxstates.                                     | ✔️          |
+| [no-console](./rules/best-practises/no-console.md)                       | No console.log/logInt/logBytesX/logString/etc & No hardhat and forge-std console.sol import statements                | ✔️          |
+| [no-empty-blocks](./rules/best-practises/no-empty-blocks.md)             | Code block has zero statements inside. Some common exceptions apply.                                                  | ✔️          |
+| [no-global-import](./rules/best-practises/no-global-import.md)           | Import statement includes an entire file instead of selected symbols                                                  | ✔️          |
+| [no-unused-import](./rules/best-practises/no-unused-import.md)           | Reports a warning when an imported name is not used                                                                   | ✔️          |
+| [no-unused-vars](./rules/best-practises/no-unused-vars.md)               | Variable "name" is unused.                                                                                            | ✔️          |
+| [payable-fallback](./rules/best-practises/payable-fallback.md)           | When fallback is not payable you will not be able to receive ethers.                                                  | ✔️          |
+| [reason-string](./rules/best-practises/reason-string.md)                 | Require or revert statement must have a reason string and check that each reason string is at most N characters long. | ✔️          |
+| [constructor-syntax](./rules/best-practises/constructor-syntax.md)       | Constructors should use the new constructor keyword.                                                                  |             |
+| [named-parameters-function](./rules/naming/named-parameters-function.md) | Enforce using named parameters when invoking a function with more than N arguments                                    |             |
         
 
 ## Miscellaneous

docs/rules/naming/named-parameters-function.md

@@ -0,0 +1,86 @@
+---
+warning:     "This is a dynamically generated file. Do not edit manually."
+layout:      "default"
+title:       "named-parameters-function | Solhint"
+---
+
+# named-parameters-function
+![Category Badge](https://img.shields.io/badge/-Best%20Practise%20Rules-informational)
+![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)
+
+## Description
+Enforce using named parameters when invoking a function with more than N arguments
+
+## Options
+This rule accepts an array of options:
+
+| Index | Description                                                                                                    | Default Value |
+| ----- | -------------------------------------------------------------------------------------------------------------- | ------------- |
+| 0     | Rule severity. Must be one of "error", "warn", "off".                                                          | warn          |
+| 1     | A Number specifying the max amount of arguments a function can have while still allowing positional arguments. | 3             |
+
+
+### Example Config
+```json
+{
+  "rules": {
+    "named-parameters-function": ["warn",3]
+  }
+}
+```
+
+
+## Examples
+### 👍 Examples of **correct** code for this rule
+
+#### Calling a function with few positional arguments
+
+```solidity
+foo(10,200)
+```
+
+#### Calling a function with few named arguments
+
+```solidity
+foo({amount: 10, price: 200})
+```
+
+#### Calling a function with many named arguments
+
+```solidity
+
+          foo({
+            amount: 10,
+            price: 200,
+            recipient: 0x690B9A9E9aa1C9dB991C7721a92d351Db4FaC990,
+            token: 0xdac17f958d2ee523a2206206994597c13d831ec7
+          })
+```
+
+### 👎 Examples of **incorrect** code for this rule
+
+#### Calling a function with many positional arguments
+
+```solidity
+
+          foo(
+             10,
+             200,
+             0x690B9A9E9aa1C9dB991C7721a92d351Db4FaC990,
+             0xdac17f958d2ee523a2206206994597c13d831ec7
+          )
+```
+
+#### With a config value of 0, using positional arguments in _any_ capacity
+
+```solidity
+foo(10)
+```
+
+## Version
+This rule is introduced in the latest version.
+
+## Resources
+- [Rule source](https://github.com/solhint-community/solhint-community/tree/master/lib/rules/naming/named-parameters-function.js)
+- [Document source](https://github.com/solhint-community/solhint-community/tree/master/docs/rules/naming/named-parameters-function.md)
+- [Test cases](https://github.com/solhint-community/solhint-community/tree/master/test/rules/naming/named-parameters-function.js)

lib/config.js

@@ -7,7 +7,7 @@ module.exports = {
 
   getNumberByPath(path, defaultValue) {
     const configVal = _.get(this, path)
-    return _.isNumber(configVal) && configVal > 0 ? configVal : defaultValue
+    return _.isNumber(configVal) ? configVal : defaultValue
   },
 
   getBooleanByPath(path, defaultValue) {

lib/rules/naming/index.js

@@ -8,6 +8,7 @@ const PrivateVarsLeadingUnderscore = require('./private-vars-leading-underscore'
 const UseForbiddenNameChecker = require('./use-forbidden-name')
 const VarNameMixedcaseChecker = require('./var-name-mixedcase')
 const NamedParametersMapping = require('./named-parameters-mapping')
+const NamedParametersFunction = require('./named-parameters-function')
 
 module.exports = function checkers(reporter, config) {
   return [
@@ -21,5 +22,6 @@ module.exports = function checkers(reporter, config) {
     new UseForbiddenNameChecker(reporter),
     new VarNameMixedcaseChecker(reporter),
     new NamedParametersMapping(reporter),
+    new NamedParametersFunction(reporter, config),
   ]
 }

lib/rules/naming/named-parameters-function.js

@@ -0,0 +1,93 @@
+const BaseChecker = require('../base-checker')
+const { severityDescription } = require('../../doc/utils')
+
+const DEFAULT_SEVERITY = 'warn'
+const DEFAULT_MAX_POSITIONAL_ARGUMENTS = 3
+
+const ruleId = 'named-parameters-function'
+const meta = {
+  type: 'naming',
+
+  docs: {
+    description:
+      'Enforce using named parameters when invoking a function with more than N arguments',
+    category: 'Best Practise Rules',
+    options: [
+      {
+        description: severityDescription,
+        default: DEFAULT_SEVERITY,
+      },
+      {
+        description:
+          'A Number specifying the max amount of arguments a function can have while still allowing positional arguments.',
+        default: JSON.stringify(DEFAULT_MAX_POSITIONAL_ARGUMENTS),
+      },
+    ],
+    examples: {
+      good: [
+        {
+          description: 'Calling a function with few positional arguments',
+          code: 'foo(10,200)',
+        },
+        {
+          description: 'Calling a function with few named arguments',
+          code: 'foo({amount: 10, price: 200})',
+        },
+        {
+          description: 'Calling a function with many named arguments',
+          code: `
+          foo({
+            amount: 10,
+            price: 200,
+            recipient: 0x690B9A9E9aa1C9dB991C7721a92d351Db4FaC990,
+            token: 0xdac17f958d2ee523a2206206994597c13d831ec7
+          })`,
+        },
+      ],
+      bad: [
+        {
+          description: 'Calling a function with many positional arguments',
+          code: `
+          foo(
+             10,
+             200,
+             0x690B9A9E9aa1C9dB991C7721a92d351Db4FaC990,
+             0xdac17f958d2ee523a2206206994597c13d831ec7
+          )`,
+        },
+        {
+          description: 'With a config value of 0, using positional arguments in _any_ capacity',
+          code: `foo(10)`,
+        },
+      ],
+    },
+  },
+
+  isDefault: false,
+  recommended: false,
+  defaultSetup: [DEFAULT_SEVERITY, DEFAULT_MAX_POSITIONAL_ARGUMENTS],
+
+  schema: { type: 'integer' },
+}
+
+class FunctionNamedParametersChecker extends BaseChecker {
+  constructor(reporter, config) {
+    super(reporter, ruleId, meta)
+    this.maxPositionalArguments = config
+      ? config.getNumber(ruleId, DEFAULT_MAX_POSITIONAL_ARGUMENTS)
+      : DEFAULT_MAX_POSITIONAL_ARGUMENTS
+  }
+
+  FunctionCall(node) {
+    if (node.names.length === 0) {
+      if (node.arguments.length > this.maxPositionalArguments) {
+        this.error(
+          node,
+          `Call to function with arity > ${this.maxPositionalArguments} is using positional arguments. Use named arguments instead.`
+        )
+      }
+    }
+  }
+}
+
+module.exports = FunctionNamedParametersChecker

test/rules/best-practises/code-complexity.js

@@ -26,6 +26,24 @@ describe('Linter - code-complexity', () => {
     assertNoErrors(report)
   })
 
+  it('GIVEN a config with max complexity of 0, it should use the default value', () => {
+    const goodReport = linter.processStr(
+      funcWith(require('../../fixtures/best-practises/code-complexity-low')),
+      {
+        rules: { 'code-complexity': ['error', 0] },
+      }
+    )
+    assertNoErrors(goodReport)
+
+    const reportWithError = linter.processStr(
+      funcWith(require('../../fixtures/best-practises/code-complexity-high')),
+      {
+        rules: { 'code-complexity': ['error', 0] },
+      }
+    )
+    assertErrorCount(reportWithError, 1)
+  })
+
   it('should raise error when cyclomatic complexity of a modifier is too high', () => {
     const report = linter.processStr(
       modifierWith(require('../../fixtures/best-practises/code-complexity-high')),

test/rules/best-practises/function-max-lines.js

@@ -35,6 +35,18 @@ describe('Linter - function-max-lines', () => {
     assertNoErrors(report)
   })
 
+  it('should use default value of 50 when 0 is specified in the config', () => {
+    const cleanReport = linter.processStr(funcWith(emptyLines(49)), {
+      rules: { 'function-max-lines': ['error', 0] },
+    })
+    assertNoErrors(cleanReport)
+
+    const reportWithError = linter.processStr(funcWith(emptyLines(51)), {
+      rules: { 'function-max-lines': ['error', 0] },
+    })
+    assertErrorCount(reportWithError, 1)
+  })
+
   function repeatLines(line, count) {
     return _.times(count)
       .map(() => line)

test/rules/best-practises/max-line-length.js

@@ -1,5 +1,10 @@
 const assert = require('assert')
-const { assertErrorMessage, assertLineNumber, assertNoErrors } = require('../../common/asserts')
+const {
+  assertErrorCount,
+  assertErrorMessage,
+  assertLineNumber,
+  assertNoErrors,
+} = require('../../common/asserts')
 const { contractWith } = require('../../common/contract-builder')
 const linter = require('../../../lib/index')
 
@@ -15,6 +20,18 @@ describe('Linter - max-line-length', () => {
     assertLineNumber(report.reports[0], 6)
   })
 
+  it('should use default value when a value of 0 is specified in the config', () => {
+    const reportWithErrors = linter.processStr(contractWith(' '.repeat(121)), {
+      rules: { 'max-line-length': ['error', 0] },
+    })
+    assertErrorCount(reportWithErrors, 1)
+
+    const cleanReport = linter.processStr(contractWith(' '.repeat(110)), {
+      rules: { 'max-line-length': ['error', 0] },
+    })
+    assertNoErrors(cleanReport)
+  })
+
   it('should raise error with an empty file', () => {
     const code = ' '.repeat(121)
 

test/rules/best-practises/max-states-count.js

@@ -46,6 +46,18 @@ describe('Linter - max-states-count', () => {
     assertNoErrors(report)
   })
 
+  it('should use default value when 0 is passed as an option', () => {
+    const cleanReport = linter.processStr(contractWith(stateDef(10)), {
+      rules: { 'max-states-count': ['error', 0] },
+    })
+    assertNoErrors(cleanReport)
+
+    const reportWithErrors = linter.processStr(contractWith(stateDef(16)), {
+      rules: { 'max-states-count': ['error', 0] },
+    })
+    assertErrorCount(reportWithErrors, 1)
+  })
+
   it('should not count immutable variables', () => {
     const code = contractWith(`
       uint public immutable a;