Merge 139f207 into 8f41eea

docs/rules.md

@@ -85,3 +85,4 @@ http://solidity.readthedocs.io/en/develop/style-guide.html)
  | **function-max-lines**        | Function body contains "count" lines but allowed no more than *maxlines*. | [*\<[default](#options)\>*,&nbsp;*\<maxlines\>*] Default *maxlines* is **45**. |
  | **code-complexity**           | Function has cyclomatic complexity "current" but allowed no more than *maxcompl*. | [*\<[default](#options)\>*,&nbsp;*\<maxcompl\>*] Default *maxcompl* is **7**. |
  | **max-states-count**          | Contract has "some count" states declarations but allowed no more than *maxstates* | [*\<[default](#options)\>*,&nbsp;*\<maxstates\>*] Default *maxstates* is **15**. |
+ | **reason-string**             | Ensure that revert and require statements contains error message but allowed no more than *maxLength* | [*\<[default](#options)\>*,&nbsp;*{ maxLength: 50}*] Default *maxLength* is **32**. |

lib/config.js

@@ -19,6 +19,10 @@ module.exports = {
     return this.getNumberByPath(`rules["${ruleName}"][1]`, defaultValue)
   },
 
+  getObjectPropertyNumber(ruleName, ruleProperty, defaultValue) {
+    return this.getNumberByPath(`rules["${ruleName}"][1][${ruleProperty}]`, defaultValue)
+  },
+
   getString(ruleName, defaultValue) {
     return this.getStringByPath(`rules["${ruleName}"][1]`, defaultValue)
   }

lib/rules/best-practises/index.js

@@ -5,6 +5,7 @@ const MaxStatesCountChecker = require('./max-states-count')
 const NoEmptyBlocksChecker = require('./no-empty-blocks')
 const NoUnusedVarsChecker = require('./no-unused-vars')
 const PayableFallbackChecker = require('./payable-fallback')
+const ReasonStringChecker = require('./reason-string')
 
 module.exports = function checkers(reporter, config) {
   return [
@@ -14,6 +15,7 @@ module.exports = function checkers(reporter, config) {
     new MaxStatesCountChecker(reporter, config),
     new NoEmptyBlocksChecker(reporter),
     new NoUnusedVarsChecker(reporter),
-    new PayableFallbackChecker(reporter)
+    new PayableFallbackChecker(reporter),
+    new ReasonStringChecker(reporter, config)
   ]
 }

lib/rules/best-practises/reason-string.js

@@ -0,0 +1,107 @@
+const _ = require('lodash')
+const BaseChecker = require('./../base-checker')
+
+const DEFAULT_MAX_CHARACTERS_LONG = 32
+
+const ruleId = 'reason-string'
+const meta = {
+  type: 'best-practises',
+
+  docs: {
+    description:
+      'Require or revert statement must have a reason string and check that each reason string is at most N characters long.',
+    category: 'Best Practise Rules'
+  },
+
+  isDefault: false,
+  recommended: false,
+  defaultSetup: ['warn', { maxLength: DEFAULT_MAX_CHARACTERS_LONG }],
+
+  schema: [
+    {
+      type: 'array',
+      items: [
+        {
+          properties: {
+            maxLength: {
+              type: 'integer'
+            }
+          },
+          additionalProperties: false
+        }
+      ],
+      uniqueItems: true,
+      minItems: 2
+    }
+  ]
+}
+
+class ReasonStringChecker extends BaseChecker {
+  constructor(reporter, config) {
+    super(reporter, ruleId, meta)
+
+    this.maxCharactersLong =
+      (config &&
+        config.getObjectPropertyNumber(ruleId, 'maxLength', DEFAULT_MAX_CHARACTERS_LONG)) ||
+      DEFAULT_MAX_CHARACTERS_LONG
+  }
+
+  enterFunctionCallArguments(ctx) {
+    if (this.isReasonStringStatement(ctx)) {
+      const functionParameters = this.getFunctionParameters(ctx)
+      const functionName = this.getFunctionName(ctx)
+
+      // Throw an error if have no message
+      if (functionParameters.length <= 1) {
+        this._errorHaveNoMessage(ctx, functionName)
+        return
+      }
+
+      const lastFunctionParameter = _.last(functionParameters)
+      const hasReasonMessage = this.hasReasonMessage(lastFunctionParameter)
+
+      // If has reason message and is too long, throw an error
+      if (hasReasonMessage) {
+        const lastParameterWithoutQuotes = lastFunctionParameter.replace(/['"]+/g, '')
+        if (lastParameterWithoutQuotes.length > this.maxCharactersLong) {
+          this._errorMessageIsTooLong(ctx, functionName)
+        }
+      }
+    }
+  }
+
+  isReasonStringStatement(ctx) {
+    const name = this.getFunctionName(ctx)
+    return name === 'revert' || name === 'require'
+  }
+
+  getFunctionName(ctx) {
+    const parent = ctx.parentCtx
+    const name = parent.children[0]
+    return name.getText()
+  }
+
+  getFunctionParameters(ctx) {
+    const parent = ctx.parentCtx
+    const nodes = parent.children[2]
+    const children = nodes.children[0].children
+    const parameters = children
+      .filter(value => value.getText() !== ',')
+      .map(value => value.getText())
+    return parameters
+  }
+
+  hasReasonMessage(str) {
+    return str.indexOf("'") >= 0 || str.indexOf('"') >= 0
+  }
+
+  _errorHaveNoMessage(ctx, key) {
+    this.error(ctx, `Provide an error message for ${key}`)
+  }
+
+  _errorMessageIsTooLong(ctx, key) {
+    this.error(ctx, `Error message for ${key} is too long`)
+  }
+}
+
+module.exports = ReasonStringChecker

test/rules/best-practises/reason-string.js

@@ -0,0 +1,94 @@
+const assert = require('assert')
+const {
+  assertNoWarnings,
+  assertNoErrors,
+  assertErrorMessage,
+  assertWarnsCount,
+  assertErrorCount
+} = require('./../../common/asserts')
+const linter = require('./../../../lib/index')
+const { funcWith } = require('./../../common/contract-builder')
+
+describe('Linter - reason-string', () => {
+  it('should raise reason string is mandatory for require', () => {
+    const code = funcWith(`require(!has(role, account));
+        role.bearer[account] = true;role.bearer[account] = true;
+    `)
+
+    const report = linter.processStr(code, {
+      rules: { 'reason-string': ['warn', { maxLength: 5 }] }
+    })
+
+    assert.ok(report.warningCount > 0)
+    assertWarnsCount(report, 1)
+    assertErrorMessage(report, 'Provide an error message for require')
+  })
+
+  it('should raise reason string is mandatory for revert', () => {
+    const code = funcWith(`revert(!has(role, account));
+        role.bearer[account] = true;role.bearer[account] = true;
+    `)
+
+    const report = linter.processStr(code, {
+      rules: { 'reason-string': ['error', { maxLength: 5 }] }
+    })
+
+    assert.ok(report.errorCount > 0)
+    assertErrorCount(report, 1)
+    assertErrorMessage(report, 'Provide an error message for revert')
+  })
+
+  it('should raise reason string maxLength 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: { 'reason-string': ['warn', { maxLength: 5 }] }
+    })
+
+    assert.ok(report.warningCount > 0)
+    assertWarnsCount(report, 1)
+    assertErrorMessage(report, 'Error message for require is too long')
+  })
+
+  it('should raise reason string maxLength error for revert', () => {
+    const code = funcWith(`revert(!has(role, account), "Roles: account already has role");
+        role.bearer[account] = true;role.bearer[account] = true;
+    `)
+
+    const report = linter.processStr(code, {
+      rules: { 'reason-string': ['error', { maxLength: 5 }] }
+    })
+
+    assert.ok(report.errorCount > 0)
+    assertErrorCount(report, 1)
+    assertErrorMessage(report, 'Error message for revert is too long')
+  })
+
+  it('should not raise warning 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: { 'reason-string': ['warn', { maxLength: 31 }] }
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+
+  it('should not raise error for revert', () => {
+    const code = funcWith(`revert(!has(role, account), "Roles: account already has role");
+        role.bearer[account] = true;role.bearer[account] = true;
+    `)
+
+    const report = linter.processStr(code, {
+      rules: { 'reason-string': ['error', { maxLength: 50 }] }
+    })
+
+    assertNoWarnings(report)
+    assertNoErrors(report)
+  })
+})