Skip to content

Commit 50bb472

Browse files
srawlinsCommit Queue
srawlins
authored and
Commit Queue
committed
Revert "Revert two recent changes to no_literal_bool_comparisons:"
This reverts commit cb3017c. Reason for revert: Re-landing two commits together. Original change's description: > Revert two recent changes to `no_literal_bool_comparisons`: > > * "[DAS] Broadens the scope of `no_literal_bool_comparisons`" > commit 0541ad3. > * "[DAS] Fixes broken cases for "Convert to boolean expression" fix" > commit 0444838. > > The first commit may have lead to broken customer tests. See > flutter/flutter#172087. > > Change-Id: Iba2c936a32ecf678318fa3aea929787dca046e09 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/440261 > Reviewed-by: Siva Annamalai <[email protected]> > Commit-Queue: Samuel Rawlins <[email protected]> Change-Id: I82594e3a77404dc0a4e22f44dd9f850f05f94ca5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/440500 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Samuel Rawlins <[email protected]>
1 parent 3c60075 commit 50bb472

File tree

5 files changed

+730
-57
lines changed

5 files changed

+730
-57
lines changed

‎pkg/analysis_server/lib/src/services/correction/dart/convert_to_boolean_expression.dart‎

Lines changed: 204 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import 'package:analysis_server/src/services/correction/fix.dart';
66
import 'package:analysis_server_plugin/edit/dart/correction_producer.dart';
77
import 'package:analyzer/dart/ast/ast.dart';
88
import 'package:analyzer/dart/ast/precedence.dart';
9+
import 'package:analyzer/dart/ast/token.dart';
10+
import 'package:analyzer/source/source_range.dart';
911
import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
1012
import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
1113
import 'package:analyzer_plugin/utilities/range_factory.dart';
@@ -27,49 +29,223 @@ class ConvertToBooleanExpression extends ResolvedCorrectionProducer {
2729
Future<void> compute(ChangeBuilder builder) async {
2830
AstNode? node = this.node;
2931
if (node is BooleanLiteral) node = node.parent;
30-
if (node is! BinaryExpression) return;
3132

32-
var rightOperand = node.rightOperand;
33-
var leftOperand = node.leftOperand;
34-
35-
Expression expression;
36-
BooleanLiteral literal;
37-
38-
var deleteRange = range.endEnd(leftOperand, rightOperand);
39-
40-
if (rightOperand is BooleanLiteral) {
41-
literal = rightOperand;
42-
expression = node.leftOperand;
43-
} else if (leftOperand is BooleanLiteral) {
44-
literal = leftOperand;
45-
expression = node.rightOperand;
46-
deleteRange = range.startStart(leftOperand, rightOperand);
47-
} else {
48-
return;
33+
if (node case BinaryExpression(
34+
:var rightOperand,
35+
:var leftOperand,
36+
operator: Token(type: var operator),
37+
)) {
38+
if (rightOperand is BooleanLiteral) {
39+
return await _processBinaryExp(
40+
builder,
41+
node,
42+
operator,
43+
rightOperand,
44+
leftOperand,
45+
currentIsLeft: false,
46+
);
47+
}
48+
if (leftOperand is BooleanLiteral) {
49+
return await _processBinaryExp(
50+
builder,
51+
node,
52+
operator,
53+
leftOperand,
54+
rightOperand,
55+
currentIsLeft: true,
56+
);
57+
}
58+
} else if (node
59+
case ConditionalExpression(
60+
:var condition,
61+
:var thenExpression,
62+
:var elseExpression,
63+
) &&
64+
var conditionalExp) {
65+
await (switch ((thenExpression, elseExpression)) {
66+
(BooleanLiteral then, BooleanLiteral elseExp) => () async {
67+
var equalValues = then.value == elseExp.value;
68+
var rangeStart =
69+
equalValues
70+
// keep `then`
71+
? range.startStart(condition, then)
72+
// keep `condition`
73+
: range.endEnd(condition, then);
74+
// remove ` : elseExp`
75+
var rangeEnd = range.endEnd(then, elseExp);
76+
await _addEdit(
77+
builder,
78+
parensRange: range.node(equalValues ? then : condition),
79+
deleteRanges: [rangeStart, rangeEnd],
80+
negated: !then.value && elseExp.value,
81+
needsParens: !equalValues && condition.needsParens(),
82+
bangBeforeParens: false,
83+
);
84+
}(),
85+
(BooleanLiteral then, Expression elseExp) => () async {
86+
var replaceRange = range.endStart(condition, elseExp);
87+
var operator = then.ifBarElseAmpersand;
88+
await _addEdit(
89+
builder,
90+
parensRange: range.node(conditionalExp.condition),
91+
deleteRanges: const <SourceRange>[],
92+
negated: !then.value,
93+
replace: (replaceRange, ' ${operator.lexeme} '),
94+
needsParens: conditionalExp.condition.needsParens(
95+
then.value ? operator : null,
96+
),
97+
bangBeforeParens: true,
98+
parensRange2:
99+
elseExp.needsParens(operator) ? range.node(elseExp) : null,
100+
);
101+
}(),
102+
(Expression then, BooleanLiteral elseExp) => () async {
103+
var rangeStart = range.endStart(condition, then);
104+
var rangeEnd = range.endEnd(then, elseExp);
105+
var operator = elseExp.ifBarElseAmpersand;
106+
await _addEdit(
107+
builder,
108+
parensRange: range.node(conditionalExp.condition),
109+
deleteRanges: [rangeEnd],
110+
negated: elseExp.value,
111+
replace: (rangeStart, ' ${operator.lexeme} '),
112+
needsParens: conditionalExp.condition.needsParens(
113+
elseExp.value ? null : operator,
114+
),
115+
bangBeforeParens: true,
116+
parensRange2: then.needsParens(operator) ? range.node(then) : null,
117+
);
118+
}(),
119+
(_, _) => null,
120+
});
49121
}
122+
}
50123

51-
var negated = !isPositiveCase(node, literal);
124+
/// This correction producer can act on a variety of code, and various edits
125+
/// might be applied.
126+
///
127+
/// The [deleteRanges] are ranges that should be deleted from the code.
128+
///
129+
/// The [replace] will be normally used to replace part of the expression
130+
/// with a new operator.
131+
///
132+
/// The [parensRange] is related to [needsParens] and [bangBeforeParens]. The
133+
/// `bang` is added only if [negated] is true. It was meant for adding the
134+
/// parens when the `bang` or the new operator given by [replace] makes the
135+
/// precedence of the expression different than the original one.
136+
///
137+
/// The [parensRange2] is meant to be used when converting conditional
138+
/// expressions and should be non-`null` if the second expression needs
139+
/// parentheses considering the new operator added by the given [replace].
140+
Future<void> _addEdit(
141+
ChangeBuilder builder, {
142+
required SourceRange parensRange,
143+
required bool needsParens,
144+
required List<SourceRange> deleteRanges,
145+
required bool negated,
146+
required bool bangBeforeParens,
147+
(SourceRange, String)? replace,
148+
SourceRange? parensRange2,
149+
}) async {
52150
await builder.addDartFileEdit(file, (builder) {
53-
if (negated) {
54-
builder.addSimpleInsertion(expression.offset, '!');
55-
if (expression.needsParens) {
56-
builder.addSimpleInsertion(expression.offset, '(');
57-
builder.addSimpleInsertion(expression.end, ')');
151+
if (bangBeforeParens) {
152+
if (negated) {
153+
builder.addSimpleInsertion(parensRange.offset, TokenType.BANG.lexeme);
154+
}
155+
if (needsParens) {
156+
builder.addSimpleInsertion(parensRange.offset, '(');
157+
builder.addSimpleInsertion(parensRange.end, ')');
158+
}
159+
} else {
160+
if (needsParens) {
161+
builder.addSimpleInsertion(parensRange.offset, '(');
162+
}
163+
if (negated) {
164+
builder.addSimpleInsertion(parensRange.offset, TokenType.BANG.lexeme);
58165
}
166+
if (needsParens) {
167+
builder.addSimpleInsertion(parensRange.end, ')');
168+
}
169+
}
170+
if (replace != null) {
171+
builder.addSimpleReplacement(replace.$1, replace.$2);
172+
}
173+
if (parensRange2 != null) {
174+
builder.addSimpleInsertion(parensRange2.offset, '(');
175+
builder.addSimpleInsertion(parensRange2.end, ')');
176+
}
177+
for (var range in deleteRanges) {
178+
builder.addDeletion(range);
59179
}
60-
builder.addDeletion(deleteRange);
61180
});
62181
}
63182

183+
Future<void> _processBinaryExp(
184+
ChangeBuilder builder,
185+
BinaryExpression binaryExp,
186+
TokenType operator,
187+
BooleanLiteral current,
188+
Expression other, {
189+
required bool currentIsLeft,
190+
}) async {
191+
List<SourceRange> deleteRanges;
192+
SourceRange parensRange;
193+
bool needsParens;
194+
switch (operator) {
195+
case TokenType.BAR || TokenType.BAR_BAR when current.value:
196+
case TokenType.AMPERSAND || TokenType.AMPERSAND_AMPERSAND
197+
when !current.value:
198+
deleteRanges = [
199+
currentIsLeft
200+
? range.endEnd(current, other)
201+
: range.startStart(other, current),
202+
];
203+
parensRange = range.node(current);
204+
needsParens = current.needsParens();
205+
default:
206+
deleteRanges = [
207+
currentIsLeft
208+
? range.startStart(current, other)
209+
: range.endEnd(other, current),
210+
];
211+
parensRange = range.node(other);
212+
needsParens = other.needsParens();
213+
}
214+
await _addEdit(
215+
builder,
216+
negated: !isPositiveCase(binaryExp, current),
217+
parensRange: parensRange,
218+
deleteRanges: deleteRanges,
219+
needsParens: needsParens,
220+
bangBeforeParens: true,
221+
);
222+
}
223+
64224
static bool isPositiveCase(
65225
BinaryExpression expression,
66226
BooleanLiteral literal,
67227
) {
68-
if (expression.operator.lexeme == '==') return literal.value;
69-
return !literal.value;
228+
return switch (expression.operator.type) {
229+
TokenType.BAR ||
230+
TokenType.BAR_BAR ||
231+
TokenType.AMPERSAND ||
232+
TokenType.AMPERSAND_AMPERSAND => true,
233+
TokenType.EQ_EQ => literal.value,
234+
TokenType.BANG_EQ || TokenType.CARET => !literal.value,
235+
_ => throw StateError('Unexpected operator ${expression.operator.type}'),
236+
};
70237
}
71238
}
72239

240+
extension on BooleanLiteral {
241+
TokenType get ifBarElseAmpersand =>
242+
value ? TokenType.BAR_BAR : TokenType.AMPERSAND_AMPERSAND;
243+
}
244+
73245
extension on Expression {
74-
bool get needsParens => precedence <= Precedence.prefix;
246+
bool needsParens([TokenType? tokenType]) =>
247+
precedence <=
248+
(tokenType != null
249+
? Precedence.forTokenType(tokenType)
250+
: Precedence.prefix);
75251
}

‎pkg/analysis_server/test/abstract_single_unit.dart‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ import 'abstract_context.dart';
1818
class AbstractSingleUnitTest extends AbstractContextTest {
1919
bool verifyNoTestUnitErrors = true;
2020

21+
/// Whether the test code should parse carrets as position shorthands.
22+
///
23+
/// Set this to `true` when the test code is using the carret operator.
24+
bool keepCaret = false;
25+
2126
TestCode? _parsedTestCode;
2227
late ParsedUnitResult testParsedResult;
2328
late ResolvedLibraryResult? testLibraryResult;
@@ -38,7 +43,10 @@ class AbstractSingleUnitTest extends AbstractContextTest {
3843

3944
String get testCode => parsedTestCode.code;
4045
set testCode(String value) {
41-
parsedTestCode = TestCode.parse(normalizeSource(value));
46+
parsedTestCode = TestCode.parse(
47+
normalizeSource(value),
48+
positionShorthand: !keepCaret,
49+
);
4250
}
4351

4452
void addTestSource(String code) {

0 commit comments

Comments
 (0)