diff --git a/lib/common/ast-types.js b/lib/common/ast-types.js index 642769c2..411f56f5 100644 --- a/lib/common/ast-types.js +++ b/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 } diff --git a/lib/rules/index.js b/lib/rules/index.js index 0999f38d..c4107fb6 100644 --- a/lib/rules/index.js +++ b/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) } } diff --git a/lib/rules/order/func-order.js b/lib/rules/order/func-order.js index 380afdcb..b48267a3 100644 --- a/lib/rules/order/func-order.js +++ b/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 diff --git a/lib/rules/order/index.js b/lib/rules/order/index.js index 0ae90fb2..382caf99 100644 --- a/lib/rules/order/index.js +++ b/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) ] } diff --git a/lib/rules/order/ordering.js b/lib/rules/order/ordering.js new file mode 100644 index 00000000..975fdeb9 --- /dev/null +++ b/lib/rules/order/ordering.js @@ -0,0 +1,160 @@ +const BaseChecker = require('./../base-checker') +const { isFallbackFunction } = require('../../common/ast-types') + +const ruleId = 'ordering' +const meta = { + type: 'order', + + docs: { + description: `Check order of elements in file and inside each contract, according to the style guide`, + category: 'Style Guide Rules', + examples: { + good: require('../../../test/fixtures/order/ordering-correct'), + bad: require('../../../test/fixtures/order/ordering-incorrect') + } + }, + + isDefault: false, + recommended: false, + defaultSetup: 'warn', + + schema: null +} + +class OrderingChecker extends BaseChecker { + constructor(reporter) { + super(reporter, ruleId, meta) + } + + SourceUnit(node) { + const children = node.children + + this.checkOrder(children, sourceUnitPartOrder) + } + + ContractDefinition(node) { + const children = node.subNodes + + this.checkOrder(children, contractPartOrder) + } + + checkOrder(children, orderFunction) { + if (children.length === 0) { + return + } + + let maxChild = children[0] + let [maxComparisonValue, maxLabel] = orderFunction(children[0]) + + for (let i = 1; i < children.length; i++) { + const [comparisonValue, label] = orderFunction(children[i]) + if (comparisonValue < maxComparisonValue) { + this.report(children[i], maxChild, label, maxLabel) + return + } + + maxChild = children[i] + maxComparisonValue = comparisonValue + maxLabel = label + } + } + + 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) + } +} + +function isConst(node) { + return ['pure', 'view', 'constant'].includes(node.stateMutability) +} + +function isTypeDeclaration(node) { + return ['StructDefinition', 'EnumDefinition'].includes(node.type) +} + +function sourceUnitPartOrder(node) { + if (node.type === 'PragmaDirective') { + return [0, 'pragma directive'] + } + + if (node.type === 'ImportDirective') { + return [10, 'import directive'] + } + + if (node.type === 'EnumDefinition' || node.type === 'StructDefinition') { + return [20, 'type definition'] + } + + if (node.type === 'ContractDefinition') { + if (node.kind === 'interface') { + return [30, 'interface'] + } + + if (node.kind === 'library') { + return [40, 'library definition'] + } + + if (node.kind === 'contract') { + return [50, 'contract definition'] + } + } + + throw new Error('Unrecognized source unit part, please report this issue') +} + +function contractPartOrder(node) { + if (isTypeDeclaration(node)) { + let label + if (node.type === 'StructDefinition') { + label = 'struct definition' + } else { + label = 'enum definition' + } + return [0, label] + } + + if (node.type === 'StateVariableDeclaration') { + return [10, 'state variable declaration'] + } + + if (node.type === 'EventDefinition') { + return [20, 'event definition'] + } + + if (node.isConstructor) { + return [30, 'constructor'] + } + + if (isFallbackFunction(node)) { + return [40, 'fallback function'] + } + + if (node.type === 'FunctionDefinition') { + if (node.visibility === 'external' && !isConst(node)) { + return [50, 'external function'] + } + 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 = OrderingChecker diff --git a/test/fixtures/order/ordering-correct.js b/test/fixtures/order/ordering-correct.js new file mode 100644 index 00000000..4a2a675c --- /dev/null +++ b/test/fixtures/order/ordering-correct.js @@ -0,0 +1,58 @@ +module.exports = [ + { + description: 'All units are in order', + code: ` +pragma solidity ^0.6.0; + +import "./some/library.sol"; +import "./some/other-library.sol"; + +enum MyEnum { + Foo, + Bar +} + +struct MyStruct { + uint x; + uint y; +} + +interface IBox { + function getValue() public; + function setValue(uint) public; +} + +library MyLibrary { + function add(uint a, uint b, uint c) public returns (uint) { + return a + b + c; + } +} + +contract MyContract { + struct InnerStruct { + bool flag; + } + + enum InnerEnum { + A, B, C + } + + uint public x; + uint public y; + + event MyEvent(address a); + + constructor () public {} + + fallback () external {} + + function myExternalFunction() external {} + function myExternalConstFunction() external const {} + function myPublicFunction() public {} + function myPublicConstFunction() public const {} + function myInternalFunction() internal {} + function myPrivateFunction() private {} +} +` + } +] diff --git a/test/fixtures/order/ordering-incorrect.js b/test/fixtures/order/ordering-incorrect.js new file mode 100644 index 00000000..c0b7d05f --- /dev/null +++ b/test/fixtures/order/ordering-incorrect.js @@ -0,0 +1,28 @@ +module.exports = [ + { + description: 'State variable declaration after function', + code: ` + contract MyContract { + function foo() public {} + + uint a; + } +` + }, + { + description: 'Library after contract', + code: ` + contract MyContract {} + + library MyLibrary {} +` + }, + { + description: 'Interface after library', + code: ` + library MyLibrary {} + + interface MyInterface {} +` + } +] diff --git a/test/rules/order/ordering.js b/test/rules/order/ordering.js new file mode 100644 index 00000000..e8a9552d --- /dev/null +++ b/test/rules/order/ordering.js @@ -0,0 +1,161 @@ +const assert = require('assert') +const linter = require('./../../../lib/index') +const contractWith = require('./../../common/contract-builder').contractWith +const correctExamples = require('../../fixtures/order/ordering-correct') +const incorrectExamples = require('../../fixtures/order/ordering-incorrect') + +describe('Linter - ordering', () => { + describe('correct examples', () => { + correctExamples.forEach(({ code, description }) => { + it(description, () => { + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 0) + }) + }) + }) + + describe('incorrect examples', () => { + incorrectExamples.forEach(({ code, description }) => { + it(description, () => { + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 1) + }) + }) + }) + + it('should raise incorrect function order error I', () => { + const code = contractWith(` + function b() private {} + function () public payable {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 1) + assert.ok(report.messages[0].message.includes('Function order is incorrect')) + }) + + it('should raise incorrect function order error for external constant funcs', () => { + const code = contractWith(` + function b() external pure {} + function c() external {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 1) + assert.ok(report.messages[0].message.includes('Function order is incorrect')) + }) + + it('should raise incorrect function order error for public constant funcs', () => { + const code = contractWith(` + function b() public pure {} + function c() public {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 1) + assert.ok(report.messages[0].message.includes('Function order is incorrect')) + }) + + it('should raise incorrect function order error for internal function', () => { + const code = contractWith(` + function c() internal {} + function b() external view {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 1) + assert.ok(report.messages[0].message.includes('Function order is incorrect')) + }) + + it('should not raise incorrect function order error', () => { + const code = contractWith(` + function A() public {} + function () public payable {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 0) + }) + + it('should not raise incorrect function order error I', () => { + const code = require('../../fixtures/order/func-order-constructor-first') + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 0) + }) + + it('should raise incorrect function order error', () => { + const code = require('../../fixtures/order/func-order-constructor-not-first') + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + assert.equal(report.errorCount, 1) + assert.ok(report.messages[0].message.includes('Function order is incorrect')) + }) + + it('should not raise error when external const goes before public ', () => { + const code = contractWith(` + function a() external view {} + function b() public {} + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 0) + }) + + it('should raise error for enum after contract', () => { + const code = ` + contract Foo {} + + enum MyEnum { A, B } + ` + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 1) + }) + + it('should raise error for enum after function', () => { + const code = contractWith(` + function foo() public {} + + enum MyEnum { A, B } + `) + + const report = linter.processStr(code, { + rules: { ordering: 'error' } + }) + + assert.equal(report.errorCount, 1) + }) +})