Merge pull request #478 from protofire/i340-name-return-values

New Rule: named-return-values

conf/rulesets/solhint-all.js

@@ -41,6 +41,7 @@ module.exports = Object.freeze({
     ],
     'modifier-name-mixedcase': 'warn',
     'named-parameters-mapping': 'off',
+    'named-return-values': 'warn',
     'private-vars-leading-underscore': [
       'warn',
       {

docs/rules.md

@@ -46,6 +46,7 @@ title:       "Rule Index of Solhint"
 | [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.                                      |              |             |
+| [named-return-values](./rules/naming/named-return-values.md)                         | Enforce the return values of a function to be named                                                           |              |             |
 | [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)                    | $~~~~~~~~$✔️ |             |

docs/rules/naming/named-return-values.md

@@ -0,0 +1,50 @@
+---
+warning:     "This is a dynamically generated file. Do not edit manually."
+layout:      "default"
+title:       "named-return-values | Solhint"
+---
+
+# named-return-values
+![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational)
+![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)
+
+## Description
+Enforce the return values of a function to be named
+
+## Options
+This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn.
+
+### Example Config
+```json
+{
+  "rules": {
+    "named-return-values": "warn"
+  }
+}
+```
+
+
+## Examples
+### 👍 Examples of **correct** code for this rule
+
+#### Function definition with named return values
+
+```solidity
+function checkBalance(address wallet) external view returns(uint256 retBalance) {}
+```
+
+### 👎 Examples of **incorrect** code for this rule
+
+#### Function definition with UNNAMED return values
+
+```solidity
+function checkBalance(address wallet) external view returns(uint256) {}
+```
+
+## Version
+This rule is introduced in the latest version.
+
+## Resources
+- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/named-return-values.js)
+- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/naming/named-return-values.md)
+- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/naming/named-return-values.js)

lib/rules/naming/index.js

@@ -11,6 +11,7 @@ const NamedParametersMappingChecker = require('./named-parameters-mapping')
 const ImmutableVarsNamingChecker = require('./immutable-vars-naming')
 const FunctionNamedParametersChecker = require('./func-named-parameters')
 const FoundryTestFunctionsChecker = require('./foundry-test-functions')
+const NamedReturnValuesChecker = require('./named-return-values')
 
 module.exports = function checkers(reporter, config) {
   return [
@@ -27,5 +28,6 @@ module.exports = function checkers(reporter, config) {
     new ImmutableVarsNamingChecker(reporter, config),
     new FunctionNamedParametersChecker(reporter, config),
     new FoundryTestFunctionsChecker(reporter, config),
+    new NamedReturnValuesChecker(reporter),
   ]
 }

lib/rules/naming/named-return-values.js

@@ -0,0 +1,60 @@
+const BaseChecker = require('../base-checker')
+const { severityDescription } = require('../../doc/utils')
+
+const DEFAULT_SEVERITY = 'warn'
+
+const ruleId = 'named-return-values'
+const meta = {
+  type: 'naming',
+
+  docs: {
+    description: `Enforce the return values of a function to be named`,
+    category: 'Style Guide Rules',
+    options: [
+      {
+        description: severityDescription,
+        default: DEFAULT_SEVERITY,
+      },
+    ],
+    examples: {
+      good: [
+        {
+          description: 'Function definition with named return values',
+          code: 'function checkBalance(address wallet) external view returns(uint256 retBalance) {}',
+        },
+      ],
+      bad: [
+        {
+          description: 'Function definition with UNNAMED return values',
+          code: 'function checkBalance(address wallet) external view returns(uint256) {}',
+        },
+      ],
+    },
+  },
+
+  isDefault: false,
+  recommended: false,
+  defaultSetup: DEFAULT_SEVERITY,
+
+  schema: null,
+}
+
+class NamedReturnValuesChecker extends BaseChecker {
+  constructor(reporter) {
+    super(reporter, ruleId, meta)
+  }
+
+  FunctionDefinition(node) {
+    if (node.returnParameters) {
+      let index = 0
+      for (const returnValue of node.returnParameters) {
+        if (!returnValue.name) {
+          this.error(node, `Named return value is missing - Index ${index}`)
+        }
+        index++
+      }
+    }
+  }
+}
+
+module.exports = NamedReturnValuesChecker

lib/rules/security/check-send-result.js

@@ -14,13 +14,13 @@ const meta = {
       good: [
         {
           description: 'result of "send" call checked with if statement',
-          code: require('../../../test/fixtures/security/send-result-checked'),
+          code: 'if(x.send(55)) {}',
         },
       ],
       bad: [
         {
           description: 'result of "send" call ignored',
-          code: require('../../../test/fixtures/security/send-result-ignored'),
+          code: 'x.send(55);',
         },
       ],
     },
@@ -44,17 +44,24 @@ class CheckSendResultChecker extends BaseChecker {
 
   validateSend(node) {
     if (node.memberName === 'send') {
-      const hasVarDeclaration = traversing.statementNotContains(node, 'VariableDeclaration')
-      const hasIfStatement = traversing.statementNotContains(node, 'IfStatement')
-      const hasRequire = traversing.someParent(node, this.isRequire)
-      const hasAssert = traversing.someParent(node, this.isAssert)
+      if (this.isNotErc777Token(node)) {
+        const hasVarDeclaration = traversing.statementNotContains(node, 'VariableDeclaration')
+        const hasIfStatement = traversing.statementNotContains(node, 'IfStatement')
+        const hasRequire = traversing.someParent(node, this.isRequire)
+        const hasAssert = traversing.someParent(node, this.isAssert)
 
-      if (!hasIfStatement && !hasVarDeclaration && !hasRequire && !hasAssert) {
-        this.error(node, 'Check result of "send" call')
+        if (!hasIfStatement && !hasVarDeclaration && !hasRequire && !hasAssert) {
+          this.error(node, 'Check result of "send" call')
+        }
       }
     }
   }
 
+  isNotErc777Token(node) {
+    const isErc777 = node.parent.type === 'FunctionCall' && node.parent.arguments.length >= 3
+    return !isErc777
+  }
+
   isRequire(node) {
     return node.type === 'FunctionCall' && node.expression.name === 'require'
   }

test/rules/naming/named-return-values.js

@@ -0,0 +1,92 @@
+const assert = require('assert')
+const linter = require('../../../lib/index')
+const contractWith = require('../../common/contract-builder').contractWith
+const { assertErrorCount, assertNoErrors, assertWarnsCount } = require('../../common/asserts')
+
+describe('Linter - named-return-values', () => {
+  it('should NOT raise error for named return values', () => {
+    const code = contractWith(
+      `function getBalanceFromTokens(address wallet) public returns(address token1, address token2, uint256 balance1, uint256 balance2) { balance = 1; }`
+    )
+    const report = linter.processStr(code, {
+      rules: { 'named-return-values': 'error' },
+    })
+    assertNoErrors(report)
+  })
+
+  it('should raise error for unnamed return values', () => {
+    const code = contractWith(
+      `function getBalanceFromTokens(address wallet) public returns(address, address, uint256, uint256) { balance = 1; }`
+    )
+    const report = linter.processStr(code, {
+      rules: { 'named-return-values': 'error' },
+    })
+
+    assertErrorCount(report, 4)
+    for (let index = 0; index < report.reports.length; index++) {
+      assert.equal(report.reports[index].message, `Named return value is missing - Index ${index}`)
+    }
+  })
+
+  it('should NOT raise error for functions without return values', () => {
+    const code = contractWith(`function writeOnStorage(address wallet) public { balance = 1; }`)
+    const report = linter.processStr(code, {
+      rules: { 'named-return-values': 'error' },
+    })
+    assertNoErrors(report)
+  })
+
+  it('should raise error for 2 unnamed return values', () => {
+    const code = contractWith(
+      `function getBalanceFromTokens(address wallet) public returns(address user, address, uint256 amount, uint256) { balance = 1; }`
+    )
+    const report = linter.processStr(code, {
+      rules: { 'named-return-values': 'error' },
+    })
+
+    assertErrorCount(report, 2)
+    assert.equal(report.reports[0].message, `Named return value is missing - Index 1`)
+    assert.equal(report.reports[1].message, `Named return value is missing - Index 3`)
+  })
+
+  it('should NOT raise error for solhint:recommended setup', () => {
+    const code = contractWith(
+      `function getBalanceFromTokens(address wallet) public returns(address, address, uint256, uint256) { balance = 1; }`
+    )
+
+    const report = linter.processStr(code, {
+      extends: 'solhint:recommended',
+      rules: { 'compiler-version': 'off' },
+    })
+
+    assertNoErrors(report)
+  })
+
+  it('should NOT raise error for solhint:default setup', () => {
+    const code = contractWith(
+      `function getBalance(address wallet) public returns(uint256) { balance = 1; }`
+    )
+
+    const report = linter.processStr(code, {
+      extends: 'solhint:default',
+    })
+
+    assertNoErrors(report)
+  })
+
+  it('should raise error for solhint:all setup', () => {
+    const code = contractWith(
+      `function getBalance(uint256 wallet) public override returns(uint256, address) { wallet = 1; }`
+    )
+
+    const report = linter.processStr(code, {
+      extends: 'solhint:all',
+      rules: { 'compiler-version': 'off' },
+    })
+
+    assertWarnsCount(report, 2)
+    for (let index = 0; index < report.reports.length; index++) {
+      assert.equal(report.reports[index].message, `Named return value is missing - Index ${index}`)
+    }
+  })
+})

test/rules/security/check-send-result.js

@@ -4,7 +4,7 @@ const funcWith = require('../../common/contract-builder').funcWith
 
 describe('Linter - check-send-result', () => {
   it('should return "send" call verification error', () => {
-    const code = funcWith(require('../../fixtures/security/send-result-ignored'))
+    const code = funcWith('x.send(55);')
 
     const report = linter.processStr(code, {
       rules: { 'check-send-result': 'error' },
@@ -15,7 +15,7 @@ describe('Linter - check-send-result', () => {
   })
 
   it('should not return "send" call verification error', () => {
-    const code = funcWith(require('../../fixtures/security/send-result-checked'))
+    const code = funcWith('if(x.send(55)) {}')
 
     const report = linter.processStr(code, {
       rules: { 'check-send-result': 'error' },
@@ -73,4 +73,14 @@ describe('Linter - check-send-result', () => {
 
     assert.equal(report.errorCount, 1)
   })
+
+  it('should not emit error when the send() is used for an ERC777', () => {
+    const code = funcWith('erc777.send(recipient, amount, "");')
+
+    const report = linter.processStr(code, {
+      rules: { 'check-send-result': 'error' },
+    })
+
+    assert.equal(report.errorCount, 0)
+  })
 })