Skip to content

Commit

Permalink
named-function-parameters rule
Browse files Browse the repository at this point in the history
  • Loading branch information
juanpcapurro committed Jul 7, 2023
1 parent eedaeb4 commit 3f69e41
Show file tree
Hide file tree
Showing 6 changed files with 300 additions and 15 deletions.
29 changes: 15 additions & 14 deletions docs/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,21 @@ title: "Rule Index of Solhint"

## Best Practise Rules

| Rule Id | Error | Recommended |
| ------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- | ----------- |
| [code-complexity](./rules/best-practises/code-complexity.md) | Function has cyclomatic complexity "current" but allowed no more than maxcompl. | |
| [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. | ✔️ |
| [no-console](./rules/best-practises/no-console.md) | No console.log/logInt/logBytesX/logString/etc & No hardhat and forge-std console.sol import statements | ✔️ |
| [no-empty-blocks](./rules/best-practises/no-empty-blocks.md) | Code block has zero statements inside. Some common exceptions apply. | ✔️ |
| [no-global-import](./rules/best-practises/no-global-import.md) | Import statement includes an entire file instead of selected symbols | ✔️ |
| [no-unused-import](./rules/best-practises/no-unused-import.md) | Reports a warning when an imported name is not used | ✔️ |
| [no-unused-vars](./rules/best-practises/no-unused-vars.md) | Variable "name" is unused. | ✔️ |
| [payable-fallback](./rules/best-practises/payable-fallback.md) | When fallback is not payable you will not be able to receive ethers. | ✔️ |
| [reason-string](./rules/best-practises/reason-string.md) | Require or revert statement must have a reason string and check that each reason string is at most N characters long. | ✔️ |
| [constructor-syntax](./rules/best-practises/constructor-syntax.md) | Constructors should use the new constructor keyword. | |
| Rule Id | Error | Recommended |
| ------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- | ----------- |
| [code-complexity](./rules/best-practises/code-complexity.md) | Function has cyclomatic complexity "current" but allowed no more than maxcompl. | |
| [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. | ✔️ |
| [no-console](./rules/best-practises/no-console.md) | No console.log/logInt/logBytesX/logString/etc & No hardhat and forge-std console.sol import statements | ✔️ |
| [no-empty-blocks](./rules/best-practises/no-empty-blocks.md) | Code block has zero statements inside. Some common exceptions apply. | ✔️ |
| [no-global-import](./rules/best-practises/no-global-import.md) | Import statement includes an entire file instead of selected symbols | ✔️ |
| [no-unused-import](./rules/best-practises/no-unused-import.md) | Reports a warning when an imported name is not used | ✔️ |
| [no-unused-vars](./rules/best-practises/no-unused-vars.md) | Variable "name" is unused. | ✔️ |
| [payable-fallback](./rules/best-practises/payable-fallback.md) | When fallback is not payable you will not be able to receive ethers. | ✔️ |
| [reason-string](./rules/best-practises/reason-string.md) | Require or revert statement must have a reason string and check that each reason string is at most N characters long. | ✔️ |
| [constructor-syntax](./rules/best-practises/constructor-syntax.md) | Constructors should use the new constructor keyword. | |
| [named-parameters-function](./rules/naming/named-parameters-function.md) | Enforce using named parameters when invoking a function with more than N arguments | |

## Miscellaneous
Expand Down
86 changes: 86 additions & 0 deletions docs/rules/naming/named-parameters-function.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
---
warning: "This is a dynamically generated file. Do not edit manually."
layout: "default"
title: "named-parameters-function | Solhint"
---

# named-parameters-function
![Category Badge](https://img.shields.io/badge/-Best%20Practise%20Rules-informational)
![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)

## Description
Enforce using named parameters when invoking a function with more than N arguments

## Options
This rule accepts an array of options:

| Index | Description | Default Value |
| ----- | -------------------------------------------------------------------------------------------------------------- | ------------- |
| 0 | Rule severity. Must be one of "error", "warn", "off". | warn |
| 1 | A Number specifying the max amount of arguments a function can have while still allowing positional arguments. | 3 |


### Example Config
```json
{
"rules": {
"named-parameters-function": ["warn",3]
}
}
```


## Examples
### 👍 Examples of **correct** code for this rule

#### Calling a function with few positional arguments

```solidity
foo(10,200)
```

#### Calling a function with few named arguments

```solidity
foo({amount: 10, price: 200})
```

#### Calling a function with many named arguments

```solidity
foo({
amount: 10,
price: 200,
recipient: 0x690B9A9E9aa1C9dB991C7721a92d351Db4FaC990,
token: 0xdac17f958d2ee523a2206206994597c13d831ec7
})
```

### 👎 Examples of **incorrect** code for this rule

#### Calling a function with many positional arguments

```solidity
foo(
10,
200,
0x690B9A9E9aa1C9dB991C7721a92d351Db4FaC990,
0xdac17f958d2ee523a2206206994597c13d831ec7
)
```

#### With a config value of 0, using positional arguments in _any_ capacity

```solidity
foo(10)
```

## Version
This rule is introduced in the latest version.

## Resources
- [Rule source](https://github.com/solhint-community/solhint-community/tree/master/lib/rules/naming/named-parameters-function.js)
- [Document source](https://github.com/solhint-community/solhint-community/tree/master/docs/rules/naming/named-parameters-function.md)
- [Test cases](https://github.com/solhint-community/solhint-community/tree/master/test/rules/naming/named-parameters-function.js)
2 changes: 1 addition & 1 deletion lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ module.exports = {

getNumberByPath(path, defaultValue) {
const configVal = _.get(this, path)
return _.isNumber(configVal) && configVal > 0 ? configVal : defaultValue
return _.isNumber(configVal) ? configVal : defaultValue
},

getBooleanByPath(path, defaultValue) {
Expand Down
2 changes: 2 additions & 0 deletions lib/rules/naming/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const PrivateVarsLeadingUnderscore = require('./private-vars-leading-underscore'
const UseForbiddenNameChecker = require('./use-forbidden-name')
const VarNameMixedcaseChecker = require('./var-name-mixedcase')
const NamedParametersMapping = require('./named-parameters-mapping')
const NamedParametersFunction = require('./named-parameters-function')

module.exports = function checkers(reporter, config) {
return [
Expand All @@ -21,5 +22,6 @@ module.exports = function checkers(reporter, config) {
new UseForbiddenNameChecker(reporter),
new VarNameMixedcaseChecker(reporter),
new NamedParametersMapping(reporter),
new NamedParametersFunction(reporter, config),
]
}
93 changes: 93 additions & 0 deletions lib/rules/naming/named-parameters-function.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
const BaseChecker = require('../base-checker')
const { severityDescription } = require('../../doc/utils')

const DEFAULT_SEVERITY = 'warn'
const DEFAULT_MAX_POSITIONAL_ARGUMENTS = 3

const ruleId = 'named-parameters-function'
const meta = {
type: 'naming',

docs: {
description:
'Enforce using named parameters when invoking a function with more than N arguments',
category: 'Best Practise Rules',
options: [
{
description: severityDescription,
default: DEFAULT_SEVERITY,
},
{
description:
'A Number specifying the max amount of arguments a function can have while still allowing positional arguments.',
default: JSON.stringify(DEFAULT_MAX_POSITIONAL_ARGUMENTS),
},
],
examples: {
good: [
{
description: 'Calling a function with few positional arguments',
code: 'foo(10,200)',
},
{
description: 'Calling a function with few named arguments',
code: 'foo({amount: 10, price: 200})',
},
{
description: 'Calling a function with many named arguments',
code: `
foo({
amount: 10,
price: 200,
recipient: 0x690B9A9E9aa1C9dB991C7721a92d351Db4FaC990,
token: 0xdac17f958d2ee523a2206206994597c13d831ec7
})`,
},
],
bad: [
{
description: 'Calling a function with many positional arguments',
code: `
foo(
10,
200,
0x690B9A9E9aa1C9dB991C7721a92d351Db4FaC990,
0xdac17f958d2ee523a2206206994597c13d831ec7
)`,
},
{
description: 'With a config value of 0, using positional arguments in _any_ capacity',
code: `foo(10)`,
},
],
},
},

isDefault: false,
recommended: false,
defaultSetup: [DEFAULT_SEVERITY, DEFAULT_MAX_POSITIONAL_ARGUMENTS],

schema: { type: 'integer' },
}

class FunctionNamedParametersChecker extends BaseChecker {
constructor(reporter, config) {
super(reporter, ruleId, meta)
this.maxPositionalArguments = config
? config.getNumber(ruleId, DEFAULT_MAX_POSITIONAL_ARGUMENTS)
: DEFAULT_MAX_POSITIONAL_ARGUMENTS
}

FunctionCall(node) {
if (node.names.length === 0) {
if (node.arguments.length > this.maxPositionalArguments) {
this.error(
node,
`Call to function with arity > ${this.maxPositionalArguments} is using positional arguments. Use named arguments instead.`
)
}
}
}
}

module.exports = FunctionNamedParametersChecker
103 changes: 103 additions & 0 deletions test/rules/naming/named-parameters-function.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
const linter = require('../../../lib/index')
const { assertNoWarnings, assertWarnsCount } = require('../../common/asserts')
const { contractWith, multiLine } = require('../../common/contract-builder')

describe('Linter - named-parameters-function', () => {
it('GIVEN a setting of 4, should NOT warn on calling with four positional arguments', () => {
const code = contractWith(
multiLine('function foo(uint a, uint b, uint c, uint d) {}', `function bar (){foo(1,2,3,4);}`)
)

const report = linter.processStr(code, {
rules: { 'named-parameters-function': ['warn', 4] },
})

assertNoWarnings(report)
})

it('GIVEN a default solhint config, THEN the rule is disabled and no errors are reported', () => {
const code = contractWith(
multiLine(
'function foo(uint a, uint b, uint c, uint d) public {}',
`function bar () public {foo(1,2,3,4);}`
)
)

const report = linter.processStr(code, {
extends: 'solhint:recommended',
rules: { 'compiler-version': 'off', 'no-empty-blocks': 'off' },
})

assertNoWarnings(report)
})

it('GIVEN a setting of 0, should NOT warn on calling with no arguments', () => {
const code = contractWith(multiLine('function foo() {}', `function bar (){foo();}`))

const report = linter.processStr(code, {
rules: { 'named-parameters-function': ['warn', 0] },
})

assertNoWarnings(report)
})

it('GIVEN a setting of 0, should warn on calls with one positional argument', () => {
const code = contractWith(multiLine('function foo(uint a) {}', `function bar (){foo(3);}`))

const report = linter.processStr(code, {
rules: { 'named-parameters-function': ['warn', 0] },
})

assertWarnsCount(report, 1)
})

it('GIVEN a setting of 2, should warn on calls with 3 positional argument', () => {
const code = contractWith(
multiLine('function foo(uint a, uint b, uint c) {}', `function bar (){foo(1,2,3);}`)
)

const report = linter.processStr(code, {
rules: { 'named-parameters-function': ['warn', 2] },
})

assertWarnsCount(report, 1)
})

it('GIVEN default settings, should NOT warn on calling with two positional arguments', () => {
const code = contractWith(
multiLine('function foo(uint a, uint b) {}', `function bar (){foo(1,2);}`)
)

const report = linter.processStr(code, {
rules: { 'named-parameters-function': 'warn' },
})

assertNoWarnings(report)
})

it('GIVEN default settings, should warn on calling with four positional arguments', () => {
const code = contractWith(
multiLine('function foo(uint a, uint b, uint c, uint d) {}', `function bar (){foo(1,2,3,4);}`)
)

const report = linter.processStr(code, {
rules: { 'named-parameters-function': 'warn' },
})

assertWarnsCount(report, 1)
})

it('GIVEN default settings, should NOT warn on calling with four named arguments', () => {
const code = contractWith(
multiLine(
'function foo(uint a, uint b, uint c, uint d) {}',
`function bar (){foo({a: 1, b: 2, c:3, d:4});}`
)
)

const report = linter.processStr(code, {
rules: { 'named-parameters-function': 'warn' },
})
assertNoWarnings(report)
})
})

0 comments on commit 3f69e41

Please sign in to comment.