diff --git a/CHANGELOG.md b/CHANGELOG.md index 15c24f51..d63bff5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ ### Added - New Rule: No unused imports [#417](https://github.com/protofire/solhint/pull/417) - New Rule: To treat immutable as constants [#458](https://github.com/protofire/solhint/pull/458) +- New Rule: Explicit-types. To forbid/enforce full type or alias for variables declaration [#467](https://github.com/protofire/solhint/pull/467) - JSON formatter support [#440](https://github.com/protofire/solhint/pull/440) - Rules List with `list-rules` command [#449](https://github.com/protofire/solhint/pull/449) - E2E tests for formatters and new `Compact formatter` [#457](https://github.com/protofire/solhint/pull/457) diff --git a/docs/rules.md b/docs/rules.md index 2f4ffc93..1e3d4d12 100644 --- a/docs/rules.md +++ b/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. | | | +| [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. | | | | [max-states-count](./rules/best-practises/max-states-count.md) | Contract has "some count" states declarations but allowed no more than maxstates. | $~~~~~~~~$✔️ | | diff --git a/docs/rules/best-practises/explicit-types.md b/docs/rules/best-practises/explicit-types.md new file mode 100644 index 00000000..96cb06a5 --- /dev/null +++ b/docs/rules/best-practises/explicit-types.md @@ -0,0 +1,71 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "explicit-types | Solhint" +--- + +# explicit-types +![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 +Forbid or enforce explicit types (like uint256) that have an alias (like uint). + +## Options +This rule accepts an array of options: + +| Index | Description | Default Value | +| ----- | ----------------------------------------------------- | ------------- | +| 0 | Rule severity. Must be one of "error", "warn", "off". | warn | +| 1 | Options need to be one of "explicit", "implicit" | explicit | + + +### Example Config +```json +{ + "rules": { + "explicit-types": ["warn","explicit"] + } +} +``` + + +## Examples +### 👍 Examples of **correct** code for this rule + +#### If explicit is selected + +```solidity +uint256 public variableName +``` + +#### If implicit is selected + +```solidity +uint public variableName +``` + +### 👎 Examples of **incorrect** code for this rule + +#### If explicit is selected + +```solidity +uint public variableName +``` + +#### If implicit is selected + +```solidity +uint256 public variableName +``` + +## Version +This rule is introduced in the latest version. + +## Resources +- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/explicit-types.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/best-practises/explicit-types.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/best-practises/explicit-types.js) diff --git a/lib/rules/best-practises/explicit-types.js b/lib/rules/best-practises/explicit-types.js new file mode 100644 index 00000000..4f2a6149 --- /dev/null +++ b/lib/rules/best-practises/explicit-types.js @@ -0,0 +1,128 @@ +const BaseChecker = require('../base-checker') +const { severityDescription, formatEnum } = require('../../doc/utils') + +const EXPLICIT_TYPES = ['uint256', 'int256', 'ufixed128x18', 'fixed128x18'] +const IMPLICIT_TYPES = ['uint', 'int', 'ufixed', 'fixed'] +const ALL_TYPES = EXPLICIT_TYPES.concat(IMPLICIT_TYPES) +const VALID_CONFIGURATION_OPTIONS = ['explicit', 'implicit'] +const DEFAULT_OPTION = 'explicit' +const DEFAULT_SEVERITY = 'warn' + +const ruleId = 'explicit-types' +const meta = { + type: 'best-practises', + + docs: { + description: 'Forbid or enforce explicit types (like uint256) that have an alias (like uint).', + category: 'Best Practise Rules', + options: [ + { + description: severityDescription, + default: DEFAULT_SEVERITY, + }, + { + description: `Options need to be one of ${formatEnum(VALID_CONFIGURATION_OPTIONS)}`, + default: DEFAULT_OPTION, + }, + ], + examples: { + good: [ + { + description: 'If explicit is selected', + code: 'uint256 public variableName', + }, + { + description: 'If implicit is selected', + code: 'uint public variableName', + }, + ], + bad: [ + { + description: 'If explicit is selected', + code: 'uint public variableName', + }, + { + description: 'If implicit is selected', + code: 'uint256 public variableName', + }, + ], + }, + }, + + isDefault: false, + recommended: true, + defaultSetup: [DEFAULT_SEVERITY, DEFAULT_OPTION], + + schema: { + type: 'string', + enum: VALID_CONFIGURATION_OPTIONS, + }, +} + +class ExplicitTypesChecker extends BaseChecker { + constructor(reporter, config) { + super(reporter, ruleId, meta) + this.configOption = (config && config.getString(ruleId, DEFAULT_OPTION)) || DEFAULT_OPTION + this.isExplicit = this.configOption === 'explicit' + + this.validateConfigOption(this.configOption) + } + + VariableDeclaration(node) { + const typeToFind = 'ElementaryTypeName' + const onlyTypes = this.findNamesOfElementaryTypeName(node, typeToFind) + + // if no variables are defined, return + if (onlyTypes && onlyTypes.length === 0) return + + // this tells if the variable needs to be checked + const varsToBeChecked = onlyTypes.filter((type) => ALL_TYPES.includes(type)) + + // if defined variables are not to be checked (example address type), return + if (varsToBeChecked && varsToBeChecked.length === 0) return + + // if explicit, it will search for implicit and viceversa + if (this.isExplicit) { + this.validateVariables(IMPLICIT_TYPES, node, varsToBeChecked) + } else { + this.validateVariables(EXPLICIT_TYPES, node, varsToBeChecked) + } + } + + validateConfigOption(configOption) { + const configOk = VALID_CONFIGURATION_OPTIONS.includes(configOption) + if (!configOk) + throw new Error('Error: Config error on [explicit-types]. See explicit-types.md.') + } + + validateVariables(configType, node, varsToBeChecked) { + const errorVars = varsToBeChecked.filter((type) => configType.includes(type)) + + if (errorVars && errorVars.length > 0) { + for (const errorVar of errorVars) { + this.error(node, `Rule is set with ${this.configOption} type [var/s: ${errorVar}]`) + } + } + } + + findNamesOfElementaryTypeName(jsonObject, typeToFind) { + const names = [] + + const searchInObject = (obj) => { + if (obj.type === typeToFind && obj.name) { + names.push(obj.name) + } + + for (const key in obj) { + if (typeof obj[key] === 'object' && obj[key] !== null) { + searchInObject(obj[key]) + } + } + } + + searchInObject(jsonObject) + return names + } +} + +module.exports = ExplicitTypesChecker diff --git a/lib/rules/best-practises/index.js b/lib/rules/best-practises/index.js index fc03f0f9..6b2ac780 100644 --- a/lib/rules/best-practises/index.js +++ b/lib/rules/best-practises/index.js @@ -9,6 +9,7 @@ const ReasonStringChecker = require('./reason-string') const NoConsoleLogChecker = require('./no-console') const NoGlobalImportsChecker = require('./no-global-import') const NoUnusedImportsChecker = require('./no-unused-import') +const ExplicitTypesChecker = require('./explicit-types') module.exports = function checkers(reporter, config, inputSrc) { return [ @@ -23,5 +24,6 @@ module.exports = function checkers(reporter, config, inputSrc) { new NoConsoleLogChecker(reporter), new NoGlobalImportsChecker(reporter), new NoUnusedImportsChecker(reporter), + new ExplicitTypesChecker(reporter, config), ] } diff --git a/test/fixtures/best-practises/explicit-types.js b/test/fixtures/best-practises/explicit-types.js new file mode 100644 index 00000000..b63c4838 --- /dev/null +++ b/test/fixtures/best-practises/explicit-types.js @@ -0,0 +1,135 @@ +const VAR_DECLARATIONS = { + uint256: { + code: 'uint256 public varUint256;', + errorsImplicit: 1, + errorsExplicit: 0, + }, + + uint: { + code: 'uint public varUint;', + errorsImplicit: 0, + errorsExplicit: 1, + }, + + int256: { + code: 'int256 public varInt256;', + errorsImplicit: 1, + errorsExplicit: 0, + }, + + int: { + code: 'int public varInt;', + errorsImplicit: 0, + errorsExplicit: 1, + }, + + ufixed128x18: { + code: 'ufixed128x18 public varUfixed128x18;', + errorsImplicit: 1, + errorsExplicit: 0, + }, + + ufixed: { + code: 'ufixed public varUfixed;', + errorsImplicit: 0, + errorsExplicit: 1, + }, + + fixed128x18: { + code: 'fixed128x18 public varFixed128x18;', + errorsImplicit: 1, + errorsExplicit: 0, + }, + + fixed: { + code: 'fixed public varFixed;', + errorsImplicit: 0, + errorsExplicit: 1, + }, + + functionParameterAndReturns1: { + code: 'function withUint256(uint256 varUint256, uint varUint) public returns(int256 returnInt256) {}', + errorsImplicit: 2, + errorsExplicit: 1, + }, + + functionParameterAndReturns2: { + code: 'function withUint256(uint varUint, uint varUint) public returns(int returnInt) {}', + errorsImplicit: 0, + errorsExplicit: 3, + }, + + eventParameter: { + code: 'event EventWithInt256(int256 varWithInt256, address addressVar, bool boolVat, int varWithInt);', + errorsImplicit: 1, + errorsExplicit: 1, + }, + + regularMap1: { + code: 'mapping(uint256 => fixed128x18) public mapWithFixed128x18;', + errorsImplicit: 2, + errorsExplicit: 0, + }, + + regularMap2: { + code: 'mapping(uint => fixed128x18) public mapWithFixed128x18;', + errorsImplicit: 1, + errorsExplicit: 1, + }, + + mapOfMapping: { + code: 'mapping(uint => mapping(fixed128x18 => int256)) mapOfMap;', + errorsImplicit: 2, + errorsExplicit: 1, + }, + + struct: { + code: 'struct Estructura { ufixed128x18 varWithUfixed128x18; uint varUint; }', + errorsImplicit: 1, + errorsExplicit: 1, + }, + + mapOfStruct: { + code: 'struct Estructura { ufixed128x18 varWithUfixed128x18; uint varUint; mapping(uint256 => Estructura) mapOfStruct; }', + errorsImplicit: 2, + errorsExplicit: 1, + }, + + structWithMap: { + code: 'struct Estructura { ufixed varWithUfixed; uint varUint; mapping(address => uint256) structWithMap; } ', + errorsImplicit: 1, + errorsExplicit: 2, + }, + + mapOfArrayStructWithMap: { + code: 'struct Estructura { ufixed varWithUfixed; uint varUint; mapping(address => uint256) structWithMap; } mapping(uint256 => Estructura[]) mapOfArrayStructWithMap;', + errorsImplicit: 2, + errorsExplicit: 2, + }, + + regularArray: { + code: 'uint256[] public arr;', + errorsImplicit: 1, + errorsExplicit: 0, + }, + + fixedArray: { + code: 'uint[] public arr = [1,2,3];', + errorsImplicit: 0, + errorsExplicit: 1, + }, + + fixedArrayOfArrays: { + code: 'uint256[] public arr = [[1,2,3]];', + errorsImplicit: 1, + errorsExplicit: 0, + }, + + mapOfFixedArray: { + code: 'uint[] public arr = [1,2,3]; mapping(uint256 => arr) mapOfFixedArray;', + errorsImplicit: 1, + errorsExplicit: 1, + }, +} + +module.exports = VAR_DECLARATIONS diff --git a/test/rules/best-practises/explicit-types.js b/test/rules/best-practises/explicit-types.js new file mode 100644 index 00000000..842329c6 --- /dev/null +++ b/test/rules/best-practises/explicit-types.js @@ -0,0 +1,95 @@ +const assert = require('assert') +const linter = require('../../../lib/index') +const contractWith = require('../../common/contract-builder').contractWith +const { assertErrorCount, assertNoErrors, assertErrorMessage } = require('../../common/asserts') +const VAR_DECLARATIONS = require('../../fixtures/best-practises/explicit-types') + +const getZeroErrosObject = () => { + const zeroErrorsExplicit = {} + const zeroErrorsImplicit = {} + for (const key in VAR_DECLARATIONS) { + const obj = VAR_DECLARATIONS[key] + if (obj.errorsImplicit === 0) { + zeroErrorsImplicit[key] = obj + } + if (obj.errorsExplicit === 0) { + zeroErrorsExplicit[key] = obj + } + } + + return { zeroErrorsImplicit, zeroErrorsExplicit } +} + +describe('Linter - explicit-types rule', () => { + it('should throw an error with a wrong configuration rule example 1', () => { + const code = contractWith('uint256 public constant SNAKE_CASE = 1;') + try { + linter.processStr(code, { + rules: { 'explicit-types': ['error', 'implicito'] }, + }) + } catch (error) { + assert.ok( + error.toString().includes('Error: Config error on [explicit-types]. See explicit-types.md.') + ) + } + }) + + it('should throw an error with a wrong configuration rule example 2', () => { + const code = contractWith('uint256 public constant SNAKE_CASE = 1;') + try { + linter.processStr(code, { + rules: { 'explicit-types': 'error' }, + }) + } catch (error) { + assert.ok( + error.toString().includes('Error: Config error on [explicit-types]. See explicit-types.md.') + ) + } + }) + + for (const key in VAR_DECLARATIONS) { + it(`should raise error for ${key} on 'implicit' mode`, () => { + const { code, errorsImplicit } = VAR_DECLARATIONS[key] + const report = linter.processStr(contractWith(code), { + rules: { 'explicit-types': ['error', 'implicit'] }, + }) + assertErrorCount(report, errorsImplicit) + if (errorsImplicit > 0) assertErrorMessage(report, `Rule is set with implicit type [var/s:`) + }) + } + + for (const key in VAR_DECLARATIONS) { + it(`should raise error for ${key} on 'explicit' mode`, () => { + const { code, errorsExplicit } = VAR_DECLARATIONS[key] + const report = linter.processStr(contractWith(code), { + rules: { 'explicit-types': ['error', 'explicit'] }, + }) + assertErrorCount(report, errorsExplicit) + if (errorsExplicit > 0) assertErrorMessage(report, `Rule is set with explicit type [var/s:`) + }) + } + + describe('Rule [explicit-types] - should not raise any error', () => { + const { zeroErrorsImplicit, zeroErrorsExplicit } = getZeroErrosObject() + + for (const key in zeroErrorsExplicit) { + it(`should NOT raise error for ${key} on 'implicit' mode`, () => { + const { code } = zeroErrorsExplicit[key] + const report = linter.processStr(contractWith(code), { + rules: { 'explicit-types': ['error', 'explicit'] }, + }) + assertNoErrors(report) + }) + } + + for (const key in zeroErrorsImplicit) { + it(`should NOT raise error for ${key} on 'implicit' mode`, () => { + const { code } = zeroErrorsImplicit[key] + const report = linter.processStr(contractWith(code), { + rules: { 'explicit-types': ['error', 'implicit'] }, + }) + assertNoErrors(report) + }) + } + }) +})