From c604e58a9b387856a630391aa0ec88acc010b88e Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 27 Aug 2019 12:33:54 +0000 Subject: [PATCH] Migration: add support for dynamic method dispatches. 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 --- pkg/nnbd_migration/lib/src/edge_builder.dart | 14 +++++--- pkg/nnbd_migration/test/api_test.dart | 36 +++++++++++++++++++ .../test/edge_builder_test.dart | 33 +++++++++++++++++ 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart index 77ef414f6ae7..64c73356ec78 100644 --- a/pkg/nnbd_migration/lib/src/edge_builder.dart +++ b/pkg/nnbd_migration/lib/src/edge_builder.dart @@ -124,6 +124,9 @@ class EdgeBuilder extends GeneralizingAstVisitor /// 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. /// @@ -175,7 +178,8 @@ class EdgeBuilder extends GeneralizingAstVisitor 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. @@ -852,8 +856,10 @@ class EdgeBuilder extends GeneralizingAstVisitor } 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) { @@ -1450,7 +1456,7 @@ class EdgeBuilder extends GeneralizingAstVisitor '(${expression.toSource()}) offset=${expression.offset}'); } ExpressionChecks expressionChecks; - if (canInsertChecks) { + if (canInsertChecks && !sourceType.type.isDynamic) { expressionChecks = ExpressionChecks(expression.end); _variables.recordExpressionChecks(source, expression, expressionChecks); } diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart index 286e462294cb..4a4e2660d3fe 100644 --- a/pkg/nnbd_migration/test/api_test.dart +++ b/pkg/nnbd_migration/test/api_test.dart @@ -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 { diff --git a/pkg/nnbd_migration/test/edge_builder_test.dart b/pkg/nnbd_migration/test/edge_builder_test.dart index 4f1a2c5bdd98..f2dab1f5b245 100644 --- a/pkg/nnbd_migration/test/edge_builder_test.dart +++ b/pkg/nnbd_migration/test/edge_builder_test.dart @@ -384,6 +384,20 @@ class C> { 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() { @@ -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 {