Skip to content

Commit 3a70984

Browse files
srawlinsCommit Queue
srawlins
authored and
Commit Queue
committed
DAS: In the 'add cast' fix, tidy and take better advantage of promotion
The motivation here was to remove the local duplicate variable, `target_final`. This variable only existed because the variable it duplicates, `target`, loses its promoted type inside the `builder.addDartFileEdit` closure. It loses it's promoted type because it is multiply assigned. This CL extracts the first half of the `compute()` method, which is solely concerned with computing the real "target" and "from type" and "to type" into a separate method. The diff looks large because of how the first half of `compute()` is moved into a helper method that is _below_ the second half of `compute()`. I think each of these changes then improves readability: extracting part of a 140-line long method into one helper, and avoiding a local duplicate variable. Change-Id: I543bf39e17f0e44011da8741b5f12380e1425f3f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416883 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Samuel Rawlins <[email protected]>
1 parent 0b6df4d commit 3a70984

File tree

2 files changed

+114
-107
lines changed

2 files changed

+114
-107
lines changed

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

Lines changed: 91 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -29,66 +29,13 @@ class AddExplicitCast extends ResolvedCorrectionProducer {
2929

3030
@override
3131
Future<void> compute(ChangeBuilder builder) async {
32-
var target = coveringNode;
33-
if (target is! Expression) {
32+
var targetAndTypes = _computeTargetAndTypes();
33+
if (targetAndTypes == null) {
3434
return;
3535
}
3636

37-
var fromType = target.staticType;
38-
if (fromType == null) {
39-
return;
40-
}
37+
var (:target, :fromType, :toType) = targetAndTypes;
4138

42-
if (fromType == typeProvider.nullType) {
43-
// There would only be a diagnostic if the `toType` is not nullable, in
44-
// which case a cast won't fix the problem.
45-
return;
46-
}
47-
DartType toType;
48-
var parent = target.parent;
49-
if (parent is CascadeExpression && target == parent.target) {
50-
target = parent;
51-
parent = target.parent;
52-
}
53-
if (parent is AssignmentExpression && target == parent.rightHandSide) {
54-
toType = parent.writeType!;
55-
} else if (parent is VariableDeclaration && target == parent.initializer) {
56-
if (parent.declaredElement2 case var declaredElement?) {
57-
toType = declaredElement.type;
58-
} else if (parent.declaredFragment case var declaredFragment?) {
59-
toType = declaredFragment.element.type;
60-
} else {
61-
return;
62-
}
63-
} else if (parent is ArgumentList) {
64-
var staticType = target.correspondingParameter?.type;
65-
if (staticType == null) return;
66-
toType = staticType;
67-
} else {
68-
return;
69-
}
70-
if (typeSystem.isAssignableTo(
71-
toType,
72-
typeSystem.promoteToNonNull(fromType),
73-
strictCasts: analysisOptions.strictCasts,
74-
)) {
75-
// The only reason that `fromType` can't be assigned to `toType` is
76-
// because it's nullable, in which case a cast won't fix the problem.
77-
return;
78-
}
79-
if (target.isToListMethodInvocation || target.isToSetMethodInvocation) {
80-
var targetTarget = (target as MethodInvocation).target;
81-
if (targetTarget != null) {
82-
var targetTargetType = targetTarget.typeOrThrow;
83-
if (targetTargetType.isDartCoreIterable ||
84-
targetTargetType.isDartCoreList ||
85-
targetTargetType.isDartCoreMap ||
86-
targetTargetType.isDartCoreSet) {
87-
target = targetTarget;
88-
fromType = targetTargetType;
89-
}
90-
}
91-
}
9239
if (target is AsExpression) {
9340
var type = target.type;
9441
await builder.addDartFileEdit(file, (builder) {
@@ -99,72 +46,68 @@ class AddExplicitCast extends ResolvedCorrectionProducer {
9946
return;
10047
}
10148

102-
var target_final = target;
103-
10449
var needsParentheses = target.precedence < Precedence.postfix;
10550
if (toType is InterfaceType &&
10651
(fromType.isDartCoreIterable ||
10752
fromType.isDartCoreList ||
10853
fromType.isDartCoreSet) &&
10954
(toType.isDartCoreList || toType.isDartCoreSet)) {
110-
var toType_final = toType;
111-
if (target.isCastMethodInvocation) {
112-
var typeArguments = (target as MethodInvocation).typeArguments;
55+
if (target is MethodInvocation && target.isCastMethodInvocation) {
56+
var typeArguments = target.typeArguments;
11357
if (typeArguments != null) {
11458
await builder.addDartFileEdit(file, (builder) {
115-
_replaceTypeArgument(builder, typeArguments, toType_final, 0);
59+
_replaceTypeArgument(builder, typeArguments, toType, 0);
11660
});
11761
}
11862
return;
11963
}
12064
await builder.addDartFileEdit(file, (builder) {
12165
if (needsParentheses) {
122-
builder.addSimpleInsertion(target_final.offset, '(');
66+
builder.addSimpleInsertion(target.offset, '(');
12367
}
124-
builder.addInsertion(target_final.end, (builder) {
68+
builder.addInsertion(target.end, (builder) {
12569
if (needsParentheses) {
12670
builder.write(')');
12771
}
12872
builder.write('.cast<');
129-
builder.writeType(toType_final.typeArguments[0]);
73+
builder.writeType(toType.typeArguments[0]);
13074
builder.write('>()');
13175
});
13276
});
13377
} else if (fromType.isDartCoreMap &&
13478
toType is InterfaceType &&
13579
toType.isDartCoreMap) {
136-
var toType_final = toType;
137-
if (target.isCastMethodInvocation) {
138-
var typeArguments = (target as MethodInvocation).typeArguments;
80+
if (target is MethodInvocation && target.isCastMethodInvocation) {
81+
var typeArguments = target.typeArguments;
13982
if (typeArguments != null) {
14083
await builder.addDartFileEdit(file, (builder) {
141-
_replaceTypeArgument(builder, typeArguments, toType_final, 0);
142-
_replaceTypeArgument(builder, typeArguments, toType_final, 1);
84+
_replaceTypeArgument(builder, typeArguments, toType, 0);
85+
_replaceTypeArgument(builder, typeArguments, toType, 1);
14386
});
14487
}
14588
return;
14689
}
14790
await builder.addDartFileEdit(file, (builder) {
14891
if (needsParentheses) {
149-
builder.addSimpleInsertion(target_final.offset, '(');
92+
builder.addSimpleInsertion(target.offset, '(');
15093
}
151-
builder.addInsertion(target_final.end, (builder) {
94+
builder.addInsertion(target.end, (builder) {
15295
if (needsParentheses) {
15396
builder.write(')');
15497
}
15598
builder.write('.cast<');
156-
builder.writeType(toType_final.typeArguments[0]);
99+
builder.writeType(toType.typeArguments[0]);
157100
builder.write(', ');
158-
builder.writeType(toType_final.typeArguments[1]);
101+
builder.writeType(toType.typeArguments[1]);
159102
builder.write('>()');
160103
});
161104
});
162105
} else {
163106
await builder.addDartFileEdit(file, (builder) {
164107
if (needsParentheses) {
165-
builder.addSimpleInsertion(target_final.offset, '(');
108+
builder.addSimpleInsertion(target.offset, '(');
166109
}
167-
builder.addInsertion(target_final.end, (builder) {
110+
builder.addInsertion(target.end, (builder) {
168111
if (needsParentheses) {
169112
builder.write(')');
170113
}
@@ -175,6 +118,78 @@ class AddExplicitCast extends ResolvedCorrectionProducer {
175118
}
176119
}
177120

121+
/// Computes the target [Expression], the "from" [DartType], and the "to"
122+
/// [DartType] for various types of casts.
123+
///
124+
/// A `null` return value means these values cannot be computed, and a
125+
/// correction cannot be made.
126+
({Expression target, DartType fromType, DartType toType})?
127+
_computeTargetAndTypes() {
128+
var target = coveringNode;
129+
if (target is! Expression) {
130+
return null;
131+
}
132+
133+
var fromType = target.staticType;
134+
if (fromType == null) {
135+
return null;
136+
}
137+
138+
if (fromType == typeProvider.nullType) {
139+
// There would only be a diagnostic if the `toType` is not nullable, in
140+
// which case a cast won't fix the problem.
141+
return null;
142+
}
143+
DartType toType;
144+
var parent = target.parent;
145+
if (parent is CascadeExpression && target == parent.target) {
146+
target = parent;
147+
parent = target.parent;
148+
}
149+
if (parent is AssignmentExpression && target == parent.rightHandSide) {
150+
toType = parent.writeType!;
151+
} else if (parent is VariableDeclaration && target == parent.initializer) {
152+
if (parent.declaredElement2 case var declaredElement?) {
153+
toType = declaredElement.type;
154+
} else if (parent.declaredFragment case var declaredFragment?) {
155+
toType = declaredFragment.element.type;
156+
} else {
157+
return null;
158+
}
159+
} else if (parent is ArgumentList) {
160+
var staticType = target.correspondingParameter?.type;
161+
if (staticType == null) return null;
162+
toType = staticType;
163+
} else {
164+
return null;
165+
}
166+
if (typeSystem.isAssignableTo(
167+
toType,
168+
typeSystem.promoteToNonNull(fromType),
169+
strictCasts: analysisOptions.strictCasts,
170+
)) {
171+
// The only reason that `fromType` can't be assigned to `toType` is
172+
// because it's nullable, in which case a cast won't fix the problem.
173+
return null;
174+
}
175+
if (target is MethodInvocation &&
176+
(target.isToListMethodInvocation || target.isToSetMethodInvocation)) {
177+
var targetTarget = target.target;
178+
if (targetTarget != null) {
179+
var targetTargetType = targetTarget.typeOrThrow;
180+
if (targetTargetType.isDartCoreIterable ||
181+
targetTargetType.isDartCoreList ||
182+
targetTargetType.isDartCoreMap ||
183+
targetTargetType.isDartCoreSet) {
184+
target = targetTarget;
185+
fromType = targetTargetType;
186+
}
187+
}
188+
}
189+
190+
return (target: target, fromType: fromType, toType: toType);
191+
}
192+
178193
/// Replace the type argument of [typeArguments] at the specified [index]
179194
/// with the corresponding type argument of [toType].
180195
void _replaceTypeArgument(

‎pkg/analysis_server/lib/src/utilities/extensions/ast.dart‎

Lines changed: 23 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,6 @@ extension DirectiveExtension on Directive {
280280
}
281281
}
282282

283-
284283
/// If [referencedUri] is a [DirectiveUriWithSource], returns the [Source]
285284
/// from it.
286285
Source? get referencedSource {
@@ -307,36 +306,6 @@ extension DirectiveExtension on Directive {
307306
}
308307

309308
extension ExpressionExtension on Expression {
310-
/// Return `true` if this expression is an invocation of the method `cast`
311-
/// from either Iterable`, `List`, `Map`, or `Set`.
312-
bool get isCastMethodInvocation {
313-
if (this case MethodInvocation self) {
314-
var element = self.methodName.element;
315-
return element is MethodElement2 && element.isCastMethod;
316-
}
317-
return false;
318-
}
319-
320-
/// Return `true` if this expression is an invocation of the method `toList`
321-
/// from `Iterable`.
322-
bool get isToListMethodInvocation {
323-
if (this case MethodInvocation self) {
324-
var element = self.methodName.element;
325-
return element is MethodElement2 && element.isToListMethod;
326-
}
327-
return false;
328-
}
329-
330-
/// Return `true` if this expression is an invocation of the method `toSet`
331-
/// from `Iterable`.
332-
bool get isToSetMethodInvocation {
333-
if (this case MethodInvocation self) {
334-
var element = self.methodName.element;
335-
return element is MethodElement2 && element.isToSetMethod;
336-
}
337-
return false;
338-
}
339-
340309
/// Whether this [Expression] should be wrapped with parentheses when we want
341310
/// to use it as operand of a logical and-expression.
342311
bool get shouldWrapParenthesisBeforeAnd {
@@ -364,6 +333,29 @@ extension MethodDeclarationExtension on MethodDeclaration {
364333
}
365334
}
366335

336+
extension MethodInvocationExtension on MethodInvocation {
337+
/// Returns whether this expression is an invocation of the method `cast`
338+
/// from either Iterable`, `List`, `Map`, or `Set`.
339+
bool get isCastMethodInvocation {
340+
var element = methodName.element;
341+
return element is MethodElement2 && element.isCastMethod;
342+
}
343+
344+
/// Returns whether this expression is an invocation of the method `toList`
345+
/// from `Iterable`.
346+
bool get isToListMethodInvocation {
347+
var element = methodName.element;
348+
return element is MethodElement2 && element.isToListMethod;
349+
}
350+
351+
/// Returns whether this expression is an invocation of the method `toSet`
352+
/// from `Iterable`.
353+
bool get isToSetMethodInvocation {
354+
var element = methodName.element;
355+
return element is MethodElement2 && element.isToSetMethod;
356+
}
357+
}
358+
367359
extension NamedTypeExtension on NamedType {
368360
String get qualifiedName {
369361
var importPrefix = this.importPrefix;

0 commit comments

Comments
 (0)