From 82c456c9a7c2c3a254d8de3a3c32722bc0fce699 Mon Sep 17 00:00:00 2001 From: Lasse Herskind <16536249+LHerskind@users.noreply.github.com> Date: Mon, 4 Nov 2024 10:04:31 +0000 Subject: [PATCH] feat: free functions don't need underscore --- docs/rules.md | 48 +++++++++---------- .../naming/private-func-leading-underscore.md | 2 +- .../naming/private-func-leading-underscore.js | 11 +++-- test/common/contract-builder.js | 9 ++++ .../gas-named-return-values.js | 1 + .../func-param-name-leading-underscore.js | 5 +- .../naming/private-func-leading-underscore.js | 12 ++++- 7 files changed, 54 insertions(+), 34 deletions(-) diff --git a/docs/rules.md b/docs/rules.md index a2b948f1..369f8104 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -26,30 +26,30 @@ title: "Rule Index of Solhint" ## 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. | $~~~~~~~~$✔️ | | -| [foundry-test-functions](./rules/naming/foundry-test-functions.md) | Enforce naming convention on functions for Foundry test cases | | | -| [func-name-mixedcase](./rules/naming/func-name-mixedcase.md) | Function name must be in mixedCase. | $~~~~~~~~$✔️ | | -| [func-named-parameters](./rules/naming/func-named-parameters.md) | Enforce named parameters for function calls with 4 or more arguments. This rule may have some false positives | | | -| [func-param-name-leading-underscore](./rules/naming/func-param-name-leading-underscore.md) | Function param name must start a single underscore | | | -| [func-param-name-mixedcase](./rules/naming/func-param-name-mixedcase.md) | Function param name must be in mixedCase. | | | -| [immutable-vars-naming](./rules/naming/immutable-vars-naming.md) | Check Immutable variables. Capitalized SNAKE_CASE or mixedCase depending on configuration. | $~~~~~~~~$✔️ | | -| [imports-order](./rules/naming/imports-order.md) | Order the imports of the contract to follow a certain hierarchy (read "Notes section") | | | -| [modifier-name-mixedcase](./rules/naming/modifier-name-mixedcase.md) | Modifier name must be in mixedCase. | | | -| [named-parameters-mapping](./rules/naming/named-parameters-mapping.md) | Solidity v0.8.18 introduced named parameters on the mappings definition. | | | -| [private-func-leading-underscore](./rules/naming/private-func-leading-underscore.md) | Private and internal function names must start with a single underscore, unless they are in a library. | | | -| [private-vars-leading-underscore](./rules/naming/private-vars-leading-underscore.md) | Non-external functions and state variables should start with a single underscore. Others, shouldn't | | | -| [private-vars-no-leading-underscore](./rules/naming/private-vars-no-leading-underscore.md) | Private and internal variable names must NOT start with a single underscore. | | | -| [use-forbidden-name](./rules/naming/use-forbidden-name.md) | Avoid to use letters 'I', 'l', 'O' as identifiers. | $~~~~~~~~$✔️ | | -| [var-name-mixedcase](./rules/naming/var-name-mixedcase.md) | Variable name must be in mixedCase. (Does not check IMMUTABLES, use immutable-vars-naming) | $~~~~~~~~$✔️ | | -| [func-order](./rules/order/func-order.md) | Function order is incorrect. | | $~~~~~~~$✔️ | -| [imports-on-top](./rules/order/imports-on-top.md) | Import statements must be on top. | $~~~~~~~~$✔️ | | -| [ordering](./rules/order/ordering.md) | Check order of elements in file and inside each contract, according to the style guide. | | | -| [visibility-modifier-order](./rules/order/visibility-modifier-order.md) | Visibility modifier must be first in list of modifiers. | $~~~~~~~~$✔️ | | +| 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. | $~~~~~~~~$✔️ | | +| [foundry-test-functions](./rules/naming/foundry-test-functions.md) | Enforce naming convention on functions for Foundry test cases | | | +| [func-name-mixedcase](./rules/naming/func-name-mixedcase.md) | Function name must be in mixedCase. | $~~~~~~~~$✔️ | | +| [func-named-parameters](./rules/naming/func-named-parameters.md) | Enforce named parameters for function calls with 4 or more arguments. This rule may have some false positives | | | +| [func-param-name-leading-underscore](./rules/naming/func-param-name-leading-underscore.md) | Function param name must start a single underscore | | | +| [func-param-name-mixedcase](./rules/naming/func-param-name-mixedcase.md) | Function param name must be in mixedCase. | | | +| [immutable-vars-naming](./rules/naming/immutable-vars-naming.md) | Check Immutable variables. Capitalized SNAKE_CASE or mixedCase depending on configuration. | $~~~~~~~~$✔️ | | +| [imports-order](./rules/naming/imports-order.md) | Order the imports of the contract to follow a certain hierarchy (read "Notes section") | | | +| [modifier-name-mixedcase](./rules/naming/modifier-name-mixedcase.md) | Modifier name must be in mixedCase. | | | +| [named-parameters-mapping](./rules/naming/named-parameters-mapping.md) | Solidity v0.8.18 introduced named parameters on the mappings definition. | | | +| [private-func-leading-underscore](./rules/naming/private-func-leading-underscore.md) | Private and internal function names must start with a single underscore, unless they are in a library or free. | | | +| [private-vars-leading-underscore](./rules/naming/private-vars-leading-underscore.md) | Non-external functions and state variables should start with a single underscore. Others, shouldn't | | | +| [private-vars-no-leading-underscore](./rules/naming/private-vars-no-leading-underscore.md) | Private and internal variable names must NOT start with a single underscore. | | | +| [use-forbidden-name](./rules/naming/use-forbidden-name.md) | Avoid to use letters 'I', 'l', 'O' as identifiers. | $~~~~~~~~$✔️ | | +| [var-name-mixedcase](./rules/naming/var-name-mixedcase.md) | Variable name must be in mixedCase. (Does not check IMMUTABLES, use immutable-vars-naming) | $~~~~~~~~$✔️ | | +| [func-order](./rules/order/func-order.md) | Function order is incorrect. | | $~~~~~~~$✔️ | +| [imports-on-top](./rules/order/imports-on-top.md) | Import statements must be on top. | $~~~~~~~~$✔️ | | +| [ordering](./rules/order/ordering.md) | Check order of elements in file and inside each contract, according to the style guide. | | | +| [visibility-modifier-order](./rules/order/visibility-modifier-order.md) | Visibility modifier must be first in list of modifiers. | $~~~~~~~~$✔️ | | ## Best Practice Rules diff --git a/docs/rules/naming/private-func-leading-underscore.md b/docs/rules/naming/private-func-leading-underscore.md index 2a232b6a..ce26b72c 100644 --- a/docs/rules/naming/private-func-leading-underscore.md +++ b/docs/rules/naming/private-func-leading-underscore.md @@ -9,7 +9,7 @@ title: "private-func-leading-underscore | Solhint" ![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow) ## Description -Private and internal function names must start with a single underscore, unless they are in a library. +Private and internal function names must start with a single underscore, unless they are in a library or free. ## Options This rule accepts an array of options: diff --git a/lib/rules/naming/private-func-leading-underscore.js b/lib/rules/naming/private-func-leading-underscore.js index b98c9747..d6995da3 100644 --- a/lib/rules/naming/private-func-leading-underscore.js +++ b/lib/rules/naming/private-func-leading-underscore.js @@ -10,7 +10,7 @@ const meta = { docs: { description: - 'Private and internal function names must start with a single underscore, unless they are in a library.', + 'Private and internal function names must start with a single underscore, unless they are in a library or free.', category: 'Style Guide Rules', options: [ { @@ -30,16 +30,17 @@ const meta = { class PrivateFuncLeadingUnderscoreChecker extends BaseChecker { constructor(reporter) { super(reporter, ruleId, meta) + this.inContract = false } ContractDefinition(node) { - if (node.kind === 'library') { - this.inLibrary = true + if (node.kind === 'contract') { + this.inContract = true } } 'ContractDefinition:exit'() { - this.inLibrary = false + this.inContract = false } FunctionDefinition(node) { @@ -49,7 +50,7 @@ class PrivateFuncLeadingUnderscoreChecker extends BaseChecker { const isPrivate = node.visibility === 'private' const isInternal = node.visibility === 'internal' || node.visibility === 'default' - const shouldHaveLeadingUnderscore = !this.inLibrary && (isPrivate || isInternal) + const shouldHaveLeadingUnderscore = this.inContract && (isPrivate || isInternal) this.validateName(node, shouldHaveLeadingUnderscore) } diff --git a/test/common/contract-builder.js b/test/common/contract-builder.js index 794a8e7d..1ae7e60e 100644 --- a/test/common/contract-builder.js +++ b/test/common/contract-builder.js @@ -1,5 +1,13 @@ const { times } = require('lodash') +function freeFunction(code) { + return ` + pragma solidity 0.4.4; + + ${code} + ` +} + function contractWith(code) { return ` pragma solidity 0.4.4; @@ -68,6 +76,7 @@ function repeatLines(line, count) { module.exports = { contractWith, libraryWith, + freeFunction, funcWith, modifierWith, multiLine, diff --git a/test/rules/gas-consumption/gas-named-return-values.js b/test/rules/gas-consumption/gas-named-return-values.js index 878ab09d..969f875c 100644 --- a/test/rules/gas-consumption/gas-named-return-values.js +++ b/test/rules/gas-consumption/gas-named-return-values.js @@ -89,6 +89,7 @@ describe('Linter - gas-named-return-values', () => { 'comprehensive-interface': 'off', 'foundry-test-functions': 'off', 'func-param-name-leading-underscore': 'off', + 'strict-override': 'off', }, }) diff --git a/test/rules/naming/func-param-name-leading-underscore.js b/test/rules/naming/func-param-name-leading-underscore.js index b83b7eff..f4ba4a9f 100644 --- a/test/rules/naming/func-param-name-leading-underscore.js +++ b/test/rules/naming/func-param-name-leading-underscore.js @@ -5,6 +5,7 @@ const { contractWith, libraryWith } = require('../../common/contract-builder') describe('Linter - func-param-name-leading-underscore', () => { const SHOULD_WARN_CASES = [ // warn when param names don't start with _ + contractWith('struct foo { uint bar; }\nfunction foo(foo memory bar) {}'), contractWith('function foo(uint bar) {}'), contractWith('function foo(uint bar) internal {}'), @@ -20,9 +21,7 @@ describe('Linter - func-param-name-leading-underscore', () => { ] const SHOULD_NOT_WARN_CASES = [ - // warn when param names start with _ - - // don't warn when private/internal/default names start with _ + contractWith('struct foo { uint bar; }\nfunction foo(foo memory _bar) {}'), // don't warn when public/external names don't start with _ contractWith('uint public foo;'), diff --git a/test/rules/naming/private-func-leading-underscore.js b/test/rules/naming/private-func-leading-underscore.js index 8526f506..765fed5e 100644 --- a/test/rules/naming/private-func-leading-underscore.js +++ b/test/rules/naming/private-func-leading-underscore.js @@ -1,6 +1,6 @@ const assert = require('assert') const linter = require('../../../lib/index') -const { contractWith, libraryWith } = require('../../common/contract-builder') +const { contractWith, libraryWith, freeFunction } = require('../../common/contract-builder') describe('Linter - private-func-leading-underscore', () => { const SHOULD_WARN_CASES = [ @@ -20,6 +20,12 @@ describe('Linter - private-func-leading-underscore', () => { libraryWith('function _foo() private {}'), libraryWith('function _foo() public {}'), libraryWith('function _foo() external {}'), + + freeFunction('function _foo() {}'), + freeFunction('function _foo() internal {}'), + freeFunction('function _foo() private {}'), + freeFunction('function _foo() public {}'), + freeFunction('function _foo() external {}'), ] const SHOULD_NOT_WARN_CASES = [ @@ -38,6 +44,10 @@ describe('Linter - private-func-leading-underscore', () => { libraryWith('function foo() internal {}'), libraryWith('function foo() private {}'), + // don't warn when internal/private functions in free functions don't start with _ + freeFunction('function foo() internal {}'), + freeFunction('function foo() private {}'), + // don't warn for constructors contractWith('constructor() public {}'),