Skip to content
This repository was archived by the owner on Jul 16, 2023. It is now read-only.

Commit dea2cc0

Browse files
roman-petrovroman-petrov
authored
feat: add static code diagnostic arguments-ordering (#1047)
1 parent ee83209 commit dea2cc0

File tree

13 files changed

+482
-0
lines changed

13 files changed

+482
-0
lines changed

‎CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# Changelog
22

3+
## Unreleased
4+
5+
* feat: add static code diagnostic [`arguments-ordering`](https://dartcodemetrics.dev/docs/rules/common/arguments-ordering).
6+
37
## 5.0.1
48

59
* fix: correctly available check rule names.

‎lib/src/analyzers/lint_analyzer/rules/rules_factory.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import 'models/rule.dart';
22
import 'rules_list/always_remove_listener/always_remove_listener_rule.dart';
3+
import 'rules_list/arguments_ordering/arguments_ordering_rule.dart';
34
import 'rules_list/avoid_banned_imports/avoid_banned_imports_rule.dart';
45
import 'rules_list/avoid_border_all/avoid_border_all_rule.dart';
56
import 'rules_list/avoid_cascade_after_if_null/avoid_cascade_after_if_null_rule.dart';
@@ -68,6 +69,7 @@ import 'rules_list/tag_name/tag_name_rule.dart';
6869

6970
final _implementedRules = <String, Rule Function(Map<String, Object>)>{
7071
AlwaysRemoveListenerRule.ruleId: AlwaysRemoveListenerRule.new,
72+
ArgumentsOrderingRule.ruleId: ArgumentsOrderingRule.new,
7173
AvoidBannedImportsRule.ruleId: AvoidBannedImportsRule.new,
7274
AvoidBorderAllRule.ruleId: AvoidBorderAllRule.new,
7375
AvoidCascadeAfterIfNullRule.ruleId: AvoidCascadeAfterIfNullRule.new,

‎lib/src/analyzers/lint_analyzer/rules/rules_list/arguments_ordering/arguments_ordering_rule.dart

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// ignore_for_file: public_member_api_docs
2+
3+
import 'package:analyzer/dart/ast/ast.dart';
4+
import 'package:analyzer/dart/ast/visitor.dart';
5+
import 'package:analyzer/dart/element/element.dart';
6+
import 'package:collection/collection.dart';
7+
8+
import '../../../../../utils/node_utils.dart';
9+
import '../../../lint_utils.dart';
10+
import '../../../models/internal_resolved_unit_result.dart';
11+
import '../../../models/issue.dart';
12+
import '../../../models/replacement.dart';
13+
import '../../../models/severity.dart';
14+
import '../../models/common_rule.dart';
15+
import '../../rule_utils.dart';
16+
17+
part 'config_parser.dart';
18+
part 'visitor.dart';
19+
20+
class ArgumentsOrderingRule extends CommonRule {
21+
static const String ruleId = 'arguments-ordering';
22+
23+
static const _warningMessage =
24+
'Named arguments should be sorted to match parameters declaration order.';
25+
static const _replaceComment = 'Sort arguments';
26+
27+
final bool _childLast;
28+
29+
ArgumentsOrderingRule([Map<String, Object> config = const {}])
30+
: _childLast = _ConfigParser.parseChildLast(config),
31+
super(
32+
id: ruleId,
33+
severity: readSeverity(config, Severity.style),
34+
excludes: readExcludes(config),
35+
includes: readIncludes(config),
36+
);
37+
38+
@override
39+
Iterable<Issue> check(InternalResolvedUnitResult source) {
40+
final visitor = _Visitor(childLast: _childLast);
41+
42+
source.unit.visitChildren(visitor);
43+
44+
return visitor.issues
45+
.map(
46+
(issue) => createIssue(
47+
rule: this,
48+
location: nodeLocation(node: issue.argumentList, source: source),
49+
message: _warningMessage,
50+
replacement: Replacement(
51+
comment: _replaceComment,
52+
replacement: issue.replacement,
53+
),
54+
),
55+
)
56+
.toList(growable: false);
57+
}
58+
}

‎lib/src/analyzers/lint_analyzer/rules/rules_list/arguments_ordering/config_parser.dart

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
part of 'arguments_ordering_rule.dart';
2+
3+
class _ConfigParser {
4+
static const _childLast = 'child-last';
5+
static const _childLastDefault = false;
6+
7+
static bool parseChildLast(Map<String, Object> config) =>
8+
config[_childLast] as bool? ?? _childLastDefault;
9+
}

‎lib/src/analyzers/lint_analyzer/rules/rules_list/arguments_ordering/visitor.dart

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
part of 'arguments_ordering_rule.dart';
2+
3+
class _Visitor extends RecursiveAstVisitor<void> {
4+
final bool childLast;
5+
6+
static const _childArg = 'child';
7+
static const _childrenArg = 'children';
8+
static const _childArgs = [_childArg, _childrenArg];
9+
10+
final _issues = <_IssueDetails>[];
11+
12+
Iterable<_IssueDetails> get issues => _issues;
13+
14+
_Visitor({required this.childLast});
15+
16+
@override
17+
void visitInstanceCreationExpression(InstanceCreationExpression node) {
18+
super.visitInstanceCreationExpression(node);
19+
20+
_checkOrder(
21+
node.argumentList,
22+
node.constructorName.staticElement?.parameters ?? [],
23+
);
24+
}
25+
26+
@override
27+
void visitMethodInvocation(MethodInvocation node) {
28+
super.visitMethodInvocation(node);
29+
final staticElement = node.methodName.staticElement;
30+
if (staticElement is FunctionElement) {
31+
_checkOrder(
32+
node.argumentList,
33+
staticElement.parameters,
34+
);
35+
}
36+
}
37+
38+
void _checkOrder(
39+
ArgumentList argumentList,
40+
List<ParameterElement> parameters,
41+
) {
42+
final sortedArguments = argumentList.arguments.sorted((a, b) {
43+
if (a is! NamedExpression && b is! NamedExpression) {
44+
return 0;
45+
}
46+
if (a is NamedExpression && b is! NamedExpression) {
47+
return 1;
48+
}
49+
if (a is! NamedExpression && b is NamedExpression) {
50+
return -1;
51+
}
52+
if (a is NamedExpression && b is NamedExpression) {
53+
final aName = a.name.label.name;
54+
final bName = b.name.label.name;
55+
56+
if (aName == bName) {
57+
return 0;
58+
}
59+
60+
// We use simplified version for "child" argument check from "sort_child_properties_last" rule
61+
// https://github.com/dart-lang/linter/blob/1933b2a2969380e5db35d6aec524fb21b0ed028b/lib/src/rules/sort_child_properties_last.dart#L140
62+
// Hopefully, this will be enough for our current needs.
63+
if (childLast &&
64+
_childArgs.any((name) => name == aName || name == bName)) {
65+
return (_childArgs.contains(aName) && !_childArgs.contains(bName)) ||
66+
(aName == _childArg)
67+
? 1
68+
: -1;
69+
}
70+
71+
return _parameterIndex(parameters, a)
72+
.compareTo(_parameterIndex(parameters, b));
73+
}
74+
75+
return 0;
76+
});
77+
78+
if (argumentList.arguments.toString() != sortedArguments.toString()) {
79+
_issues.add(
80+
_IssueDetails(
81+
argumentList: argumentList,
82+
replacement: '(${sortedArguments.join(', ')})',
83+
),
84+
);
85+
}
86+
}
87+
88+
static int _parameterIndex(
89+
List<ParameterElement> parameters,
90+
NamedExpression argumentExpression,
91+
) =>
92+
parameters.indexWhere(
93+
(parameter) => parameter.name == argumentExpression.name.label.name,
94+
);
95+
}
96+
97+
class _IssueDetails {
98+
const _IssueDetails({
99+
required this.argumentList,
100+
required this.replacement,
101+
});
102+
103+
final ArgumentList argumentList;
104+
final String replacement;
105+
}

‎test/src/analyzers/lint_analyzer/rules/rules_list/arguments_ordering/arguments_ordering_rule_test.dart

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
import 'package:dart_code_metrics/src/analyzers/lint_analyzer/models/severity.dart';
2+
import 'package:dart_code_metrics/src/analyzers/lint_analyzer/rules/rules_list/arguments_ordering/arguments_ordering_rule.dart';
3+
import 'package:test/test.dart';
4+
5+
import '../../../../../helpers/rule_test_helper.dart';
6+
7+
const _classExamplePath = 'arguments_ordering/examples/class_example.dart';
8+
const _functionExamplePath =
9+
'arguments_ordering/examples/function_example.dart';
10+
const _widgetExamplePath = 'arguments_ordering/examples/widget_example.dart';
11+
const _widgetExampleChildLastPath =
12+
'arguments_ordering/examples/widget_example_child_last.dart';
13+
const _mixedExamplePath = 'arguments_ordering/examples/mixed_example.dart';
14+
15+
void main() {
16+
group('ArgumentsOrderingRule', () {
17+
test('initialization', () async {
18+
final unit = await RuleTestHelper.resolveFromFile(_classExamplePath);
19+
final issues = ArgumentsOrderingRule().check(unit);
20+
21+
RuleTestHelper.verifyInitialization(
22+
issues: issues,
23+
ruleId: 'arguments-ordering',
24+
severity: Severity.style,
25+
);
26+
});
27+
28+
test('reports issues with class constructor arguments', () async {
29+
final unit = await RuleTestHelper.resolveFromFile(_classExamplePath);
30+
final issues = ArgumentsOrderingRule().check(unit);
31+
32+
RuleTestHelper.verifyIssues(
33+
issues: issues,
34+
startLines: [16, 17, 18, 19, 20, 21],
35+
startColumns: [26, 26, 26, 26, 26, 26],
36+
locationTexts: [
37+
"(name: 42, surname: '', age: '')",
38+
"(surname: '', name: '', age: 42)",
39+
"(surname: '', age: 42, name: '')",
40+
"(age: 42, surname: '', name: '')",
41+
"(age: 42, name: '', surname: '')",
42+
"(age: 42, name: '')",
43+
],
44+
replacements: [
45+
"(name: 42, age: '', surname: '')",
46+
"(name: '', age: 42, surname: '')",
47+
"(name: '', age: 42, surname: '')",
48+
"(name: '', age: 42, surname: '')",
49+
"(name: '', age: 42, surname: '')",
50+
"(name: '', age: 42)",
51+
],
52+
);
53+
});
54+
55+
test('reports issues with function arguments', () async {
56+
final unit = await RuleTestHelper.resolveFromFile(_functionExamplePath);
57+
final issues = ArgumentsOrderingRule().check(unit);
58+
59+
RuleTestHelper.verifyIssues(
60+
issues: issues,
61+
startLines: [8, 9, 10, 11, 12, 13],
62+
startColumns: [32, 32, 32, 32, 32, 32],
63+
locationTexts: [
64+
"(name: 42, surname: '', age: '')",
65+
"(surname: '', name: '', age: 42)",
66+
"(surname: '', age: 42, name: '')",
67+
"(age: 42, surname: '', name: '')",
68+
"(age: 42, name: '', surname: '')",
69+
"(age: 42, name: '')",
70+
],
71+
replacements: [
72+
"(name: 42, age: '', surname: '')",
73+
"(name: '', age: 42, surname: '')",
74+
"(name: '', age: 42, surname: '')",
75+
"(name: '', age: 42, surname: '')",
76+
"(name: '', age: 42, surname: '')",
77+
"(name: '', age: 42)",
78+
],
79+
);
80+
});
81+
82+
test('reports issues with Flutter widget arguments', () async {
83+
final unit = await RuleTestHelper.resolveFromFile(_widgetExamplePath);
84+
final issues = ArgumentsOrderingRule().check(unit);
85+
86+
RuleTestHelper.verifyIssues(
87+
issues: issues,
88+
startLines: [17, 18, 19, 20, 21],
89+
startColumns: [32, 32, 32, 32, 32],
90+
locationTexts: [
91+
"(name: '', age: 42, child: Container())",
92+
"(child: Container(), name: '', age: 42)",
93+
"(child: Container(), age: 42, name: '')",
94+
"(age: 42, child: Container(), name: '')",
95+
"(age: 42, name: '', child: Container())",
96+
],
97+
replacements: [
98+
"(name: '', child: Container(), age: 42)",
99+
"(name: '', child: Container(), age: 42)",
100+
"(name: '', child: Container(), age: 42)",
101+
"(name: '', child: Container(), age: 42)",
102+
"(name: '', child: Container(), age: 42)",
103+
],
104+
);
105+
});
106+
107+
test(
108+
'reports issues with Flutter widget arguments and "childLast" config option',
109+
() async {
110+
final unit =
111+
await RuleTestHelper.resolveFromFile(_widgetExampleChildLastPath);
112+
final issues = ArgumentsOrderingRule({'child-last': true}).check(unit);
113+
114+
RuleTestHelper.verifyIssues(
115+
issues: issues,
116+
startLines: [15, 18, 20, 22, 24, 26],
117+
startColumns: [32, 17, 17, 17, 17, 17],
118+
locationTexts: [
119+
"(name: '', child: Container(), children: [])",
120+
"(name: '', child: Container(), children: [])",
121+
"(child: Container(), children: [], name: '')",
122+
"(child: Container(), name: '', children: [])",
123+
"(children: [], child: Container(), name: '')",
124+
"(children: [], name: '', child: Container())",
125+
],
126+
replacements: [
127+
"(name: '', children: [], child: Container())",
128+
"(name: '', children: [], child: Container())",
129+
"(name: '', children: [], child: Container())",
130+
"(name: '', children: [], child: Container())",
131+
"(name: '', children: [], child: Container())",
132+
"(name: '', children: [], child: Container())",
133+
],
134+
);
135+
},
136+
);
137+
138+
test('reports issues with mixed (named + unnamed) arguments', () async {
139+
final unit = await RuleTestHelper.resolveFromFile(_mixedExamplePath);
140+
final issues = ArgumentsOrderingRule().check(unit);
141+
142+
RuleTestHelper.verifyIssues(
143+
issues: issues,
144+
startLines: [6, 7, 8, 9, 10],
145+
startColumns: [20, 20, 20, 20, 20],
146+
locationTexts: [
147+
"('a', c: 'c', 'b', d: 'd')",
148+
"('a', d: 'd', 'b', c: 'c')",
149+
"('a', 'b', d: 'd', c: 'c')",
150+
"('a', c: 'c', 'b')",
151+
"(c: 'c', 'a', 'b')",
152+
],
153+
replacements: [
154+
"('a', 'b', c: 'c', d: 'd')",
155+
"('a', 'b', c: 'c', d: 'd')",
156+
"('a', 'b', c: 'c', d: 'd')",
157+
"('a', 'b', c: 'c')",
158+
"('a', 'b', c: 'c')",
159+
],
160+
);
161+
});
162+
});
163+
}

‎test/src/analyzers/lint_analyzer/rules/rules_list/arguments_ordering/examples/class_example.dart

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
class Person {
2+
Person({
3+
required this.name,
4+
required this.age,
5+
this.surname,
6+
});
7+
8+
final String name;
9+
final int age;
10+
final String? surname;
11+
}
12+
13+
final goodPerson1 = Person(name: '', age: 42, surname: '');
14+
final goodPerson2 = Person(name: '', age: 42);
15+
16+
final badPerson1 = Person(name: 42, surname: '', age: ''); // LINT
17+
final badPerson2 = Person(surname: '', name: '', age: 42); // LINT
18+
final badPerson3 = Person(surname: '', age: 42, name: ''); // LINT
19+
final badPerson4 = Person(age: 42, surname: '', name: ''); // LINT
20+
final badPerson5 = Person(age: 42, name: '', surname: ''); // LINT
21+
final badPerson6 = Person(age: 42, name: ''); // LINT

‎test/src/analyzers/lint_analyzer/rules/rules_list/arguments_ordering/examples/function_example.dart

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
Person? createPerson(
2+
{required String name, required int age, String? surname}) =>
3+
null;
4+
5+
final goodPerson1 = createPerson(name: '', age: 42, surname: '');
6+
final goodPerson2 = createPerson(name: '', age: 42);
7+
8+
final badPerson1 = createPerson(name: 42, surname: '', age: ''); // LINT
9+
final badPerson2 = createPerson(surname: '', name: '', age: 42); // LINT
10+
final badPerson3 = createPerson(surname: '', age: 42, name: ''); // LINT
11+
final badPerson4 = createPerson(age: 42, surname: '', name: ''); // LINT
12+
final badPerson5 = createPerson(age: 42, name: '', surname: ''); // LINT
13+
final badPerson6 = createPerson(age: 42, name: ''); // LINT

0 commit comments

Comments
 (0)