Skip to content
This repository was archived by the owner on Nov 20, 2024. It is now read-only.

Commit 864b051

Browse files
dikmaxpq
dikmax
authored and
pq
committed
Rule prefer_contains. (#366)
Prefer contains over indexOf() != -1. Closes #364.
1 parent def7844 commit 864b051

File tree

4 files changed

+306
-0
lines changed

4 files changed

+306
-0
lines changed

‎lib/src/rules.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import 'package:linter/src/rules/package_api_docs.dart';
4242
import 'package:linter/src/rules/package_prefixed_library_names.dart';
4343
import 'package:linter/src/rules/parameter_assignments.dart';
4444
import 'package:linter/src/rules/prefer_const_constructors.dart';
45+
import 'package:linter/src/rules/prefer_contains.dart';
4546
import 'package:linter/src/rules/prefer_final_fields.dart';
4647
import 'package:linter/src/rules/prefer_final_locals.dart';
4748
import 'package:linter/src/rules/prefer_is_empty.dart';
@@ -102,6 +103,7 @@ void registerLintRules() {
102103
..register(new PackagePrefixedLibraryNames())
103104
..register(new ParameterAssignments())
104105
..register(new PreferConstConstructors())
106+
..register(new PreferContainsOverIndexOf())
105107
..register(new PreferFinalFields())
106108
..register(new PreferFinalLocals())
107109
..register(new PreferIsEmpty())

‎lib/src/rules/prefer_contains.dart

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
// Copyright (c) 2016, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
library linter.src.rules.prefer_contains;
6+
7+
import 'package:analyzer/analyzer.dart';
8+
import 'package:analyzer/context/declared_variables.dart';
9+
import 'package:analyzer/dart/ast/ast.dart';
10+
import 'package:analyzer/dart/ast/token.dart';
11+
import 'package:analyzer/dart/ast/visitor.dart';
12+
import 'package:analyzer/dart/element/element.dart';
13+
import 'package:analyzer/dart/element/type.dart';
14+
import 'package:analyzer/error/listener.dart';
15+
import 'package:linter/src/analyzer.dart';
16+
import 'package:linter/src/util/dart_type_utilities.dart';
17+
18+
const alwaysFalse =
19+
'Always false because indexOf is always greater or equal -1.';
20+
const alwaysTrue = 'Always true because indexOf is always greater or equal -1.';
21+
22+
const desc = 'Use contains for Lists and Strings.';
23+
const details = '''
24+
**DO NOT** use `.indexOf` to see if a collection contains an element.
25+
26+
The `Iterable` contract does not require that a collection know its length or be
27+
able to provide it in constant time. Calling `.length` just to see if the
28+
collection contains anything can be painfully slow.
29+
30+
Instead, there are faster and more readable getters: `.isEmpty` and
31+
`.isNotEmpty`. Use the one that doesn’t require you to negate the result.
32+
33+
**GOOD:**
34+
```
35+
if (lunchBox.isEmpty) return 'so hungry...';
36+
if (words.isNotEmpty) return words.join(' ');
37+
```
38+
39+
**BAD:**
40+
```
41+
if (lunchBox.length == 0) return 'so hungry...';
42+
if (words.length != 0) return words.join(' ');
43+
```
44+
''';
45+
46+
const useContains = 'Use contains instead of indexOf';
47+
48+
class PreferContainsOverIndexOf extends LintRule {
49+
PreferContainsOverIndexOf()
50+
: super(
51+
name: 'prefer_contains',
52+
description: desc,
53+
details: details,
54+
group: Group.style);
55+
56+
@override
57+
AstVisitor getVisitor() => new _Visitor(this);
58+
59+
void reportLintWithDescription(AstNode node, String description) {
60+
if (node != null) {
61+
reporter.reportErrorForNode(new _LintCode(name, description), node, []);
62+
}
63+
}
64+
}
65+
66+
class _Visitor extends SimpleAstVisitor {
67+
final PreferContainsOverIndexOf rule;
68+
69+
_Visitor(this.rule);
70+
71+
@override
72+
visitSimpleIdentifier(SimpleIdentifier identifier) {
73+
// Should be "indexOf".
74+
final Element propertyElement = identifier.bestElement;
75+
if (propertyElement?.name != 'indexOf') {
76+
return;
77+
}
78+
79+
AstNode indexOfAccess;
80+
InterfaceType type;
81+
82+
final AstNode parent = identifier.parent;
83+
if (parent is MethodInvocation && identifier == parent.methodName) {
84+
indexOfAccess = parent;
85+
if (parent.target?.bestType is! InterfaceType) {
86+
return;
87+
}
88+
type = parent.target?.bestType;
89+
} else {
90+
return;
91+
}
92+
93+
if (!DartTypeUtilities
94+
.implementsAnyInterface(type, <InterfaceTypeDefinition>[
95+
new InterfaceTypeDefinition('Iterable', 'dart.core'),
96+
new InterfaceTypeDefinition('String', 'dart.core'),
97+
])) {
98+
return;
99+
}
100+
101+
// Going up in AST structure to find binary comparison operator for this
102+
// `indexOf` access. Most of the time it will be a parent, but sometimes
103+
// it can be wrapped in parentheses or `as` operator.
104+
AstNode search = indexOfAccess;
105+
while (
106+
search != null && search is Expression && search is! BinaryExpression) {
107+
search = search.parent;
108+
}
109+
110+
if (search is! BinaryExpression) {
111+
return;
112+
}
113+
114+
final BinaryExpression binaryExpression = search;
115+
final Token operator = binaryExpression.operator;
116+
117+
final AnalysisContext context = propertyElement.context;
118+
final TypeProvider typeProvider = context.typeProvider;
119+
final TypeSystem typeSystem = context.typeSystem;
120+
121+
final DeclaredVariables declaredVariables = context.declaredVariables;
122+
123+
// Comparing constants with result of indexOf.
124+
125+
final ConstantVisitor visitor = new ConstantVisitor(
126+
new ConstantEvaluationEngine(typeProvider, declaredVariables,
127+
typeSystem: typeSystem),
128+
new ErrorReporter(
129+
AnalysisErrorListener.NULL_LISTENER, rule.reporter.source));
130+
131+
final DartObjectImpl rightValue = binaryExpression.rightOperand.accept(visitor);
132+
if (rightValue?.type?.name == "int") {
133+
// Constant is on right side of comparison operator
134+
_checkConstant(binaryExpression, rightValue.toIntValue(), operator.type);
135+
return;
136+
}
137+
138+
final DartObjectImpl leftValue = binaryExpression.leftOperand.accept(visitor);
139+
if (leftValue?.type?.name == "int") {
140+
// Constants is on left side of comparison operator
141+
_checkConstant(binaryExpression, leftValue.toIntValue(),
142+
_invertedTokenType(operator.type));
143+
}
144+
}
145+
146+
void _checkConstant(Expression expression, int value, TokenType type) {
147+
if (value == -1) {
148+
if (type == TokenType.EQ_EQ ||
149+
type == TokenType.BANG_EQ ||
150+
type == TokenType.LT_EQ ||
151+
type == TokenType.GT) {
152+
rule.reportLintWithDescription(expression, useContains);
153+
} else if (type == TokenType.LT) {
154+
// indexOf < -1 is always false
155+
rule.reportLintWithDescription(expression, alwaysFalse);
156+
} else if (type == TokenType.GT_EQ) {
157+
// indexOf >= -1 is always true
158+
rule.reportLintWithDescription(expression, alwaysTrue);
159+
}
160+
} else if (value == 0) {
161+
// 'indexOf >= 0' is same as 'contains',
162+
// and 'indexOf < 0' is same as '!contains'
163+
if (type == TokenType.GT_EQ || type == TokenType.LT) {
164+
rule.reportLintWithDescription(expression, useContains);
165+
}
166+
} else if (value < -1) {
167+
// 'indexOf' is always >= -1, so comparing with lesser values makes
168+
// no sense.
169+
if (type == TokenType.EQ_EQ ||
170+
type == TokenType.LT_EQ ||
171+
type == TokenType.LT) {
172+
rule.reportLintWithDescription(expression, alwaysFalse);
173+
} else if (type == TokenType.BANG_EQ ||
174+
type == TokenType.GT_EQ ||
175+
type == TokenType.GT) {
176+
rule.reportLintWithDescription(expression, alwaysTrue);
177+
}
178+
}
179+
}
180+
181+
TokenType _invertedTokenType(TokenType type) {
182+
switch (type) {
183+
case TokenType.LT_EQ:
184+
return TokenType.GT_EQ;
185+
186+
case TokenType.LT:
187+
return TokenType.GT;
188+
189+
case TokenType.GT:
190+
return TokenType.LT;
191+
192+
case TokenType.GT_EQ:
193+
return TokenType.LT_EQ;
194+
195+
default:
196+
return type;
197+
}
198+
}
199+
}
200+
201+
// TODO create common MultiMessageLintCode class
202+
class _LintCode extends LintCode {
203+
static final registry = <String, LintCode>{};
204+
205+
factory _LintCode(String name, String message) => registry.putIfAbsent(
206+
name + message, () => new _LintCode._(name, message));
207+
208+
_LintCode._(String name, String message) : super(name, message);
209+
}

‎test/mock_sdk.dart

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ abstract class String extends Object implements Comparable<String> {
4949
bool get isEmpty => false;
5050
bool get isNotEmpty => false;
5151
int get length => 0;
52+
bool contains(String other, [int startIndex = 0]);
53+
int indexOf(String other, [int start]);
5254
String toUpperCase();
5355
List<int> get codeUnits;
5456
}
@@ -94,6 +96,7 @@ class Iterator<E> {
9496
9597
abstract class Iterable<E> {
9698
Iterator<E> get iterator;
99+
bool contains(Object element);
97100
bool get isEmpty;
98101
bool get isNotEmpty;
99102
E get first;
@@ -107,6 +110,7 @@ abstract class List<E> implements Iterable<E> {
107110
void operator []=(int index, E value);
108111
Iterator<E> get iterator => null;
109112
void clear();
113+
int indexOf(Object element);
110114
bool get isEmpty;
111115
bool get isNotEmpty;
112116
}

‎test/rules/prefer_contains.dart

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
// Copyright (c) 2016, the Dart project authors. Please see the AUTHORS file
2+
3+
// for details. All rights reserved. Use of this source code is governed by a
4+
// BSD-style license that can be found in the LICENSE file.
5+
6+
// test w/ `pub run test -N prefer_contains`
7+
8+
const int MINUS_ONE = -1;
9+
10+
List<int> list = [];
11+
12+
List get getter => [];
13+
14+
typedef List F();
15+
16+
F a() {
17+
return () => [];
18+
}
19+
20+
bool le = list.indexOf(1) > -1; //LINT
21+
22+
bool le2 = [].indexOf(1) > -1; //LINT
23+
24+
bool le3 = ([].indexOf(1) as int) > -1; //LINT
25+
26+
bool le4 = -1 < list.indexOf(1); //LINT
27+
28+
bool le5 = [].indexOf(1) < MINUS_ONE; //LINT
29+
30+
bool le6 = MINUS_ONE < [].indexOf(1); //LINT
31+
32+
bool ge = getter.indexOf(1) != -1; //LINT
33+
34+
bool ce = a()().indexOf(1) == -1; //LINT
35+
36+
bool se = "aaa".indexOf('a') == -1; //LINT
37+
38+
int ser = "aaa".indexOf('a', 2); //OK
39+
40+
bool mixed = list.indexOf(1) + "a".indexOf("ab") > 0; //OK
41+
42+
condition() {
43+
final int a = list.indexOf(1) > -1 ? 2 : 3; //LINT
44+
list..indexOf(1);
45+
}
46+
47+
bool le7 = [].indexOf(1) > 1; //OK
48+
49+
testOperators() {
50+
[].indexOf(1) == -1; // LINT
51+
[].indexOf(1) != -1; // LINT
52+
[].indexOf(1) > -1; // LINT
53+
[].indexOf(1) >= -1; // LINT
54+
[].indexOf(1) < -1; // LINT
55+
[].indexOf(1) <= -1; // LINT
56+
57+
[].indexOf(1) == -2; // LINT
58+
[].indexOf(1) != -2; // LINT
59+
[].indexOf(1) > -2; // LINT
60+
[].indexOf(1) >= -2; // LINT
61+
[].indexOf(1) < -2; // LINT
62+
[].indexOf(1) <= -2; // LINT
63+
64+
[].indexOf(1) == 0; // OK
65+
[].indexOf(1) != 0; // OK
66+
[].indexOf(1) > 0; // OK
67+
[].indexOf(1) >= 0; // LINT
68+
[].indexOf(1) < 0; // LINT
69+
[].indexOf(1) <= 0; // OK
70+
71+
-1 == [].indexOf(1); // LINT
72+
-1 != [].indexOf(1); // LINT
73+
-1 < [].indexOf(1); // LINT
74+
-1 <= [].indexOf(1); // LINT
75+
-1 > [].indexOf(1); // LINT
76+
-1 >= [].indexOf(1); // LINT
77+
78+
-2 == [].indexOf(1); // LINT
79+
-2 != [].indexOf(1); // LINT
80+
-2 < [].indexOf(1); // LINT
81+
-2 <= [].indexOf(1); // LINT
82+
-2 > [].indexOf(1); // LINT
83+
-2 >= [].indexOf(1); // LINT
84+
85+
0 == [].indexOf(1); // OK
86+
0 != [].indexOf(1); // OK
87+
0 < [].indexOf(1); // OK
88+
0 <= [].indexOf(1); // LINT
89+
0 > [].indexOf(1); // LINT
90+
0 >= [].indexOf(1); // OK
91+
}

0 commit comments

Comments
 (0)