linter: Fix unnecessary_parenthesis with list literals
Fixes https://github.com/dart-lang/linter/issues/3046
The diff looks bigger than it is. The gist is:
* There is a check for if the ParenthesizedExpression is immediately
wrapped with parens. Move that check higher.
* There are broad exceptions for ConditionalExpressions and
TypeLiterals being wrapped with parens. Also move those higher.
* Add allowances for wrapped expressions which "directly" contain
whitespace, except for a few cases where the parens are still
considered unnecessary, such as an expression that is an argument.
* Add allowances for PrefixExpressions and PostfixExpressions which
are the "targets" of property access or method invocations.
Cq-Include-Trybots: luci.dart.try:flutter-analyze-try,analyzer-win-release-try,pkg-win-release-try
Change-Id: I808586df874b20b064d4f4a0bd7775b2ec28442a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366891
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
diff --git a/pkg/analyzer/tool/wolf/generate.dart b/pkg/analyzer/tool/wolf/generate.dart
index 79c0c0e..c84d2c9 100644
--- a/pkg/analyzer/tool/wolf/generate.dart
+++ b/pkg/analyzer/tool/wolf/generate.dart
@@ -106,7 +106,7 @@
_addInstruction('yield_', []);
// Invocations and tearoffs
_addInstruction('call',
- [(callDescriptor('callDescriptor')), (argumentNames('argumentNames'))]);
+ [callDescriptor('callDescriptor'), argumentNames('argumentNames')]);
}
_NontrivialEncoding encoding(String type,
diff --git a/pkg/linter/lib/src/rules/unnecessary_parenthesis.dart b/pkg/linter/lib/src/rules/unnecessary_parenthesis.dart
index 669e006..122fab5 100644
--- a/pkg/linter/lib/src/rules/unnecessary_parenthesis.dart
+++ b/pkg/linter/lib/src/rules/unnecessary_parenthesis.dart
@@ -97,11 +97,13 @@
// `[...(p as List)]` is OK.
if (parent is SpreadElement) return;
+ var expression = node.expression;
+
// Don't over-report on records missing trailing commas.
// `(int,) r = (3);` is OK.
if (parent is VariableDeclaration &&
parent.declaredElement?.type is RecordType) {
- if (node.expression is! RecordLiteral) return;
+ if (expression is! RecordLiteral) return;
}
// `g((3)); => g((int,) i) { }` is OK.
if (parent is ArgumentList) {
@@ -113,10 +115,29 @@
// `g(i: (3)); => g({required (int,) i}) { }` is OK.
if (parent is NamedExpression &&
parent.staticParameterElement?.type is RecordType) {
- if (node.expression is! RecordLiteral) return;
+ if (expression is! RecordLiteral) return;
}
- var expression = node.expression;
+ // Directly wrapped into parentheses already - always report.
+ if (parent is ParenthesizedExpression ||
+ parent is InterpolationExpression ||
+ (parent is ArgumentList && parent.arguments.length == 1) ||
+ (parent is IfStatement && node == parent.expression) ||
+ (parent is IfElement && node == parent.expression) ||
+ (parent is WhileStatement && node == parent.condition) ||
+ (parent is DoStatement && node == parent.condition) ||
+ (parent is SwitchStatement && node == parent.expression) ||
+ (parent is SwitchExpression && node == parent.expression)) {
+ rule.reportLint(node);
+ return;
+ }
+
+ // `(foo ? bar : baz)` is OK.
+ if (expression is ConditionalExpression) return;
+
+ // `(List<int>).toString()` is OK.
+ if (expression is TypeLiteral) return;
+
if (expression is SimpleIdentifier ||
expression.containsNullAwareInvocationInChain()) {
if (parent is PropertyAccess) {
@@ -190,25 +211,6 @@
}
}
- // Directly wrapped into parentheses already - always report.
- if (parent is ParenthesizedExpression ||
- parent is InterpolationExpression ||
- (parent is ArgumentList && parent.arguments.length == 1) ||
- (parent is IfStatement && node == parent.expression) ||
- (parent is IfElement && node == parent.expression) ||
- (parent is WhileStatement && node == parent.condition) ||
- (parent is DoStatement && node == parent.condition) ||
- (parent is SwitchStatement && node == parent.expression) ||
- (parent is SwitchExpression && node == parent.expression)) {
- rule.reportLint(node);
- return;
- }
-
- // `(foo ? bar : baz)` is OK.
- if (expression is ConditionalExpression) {
- return;
- }
-
// `a..b = (c..d)` is OK.
if (expression is CascadeExpression ||
node.thisOrAncestorMatching(
@@ -252,11 +254,24 @@
}
}
- // `switch` at the beginning of a statement will be parsed as a
- // switch statement, the parenthesis are required to parse as a switch
- // expression instead.
- if (node.expression is SwitchExpression) {
- if (parent is ExpressionStatement) {
+ // `switch` at the beginning of a statement will be parsed as a switch
+ // statement, the parenthesis are required to parse as a switch expression
+ // instead.
+ if (parent is ExpressionStatement && expression is SwitchExpression) {
+ return;
+ }
+
+ if (expression.directlyContainsWhitespace) {
+ // An expression with internal whitespace can be made more readable when
+ // wrapped in parentheses in many cases. But when the parentheses are
+ // inside one of the following nodes, the readability is not affected.
+ if (parent is! AssignmentExpression &&
+ parent is! ConstructorFieldInitializer &&
+ parent is! VariableDeclaration &&
+ parent is! ExpressionFunctionBody &&
+ parent is! ReturnStatement &&
+ parent is! YieldStatement &&
+ !node.isArgument) {
return;
}
}
@@ -265,53 +280,35 @@
if (parent is BinaryExpression) return;
if (parent is ConditionalExpression) return;
if (parent is CascadeExpression) return;
- if (parent is FunctionExpressionInvocation) {
- if (expression is PrefixedIdentifier) {
- rule.reportLint(node);
- }
+ if (parent is FunctionExpressionInvocation &&
+ expression is! PrefixedIdentifier) {
return;
}
if (parent is AsExpression) return;
if (parent is IsExpression) return;
- // A prefix expression (! or -) can have an argument wrapped in
- // "unnecessary" parens if that argument has potentially confusing
- // whitespace after its first token.
- if (parent is PrefixExpression && node.expression.startsWithWhitespace) {
- return;
- }
-
- // Another case of the above exception, something like
- // `!(const [7]).contains(5);`, where the _parent's_ parent is the
- // PrefixExpression.
- if (parent is MethodInvocation) {
- var target = parent.target;
+ if (parent
+ case MethodInvocation(:var target) || PropertyAccess(:var target)) {
+ // Another case of the above exception, something like
+ // `!(const [7]).contains(5);`, where the _parent's_ parent is the
+ // PrefixExpression.
if (parent.parent is PrefixExpression &&
target == node &&
- node.expression.startsWithWhitespace) return;
+ expression.directlyContainsWhitespace) {
+ return;
+ }
+
+ // `(p++).toString()` is OK. `(++p).toString()` is OK.
+ if (expression is PostfixExpression && target == node) return;
+ if (expression is PrefixExpression && target == node) return;
}
// Something like `({1, 2, 3}).forEach(print);`.
// The parens cannot be removed because then the curly brackets are not
// interpreted as a set-or-map literal.
- if (node.wouldBeParsedAsStatementBlock) {
- return;
- }
-
- // TODO(asashour): an API to the AST for better usage
- // Precedence isn't sufficient (e.g. PostfixExpression requires parenthesis)
- if (expression is PropertyAccess ||
- expression is ConstructorReference ||
- expression is PrefixedIdentifier ||
- expression is MethodInvocation ||
- expression is IndexExpression ||
- expression is Literal ||
- parent.precedence < expression.precedence) {
- rule.reportLint(node);
- }
- } else {
- rule.reportLint(node);
+ if (node.wouldBeParsedAsStatementBlock) return;
}
+ rule.reportLint(node);
}
}
@@ -357,26 +354,34 @@
}
extension on Expression? {
- /// Returns whether this "starts" with whitespace.
- ///
- /// That is, is there definitely whitespace after the first token?
- bool get startsWithWhitespace {
+ /// Returns whether this directly contains whitespace.
+ bool get directlyContainsWhitespace {
var self = this;
- return
- // As in, `!(await foo)`.
+ return self is AsExpression ||
+ self is AssignmentExpression ||
self is AwaitExpression ||
- // As in, `!(new Foo())`.
- (self is InstanceCreationExpression && self.keyword != null) ||
- // No TypedLiteral (ListLiteral, MapLiteral, SetLiteral) accepts `-`
- // or `!` as a prefix operator, but this method can be called
- // recursively, so this catches things like
- // `!(const [].contains(42))`.
- (self is TypedLiteral && self.constKeyword != null) ||
- // As in, `!(const List(3).contains(7))`, and chains like
- // `-(new List(3).skip(1).take(3).skip(1).length)`.
- (self is MethodInvocation && self.target.startsWithWhitespace) ||
- // As in, `-(new List(3).length)`, and chains like
- // `-(new List(3).length.bitLength.bitLength)`.
- (self is PropertyAccess && self.target.startsWithWhitespace);
+ self is BinaryExpression ||
+ self is IsExpression ||
+ // As in, `!(new Foo())`.
+ (self is InstanceCreationExpression && self.keyword != null) ||
+ // No TypedLiteral (ListLiteral, MapLiteral, SetLiteral) accepts `-`
+ // or `!` as a prefix operator, but this method can be called
+ // recursively, so this catches things like
+ // `!(const [].contains(42))`.
+ (self is TypedLiteral && self.constKeyword != null) ||
+ // As in, `!(const List(3).contains(7))`, and chains like
+ // `-(new List(3).skip(1).take(3).skip(1).length)`.
+ (self is MethodInvocation && self.target.directlyContainsWhitespace) ||
+ // As in, `-(new List(3).length)`, and chains like
+ // `-(new List(3).length.bitLength.bitLength)`.
+ (self is PropertyAccess && self.target.directlyContainsWhitespace);
}
}
+
+extension on Expression {
+ /// Whether this expression is directly inside an argument list or the
+ /// expression of a named argument.
+ bool get isArgument =>
+ parent is ArgumentList ||
+ (parent is NamedExpression && parent?.parent is ArgumentList);
+}
diff --git a/pkg/linter/test/rules/unnecessary_parenthesis_test.dart b/pkg/linter/test/rules/unnecessary_parenthesis_test.dart
index ce79949..63536bb 100644
--- a/pkg/linter/test/rules/unnecessary_parenthesis_test.dart
+++ b/pkg/linter/test/rules/unnecessary_parenthesis_test.dart
@@ -17,6 +17,116 @@
@override
String get lintRule => 'unnecessary_parenthesis';
+ test_asExpressionInside_targetOfIndexAssignmentExpression() async {
+ await assertNoDiagnostics(r'''
+void f(Object o) {
+ (o as List)[7] = 7;
+}
+''');
+ }
+
+ test_asExpressionInside_targetOfIndexExpression() async {
+ await assertNoDiagnostics(r'''
+void f(Object o) {
+ (o as List)[7];
+}
+''');
+ }
+
+ test_asExpressionInside_targetOfMethodInvocation() async {
+ await assertNoDiagnostics(r'''
+void f() {
+ (2 as num).toString();
+}
+''');
+ }
+
+ test_asExpressionInside_targetOfPrefixExpression() async {
+ await assertNoDiagnostics(r'''
+void f(Object o) {
+ !(o as bool);
+}
+''');
+ }
+
+ test_assignmentInside_await() async {
+ await assertNoDiagnostics(r'''
+void f(Future<void> f, Future<void>? g) async {
+ await (g ??= f);
+}
+''');
+ }
+
+ test_binaryExpressionInside_namedArgument() async {
+ await assertDiagnostics(r'''
+void f({required int p}) {
+ f(p: (1 + 3));
+}
+''', [
+ lint(34, 7),
+ ]);
+ }
+
+ test_binaryExpressionInside_positionalArgument() async {
+ await assertDiagnostics(r'''
+void f(int p) {
+ f((1 + 3));
+}
+''', [
+ lint(20, 7),
+ ]);
+ }
+
+ test_binaryExpressionInside_prefixExpression() async {
+ await assertNoDiagnostics(r'''
+var x = ~(1 | 2);
+''');
+ }
+
+ test_binaryExpressionInside_returnExpression() async {
+ await assertDiagnostics(r'''
+bool f() {
+ return (1 > 1);
+}
+''', [
+ lint(20, 7),
+ ]);
+ }
+
+ test_conditionalExpressionInside_argument() async {
+ await assertDiagnostics(r'''
+void f(int p) {
+ print((1 == 1 ? 2 : 3));
+}
+''', [
+ lint(24, 16),
+ ]);
+ }
+
+ test_conditionalExpressionInside_listLiteral() async {
+ await assertNoDiagnostics(r'''
+void f() {
+ [(1 == 1 ? 2 : 3)];
+}
+''');
+ }
+
+ test_conditionalExpressionInside_stringInterpolation() async {
+ await assertDiagnostics(r'''
+void f() {
+ '${(1 == 1 ? 2 : 3)}';
+}
+''', [
+ lint(16, 16),
+ ]);
+ }
+
+ test_conditionalInside_expressionBody() async {
+ await assertNoDiagnostics(r'''
+int f() => (1 == 1 ? 2 : 3);
+''');
+ }
+
/// https://github.com/dart-lang/linter/issues/4060
test_constantPattern() async {
await assertNoDiagnostics(r'''
@@ -105,6 +215,38 @@
''');
}
+ test_equalityInside_expressionBody() async {
+ await assertDiagnostics(r'''
+bool f() => (1 == 1);
+''', [
+ lint(12, 8),
+ ]);
+ }
+
+ test_inListLiteral() async {
+ await assertDiagnostics(r'''
+final items = [1, (DateTime.now())];
+''', [
+ lint(18, 16),
+ ]);
+ }
+
+ test_isExpressionInside_targetOfMethodInvocation() async {
+ await assertNoDiagnostics(r'''
+void f(Object o) {
+ (o is num).toString();
+}
+''');
+ }
+
+ test_notEqualInside_returnExpression() async {
+ await assertNoDiagnostics(r'''
+bool f() {
+ return (1 != 1);
+}
+''');
+ }
+
/// https://github.com/dart-lang/linter/issues/4041
test_nullAware_cascadeAssignment() async {
await assertNoDiagnostics(r'''
@@ -125,6 +267,32 @@
''');
}
+ test_positionalArgument() async {
+ await assertDiagnostics(r'''
+void f() {
+ print((1 + 2));
+}
+''', [
+ lint(19, 7),
+ ]);
+ }
+
+ test_postfixExpressionInside_targetOfMethodInvocation() async {
+ await assertNoDiagnostics(r'''
+void f(int i) {
+ (i++).toString();
+}
+''');
+ }
+
+ test_prefixExpressionInside_targetOfMethodInvocation() async {
+ await assertNoDiagnostics(r'''
+void f(bool b) {
+ (!b).toString();
+}
+''');
+ }
+
test_record_assignment() async {
await assertDiagnostics(r'''
void f() {
@@ -141,7 +309,7 @@
g(i: ((3,)));
}
-void g({required (int,) i}) { }
+void g({required (int,) i}) {}
''', [
lint(18, 6),
]);
@@ -153,7 +321,7 @@
g(((3,)));
}
-void g((int,) i) { }
+void g((int,) i) {}
''', [
lint(15, 6),
]);
@@ -178,7 +346,7 @@
g(i: (3));
}
-void g({required (int,) i}) { }
+void g({required (int,) i}) {}
''', [
error(
CompileTimeErrorCode.RECORD_LITERAL_ONE_POSITIONAL_NO_TRAILING_COMMA,
@@ -195,7 +363,7 @@
g((3));
}
-void g((int,) i) { }
+void g((int,) i) {}
''', [
error(
CompileTimeErrorCode.RECORD_LITERAL_ONE_POSITIONAL_NO_TRAILING_COMMA,
@@ -280,4 +448,28 @@
}
''');
}
+
+ test_typeLiteralInside() async {
+ await assertNoDiagnostics(r'''
+void f() {
+ (List<int>).toString();
+}
+''');
+ }
+
+ test_unaryExpressionInside_targetOfMethodInvocation() async {
+ await assertNoDiagnostics(r'''
+void f(int p) {
+ (p++).toString();
+}
+''');
+ }
+
+ test_unaryExpressionInside_targetOfPropertyAccess() async {
+ await assertNoDiagnostics(r'''
+void f(int p) {
+ (p++).hashCode;
+}
+''');
+ }
}
diff --git a/pkg/linter/test_data/rules/unnecessary_parenthesis.dart b/pkg/linter/test_data/rules/unnecessary_parenthesis.dart
index 528d31e..70bb595 100644
--- a/pkg/linter/test_data/rules/unnecessary_parenthesis.dart
+++ b/pkg/linter/test_data/rules/unnecessary_parenthesis.dart
@@ -39,7 +39,6 @@
(List<int>.filled)(3, 0); // LINT
(List.filled)<int>(3, 0); // OK
var tearoff = (List<int>.filled); // LINT
- (List<int>).toString(); //OK
}
var a, b, c, d;
@@ -53,8 +52,6 @@
// OK because it may be hard to know all of the precedence rules.
if ((a && b) || c && d) true; // OK
(await new Future.value(1)).toString(); // OK
- ('' as String).toString(); // OK
- !(true as bool); // OK
(b - a) as num; // OK
(b - a) is num; // OK
a = (a); // LINT
@@ -67,8 +64,6 @@
(a ?? true) ? true : true; // OK
true ? [] : []
..add(''); // OK
- m(p: (1 + 3)); // LINT
- (a++).toString(); // OK
// OK because it is unobvious where cascades fall in precedence.
a..b = (c..d); // OK
@@ -134,13 +129,8 @@
print(!({"a": "b"}["a"]!.isEmpty)); // LINT
- print((1 + 2)); // LINT
-
- print((1 == 1 ? 2 : 3)); // LINT
print('a'.substring((1 == 1 ? 2 : 3), 4)); // OK
var a1 = (1 == 1 ? 2 : 3); // OK
- print('${(1 == 1 ? 2 : 3)}'); // LINT
- print([(1 == 1 ? 2 : 3)]); // OK
var a2 = (1 == 1); // OK
a2 = (1 == 1); // OK
@@ -152,15 +142,15 @@
var a5 = ((a4.isEmpty), 2); // LINT
var a6 = (1, (2)); // LINT
- withManyArgs((''), false, 1); // LINT
- withManyArgs('', (a4.isEmpty), 1); // LINT
- withManyArgs('', (''.isEmpty), 1); // LINT
- withManyArgs('', false, (1)); // LINT
+ /*withManyArgs((''), false, 1); // LIxNT
+ withManyArgs('', (a4.isEmpty), 1); // LIxNT
+ withManyArgs('', (''.isEmpty), 1); // LIxNT
+ withManyArgs('', false, (1)); // LIxNT
- var a7 = (double.infinity).toString(); // LINT
+ var a7 = (double.infinity).toString(); // LIxNT
var list2 = ["a", null];
- var a8 = (list2.first)!.length; // LINT
+ var a8 = (list2.first)!.length; // LIxNT*/
// Null-aware index expression before `:` needs to be parenthesized to avoid
// being interpreted as a conditional expression.
@@ -170,13 +160,13 @@
void withManyArgs(String a, bool b, int c) {}
-bool testTernaryAndEquality() {
+void testTernaryAndEquality() {
if ((1 == 1 ? true : false)) // LINT
{
- return (1 != 1); // OK
+ //
} else if ((1 == 1 ? true : false)) // LINT
{
- return (1 > 1); // LINT
+ //
}
while ((1 == 1)) // LINT
{
@@ -185,19 +175,14 @@
switch ((5 == 6)) // LINT
{
case true:
- return false;
+ return;
default:
- return true;
+ return;
}
}
-int test2() => (1 == 1 ? 2 : 3); // OK
-bool test3() => (1 == 1); // LINT
-
Invocation? invocation() => null;
-m({p}) => null;
-
bool Function(dynamic) get fn => (x) => x is bool ? x : false;
extension<T> on Set<T> {
