Migration: do not union inferred types in overrides.
Previously, when handling an override such as:
abstract class A {
int/*?*/ f();
}
class C implements A {
f() => 0;
}
we would create a union edge between the implicit return type of C.f
and the explicit return type of A.f. This was a problem because
nullability information can propagate bidirectionally through union
edges, so it could result in types unnecessarily becoming nullable,
e.g.:
abstract class A {
int/*?*/ f();
}
abstract class B {
int f(); // Should not need to be made nullable
}
class C implements A, B {
f() => 0;
}
This CL fixes the problem by just making ordinary unidirectional edges
for overrides involving inferred types.
Change-Id: I63a5f1f640b5543fcf39304087592e984aa66694
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/141853
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Mike Fairhurst <mfairhurst@google.com>
diff --git a/pkg/nnbd_migration/lib/src/edge_builder.dart b/pkg/nnbd_migration/lib/src/edge_builder.dart
index bba1dd7..a407c0c 100644
--- a/pkg/nnbd_migration/lib/src/edge_builder.dart
+++ b/pkg/nnbd_migration/lib/src/edge_builder.dart
@@ -2209,7 +2209,7 @@
_currentFunctionType.returnType,
overriddenFunctionType.returnType,
ReturnTypeInheritanceOrigin(source, node),
- isUnion: true);
+ isUnion: false);
} else {
_checkAssignment(ReturnTypeInheritanceOrigin(source, node),
source: _currentFunctionType.returnType,
@@ -2250,7 +2250,7 @@
if (_isUntypedParameter(normalParameter)) {
_linkDecoratedTypes(
overriddenParameterType, currentParameterType, origin,
- isUnion: true);
+ isUnion: false);
} else {
_checkAssignment(origin,
source: overriddenParameterType,
diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart
index dd1dc53..7ddd169 100644
--- a/pkg/nnbd_migration/test/api_test.dart
+++ b/pkg/nnbd_migration/test/api_test.dart
@@ -2643,6 +2643,64 @@
await _checkSingleFileChanges(content, expected);
}
+ Future<void> test_implicit_parameter_type_override_does_not_union() async {
+ var content = '''
+abstract class A {
+ void f(int/*?*/ i);
+}
+abstract class B {
+ void f(int i);
+}
+class C implements A, B {
+ void f(i) {}
+}
+''';
+ // Even though the parameter type of C.f is implicit, its nullability
+ // shouldn't be unioned with that of A and B, because that would
+ // unnecessarily force B.f's parameter type to be nullable.
+ var expected = '''
+abstract class A {
+ void f(int?/*?*/ i);
+}
+abstract class B {
+ void f(int i);
+}
+class C implements A, B {
+ void f(i) {}
+}
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
+ Future<void> test_implicit_return_type_override_does_not_union() async {
+ var content = '''
+abstract class A {
+ int/*?*/ f();
+}
+abstract class B {
+ int f();
+}
+class C implements A, B {
+ f() => 0;
+}
+''';
+ // Even though the return type of C.f is implicit, its nullability shouldn't
+ // be unioned with that of A and B, because that would unnecessarily force
+ // B.f's return type to be nullable.
+ var expected = '''
+abstract class A {
+ int?/*?*/ f();
+}
+abstract class B {
+ int f();
+}
+class C implements A, B {
+ f() => 0;
+}
+''';
+ await _checkSingleFileChanges(content, expected);
+ }
+
Future<void> test_implicit_type_parameter_bound_nullable() async {
var content = '''
class C<T> {
@@ -2686,78 +2744,6 @@
await _checkSingleFileChanges(content, expected);
}
- Future<void> test_inferred_method_parameter_type_non_nullable() async {
- var content = '''
-class B {
- void f(int i) {
- assert(i != null);
- }
-}
-class C extends B {
- void f(i) {}
-}
-void g(C c, int i, bool b) {
- if (b) {
- c.f(i);
- }
-}
-void h(C c) {
- g(c, null, false);
-}
-''';
- // B.f's parameter type is `int`. Since C.f's parameter type is inferred
- // from B.f's, it has a parameter type of `int` too. Therefore there must
- // be a null check in g().
- var expected = '''
-class B {
- void f(int i) {
- assert(i != null);
- }
-}
-class C extends B {
- void f(i) {}
-}
-void g(C c, int? i, bool b) {
- if (b) {
- c.f(i!);
- }
-}
-void h(C c) {
- g(c, null, false);
-}
-''';
- await _checkSingleFileChanges(content, expected);
- }
-
- Future<void> test_inferred_method_parameter_type_nullable() async {
- var content = '''
-class B {
- void f(int i) {}
-}
-class C extends B {
- void f(i) {}
-}
-void g(C c) {
- c.f(null);
-}
-''';
- // The call to C.f from g forces C.f's parameter to be nullable. Since
- // C.f's parameter type is inferred from B.f's parameter type, B.f's
- // parameter must be nullable too.
- var expected = '''
-class B {
- void f(int? i) {}
-}
-class C extends B {
- void f(i) {}
-}
-void g(C c) {
- c.f(null);
-}
-''';
- await _checkSingleFileChanges(content, expected);
- }
-
Future<void> test_inferred_method_return_type_non_nullable() async {
var content = '''
class B {
@@ -2783,31 +2769,6 @@
await _checkSingleFileChanges(content, expected);
}
- Future<void> test_inferred_method_return_type_nullable() async {
- var content = '''
-class B {
- int f() => null;
-}
-class C extends B {
- f() => 1;
-}
-int g(C c) => c.f();
-''';
- // B.f's return type is `int?`. Since C.f's return type is inferred from
- // B.f's, it has a return type of `int?` too. Therefore g's return type
- // must be `int?`.
- var expected = '''
-class B {
- int? f() => null;
-}
-class C extends B {
- f() => 1;
-}
-int? g(C c) => c.f();
-''';
- await _checkSingleFileChanges(content, expected);
- }
-
Future<void> test_insert_as_prefixed_type() async {
var content = '''
import 'dart:async' as a;
diff --git a/pkg/nnbd_migration/test/edge_builder_test.dart b/pkg/nnbd_migration/test/edge_builder_test.dart
index b529b8c..c75b494 100644
--- a/pkg/nnbd_migration/test/edge_builder_test.dart
+++ b/pkg/nnbd_migration/test/edge_builder_test.dart
@@ -4279,7 +4279,7 @@
''');
var bReturnType = decoratedMethodType('f/*B*/').positionalParameters[0];
var cReturnType = decoratedMethodType('f/*C*/').positionalParameters[0];
- assertUnion(bReturnType.node, cReturnType.node);
+ assertEdge(bReturnType.node, cReturnType.node, hard: true);
}
Future<void> test_method_parameterType_inferred_named() async {
@@ -4293,7 +4293,7 @@
''');
var bReturnType = decoratedMethodType('f/*B*/').namedParameters['x'];
var cReturnType = decoratedMethodType('f/*C*/').namedParameters['x'];
- assertUnion(bReturnType.node, cReturnType.node);
+ assertEdge(bReturnType.node, cReturnType.node, hard: true);
}
Future<void> test_method_returnType_inferred() async {
@@ -4307,7 +4307,7 @@
''');
var bReturnType = decoratedMethodType('f/*B*/').returnType;
var cReturnType = decoratedMethodType('f/*C*/').returnType;
- assertUnion(bReturnType.node, cReturnType.node);
+ assertEdge(cReturnType.node, bReturnType.node, hard: true);
}
Future<void>
diff --git a/pkg/nnbd_migration/test/instrumentation_test.dart b/pkg/nnbd_migration/test/instrumentation_test.dart
index d9d01f1..5afbb431 100644
--- a/pkg/nnbd_migration/test/instrumentation_test.dart
+++ b/pkg/nnbd_migration/test/instrumentation_test.dart
@@ -991,8 +991,8 @@
.node;
expect(
edges.where((e) =>
- e.sourceNode == derivedParamArgNode &&
- e.destinationNode == baseParamArgNode),
+ e.sourceNode == baseParamArgNode &&
+ e.destinationNode == derivedParamArgNode),
hasLength(1));
}
