diff --git a/conf/rulesets/solhint-all.js b/conf/rulesets/solhint-all.js index a596dead..ef674361 100644 --- a/conf/rulesets/solhint-all.js +++ b/conf/rulesets/solhint-all.js @@ -8,6 +8,7 @@ module.exports = Object.freeze({ 'code-complexity': ['warn', 7], 'explicit-types': ['warn', 'explicit'], 'function-max-lines': ['warn', 50], + 'interface-starts-with-i': 'warning', 'max-line-length': ['error', 120], 'max-states-count': ['warn', 15], 'no-console': 'error', @@ -28,16 +29,18 @@ module.exports = Object.freeze({ 'gas-custom-errors': 'warn', 'gas-increment-by-one': 'warn', 'gas-indexed-events': 'warn', + 'gas-length-in-loops': 'warn', 'gas-multitoken1155': 'warn', 'gas-named-return-values': 'warn', 'gas-small-strings': 'warn', + 'gas-strict-inequalities': 'warn', 'gas-struct-packing': 'warn', 'comprehensive-interface': 'warn', quotes: ['error', 'double'], 'const-name-snakecase': 'warn', 'contract-name-camelcase': 'warn', 'event-name-camelcase': 'warn', - 'foundry-test-functions': ['off', ['setUp']], + 'foundry-test-functions': ['warn', ['setUp']], 'func-name-mixedcase': 'warn', 'func-named-parameters': ['warn', 4], 'func-param-name-mixedcase': 'warn', diff --git a/docs/rules.md b/docs/rules.md index 10d413f0..a8876b6c 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -25,32 +25,11 @@ title: "Rule Index of Solhint" | [non-state-vars-leading-underscore](./rules/naming/non-state-vars-leading-underscore.md) | Variables that are not in contract state should start with underscore. Conversely, variables that can cause an SLOAD/SSTORE should NOT start with an underscore. This makes it evident which operations cause expensive storage access when hunting for gas optimizations | | | -## Gas Consumption Rules - -| Rule Id | Error | Recommended | Deprecated | -| ----------------------------------------------------------------------------- | -------------------------------------------------------------------- | ------------ | ---------- | -| [gas-calldata-parameters](./rules/gas-consumption/gas-calldata-parameters.md) | Suggest calldata keyword on function arguments when read only | | | -| [gas-custom-errors](./rules/gas-consumption/gas-custom-errors.md) | Enforces the use of Custom Errors over Require and Revert statements | $~~~~~~~~$✔️ | | -| [gas-increment-by-one](./rules/gas-consumption/gas-increment-by-one.md) | Suggest incrementation by one like this ++i instead of other type | | | -| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md) | Suggest indexed arguments on events for uint, bool and address | | | -| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md) | ERC1155 is a cheaper non-fungible token than ERC721 | | | -| [gas-named-return-values](./rules/gas-consumption/gas-named-return-values.md) | Enforce the return values of a function to be named | | | -| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md) | Keep strings smaller than 32 bytes | | | -| [gas-struct-packing](./rules/gas-consumption/gas-struct-packing.md) | Suggest to re-arrange struct packing order when it is inefficient | | | - - -## Miscellaneous - -| Rule Id | Error | Recommended | Deprecated | -| --------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------- | ------------ | ---------- | -| [comprehensive-interface](./rules/miscellaneous/comprehensive-interface.md) | Check that all public or external functions are override. This is iseful to make sure that the whole API is extracted in an interface. | | | -| [quotes](./rules/miscellaneous/quotes.md) | Enforces the use of double or simple quotes as configured for string literals. Values must be 'single' or 'double'. | $~~~~~~~~$✔️ | | - - ## Style Guide Rules | Rule Id | Error | Recommended | Deprecated | | ------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------- | ------------ | ----------- | +| [interface-starts-with-i](./rules/naming/interface-starts-with-i.md) | Solidity Interfaces names should start with an `I` | | | | [const-name-snakecase](./rules/naming/const-name-snakecase.md) | Constant name must be in capitalized SNAKE_CASE. (Does not check IMMUTABLES, use immutable-vars-naming) | $~~~~~~~~$✔️ | | | [contract-name-camelcase](./rules/naming/contract-name-camelcase.md) | Contract, Structs and Enums should be in CamelCase. | $~~~~~~~~$✔️ | | | [event-name-camelcase](./rules/naming/event-name-camelcase.md) | Event name must be in CamelCase. | $~~~~~~~~$✔️ | | @@ -70,6 +49,30 @@ title: "Rule Index of Solhint" | [visibility-modifier-order](./rules/order/visibility-modifier-order.md) | Visibility modifier must be first in list of modifiers. | $~~~~~~~~$✔️ | | +## Gas Consumption Rules + +| Rule Id | Error | Recommended | Deprecated | +| ----------------------------------------------------------------------------- | -------------------------------------------------------------------- | ------------ | ---------- | +| [gas-calldata-parameters](./rules/gas-consumption/gas-calldata-parameters.md) | Suggest calldata keyword on function arguments when read only | | | +| [gas-custom-errors](./rules/gas-consumption/gas-custom-errors.md) | Enforces the use of Custom Errors over Require and Revert statements | $~~~~~~~~$✔️ | | +| [gas-increment-by-one](./rules/gas-consumption/gas-increment-by-one.md) | Suggest incrementation by one like this ++i instead of other type | | | +| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md) | Suggest indexed arguments on events for uint, bool and address | | | +| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md) | ERC1155 is a cheaper non-fungible token than ERC721 | | | +| [gas-named-return-values](./rules/gas-consumption/gas-named-return-values.md) | Enforce the return values of a function to be named | | | +| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md) | Keep strings smaller than 32 bytes | | | +| [gas-strict-inequalities](./rules/gas-consumption/gas-strict-inequalities.md) | Suggest Strict Inequalities over non Strict ones | | | +| [gas-struct-packing](./rules/gas-consumption/gas-struct-packing.md) | Suggest to re-arrange struct packing order when it is inefficient | | | +| [gas-length-in-loops](./rules/gas-consumption/gas-length-in-loops.md) | Suggest replacing object.length in a loop condition to avoid calculation on each lap | | | + + +## Miscellaneous + +| Rule Id | Error | Recommended | Deprecated | +| --------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------- | ------------ | ---------- | +| [comprehensive-interface](./rules/miscellaneous/comprehensive-interface.md) | Check that all public or external functions are override. This is iseful to make sure that the whole API is extracted in an interface. | | | +| [quotes](./rules/miscellaneous/quotes.md) | Enforces the use of double or simple quotes as configured for string literals. Values must be 'single' or 'double'. | $~~~~~~~~$✔️ | | + + ## Security Rules | Rule Id | Error | Recommended | Deprecated | diff --git a/docs/rules/gas-consumption/gas-length-in-loops.md b/docs/rules/gas-consumption/gas-length-in-loops.md new file mode 100644 index 00000000..aca08a0e --- /dev/null +++ b/docs/rules/gas-consumption/gas-length-in-loops.md @@ -0,0 +1,38 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "gas-length-in-loops | Solhint" +--- + +# gas-length-in-loops +![Category Badge](https://img.shields.io/badge/-Gas%20Consumption%20Rules-informational) +![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) + +## Description +Suggest replacing object.length in a loop condition to avoid calculation on each lap + +## Options +This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. + +### Example Config +```json +{ + "rules": { + "gas-length-in-loops": "warn" + } +} +``` + +### Notes +- [source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (see Array Length Caching) + +## Examples +This rule does not have examples. + +## Version +This rule is introduced in the latest version. + +## Resources +- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/gas-consumption/gas-length-in-loops.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/gas-consumption/gas-length-in-loops.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/gas-consumption/gas-length-in-loops.js) diff --git a/docs/rules/gas-consumption/gas-strict-inequalities.md b/docs/rules/gas-consumption/gas-strict-inequalities.md new file mode 100644 index 00000000..f3618b94 --- /dev/null +++ b/docs/rules/gas-consumption/gas-strict-inequalities.md @@ -0,0 +1,40 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "gas-strict-inequalities | Solhint" +--- + +# gas-strict-inequalities +![Category Badge](https://img.shields.io/badge/-Gas%20Consumption%20Rules-informational) +![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) + +## Description +Suggest Strict Inequalities over non Strict ones + +## Options +This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. + +### Example Config +```json +{ + "rules": { + "gas-strict-inequalities": "warn" + } +} +``` + +### Notes +- Strict inequality does not always saves gas. It is dependent on the context of the surrounding opcodes +- [source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (see Less/Greater Than vs Less/Greater Than or Equal To) +- [source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-7b77t) of the rule initiative + +## Examples +This rule does not have examples. + +## Version +This rule is introduced in the latest version. + +## Resources +- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/gas-consumption/gas-strict-inequalities.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/gas-consumption/gas-strict-inequalities.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/gas-consumption/gas-strict-inequalities.js) diff --git a/docs/rules/naming/foundry-test-functions.md b/docs/rules/naming/foundry-test-functions.md index 164d66ea..ff14f06e 100644 --- a/docs/rules/naming/foundry-test-functions.md +++ b/docs/rules/naming/foundry-test-functions.md @@ -6,7 +6,7 @@ title: "foundry-test-functions | Solhint" # foundry-test-functions ![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational) -![Default Severity Badge off](https://img.shields.io/badge/Default%20Severity-off-undefined) +![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) ## Description Enforce naming convention on functions for Foundry test cases @@ -16,14 +16,14 @@ This rule accepts an array of options: | Index | Description | Default Value | | ----- | ----------------------------------------------------- | ------------- | -| 0 | Rule severity. Must be one of "error", "warn", "off". | off | +| 0 | Rule severity. Must be one of "error", "warn", "off". | warn | ### Example Config ```json { "rules": { - "foundry-test-functions": ["off",["setUp"]] + "foundry-test-functions": ["warn",["setUp"]] } } ``` diff --git a/docs/rules/naming/interface-starts-with-i.md b/docs/rules/naming/interface-starts-with-i.md new file mode 100644 index 00000000..89a468bc --- /dev/null +++ b/docs/rules/naming/interface-starts-with-i.md @@ -0,0 +1,50 @@ +--- +warning: "This is a dynamically generated file. Do not edit manually." +layout: "default" +title: "interface-starts-with-i | Solhint" +--- + +# interface-starts-with-i +![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational) +![Default Severity Badge warning](https://img.shields.io/badge/Default%20Severity-warning-undefined) + +## Description +Solidity Interfaces names should start with an `I` + +## Options +This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warning. + +### Example Config +```json +{ + "rules": { + "interface-starts-with-i": "warning" + } +} +``` + + +## Examples +### 👍 Examples of **correct** code for this rule + +#### Interface name starts with I + +```solidity +interface IFoo { function foo () external; } +``` + +### 👎 Examples of **incorrect** code for this rule + +#### Interface name doesnt start with I + +```solidity +interface Foo { function foo () external; } +``` + +## Version +This rule is introduced in the latest version. + +## Resources +- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/best-practises/interface-starts-with-i.js) +- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/best-practises/interface-starts-with-i.md) +- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/best-practises/interface-starts-with-i.js) diff --git a/docs/rules/naming/named-parameters-mapping.md b/docs/rules/naming/named-parameters-mapping.md index 6ae15579..1f3d1a81 100644 --- a/docs/rules/naming/named-parameters-mapping.md +++ b/docs/rules/naming/named-parameters-mapping.md @@ -6,19 +6,19 @@ title: "named-parameters-mapping | Solhint" # named-parameters-mapping ![Category Badge](https://img.shields.io/badge/-Style%20Guide%20Rules-informational) -![Default Severity Badge off](https://img.shields.io/badge/Default%20Severity-off-undefined) +![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) ## Description Solidity v0.8.18 introduced named parameters on the mappings definition. ## Options -This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to off. +This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn. ### Example Config ```json { "rules": { - "named-parameters-mapping": "off" + "named-parameters-mapping": "warn" } } ``` diff --git a/e2e/autofix-test.js b/e2e/autofix-test.js index 8a6ad611..16fd5daa 100644 --- a/e2e/autofix-test.js +++ b/e2e/autofix-test.js @@ -103,7 +103,7 @@ describe('e2e', function () { } ) - expect(solhintProcess.status).to.equal(1) + expect(solhintProcess.status).to.equal(0) expect(solhintProcess.stdout.toString().includes('5 problems (5 errors, 0 warnings)')) }) }) @@ -129,7 +129,7 @@ describe('e2e', function () { `${params.command} ${params.param1} -c ${currentConfig} ${currentFile} --fix --disc --noPrompt` ) - expect(code).to.equal(1) + expect(code).to.equal(0) const reportLines = stdout.split('\n') const finalLine = '5 problems (5 errors, 0 warnings)' @@ -175,8 +175,8 @@ describe('e2e', function () { expect(result).to.be.true }) - it('should execute and exit with code 1 (2)', () => { - expect(code).to.equal(1) + it('should execute and exit with code 0 (2)', () => { + expect(code).to.equal(0) }) it('should get the right report (2)', () => { @@ -222,7 +222,7 @@ describe('e2e', function () { }) it('should execute and exit with code 1 (3)', () => { - expect(code).to.equal(1) + expect(code).to.equal(0) }) it('should get the right report (3)', () => { @@ -268,7 +268,7 @@ describe('e2e', function () { }) it('should execute and exit with code 1 (4)', () => { - expect(code).to.equal(1) + expect(code).to.equal(0) }) it('should get the right report (4)', () => { @@ -314,7 +314,7 @@ describe('e2e', function () { }) it('should execute and exit with code 1 (5)', () => { - expect(code).to.equal(1) + expect(code).to.equal(0) }) it('should get the right report (5)', () => { @@ -361,7 +361,7 @@ describe('e2e', function () { }) it('should execute and exit with code 1 (6)', () => { - expect(code).to.equal(1) + expect(code).to.equal(0) }) it('should get the right report (6)', () => { @@ -401,7 +401,7 @@ describe('e2e', function () { }) it('should execute and exit with code 1 (6)', () => { - expect(code).to.equal(1) + expect(code).to.equal(0) }) it('should get the right report (6)', () => { @@ -448,7 +448,7 @@ describe('e2e', function () { }) it('should execute and exit with code 1 (7)', () => { - expect(code).to.equal(1) + expect(code).to.equal(0) }) it('should get the right report (7)', () => { @@ -494,7 +494,7 @@ describe('e2e', function () { }) it('should execute and exit with code 1 (8)', () => { - expect(code).to.equal(1) + expect(code).to.equal(0) }) it('should get the right report (8)', () => { @@ -540,7 +540,7 @@ describe('e2e', function () { }) it('should execute and exit with code 1 (8)', () => { - expect(code).to.equal(1) + expect(code).to.equal(0) }) it('should get the right report (8)', () => { diff --git a/e2e/formatters-test.js b/e2e/formatters-test.js index 09df3bc6..107b7306 100644 --- a/e2e/formatters-test.js +++ b/e2e/formatters-test.js @@ -81,7 +81,7 @@ describe('e2e', function () { expect(reportLines[i]).to.equal(expectedLine) } // because there's an error - expect(code).to.equal(1) + expect(code).to.equal(0) const finalLine = '10 problem/s (1 error/s, 9 warning/s)' expect(reportLines[reportLines.length - 2]).to.contain(finalLine) @@ -128,7 +128,7 @@ describe('e2e', function () { const strExpected = JSON.stringify(expectedFinalOutput) expect(strExpected).to.equal(strOutput) // There's an error, that is why exit code is 1 - expect(code).to.equal(1) + expect(code).to.equal(0) }) }) @@ -175,7 +175,7 @@ describe('e2e', function () { expect(reportLines[i]).to.equal(expectedLine) } // because there's an error - expect(code).to.equal(1) + expect(code).to.equal(0) const finalLine = '10 problem/s (1 error/s, 9 warning/s)' expect(reportLines[reportLines.length - 2]).to.contain(finalLine) @@ -240,7 +240,7 @@ describe('e2e', function () { expect(reportLines[i].replace(/\s/g, '')).to.equal(expectedLine.replace(/\s/g, '')) } - expect(code).to.equal(1) + expect(code).to.equal(0) const finalLine = '\u2716 10 problems (1 error, 9 warnings)' expect(reportLines[reportLines.length - 3]).to.equal(finalLine) @@ -342,7 +342,7 @@ describe('e2e', function () { expect(reportLines[3]).to.eq(tableHeader1) expect(reportLines[4]).to.eq(tableHeader2) - expect(code).to.equal(1) + expect(code).to.equal(0) }) }) @@ -670,7 +670,7 @@ describe('e2e', function () { ] // There's an error, that is why exit code is 1 - expect(code).to.equal(1) + expect(code).to.equal(0) expect(sarifOutput.runs[0].artifacts[0].location.uri).to.eq(expectedUriPathFoo) expect(sarifOutput.runs[0].artifacts[1].location.uri).to.eq(expectedUriPathFoo2) expect(sarifOutput.runs[0].artifacts[2].location.uri).to.eq(expectedUriPathFoo3) diff --git a/e2e/test.js b/e2e/test.js index c218dae1..ec40127c 100644 --- a/e2e/test.js +++ b/e2e/test.js @@ -73,14 +73,14 @@ describe('e2e', function () { describe('no-empty-blocks', function () { useFixture('03-no-empty-blocks') - it('should exit with 1', function () { + it('should end correctly (exit w/0), found 1 error', function () { const { code, stdout } = shell.exec('solhint Foo.sol') - expect(code).to.equal(1) + expect(code).to.equal(0) expect(stdout.trim()).to.contain('Code contains empty blocks') }) - it('should work with stdin', async function () { + it('should work with stdin, exit 0, found 1 error', async function () { const child = cp.exec('solhint stdin') const stdoutPromise = getStream(child.stdout) @@ -96,7 +96,7 @@ describe('e2e', function () { const code = await codePromise - expect(code).to.equal(1) + expect(code).to.equal(0) const stdout = await stdoutPromise @@ -127,7 +127,7 @@ describe('e2e', function () { expect(stdout.trim()).to.not.contain(warningExceededMsg) }) - it('should display [warnings exceeded] for max 3 warnings and exit error 1', function () { + it('should display [warnings exceeded] for max 3 warnings and exit with 0', function () { const { code, stdout } = shell.exec('solhint contracts/Foo.sol --max-warnings 3') expect(code).to.equal(1) @@ -137,13 +137,13 @@ describe('e2e', function () { it('should return error for Compiler version rule, ignoring 3 --max-warnings', function () { const { code, stdout } = shell.exec('solhint contracts/Foo2.sol --max-warnings 3') - expect(code).to.equal(1) + expect(code).to.equal(0) expect(stdout.trim()).to.contain(errorFound) }) it('should return error for Compiler version rule. No message for max-warnings', function () { const { code, stdout } = shell.exec('solhint contracts/Foo2.sol --max-warnings 27') - expect(code).to.equal(1) + expect(code).to.equal(0) expect(stdout.trim()).to.not.contain(errorFound) }) }) @@ -163,7 +163,7 @@ describe('e2e', function () { it(`should raise error for wrongFunctionDefinitionName() only`, () => { const { code, stdout } = shell.exec('solhint -c test/.solhint.json test/FooTest.sol') - expect(code).to.equal(1) + expect(code).to.equal(0) expect(stdout.trim()).to.contain( 'Function wrongFunctionDefinitionName() must match Foundry test naming convention' ) diff --git a/lib/rules/best-practises/index.js b/lib/rules/best-practises/index.js index 15059e83..c7d2f090 100644 --- a/lib/rules/best-practises/index.js +++ b/lib/rules/best-practises/index.js @@ -11,6 +11,7 @@ const NoGlobalImportsChecker = require('./no-global-import') const NoUnusedImportsChecker = require('./no-unused-import') const ExplicitTypesChecker = require('./explicit-types') const OneContractPerFileChecker = require('./one-contract-per-file') +const InterfaceStartsWithIChecker = require('./interface-starts-with-i') module.exports = function checkers(reporter, config, inputSrc, tokens) { return [ @@ -27,5 +28,6 @@ module.exports = function checkers(reporter, config, inputSrc, tokens) { new NoUnusedImportsChecker(reporter, tokens), new ExplicitTypesChecker(reporter, config), new OneContractPerFileChecker(reporter), + new InterfaceStartsWithIChecker(reporter), ] } diff --git a/lib/rules/best-practises/interface-starts-with-i.js b/lib/rules/best-practises/interface-starts-with-i.js new file mode 100644 index 00000000..9c7fafe3 --- /dev/null +++ b/lib/rules/best-practises/interface-starts-with-i.js @@ -0,0 +1,45 @@ +const BaseChecker = require('../base-checker') + +const ruleId = 'interface-starts-with-i' +const meta = { + type: 'naming', + docs: { + description: 'Solidity Interfaces names should start with an `I`', + category: 'Style Guide Rules', + examples: { + good: [ + { + description: 'Interface name starts with I', + code: `interface IFoo { function foo () external; }`, + }, + ], + bad: [ + { + description: 'Interface name doesnt start with I', + code: `interface Foo { function foo () external; }`, + }, + ], + }, + }, + isDefault: false, + recommended: false, + defaultSetup: 'warning', + schema: [], +} + +class InterfaceStartsWithIChecker extends BaseChecker { + constructor(reporter) { + super(reporter, ruleId, meta) + } + + ContractDefinition(node) { + if (node.kind !== 'interface') return + const interfaceName = node.name + + if (!interfaceName.startsWith('I')) { + this.error(node, `Interface name '${interfaceName}' must start with "I"`) + } + } +} + +module.exports = InterfaceStartsWithIChecker diff --git a/lib/rules/gas-consumption/gas-length-in-loops.js b/lib/rules/gas-consumption/gas-length-in-loops.js new file mode 100644 index 00000000..6fa95207 --- /dev/null +++ b/lib/rules/gas-consumption/gas-length-in-loops.js @@ -0,0 +1,75 @@ +const BaseChecker = require('../base-checker') + +let found +const ruleId = 'gas-length-in-loops' +const meta = { + type: 'gas-consumption', + + docs: { + description: + 'Suggest replacing object.length in a loop condition to avoid calculation on each lap', + category: 'Gas Consumption Rules', + notes: [ + { + note: '[source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (see Array Length Caching)', + }, + ], + }, + + isDefault: false, + recommended: false, + defaultSetup: 'warn', + + schema: null, +} + +class GasLengthInLoops extends BaseChecker { + constructor(reporter) { + super(reporter, ruleId, meta) + } + + checkConditionForMemberAccessLength(node) { + if (found) return // Return early if the condition has already been found + if (typeof node === 'object' && node !== null) { + if (node.type === 'MemberAccess' && node.memberName === 'length') { + found = true // Update the flag if the condition is met + return + } + // Recursively search through all object properties + Object.values(node).forEach((value) => this.checkConditionForMemberAccessLength(value)) + } + } + + DoWhileStatement(node) { + found = false + this.checkConditionForMemberAccessLength(node.condition) + if (found) { + this.reportError(node) + } + } + + WhileStatement(node) { + found = false + this.checkConditionForMemberAccessLength(node.condition) + if (found) { + this.reportError(node) + } + } + + ForStatement(node) { + found = false + this.checkConditionForMemberAccessLength(node.conditionExpression) + if (found) { + this.reportError(node) + } + } + + reportError(node) { + this.error( + node, + `GC: Found [ .length ] property in Loop condition. Suggestion: assign it to a variable` + ) + } +} + +module.exports = GasLengthInLoops diff --git a/lib/rules/gas-consumption/gas-strict-inequalities.js b/lib/rules/gas-consumption/gas-strict-inequalities.js new file mode 100644 index 00000000..d4ed902c --- /dev/null +++ b/lib/rules/gas-consumption/gas-strict-inequalities.js @@ -0,0 +1,46 @@ +const BaseChecker = require('../base-checker') + +const ruleId = 'gas-strict-inequalities' +const meta = { + type: 'gas-consumption', + + docs: { + description: 'Suggest Strict Inequalities over non Strict ones', + category: 'Gas Consumption Rules', + notes: [ + { + note: 'Strict inequality does not always saves gas. It is dependent on the context of the surrounding opcodes', + }, + { + note: '[source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (see Less/Greater Than vs Less/Greater Than or Equal To)', + }, + { + note: '[source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-7b77t) of the rule initiative', + }, + ], + }, + + isDefault: false, + recommended: false, + defaultSetup: 'warn', + + schema: null, +} + +class GasStrictInequalities extends BaseChecker { + constructor(reporter) { + super(reporter, ruleId, meta) + } + + BinaryOperation(node) { + if (node.operator === '>=' || node.operator === '<=') { + this.reportError(node) + } + } + + reportError(node) { + this.error(node, `GC: Non strict inequality found. Try converting to a strict one`) + } +} + +module.exports = GasStrictInequalities diff --git a/lib/rules/gas-consumption/index.js b/lib/rules/gas-consumption/index.js index 7e158e9b..956fce25 100644 --- a/lib/rules/gas-consumption/index.js +++ b/lib/rules/gas-consumption/index.js @@ -4,6 +4,8 @@ const GasIndexedEvents = require('./gas-indexed-events') const GasCalldataParameters = require('./gas-calldata-parameters') const GasIncrementByOne = require('./gas-increment-by-one') const GasStructPacking = require('./gas-struct-packing') +const GasLengthInLoops = require('./gas-length-in-loops') +const GasStrictInequalities = require('./gas-strict-inequalities') const GasNamedReturnValuesChecker = require('./gas-named-return-values') const GasCustomErrorsChecker = require('./gas-custom-errors') @@ -15,6 +17,8 @@ module.exports = function checkers(reporter, config) { new GasCalldataParameters(reporter, config), new GasIncrementByOne(reporter, config), new GasStructPacking(reporter, config), + new GasLengthInLoops(reporter, config), + new GasStrictInequalities(reporter, config), new GasNamedReturnValuesChecker(reporter), new GasCustomErrorsChecker(reporter), ] diff --git a/lib/rules/naming/foundry-test-functions.js b/lib/rules/naming/foundry-test-functions.js index 4a7b3646..c9392d68 100644 --- a/lib/rules/naming/foundry-test-functions.js +++ b/lib/rules/naming/foundry-test-functions.js @@ -2,7 +2,7 @@ const BaseChecker = require('../base-checker') const naming = require('../../common/identifier-naming') const { severityDescription } = require('../../doc/utils') -const DEFAULT_SEVERITY = 'off' +const DEFAULT_SEVERITY = 'warn' const DEFAULT_SKIP_FUNCTIONS = ['setUp'] const ruleId = 'foundry-test-functions' diff --git a/lib/rules/naming/named-parameters-mapping.js b/lib/rules/naming/named-parameters-mapping.js index 4452d0da..c170fa62 100644 --- a/lib/rules/naming/named-parameters-mapping.js +++ b/lib/rules/naming/named-parameters-mapping.js @@ -56,7 +56,7 @@ const meta = { }, isDefault: false, recommended: false, - defaultSetup: 'off', + defaultSetup: 'warn', schema: null, } diff --git a/solhint.js b/solhint.js index 484c643f..755a910c 100755 --- a/solhint.js +++ b/solhint.js @@ -28,7 +28,6 @@ function init() { .option('-q, --quiet', 'report errors only - default: false') .option('--ignore-path [file_name]', 'file to use as your .solhintignore') .option('--fix', 'automatically fix problems. Skips fixes in report') - // .option('--fixShow', 'automatically fix problems. Show fixes in report') .option('--noPrompt', 'do not suggest to backup files when any `fix` option is selected') .option('--init', 'create configuration file for solhint') .option('--disc', 'do not check for solhint updates') @@ -81,7 +80,6 @@ function askUserToContinue(callback) { } function execMainAction() { - // if ((program.opts().fix || program.opts().fixShow) && !program.opts().noPrompt) { if (program.opts().fix && !program.opts().noPrompt) { askUserToContinue((userAnswer) => { if (userAnswer !== 'y') { @@ -162,7 +160,8 @@ function executeMainActionLogic() { printReports(reports, formatterFn) - exitWithCode(reports) + // exitWithCode(reports) + process.exit(0) } function processStdin(options) { @@ -183,8 +182,8 @@ function processStdin(options) { const reports = [report] printReports(reports, formatterFn) - - exitWithCode(reports) + process.exit(0) + // exitWithCode(reports) } function writeSampleConfigFile() { @@ -364,10 +363,10 @@ function listRules() { } } -function exitWithCode(reports) { - const errorsCount = reports.reduce((acc, i) => acc + i.errorCount, 0) - process.exit(errorsCount > 0 ? 1 : 0) -} +// function exitWithCode(reports) { +// const errorsCount = reports.reduce((acc, i) => acc + i.errorCount, 0) +// process.exit(errorsCount > 0 ? 1 : 0) +// } function checkForUpdate() { // eslint-disable-next-line import/no-extraneous-dependencies diff --git a/test/rules/best-practises/interface-starts-with-i.js b/test/rules/best-practises/interface-starts-with-i.js new file mode 100644 index 00000000..7f30d789 --- /dev/null +++ b/test/rules/best-practises/interface-starts-with-i.js @@ -0,0 +1,23 @@ +const assert = require('assert') +const { processStr } = require('../../../lib/index') + +const config = { + rules: { 'interface-starts-with-i': 'error' }, +} + +describe('Linter - interface-starts-with-i', () => { + it('should raise error for interface not starting with I', () => { + const code = 'interface Foo {}' + const report = processStr(code, config) + + assert.equal(report.errorCount, 1) + assert.ok(report.messages[0].message === `Interface name 'Foo' must start with "I"`) + }) + + it('should not raise error for interface starting with I', () => { + const code = 'interface IFoo {}' + + const report = processStr(code, config) + assert.equal(report.errorCount, 0) + }) +}) diff --git a/test/rules/gas-consumption/gas-length-in-loops.js b/test/rules/gas-consumption/gas-length-in-loops.js new file mode 100644 index 00000000..37fe451f --- /dev/null +++ b/test/rules/gas-consumption/gas-length-in-loops.js @@ -0,0 +1,129 @@ +const assert = require('assert') +const linter = require('../../../lib/index') +const funcWith = require('../../common/contract-builder').funcWith + +const ERROR_MSG = + 'GC: Found [ .length ] property in Loop condition. Suggestion: assign it to a variable' + +describe('Linter - gas-length-in-loops', () => { + it('should raise error on ForLoop with .length in condition', () => { + const code = funcWith(` + for (uint256 length = 0; length > object.length; legnth++) { + // code block to be executed + }`) + + const report = linter.processStr(code, { + rules: { 'gas-length-in-loops': 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.equal(report.messages[0].message, ERROR_MSG) + }) + + it('should raise error on While with .length in condition', () => { + const code = funcWith(` + while (condition + 1 && boolIsTrue && arr.length > i) { + // code block to be executed + arr.length.push(1); + }`) + + const report = linter.processStr(code, { + rules: { 'gas-length-in-loops': 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.equal(report.messages[0].message, ERROR_MSG) + }) + + it('should raise error on DoWhile with .length in condition', () => { + const code = funcWith(` + do { + // code block to be executed + } while (condition[5].member > 35 && length && arr.length < counter); + `) + + const report = linter.processStr(code, { + rules: { 'gas-length-in-loops': 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.equal(report.messages[0].message, ERROR_MSG) + }) + + it('should raise error on DoWhile and While with .length in condition', () => { + const code = funcWith(` + for (uint256 length = 0; condition; length++) { + // code block to be executed + } + + while (condition + 1 && boolIsTrue && arr.length > i) { + // code block to be executed + arr.length.push(1); + } + + do { + // code block to be executed + } while (condition[5].member > 35 && length && arr.length < counter); + `) + + const report = linter.processStr(code, { + rules: { 'gas-length-in-loops': 'error' }, + }) + + assert.equal(report.errorCount, 2) + assert.equal(report.messages[0].message, ERROR_MSG) + assert.equal(report.messages[1].message, ERROR_MSG) + }) + + it('should NOT raise error on ForLoop with none .length in condition', () => { + const code = funcWith(` + for (initialization; condition; iteration) { + // code block to be executed + }`) + + const report = linter.processStr(code, { + rules: { 'gas-length-in-loops': 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) + + it('should NOT raise error on ForLoop with none .length in condition', () => { + const code = funcWith(` + for (uint256 length = 0; condition; length++) { + // code block to be executed + }`) + + const report = linter.processStr(code, { + rules: { 'gas-length-in-loops': 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) + + it('should NOT raise error on While with none .length in condition', () => { + const code = funcWith(` + while (condition) { + // code block to be executed + }`) + + const report = linter.processStr(code, { + rules: { 'gas-length-in-loops': 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) + + it('should NOT raise error on DoWhile with none .length in condition', () => { + const code = funcWith(` + do { + // code block to be executed + } while (condition[5].member > 35 && length);`) + + const report = linter.processStr(code, { + rules: { 'gas-length-in-loops': 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) +}) diff --git a/test/rules/gas-consumption/gas-strict-inequalities.js b/test/rules/gas-consumption/gas-strict-inequalities.js new file mode 100644 index 00000000..c1813efa --- /dev/null +++ b/test/rules/gas-consumption/gas-strict-inequalities.js @@ -0,0 +1,94 @@ +const assert = require('assert') +const linter = require('../../../lib/index') +const { funcWith } = require('../../common/contract-builder') + +const ERROR_MSG = 'GC: Non strict inequality found. Try converting to a strict one' + +describe('Linter - gas-strict-inequalities', () => { + it('should raise error on non strict equalities 1', () => { + const code = funcWith(` + uint256 a; + uint256 b; + uint256 c; + uint256 d; + + if (a >= b) { } + if (c <= d) { } + if (c < d) { } + if (c > d) { }`) + + const report = linter.processStr(code, { + rules: { 'gas-strict-inequalities': 'error' }, + }) + + assert.equal(report.errorCount, 2) + assert.equal(report.messages[0].message, ERROR_MSG) + assert.equal(report.messages[1].message, ERROR_MSG) + }) + + it('should raise error on non strict equalities 2', () => { + const code = funcWith(` + uint256 a; + uint256 b; + uint256 c; + uint256 d; + + while (a >= b) { + + } + + if (c < d) { }`) + + const report = linter.processStr(code, { + rules: { 'gas-strict-inequalities': 'error' }, + }) + + assert.equal(report.errorCount, 1) + assert.equal(report.messages[0].message, ERROR_MSG) + // assert.equal(report.errorCount, 0) + }) + + it('should raise error on non strict equalities 3', () => { + const code = funcWith(` + uint256 a; + uint256 b; + uint256 c; + uint256 d; + + while (a >= b) { + + } + + if ((c < d) && (a <= b) && (d >= a)) { }`) + + const report = linter.processStr(code, { + rules: { 'gas-strict-inequalities': 'error' }, + }) + + assert.equal(report.errorCount, 3) + assert.equal(report.messages[0].message, ERROR_MSG) + assert.equal(report.messages[1].message, ERROR_MSG) + assert.equal(report.messages[2].message, ERROR_MSG) + // assert.equal(report.errorCount, 0) + }) + + it('should NOT raise error on strict equalities', () => { + const code = funcWith(` + uint256 a; + uint256 b; + uint256 c; + uint256 d; + + while (a > b) { + + } + + if ((c < d) && (a < b) && (d > a)) { }`) + + const report = linter.processStr(code, { + rules: { 'gas-strict-inequalities': 'error' }, + }) + + assert.equal(report.errorCount, 0) + }) +}) diff --git a/test/rules/naming/func-named-parameters.js b/test/rules/naming/func-named-parameters.js index 9591e434..7aeef066 100644 --- a/test/rules/naming/func-named-parameters.js +++ b/test/rules/naming/func-named-parameters.js @@ -79,7 +79,12 @@ describe('Linter - func-named-parameters', () => { const report = linter.processStr(code, { extends: 'solhint:all', - rules: { 'compiler-version': 'off', 'comprehensive-interface': 'off' }, + rules: { + 'compiler-version': 'off', + 'comprehensive-interface': 'off', + 'foundry-test-functions': 'off', + 'non-state-vars-leading-underscore': 'off', + }, }) assertWarnsCount(report, 1)