dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
629 stars 170 forks source link

A lint similar to always_specify_types, for always specifying all parameters of a function/constructor #3593

Open rrousselGit opened 2 years ago

rrousselGit commented 2 years ago

Describe the rule you'd like to see implemented

The idea of this rule is to basically act as if every single parameter has the required keyword. The idea is that as opposed to the required keyword, this lint would be an opinionated decision from the team maintaining using this lint. In a way, this is very similar to that of always_specify_types. Teams enabling this voluntarily accept more verbose code for safer code.

Always passing all parameters of functions can help maintain code by making it explicit that a value isn't specified. For example:

fn(param: null);

vs:

fn();

With the former, you know for sure that this usage doesn't want to pass a value to param. With the later, the intention is less clear. The author of that code could've forgotten to pass the value. Or maybe the reader doesn't know about the existence of param.


While the required keyword exists, due to various constraints, it's not always possible to use it.

For example, the source that is being worked on may be a package. In this scenario, marking all parameters as required would have undesired consequences on the users of the package (such as unnecessary breaking changes, or bothering users who don't like this practice).

Similarly, not all code can be edited to have this required keyword, as some code may be coming from packages/SDK. A codebase using this lint would expect all parameters of said package/SDK to also be "required", yet the implementation wouldn't match that.

Examples

Nothing special to say here. When this lint is enabled, both positional and named parameters would be required, even is marked as optional:

void fn([int? a]) {}
void fn2({int? a}) {}

void main() {
  fn(); // KO, expected 1 parameter but received 0
  fn2(); // KO, expected a named parameter "a" but it is missing
}
rrousselGit commented 2 years ago

Such a feature probably would need an annotation for explicitly marking parameters as optional. Kind of like the @optionalTypeArg

In particular, I'm thinking about copyWith methods.

bwilkerson commented 2 years ago

It seems to me that there are at least two valid reasons to make a parameter optional:

While I see why this lint would be reasonable where the API falls into the first category, it seems like the lint would be nothing but false positives for the second category. I don't know how often the second pattern is used, but it isn't non-zero.

Within the package under development a user could mark truly optional parameters as such (using the suggested annotation). But the user of this lint will have no such recourse when using an API from a different package. I am, therefore, somewhat concerned about the potential number of false positives.

rrousselGit commented 2 years ago

The same logic could be applied to always_specify_types and its @optionalTypeArg I think. It it comes down to that, making a PR should be simple.

And I think it would be legitimate to exclude copyWith by default. Besides copyWith, sentinel values are fairly rare