Skip to content

Commit f48e7e9

Browse files
stereotype441commit-bot@chromium.org
stereotype441
authored and
[email protected]
committed
Migration: assign better quality node targets in NodeBuilder.
This should help ensure that descriptions of nullabilities that show up in the stack trace refer to parts of the program the user is familiar with, rather than simply saying "explicit type". Change-Id: If53198cc45077a664e44859c7c0770ec9831c765 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/146964 Reviewed-by: Mike Fairhurst <[email protected]>
1 parent 022a6a2 commit f48e7e9

File tree

5 files changed

+340
-57
lines changed

5 files changed

+340
-57
lines changed

‎pkg/nnbd_migration/lib/src/node_builder.dart

Lines changed: 135 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,34 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
6767
this._typeProvider,
6868
{this.instrumentation});
6969

70+
NullabilityNodeTarget get safeTarget {
71+
var target = _target;
72+
if (target != null) return target;
73+
assert(false, 'Unknown nullability node target');
74+
return NullabilityNodeTarget.text('unknown');
75+
}
76+
77+
@override
78+
DecoratedType visitAsExpression(AsExpression node) {
79+
node.expression?.accept(this);
80+
_pushNullabilityNodeTarget(
81+
NullabilityNodeTarget.text('cast type'), () => node.type?.accept(this));
82+
return null;
83+
}
84+
7085
@override
7186
DecoratedType visitCatchClause(CatchClause node) {
72-
DecoratedType exceptionType = node.exceptionType?.accept(this);
87+
var exceptionElement = node.exceptionParameter?.staticElement;
88+
var target = exceptionElement == null
89+
? NullabilityNodeTarget.text('exception type')
90+
: NullabilityNodeTarget.element(exceptionElement);
91+
DecoratedType exceptionType = _pushNullabilityNodeTarget(
92+
target, () => node.exceptionType?.accept(this));
7393
if (node.exceptionParameter != null) {
7494
// If there is no `on Type` part of the catch clause, the type is dynamic.
7595
if (exceptionType == null) {
76-
var target = NullabilityNodeTarget.text('exception').withCodeRef(node);
77-
exceptionType = DecoratedType.forImplicitType(
78-
_typeProvider, _typeProvider.dynamicType, _graph, target);
96+
exceptionType = DecoratedType.forImplicitType(_typeProvider,
97+
_typeProvider.dynamicType, _graph, target.withCodeRef(node));
7998
instrumentation?.implicitType(
8099
source, node.exceptionParameter, exceptionType);
81100
}
@@ -170,14 +189,23 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
170189
return null;
171190
}
172191

192+
@override
193+
DecoratedType visitConstructorName(ConstructorName node) {
194+
_pushNullabilityNodeTarget(NullabilityNodeTarget.text('constructed type'),
195+
() => node.type?.accept(this));
196+
node.name?.accept(this);
197+
return null;
198+
}
199+
173200
@override
174201
DecoratedType visitDeclaredIdentifier(DeclaredIdentifier node) {
175202
node.metadata.accept(this);
176-
DecoratedType type = node.type?.accept(this);
203+
var declaredElement = node.declaredElement;
204+
var target = NullabilityNodeTarget.element(declaredElement);
205+
DecoratedType type =
206+
_pushNullabilityNodeTarget(target, () => node.type?.accept(this));
177207
if (node.identifier != null) {
178208
if (type == null) {
179-
var declaredElement = node.declaredElement;
180-
var target = NullabilityNodeTarget.element(declaredElement);
181209
type = DecoratedType.forImplicitType(
182210
_typeProvider, declaredElement.type, _graph, target);
183211
instrumentation?.implicitType(source, node, type);
@@ -258,7 +286,9 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
258286
DecoratedType visitExtensionDeclaration(ExtensionDeclaration node) {
259287
node.metadata.accept(this);
260288
node.typeParameters?.accept(this);
261-
var type = node.extendedType.accept(this);
289+
var type = _pushNullabilityNodeTarget(
290+
NullabilityNodeTarget.text('extended type'),
291+
() => node.extendedType.accept(this));
262292
_variables.recordDecoratedElementType(node.declaredElement, type);
263293
node.members.accept(this);
264294
return null;
@@ -276,12 +306,10 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
276306
for (var parameter in node.parameters) {
277307
var element = parameter.declaredElement;
278308
NullabilityNodeTarget newTarget;
279-
if (_target == null) {
280-
newTarget = null;
281-
} else if (element.isNamed) {
282-
newTarget = _target.namedParameter(element.name);
309+
if (element.isNamed) {
310+
newTarget = safeTarget.namedParameter(element.name);
283311
} else {
284-
newTarget = _target.positionalParameter(index++);
312+
newTarget = safeTarget.positionalParameter(index++);
285313
}
286314
_pushNullabilityNodeTarget(newTarget, () => parameter.accept(this));
287315
}
@@ -310,6 +338,16 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
310338
return null;
311339
}
312340

341+
@override
342+
DecoratedType visitFunctionExpressionInvocation(
343+
FunctionExpressionInvocation node) {
344+
node.function?.accept(this);
345+
_pushNullabilityNodeTarget(NullabilityNodeTarget.text('type argument'),
346+
() => node.typeArguments?.accept(this));
347+
node.argumentList?.accept(this);
348+
return null;
349+
}
350+
313351
@override
314352
DecoratedType visitFunctionTypeAlias(FunctionTypeAlias node) {
315353
node.metadata.accept(this);
@@ -335,7 +373,7 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
335373
DecoratedType decoratedFunctionType;
336374
try {
337375
node.typeParameters?.accept(this);
338-
node.parameters?.accept(this);
376+
_pushNullabilityNodeTarget(target, () => node.parameters?.accept(this));
339377
// Note: we don't pass _typeFormalBounds into DecoratedType because we're
340378
// not defining a generic function type, we're defining a generic typedef
341379
// of an ordinary (non-generic) function type.
@@ -365,20 +403,31 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
365403
DecoratedType decoratedFunctionType;
366404
node.typeParameters?.accept(this);
367405
var target = NullabilityNodeTarget.element(node.declaredElement);
368-
var returnType = node.functionType.returnType;
369-
if (returnType != null) {
370-
_pushNullabilityNodeTarget(target.returnType(), () {
371-
decoratedFunctionType = node.functionType.accept(this);
372-
});
373-
} else {
406+
_pushNullabilityNodeTarget(target, () {
374407
decoratedFunctionType = node.functionType.accept(this);
375-
}
408+
});
376409
_variables.recordDecoratedElementType(
377410
(node.declaredElement as GenericTypeAliasElement).function,
378411
decoratedFunctionType);
379412
return null;
380413
}
381414

415+
@override
416+
DecoratedType visitIsExpression(IsExpression node) {
417+
node.expression?.accept(this);
418+
_pushNullabilityNodeTarget(NullabilityNodeTarget.text('tested type'),
419+
() => node.type?.accept(this));
420+
return null;
421+
}
422+
423+
@override
424+
DecoratedType visitListLiteral(ListLiteral node) {
425+
_pushNullabilityNodeTarget(NullabilityNodeTarget.text('list element type'),
426+
() => node.typeArguments?.accept(this));
427+
node.elements.accept(this);
428+
return null;
429+
}
430+
382431
@override
383432
DecoratedType visitMethodDeclaration(MethodDeclaration node) {
384433
_handleExecutableDeclaration(
@@ -394,6 +443,16 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
394443
return null;
395444
}
396445

446+
@override
447+
DecoratedType visitMethodInvocation(MethodInvocation node) {
448+
node.target?.accept(this);
449+
node.methodName?.accept(this);
450+
_pushNullabilityNodeTarget(NullabilityNodeTarget.text('type argument'),
451+
() => node.typeArguments?.accept(this));
452+
node.argumentList?.accept(this);
453+
return null;
454+
}
455+
397456
@override
398457
visitMixinDeclaration(MixinDeclaration node) {
399458
node.metadata.accept(this);
@@ -405,6 +464,26 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
405464
return null;
406465
}
407466

467+
@override
468+
DecoratedType visitSetOrMapLiteral(SetOrMapLiteral node) {
469+
var typeArguments = node.typeArguments;
470+
if (typeArguments != null) {
471+
var arguments = typeArguments.arguments;
472+
if (arguments.length == 2) {
473+
_pushNullabilityNodeTarget(NullabilityNodeTarget.text('map key type'),
474+
() => arguments[0].accept(this));
475+
_pushNullabilityNodeTarget(NullabilityNodeTarget.text('map value type'),
476+
() => arguments[1].accept(this));
477+
} else {
478+
_pushNullabilityNodeTarget(
479+
NullabilityNodeTarget.text('set element type'),
480+
() => typeArguments.accept(this));
481+
}
482+
}
483+
node.elements.accept(this);
484+
return null;
485+
}
486+
408487
@override
409488
DecoratedType visitSimpleFormalParameter(SimpleFormalParameter node) {
410489
return _handleFormalParameter(node, node.type, null, null);
@@ -414,8 +493,7 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
414493
DecoratedType visitTypeAnnotation(TypeAnnotation node) {
415494
assert(node != null); // TODO(paulberry)
416495
var type = node.type;
417-
var target = (_target ?? NullabilityNodeTarget.text('explicit type'))
418-
.withCodeRef(node);
496+
var target = safeTarget.withCodeRef(node);
419497
if (type.isVoid || type.isDynamic) {
420498
var nullabilityNode = NullabilityNode.forTypeAnnotation(target);
421499
var decoratedType = DecoratedType(type, nullabilityNode);
@@ -436,8 +514,11 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
436514
.toList();
437515
instrumentation?.implicitTypeArguments(source, node, typeArguments);
438516
} else {
439-
typeArguments =
440-
node.typeArguments.arguments.map((t) => t.accept(this)).toList();
517+
int index = 0;
518+
typeArguments = node.typeArguments.arguments
519+
.map((t) => _pushNullabilityNodeTarget(
520+
target.typeArgument(index++), () => t.accept(this)))
521+
.toList();
441522
}
442523
} else {
443524
assert(false); // TODO(paulberry): is this possible?
@@ -453,7 +534,7 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
453534
// If [_target] is non-null, then it represents the return type for
454535
// a FunctionTypeAlias. Otherwise, create a return type target for
455536
// `target`.
456-
_pushNullabilityNodeTarget(_target ?? target.returnType(), () {
537+
_pushNullabilityNodeTarget(target.returnType(), () {
457538
decoratedReturnType = returnType.accept(this);
458539
});
459540
}
@@ -508,11 +589,12 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
508589
var element = node.declaredElement;
509590
var bound = node.bound;
510591
DecoratedType decoratedBound;
592+
var target = NullabilityNodeTarget.typeParameterBound(element);
511593
if (bound != null) {
512-
decoratedBound = bound.accept(this);
594+
decoratedBound =
595+
_pushNullabilityNodeTarget(target, () => bound.accept(this));
513596
} else {
514-
var nullabilityNode = NullabilityNode.forInferredType(
515-
NullabilityNodeTarget.typeParameterBound(element));
597+
var nullabilityNode = NullabilityNode.forInferredType(target);
516598
decoratedBound = DecoratedType(_typeProvider.objectType, nullabilityNode);
517599
_graph.connect(_graph.always, nullabilityNode,
518600
AlwaysNullableTypeOrigin.forElement(element, false));
@@ -525,7 +607,9 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
525607
DecoratedType visitVariableDeclarationList(VariableDeclarationList node) {
526608
node.metadata.accept(this);
527609
var typeAnnotation = node.type;
528-
var type = typeAnnotation?.accept(this);
610+
var type = _pushNullabilityNodeTarget(
611+
NullabilityNodeTarget.element(node.variables.first.declaredElement),
612+
() => typeAnnotation?.accept(this));
529613
var hint = getPrefixHint(node.firstTokenAfterCommentAndMetadata);
530614
if (hint != null && hint.kind == HintCommentKind.late_) {
531615
_variables.recordLateHint(source, node, hint);
@@ -624,7 +708,7 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
624708
var declaredElement = node.declaredElement;
625709
node.metadata?.accept(this);
626710
DecoratedType decoratedType;
627-
var target = NullabilityNodeTarget.element(declaredElement);
711+
var target = safeTarget;
628712
if (parameters == null) {
629713
if (type != null) {
630714
decoratedType = type.accept(this);
@@ -720,31 +804,34 @@ class NodeBuilder extends GeneralizingAstVisitor<DecoratedType>
720804
supertypes.addAll(onClause.superclassConstraints);
721805
}
722806
var decoratedSupertypes = <ClassElement, DecoratedType>{};
723-
for (var supertype in supertypes) {
724-
DecoratedType decoratedSupertype;
725-
if (supertype == null) {
726-
var target = NullabilityNodeTarget.text('implicit object supertype')
727-
.withCodeRef(astNode);
728-
var nullabilityNode = NullabilityNode.forInferredType(target);
729-
_graph.makeNonNullableUnion(
730-
nullabilityNode, NonNullableObjectSuperclass(source, astNode));
731-
decoratedSupertype =
732-
DecoratedType(_typeProvider.objectType, nullabilityNode);
733-
} else {
734-
decoratedSupertype = supertype.accept(this);
807+
_pushNullabilityNodeTarget(
808+
NullabilityNodeTarget.element(declaredElement).supertype, () {
809+
for (var supertype in supertypes) {
810+
DecoratedType decoratedSupertype;
811+
if (supertype == null) {
812+
var nullabilityNode =
813+
NullabilityNode.forInferredType(_target.withCodeRef(astNode));
814+
_graph.makeNonNullableUnion(
815+
nullabilityNode, NonNullableObjectSuperclass(source, astNode));
816+
decoratedSupertype =
817+
DecoratedType(_typeProvider.objectType, nullabilityNode);
818+
} else {
819+
decoratedSupertype = supertype.accept(this);
820+
}
821+
var class_ = (decoratedSupertype.type as InterfaceType).element;
822+
decoratedSupertypes[class_] = decoratedSupertype;
735823
}
736-
var class_ = (decoratedSupertype.type as InterfaceType).element;
737-
decoratedSupertypes[class_] = decoratedSupertype;
738-
}
824+
});
739825
_variables.recordDecoratedDirectSupertypes(
740826
declaredElement, decoratedSupertypes);
741827
}
742828

743-
void _pushNullabilityNodeTarget(NullabilityNodeTarget target, Function() fn) {
829+
T _pushNullabilityNodeTarget<T>(
830+
NullabilityNodeTarget target, T Function() fn) {
744831
NullabilityNodeTarget previousTarget = _target;
745832
try {
746833
_target = target;
747-
fn();
834+
return fn();
748835
} finally {
749836
_target = previousTarget;
750837
}

‎pkg/nnbd_migration/lib/src/nullability_node_target.dart

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ abstract class NullabilityNodeTarget {
5656
return '$description (${codeReference.shortName})';
5757
}
5858

59+
NullabilityNodeTarget get supertype => _NullabilityNodeTarget_Supertype(this);
60+
5961
/// Creates a new [NullabilityNodeTarget] representing a named function
6062
/// parameter of this target.
6163
NullabilityNodeTarget namedParameter(String name) =>
@@ -158,6 +160,14 @@ class _NullabilityNodeTarget_ReturnType extends _NullabilityNodeTarget_Part {
158160
String get description => 'return type of ${inner.description}';
159161
}
160162

163+
/// Nullability node target representing one of a class's supertypes.
164+
class _NullabilityNodeTarget_Supertype extends _NullabilityNodeTarget_Part {
165+
_NullabilityNodeTarget_Supertype(NullabilityNodeTarget inner) : super(inner);
166+
167+
@override
168+
String get description => 'supertype of ${inner.description}';
169+
}
170+
161171
/// Nullability node target for which we only know a string description.
162172
class _NullabilityNodeTarget_Text extends NullabilityNodeTarget {
163173
final String name;

‎pkg/nnbd_migration/test/front_end/info_builder_test.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1019,7 +1019,7 @@ void f(A a) => a.m = null;
10191019
// Entry 0 is the nullability of the type of A.m.
10201020
// TODO(srawlins): "A" is probably incorrect here. Should be "A.m".
10211021
assertTraceEntry(unit, entries[0], 'A', unit.content.indexOf('int?'),
1022-
contains('explicit type'));
1022+
contains('A.m (test.dart:2:3)'));
10231023
}
10241024

10251025
Future<void> test_removal_handles_offsets_correctly() async {

0 commit comments

Comments
 (0)