Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added rule avoid_final_parameters #3045

Merged
merged 2 commits into from
Oct 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Guillermo López-Anglada <[email protected]>
Parker Lougheed <[email protected]>
David Martos <[email protected]>
Juan Pablo Paulsen <[email protected]>
Mathieu Payette <[email protected]>
Luke Pighetti <[email protected]>
Sem van Rij <[email protected]>
Michael Joseph Rosenthal <[email protected]>
Expand Down
1 change: 1 addition & 0 deletions example/all.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ linter:
- avoid_equals_and_hash_code_on_mutable_classes
- avoid_escaping_inner_quotes
- avoid_field_initializers_in_const_classes
- avoid_final_parameters
- avoid_function_literals_in_foreach_calls
- avoid_implementing_value_types
- avoid_init_to_null
Expand Down
2 changes: 2 additions & 0 deletions lib/src/rules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import 'rules/avoid_empty_else.dart';
import 'rules/avoid_equals_and_hash_code_on_mutable_classes.dart';
import 'rules/avoid_escaping_inner_quotes.dart';
import 'rules/avoid_field_initializers_in_const_classes.dart';
import 'rules/avoid_final_parameters.dart';
import 'rules/avoid_function_literals_in_foreach_calls.dart';
import 'rules/avoid_implementing_value_types.dart';
import 'rules/avoid_init_to_null.dart';
Expand Down Expand Up @@ -220,6 +221,7 @@ void registerLintRules({bool inTestMode = false}) {
..register(AvoidEmptyElse())
..register(AvoidEscapingInnerQuotes())
..register(AvoidFieldInitializersInConstClasses())
..register(AvoidFinalParameters())
..register(AvoidFunctionLiteralInForeachMethod())
..register(AvoidImplementingValueTypes())
..register(AvoidInitToNull())
Expand Down
103 changes: 103 additions & 0 deletions lib/src/rules/avoid_final_parameters.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';

import '../analyzer.dart';

const _desc = r'Avoid final for parameter declarations';

const _details = r'''

**AVOID** declaring parameters as final.

Declaring parameters as final can lead to unecessarily verbose code, especially
when using the "parameter_assignments" rule.

**GOOD:**
```dart
void badParameter(String label) { // OK
print(label);
}
```

**BAD:**
```dart
void goodParameter(final String label) { // LINT
print(label);
}
```

**GOOD:**
```dart
void badExpression(int value) => print(value); // OK
```

**BAD:**
```dart
void goodExpression(final int value) => print(value); // LINT
```

**GOOD:**
```dart
[1, 4, 6, 8].forEach((value) => print(value + 2)); // OK
```

**BAD:**
```dart
[1, 4, 6, 8].forEach((final value) => print(value + 2)); // LINT
```

''';

class AvoidFinalParameters extends LintRule {
AvoidFinalParameters()
: super(
name: 'avoid_final_parameters',
description: _desc,
details: _details,
group: Group.style);

@override
List<String> get incompatibleRules => const ['prefer_final_parameters'];

@override
void registerNodeProcessors(
NodeLintRegistry registry, LinterContext context) {
var visitor = _Visitor(this);
registry.addConstructorDeclaration(this, visitor);
registry.addFunctionExpression(this, visitor);
registry.addMethodDeclaration(this, visitor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although there are a couple of places where FormalParameterList can appear in the AST that aren't covered here, I believe that final is invalid in those places. As a result, it might be easier today, and require less maintenance in the future, to register to visit FormalParameterList directly than to register to visit the parents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bwilkerson Just to be on the same page, can you provide 1 or 2 small examples of these other places ? To stay as consistent as possible, I made this rule reflect the opposite of prefer_final_parameters. That being said, I wouldn't mind doing slight changes to this PR if I understand them lol.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the course of replying I decided that it's better to leave the code the way you have it. Sorry for the noise.

Nodes of type FormalParameterList appear as a child of the following node types: ConstructorDeclaration, FieldFormalParameter, FunctionExpression, FunctionTypeAlias, FunctionTypedFormalParameter, GenericFunctionType, and MethodDeclaration. The ones that are currently missing are in typedefs, both the old and new syntactic form, and in formal parameters whose type is a function. I believe that in all those locations any use of final is an error. Which means that if you did what I suggested we'd be double reporting in those places, which is something we strive to avoid.

}
}

class _Visitor extends SimpleAstVisitor {
final LintRule rule;

_Visitor(this.rule);

/// Report the lint for parameters in the [parameters] list that are final.
void _reportApplicableParameters(FormalParameterList? parameters) {
if (parameters != null) {
for (var param in parameters.parameters) {
if (param.isFinal) {
rule.reportLint(param);
}
}
}
}

@override
void visitConstructorDeclaration(ConstructorDeclaration node) =>
_reportApplicableParameters(node.parameters);

@override
void visitFunctionExpression(FunctionExpression node) =>
_reportApplicableParameters(node.parameters);

@override
void visitMethodDeclaration(MethodDeclaration node) =>
_reportApplicableParameters(node.parameters);
}
3 changes: 2 additions & 1 deletion lib/src/rules/prefer_final_parameters.dart
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ class PreferFinalParameters extends LintRule {
group: Group.style);

@override
List<String> get incompatibleRules => const ['unnecessary_final'];
List<String> get incompatibleRules =>
const ['unnecessary_final', 'avoid_final_parameters'];

@override
void registerNodeProcessors(
Expand Down
139 changes: 139 additions & 0 deletions test_data/rules/avoid_final_parameters.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

// test w/ `dart test -N avoid_final_parameters`

void badRequiredPositional(final String label) { // LINT
print(label);
}

void goodRequiredPositional(String label) { // OK
print(label);
}

void badOptionalPosition([final String? label]) { // LINT
print(label);
}

void goodOptionalPosition([String? label]) { // OK
print(label);
}

void badRequiredNamed({required final String label}) { // LINT
print(label);
}

void goodRequiredNamed({required String label}) { // OK
print(label);
}

void badOptionalNamed({final String? label}) { // LINT
print(label);
}

void goodOptionalNamed({String? label}) { // OK
print(label);
}

void badExpression(final int value) => print(value); // LINT

void goodExpression(int value) => print(value); // OK

bool? _testingVariable;

void set badSet(final bool setting) => _testingVariable = setting; // LINT

void set goodSet(bool setting) => _testingVariable = setting; // OK

var badClosure = (final Object random) { // LINT
print(random);
};

var goodClosure = (Object random) { // OK
print(random);
};

var _testingList = [1, 7, 15, 20];

void useBadClosureArgument() {
_testingList.forEach((final element) => print(element + 4)); // LINT
}

void useGoodClosureArgument() {
_testingList.forEach((element) => print(element + 4)); // OK
}

void useGoodTypedClosureArgument() {
_testingList.forEach((int element) => print(element + 4)); // OK
}

void badMixedLast(final String bad, String good) { // LINT
print(bad);
print(good);
}

void badMixedFirst(String goodFirst, final String badSecond) { // LINT
print(goodFirst);
print(badSecond);
}

// LINT [+1]
void badMixedMiddle(final String badFirst, String goodSecond, final String badThird) { // LINT
print(badFirst);
print(goodSecond);
print(badThird);
}

void goodMultiple(String bad, String good) { // OK
print(bad);
print(good);
}

class C {
String value = '';
int _contents = 0;

C(final String content) { // LINT
_contents = content.length;
}

C.bad(final int contents) : _contents = contents; // LINT

C.good(int contents) : _contents = contents; // OK

C.badValue(final String value) : this.value = value; // LINT

C.goodValue(this.value); // OK

factory C.goodFactory(String value) { // OK
return C(value);
}

factory C.badFactory(final String value) { // LINT
return C(value);
}

void set badContents(final int contents) => _contents = contents; // LINT
void set goodContents(int contents) => _contents = contents; // OK

int get contentValue => _contents + 4; // OK

void badMethod(final String bad) { // LINT
print(bad);
}

void goodMethod(String good) { // OK
print(good);
}

@override
C operator +(final C other) { // LINT
return C.good(contentValue + other.contentValue);
}

@override
C operator -(C other) { // OK
return C.good(contentValue + other.contentValue);
}
}