Deprecate func-order and add ordering rule

lib/common/ast-types.js

@@ -6,7 +6,17 @@ function isFunctionDefinition(node) {
   return node.type === 'FunctionDefinition'
 }
 
+function isStructDefinition(node) {
+  return node.type === 'StructDefinition'
+}
+
+function isEnumDefinition(node) {
+  return node.type === 'EnumDefinition'
+}
+
 module.exports = {
   isFallbackFunction,
-  isFunctionDefinition
+  isFunctionDefinition,
+  isStructDefinition,
+  isEnumDefinition
 }

lib/rules/index.js

@@ -9,8 +9,11 @@ const miscellaneous = require('./miscellaneous/index')
 const configObject = require('./../config')
 const { validSeverityMap } = require('../config/config-validator')
 
-const notifyRuleDeprecated = _.memoize(ruleId => {
-  console.warn(chalk.yellow(`[solhint] Warning: rule '${ruleId}' is deprecated.`))
+const notifyRuleDeprecated = _.memoize((ruleId, deprecationMessage) => {
+  const message = deprecationMessage
+    ? `[solhint] Warning: rule '${ruleId}' is deprecated, ${deprecationMessage}.`
+    : `[solhint] Warning: rule '${ruleId}' is deprecated.`
+  console.warn(chalk.yellow(message))
 })
 
 const notifyRuleDoesntExist = _.memoize(ruleId => {
@@ -36,7 +39,7 @@ module.exports = function checkers(reporter, configVals, inputSrc, tokens, fileN
   // show warnings for deprecated rules
   for (const rule of enabledRules) {
     if (rule.meta && rule.meta.deprecated) {
-      notifyRuleDeprecated(rule.ruleId)
+      notifyRuleDeprecated(rule.ruleId, rule.meta.deprecationMessage)
     }
   }
 

lib/rules/order/func-order.js

@@ -1,9 +1,6 @@
 const BaseChecker = require('./../base-checker')
-const TreeTraversing = require('./../../common/tree-traversing')
 const { isFallbackFunction } = require('../../common/ast-types')
 
-const traversing = new TreeTraversing()
-
 const ruleId = 'func-order'
 const meta = {
   type: 'order',
@@ -28,6 +25,8 @@ const meta = {
   },
 
   isDefault: false,
+  deprecated: true,
+  deprecationMessage: "use 'ordering' instead",
   recommended: false,
   defaultSetup: 'warn',
 
@@ -40,189 +39,94 @@ class FuncOrderChecker extends BaseChecker {
   }
 
   ContractDefinition(node) {
-    this._assignOrderAnalysisTo(node)
-  }
+    const children = node.subNodes
 
-  StructDefinition(node) {
-    this._assignOrderAnalysisTo(node)
-  }
+    if (children.length === 0) {
+      return
+    }
 
-  _assignOrderAnalysisTo(node) {
-    const nodeName = node.name
-    node.funcOrderAnalysis = new FunctionOrderAnalysis(nodeName, this.reporter, this.ruleId)
-  }
+    let maxChild = children[0]
+    let [maxComparisonValue, maxLabel] = getComparisonValueAndLabel(children[0])
 
-  'ConstructorDefinition:exit'(node) {
-    const contract = traversing.findParentType(node, 'ContractDefinition')
-    contract.funcOrderAnalysis.process(node)
-  }
+    for (let i = 1; i < children.length; i++) {
+      const [comparisonValue, label] = getComparisonValueAndLabel(children[i])
+      if (comparisonValue < maxComparisonValue) {
+        this.report(children[i], maxChild, label, maxLabel)
+        return
+      }
 
-  FunctionDefinition(node) {
-    const contract = traversing.findParentType(node, 'ContractDefinition')
-    contract.funcOrderAnalysis.process(node)
+      maxChild = children[i]
+      maxComparisonValue = comparisonValue
+      maxLabel = label
+    }
   }
-}
 
-class State {
-  constructor(config) {
-    this.name = config.name
-    this.after = config.after
-    this.rules = config.rules
+  report(node, nodeBefore, label, labelBefore) {
+    const message = `Function order is incorrect, ${label} can not go after ${labelBefore} (line ${
+      nodeBefore.loc.start.line
+    })`
+    this.reporter.error(node, this.ruleId, message)
   }
+}
 
-  nextState(name) {
-    const items = this.rules.filter(i => i.on === name).map(i => i.goTo)
-
-    return items.length > 0 ? items[0] : null
-  }
+function isConst(node) {
+  return ['pure', 'view', 'constant'].includes(node.stateMutability)
 }
 
-const STATES = {
-  START: new State({
-    name: 'START',
-    after: '',
-    rules: [
-      { on: 'constructor', goTo: 'AFTER_CONSTR' },
-      { on: 'fallback', goTo: 'AFTER_FALLBACK' },
-      { on: 'external', goTo: 'AFTER_EXTERNAL' },
-      { on: 'external_const', goTo: 'AFTER_EXTERNAL_CONSTANT' },
-      { on: 'public', goTo: 'AFTER_PUBLIC' },
-      { on: 'public_const', goTo: 'AFTER_PUBLIC_CONSTANT' },
-      { on: 'internal', goTo: 'AFTER_INTERNAL' },
-      { on: 'private', goTo: 'AFTER_PRIVATE' }
-    ]
-  }),
-
-  AFTER_CONSTR: new State({
-    name: 'AFTER_CONSTR',
-    after: 'constructor',
-    rules: [
-      { on: 'fallback', goTo: 'AFTER_FALLBACK' },
-      { on: 'external', goTo: 'AFTER_EXTERNAL' },
-      { on: 'external_const', goTo: 'AFTER_EXTERNAL_CONSTANT' },
-      { on: 'public', goTo: 'AFTER_PUBLIC' },
-      { on: 'public_const', goTo: 'AFTER_PUBLIC_CONSTANT' },
-      { on: 'internal', goTo: 'AFTER_INTERNAL' },
-      { on: 'private', goTo: 'AFTER_PRIVATE' }
-    ]
-  }),
-
-  AFTER_FALLBACK: new State({
-    name: 'AFTER_CONSTR',
-    after: 'fallback',
-    rules: [
-      { on: 'external', goTo: 'AFTER_EXTERNAL' },
-      { on: 'external_const', goTo: 'AFTER_EXTERNAL_CONSTANT' },
-      { on: 'public', goTo: 'AFTER_PUBLIC' },
-      { on: 'public_const', goTo: 'AFTER_PUBLIC_CONSTANT' },
-      { on: 'internal', goTo: 'AFTER_INTERNAL' },
-      { on: 'private', goTo: 'AFTER_PRIVATE' }
-    ]
-  }),
-
-  AFTER_EXTERNAL: new State({
-    name: 'AFTER_EXTERNAL',
-    after: 'external',
-    rules: [
-      { on: 'external', goTo: 'AFTER_EXTERNAL' },
-      { on: 'external_const', goTo: 'AFTER_EXTERNAL_CONSTANT' },
-      { on: 'public', goTo: 'AFTER_PUBLIC' },
-      { on: 'public_const', goTo: 'AFTER_PUBLIC_CONSTANT' },
-      { on: 'internal', goTo: 'AFTER_INTERNAL' },
-      { on: 'private', goTo: 'AFTER_PRIVATE' }
-    ]
-  }),
-
-  AFTER_EXTERNAL_CONSTANT: new State({
-    name: 'AFTER_EXTERNAL_CONSTANT',
-    after: 'external constant',
-    rules: [
-      { on: 'external_const', goTo: 'AFTER_EXTERNAL_CONSTANT' },
-      { on: 'public', goTo: 'AFTER_PUBLIC' },
-      { on: 'public_const', goTo: 'AFTER_PUBLIC_CONSTANT' },
-      { on: 'internal', goTo: 'AFTER_INTERNAL' },
-      { on: 'private', goTo: 'AFTER_PRIVATE' }
-    ]
-  }),
-
-  AFTER_PUBLIC: new State({
-    name: 'AFTER_PUBLIC',
-    after: 'public',
-    rules: [
-      { on: 'public', goTo: 'AFTER_PUBLIC' },
-      { on: 'public_const', goTo: 'AFTER_PUBLIC_CONSTANT' },
-      { on: 'internal', goTo: 'AFTER_INTERNAL' },
-      { on: 'private', goTo: 'AFTER_PRIVATE' }
-    ]
-  }),
-
-  AFTER_PUBLIC_CONSTANT: new State({
-    name: 'AFTER_PUBLIC_CONSTANT',
-    after: 'public constant',
-    rules: [
-      { on: 'public_const', goTo: 'AFTER_PUBLIC_CONSTANT' },
-      { on: 'internal', goTo: 'AFTER_INTERNAL' },
-      { on: 'private', goTo: 'AFTER_PRIVATE' }
-    ]
-  }),
-
-  AFTER_INTERNAL: new State({
-    name: 'AFTER_INTERNAL',
-    after: 'internal',
-    rules: [{ on: 'internal', goTo: 'AFTER_INTERNAL' }, { on: 'private', goTo: 'AFTER_PRIVATE' }]
-  }),
-
-  AFTER_PRIVATE: new State({
-    name: 'AFTER_PRIVATE',
-    after: 'private',
-    rules: [{ on: 'private', goTo: 'AFTER_PRIVATE' }]
-  })
+function isTypeDeclaration(node) {
+  return ['StructDefinition', 'EnumDefinition'].includes(node.type)
 }
 
-class FunctionOrderAnalysis {
-  constructor(contractName, reporter, ruleId) {
-    this.curState = STATES.START
-    this.reporter = reporter
-    this.ruleId = ruleId
-    this.funcTypeParser = new FuncTypeParser(contractName)
+function getComparisonValueAndLabel(node) {
+  if (isTypeDeclaration(node)) {
+    let label
+    if (node.type === 'StructDefinition') {
+      label = 'struct definition'
+    } else {
+      label = 'enum definition'
+    }
+    return [0, label]
   }
 
-  process(node) {
-    const name = this.funcTypeParser.funcType(node)
-    const newState = this.curState.nextState(name)
+  if (node.type === 'StateVariableDeclaration') {
+    return [10, 'state variable declaration']
+  }
 
-    if (newState) {
-      this.curState = STATES[newState]
-    } else {
-      const afterState = this.curState.after
-      const message = `Function order is incorrect, ${name} function can not go after ${afterState} function.`
-      this.reporter.error(node, this.ruleId, message)
-    }
+  if (node.type === 'EventDefinition') {
+    return [20, 'event definition']
   }
-}
 
-class FuncTypeParser {
-  funcType(node) {
-    if (node.isConstructor) {
-      return 'constructor'
-    } else if (isFallbackFunction(node)) {
-      return 'fallback'
-    } else {
-      return this.funcModifierType(node)
-    }
+  if (node.isConstructor) {
+    return [30, 'constructor']
   }
 
-  funcModifierType(node) {
-    const modifiers = ['pure', 'view', 'constant']
-    const publicVisibility = ['external', 'public']
-    const { visibility, stateMutability } = node
+  if (isFallbackFunction(node)) {
+    return [40, 'fallback function']
+  }
 
-    if (publicVisibility.includes(visibility) && modifiers.includes(stateMutability)) {
-      return `${visibility}_const`
+  if (node.type === 'FunctionDefinition') {
+    if (node.visibility === 'external' && !isConst(node)) {
+      return [50, 'external function']
     }
-
-    return visibility
+    if (node.visibility === 'external' && isConst(node)) {
+      return [60, 'external const function']
+    }
+    if (node.visibility === 'public' && !isConst(node)) {
+      return [70, 'public function']
+    }
+    if (node.visibility === 'public' && isConst(node)) {
+      return [80, 'public const function']
+    }
+    if (node.visibility === 'internal') {
+      return [90, 'internal function']
+    }
+    if (node.visibility === 'private') {
+      return [100, 'private function']
+    }
+    throw new Error('Unknown order for function, please report this issue')
   }
+
+  throw new Error('Unrecognized contract part, please report this issue')
 }
 
 module.exports = FuncOrderChecker

lib/rules/order/index.js

@@ -1,11 +1,13 @@
 const FuncOrderChecker = require('./func-order')
 const ImportsOnTopChecker = require('./imports-on-top')
 const VisibilityModifierOrderChecker = require('./visibility-modifier-order')
+const OrderingChecker = require('./ordering')
 
 module.exports = function order(reporter, tokens) {
   return [
     new FuncOrderChecker(reporter),
     new ImportsOnTopChecker(reporter),
-    new VisibilityModifierOrderChecker(reporter, tokens)
+    new VisibilityModifierOrderChecker(reporter, tokens),
+    new OrderingChecker(reporter)
   ]
 }