New rule `func-named-parameters` enforced on built-in functions
pcaversaccio opened this issue · 9 comments
As reported here, this new rule is enforced also on the built-in functions like keccak256, abi.encode, abi.encodePacked, bytes20 etc. for which you can't use named parameters.
Example:
modifier guard(bytes32 salt) {
salt = keccak256(abi.encode(msg.sender, block.chainid, salt));
_;
}will result in:
warning Missing named parameters. Max unnamed parameters value is 2 func-named-parametersMy recommendation is to remove this rule on the built-in functions.
Update: It's even enforced on the Yul functions like add etc.
ohh !
good catch @pcaversaccio thanks a lot for the collaboration
will try to fix it asap
@pcaversaccio
Looking at this functionality, if we configure solhint like this:
{ rules: { 'func-named-parameters': ['error', 0] }, }
Which means ALL parameters should have naming
OR ['error', 1] or ['error', 2]
There are a lot of false positive, even in the simplest statements such as address(0);
Seems there is no easy way by searching in the AST code to determine if a function a built in or not.
To remove builtin function from this rule, we should have an external file with all those functions (list) or something similar to be maintained and updated over time...
And even doing so, there will be false positives on functions used by libraries and such.
So I think our best shot is to keep the rule as it is but make number 3 the minimum setting... so 1 or 2 unnamed parameters will be ok. This will cover almost every builtin function... Of course there will be some false positives as well...
Yeah, that's correct. I also reproduced this. Well, an issue I can see with this solution is that for instance if you use Foundry as a testing framework, you have a lot of cheatcodes that assume more than 2 unnamed parameters, like vm.expectEmit or many events will have 3 params. Also, abi.encode or abi.encodePacked will be an issue. Just think about how you build an EIP-712 domain separator. My personal opinion on the current implementation is that it should not be part of the recommended rules.
@pcaversaccio
Ok, Yes, I agree with you. Thanks a lot for your input.
So I think I will:
- Definitely remove the rule from recommended
- Increase the DEFAULT_MAX_UNNAMED_ARGUMENTS = 4 (meaning, prevent lower values than that)
- Put some info about false positives when using this rule
What do you think ?
@pcaversaccio Ok, Yes, I agree with you. Thanks a lot for your input. So I think I will:
- Definitely remove the rule from recommended
- Increase the DEFAULT_MAX_UNNAMED_ARGUMENTS = 4 (meaning, prevent lower values than that)
- Put some info about false positives when using this rule
What do you think ?
Sounds good to me.
fixed in
#472
Are you gonna make a new release?
yes, I'm not sure exactly when... but at most, this friday there will be a new one
for the time being i think you can use it with a larger value and it will work with few false positives
Hey man, thanks a lot for your help. I really appreciate!
cool, sounds like an appropriate plan! thx for the swift response.