Merge pull request #475 from protofire/i349-enforce-custom-errors

New Rule: enforce custom errors over require and revert statements

conf/rulesets/solhint-all.js

@@ -6,6 +6,7 @@
 module.exports = Object.freeze({
   rules: {
     'code-complexity': ['warn', 7],
+    'custom-errors': 'warn',
     'explicit-types': ['warn', 'explicit'],
     'function-max-lines': ['warn', 50],
     'max-line-length': ['error', 120],

conf/rulesets/solhint-recommended.js

@@ -5,6 +5,7 @@
 
 module.exports = Object.freeze({
   rules: {
+    'custom-errors': 'warn',
     'explicit-types': ['warn', 'explicit'],
     'max-states-count': ['warn', 15],
     'no-console': 'error',

docs/rules.md

@@ -9,6 +9,7 @@ title:       "Rule Index of Solhint"
 | Rule Id                                                            | Error                                                                                                                 | Recommended  | Deprecated |
 | ------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- | ------------ | ---------- |
 | [code-complexity](./rules/best-practises/code-complexity.md)       | Function has cyclomatic complexity "current" but allowed no more than maxcompl.                                       |              |            |
+| [custom-errors](./rules/best-practises/custom-errors.md)           | Enforces the use of Custom Errors over Require and Revert statements                                                  | $~~~~~~~~$✔️ |            |
 | [explicit-types](./rules/best-practises/explicit-types.md)         | Forbid or enforce explicit types (like uint256) that have an alias (like uint).                                       | $~~~~~~~~$✔️ |            |
 | [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.                                                                              |              |            |

docs/rules/best-practises/custom-errors.md

@@ -0,0 +1,71 @@
+---
+warning:     "This is a dynamically generated file. Do not edit manually."
+layout:      "default"
+title:       "custom-errors | Solhint"
+---
+
+# custom-errors
+![Recommended Badge](https://img.shields.io/badge/-Recommended-brightgreen)
+![Category Badge](https://img.shields.io/badge/-Best%20Practise%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
+Enforces the use of Custom Errors over Require and Revert statements
+
+## Options
+This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn.
+
+### Example Config
+```json
+{
+  "rules": {
+    "custom-errors": "warn"
+  }
+}
+```
+
+
+## Examples
+### 👍 Examples of **correct** code for this rule
+
+#### Use of Custom Errors
+
+```solidity
+revert CustomErrorFunction();
+```
+
+#### Use of Custom Errors with arguments
+
+```solidity
+revert CustomErrorFunction({ msg: "Insufficient Balance" });
+```
+
+### 👎 Examples of **incorrect** code for this rule
+
+#### Use of require statement
+
+```solidity
+require(userBalance >= availableAmount, "Insufficient Balance");
+```
+
+#### Use of plain revert statement
+
+```solidity
+revert();
+```
+
+#### Use of revert statement with message
+
+```solidity
+revert("Insufficient Balance");
+```
+
+## Version
+This rule is introduced in the latest version.
+
+## Resources
+- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/custom-errors.js)
+- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/best-practises/custom-errors.md)
+- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/best-practises/custom-errors.js)

docs/rules/best-practises/explicit-types.md

@@ -63,7 +63,7 @@ uint256 public variableName
 ```
 
 ## Version
-This rule is introduced in the latest version.
+This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1)
 
 ## Resources
 - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/explicit-types.js)

docs/rules/best-practises/no-unused-import.md

@@ -51,7 +51,7 @@ This rule accepts a string option of rule severity. Must be one of "error", "war
 ```
 
 ## Version
-This rule is introduced in the latest version.
+This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1)
 
 ## Resources
 - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/no-unused-import.js)

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

@@ -60,7 +60,7 @@ functionName(_senderAddress, 1e18, _tokenAddress, _receiverAddress )
 ```
 
 ## Version
-This rule is introduced in the latest version.
+This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1)
 
 ## Resources
 - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/func-named-parameters.js)

docs/rules/naming/immutable-vars-naming.md

@@ -37,7 +37,7 @@ This rule accepts an array of options:
 This rule does not have examples.
 
 ## Version
-This rule is introduced in the latest version.
+This rule was introduced in [Solhint 3.5.1](https://github.com/protofire/solhint/tree/v3.5.1)
 
 ## Resources
 - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/naming/immutable-vars-naming.js)

lib/rules/best-practises/custom-errors.js

@@ -0,0 +1,76 @@
+const BaseChecker = require('../base-checker')
+const { severityDescription } = require('../../doc/utils')
+
+const DEFAULT_SEVERITY = 'warn'
+
+const ruleId = 'custom-errors'
+const meta = {
+  type: 'best-practises',
+
+  docs: {
+    description: 'Enforces the use of Custom Errors over Require and Revert statements',
+    category: 'Best Practise Rules',
+    options: [
+      {
+        description: severityDescription,
+        default: DEFAULT_SEVERITY,
+      },
+    ],
+    examples: {
+      good: [
+        {
+          description: 'Use of Custom Errors',
+          code: 'revert CustomErrorFunction();',
+        },
+        {
+          description: 'Use of Custom Errors with arguments',
+          code: 'revert CustomErrorFunction({ msg: "Insufficient Balance" });',
+        },
+      ],
+      bad: [
+        {
+          description: 'Use of require statement',
+          code: 'require(userBalance >= availableAmount, "Insufficient Balance");',
+        },
+        {
+          description: 'Use of plain revert statement',
+          code: 'revert();',
+        },
+        {
+          description: 'Use of revert statement with message',
+          code: 'revert("Insufficient Balance");',
+        },
+      ],
+    },
+  },
+
+  isDefault: false,
+  recommended: true,
+  defaultSetup: DEFAULT_SEVERITY,
+
+  schema: null,
+}
+
+class CustomErrorsChecker extends BaseChecker {
+  constructor(reporter) {
+    super(reporter, ruleId, meta)
+  }
+
+  FunctionCall(node) {
+    let errorStr = ''
+    if (node.expression.name === 'require') {
+      errorStr = 'require'
+    } else if (
+      node.expression.name === 'revert' &&
+      (node.arguments.length === 0 || node.arguments[0].type === 'StringLiteral')
+    ) {
+      errorStr = 'revert'
+    }
+
+    if (errorStr !== '') {
+      this.error(node, `Use Custom Errors instead of ${errorStr} statements`)
+    }
+  }
+}
+
+module.exports = CustomErrorsChecker

lib/rules/best-practises/index.js

@@ -10,6 +10,7 @@ const NoConsoleLogChecker = require('./no-console')
 const NoGlobalImportsChecker = require('./no-global-import')
 const NoUnusedImportsChecker = require('./no-unused-import')
 const ExplicitTypesChecker = require('./explicit-types')
+const CustomErrorsChecker = require('./custom-errors')
 
 module.exports = function checkers(reporter, config, inputSrc) {
   return [
@@ -25,5 +26,6 @@ module.exports = function checkers(reporter, config, inputSrc) {
     new NoGlobalImportsChecker(reporter),
     new NoUnusedImportsChecker(reporter),
     new ExplicitTypesChecker(reporter, config),
+    new CustomErrorsChecker(reporter),
   ]
 }

test/rules/best-practises/custom-errors.js

@@ -0,0 +1,103 @@
+const assert = require('assert')
+const {
+  assertNoWarnings,
+  assertNoErrors,
+  assertErrorMessage,
+  assertErrorCount,
+  assertWarnsCount,
+} = require('../../common/asserts')
+const linter = require('../../../lib/index')
+const { funcWith } = require('../../common/contract-builder')
+
+describe('Linter - custom-errors', () => {
+  it('should raise error for revert()', () => {
+    const code = funcWith(`revert();`)
+    const report = linter.processStr(code, {
+      rules: { 'custom-errors': 'error' },
+    })
+
+    assertErrorCount(report, 1)
+    assertErrorMessage(report, 'Use Custom Errors instead of revert statement')
+  })
+
+  it('should raise error for revert([string])', () => {
+    const code = funcWith(`revert("Insufficent funds");`)
+    const report = linter.processStr(code, {
+      rules: { 'custom-errors': 'error' },
+    })
+
+    assertErrorCount(report, 1)
+    assertErrorMessage(report, 'Use Custom Errors instead of revert statement')
+  })
+
+  it('should NOT raise error for revert ErrorFunction()', () => {
+    const code = funcWith(`revert ErrorFunction();`)
+    const report = linter.processStr(code, {
+      rules: { 'custom-errors': 'error' },
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
+  it('should NOT raise error for revert ErrorFunction() with arguments', () => {
+    const code = funcWith(`revert ErrorFunction({ msg: "Insufficent funds msg" });`)
+    const report = linter.processStr(code, {
+      rules: { 'custom-errors': 'error' },
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
+  it('should raise error for require', () => {
+    const code = funcWith(`require(!has(role, account), "Roles: account already has role");
+        role.bearer[account] = true;role.bearer[account] = true;
+    `)
+    const report = linter.processStr(code, {
+      rules: { 'custom-errors': 'error' },
+    })
+
+    assertErrorCount(report, 1)
+    assertErrorMessage(report, 'Use Custom Errors instead of require statement')
+  })
+
+  it('should NOT raise error for regular function call', () => {
+    const code = funcWith(`callOtherFunction();
+        role.bearer[account] = true;role.bearer[account] = true;
+    `)
+    const report = linter.processStr(code, {
+      rules: { 'custom-errors': 'error' },
+    })
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
+  it('should raise error for require, revert message and not for revert CustomError() for [recommended] config', () => {
+    const code = funcWith(`require(!has(role, account), "Roles: account already has role");
+        revert("RevertMessage");
+        revert CustomError();
+    `)
+    const report = linter.processStr(code, {
+      extends: 'solhint:recommended',
+      rules: { 'compiler-version': 'off' },
+    })
+
+    assertWarnsCount(report, 2)
+    assert.equal(report.reports[0].message, 'Use Custom Errors instead of require statements')
+    assert.equal(report.reports[1].message, 'Use Custom Errors instead of revert statements')
+  })
+
+  it('should NOT raise error for default config', () => {
+    const code = funcWith(`require(!has(role, account), "Roles: account already has role");
+        revert("RevertMessage");
+        revert CustomError();
+    `)
+    const report = linter.processStr(code, {
+      extends: 'solhint:default',
+    })
+
+    assertWarnsCount(report, 0)
+    assertErrorCount(report, 0)
+  })
+})