Skip to content

Commit

Permalink
Migration: add support for dynamic method dispatches.
Browse files Browse the repository at this point in the history
Should address 191 migration tool exceptions with this line in their
stacktrace:

EdgeBuilder.visitMethodInvocation (package:nnbd_migration/src/edge_builder.dart:849:7)

Change-Id: I0e29331a11c2078fea353f7511bb8f77765897d1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/114582
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
  • Loading branch information
stereotype441 authored and commit-bot@chromium.org committed Aug 27, 2019
1 parent c7ac27b commit c604e58
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 4 deletions.
14 changes: 10 additions & 4 deletions pkg/nnbd_migration/lib/src/edge_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
/// For convenience, a [DecoratedType] representing `Null`.
final DecoratedType _nullType;

/// For convenience, a [DecoratedType] representing `dynamic`.
final DecoratedType _dynamicType;

/// The [DecoratedType] of the innermost function or method being visited, or
/// `null` if the visitor is not inside any function or method.
///
Expand Down Expand Up @@ -175,7 +178,8 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
DecoratedType(_typeProvider.boolType, _graph.never),
_nonNullableTypeType =
DecoratedType(_typeProvider.typeType, _graph.never),
_nullType = DecoratedType(_typeProvider.nullType, _graph.always);
_nullType = DecoratedType(_typeProvider.nullType, _graph.always),
_dynamicType = DecoratedType(_typeProvider.dynamicType, _graph.always);

/// Gets the decorated type of [element] from [_variables], performing any
/// necessary substitutions.
Expand Down Expand Up @@ -852,8 +856,10 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
}
var callee = node.methodName.staticElement;
if (callee == null) {
// TODO(paulberry)
_unimplemented(node, 'Unresolved method name');
// Dynamic dispatch. The return type is `dynamic`.
// TODO(paulberry): would it be better to assume a return type of `Never`
// so that we don't unnecessarily propagate nullabilities everywhere?
return _dynamicType;
}
var calleeType = getOrComputeElementType(callee, targetType: targetType);
if (callee is PropertyAccessorElement) {
Expand Down Expand Up @@ -1450,7 +1456,7 @@ class EdgeBuilder extends GeneralizingAstVisitor<DecoratedType>
'(${expression.toSource()}) offset=${expression.offset}');
}
ExpressionChecks expressionChecks;
if (canInsertChecks) {
if (canInsertChecks && !sourceType.type.isDynamic) {
expressionChecks = ExpressionChecks(expression.end);
_variables.recordExpressionChecks(source, expression, expressionChecks);
}
Expand Down
36 changes: 36 additions & 0 deletions pkg/nnbd_migration/test/api_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -821,6 +821,42 @@ int f(int i) {
await _checkSingleFileChanges(content, expected);
}

test_dynamic_method_call() async {
var content = '''
class C {
int g(int i) => i;
}
int f(bool b, dynamic d) {
if (b) return 0;
return d.g(null);
}
main() {
f(true, null);
f(false, C());
}
''';
// `d.g(null)` is a dynamic call, so we can't tell that it will target `C.g`
// at runtime. So we can't figure out that we need to make g's argument and
// return types nullable.
//
// We do, however, make f's return type nullable, since there is no way of
// knowing whether a dynamic call will return `null`.
var expected = '''
class C {
int g(int i) => i;
}
int? f(bool b, dynamic d) {
if (b) return 0;
return d.g(null);
}
main() {
f(true, null);
f(false, C());
}
''';
await _checkSingleFileChanges(content, expected);
}

test_field_formal_param_typed() async {
var content = '''
class C {
Expand Down
33 changes: 33 additions & 0 deletions pkg/nnbd_migration/test/edge_builder_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,20 @@ class C<T extends List<int>> {
hard: false);
}

test_assign_dynamic_to_other_type() async {
await analyze('''
int f(dynamic d) => d;
''');
// There is no explicit null check necessary, since `dynamic` is
// downcastable to any type, nullable or not.
expect(checkExpression('d;'), isNull);
// But we still create an edge, to make sure that the possibility of `null`
// propagates to callees.
assertEdge(decoratedTypeAnnotation('dynamic').node,
decoratedTypeAnnotation('int').node,
hard: true);
}

test_assign_null_to_generic_type() async {
await analyze('''
main() {
Expand Down Expand Up @@ -2204,6 +2218,25 @@ class C {
assertEdge(decoratedTypeAnnotation('int k').node, never, hard: true);
}

test_methodInvocation_dynamic() async {
await analyze('''
class C {
int g(int i) => i;
}
int f(dynamic d, int j) {
return d.g(j);
}
''');
// The call `d.g(j)` is dynamic, so we can't tell what method it resolves
// to. There's no reason to assume it resolves to `C.g`.
assertNoEdge(decoratedTypeAnnotation('int j').node,
decoratedTypeAnnotation('int i').node);
assertNoEdge(decoratedTypeAnnotation('int g').node,
decoratedTypeAnnotation('int f').node);
// We do, however, assume that it might return anything, including `null`.
assertEdge(always, decoratedTypeAnnotation('int f').node, hard: false);
}

test_methodInvocation_parameter_contravariant() async {
await analyze('''
class C<T> {
Expand Down

0 comments on commit c604e58

Please sign in to comment.