Skip to content

Commit f907e56

Browse files
stereotype441commit-bot@chromium.org
stereotype441
authored and
[email protected]
committed
Migration: never produce a type of the form FutureOr<T?>?.
`FutureOr<T?>` is already nullable, because `FutureOr<T?>` == `FutureOr<T | Null>` == `Future<T | Null> | T | Null`. So the second `?` is redundant. Also fix a bug where we were treating a `Null` type as `Never` if we couldn't find a reason to require it to be nullable; this was inconsistent with how the FixBuilder was handling `Null` (which is to leave it unchanged). Change-Id: I5e0e4b7fb449988d8ff0dd87a2b4c3da4a6ecebb Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155506 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]>
1 parent 69ba6e5 commit f907e56

File tree

5 files changed

+153
-6
lines changed

5 files changed

+153
-6
lines changed

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

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1035,8 +1035,7 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
10351035
var decoratedType = _fixBuilder._variables
10361036
.decoratedTypeAnnotation(_fixBuilder.source, node);
10371037
if (!typeIsNonNullableByContext(node)) {
1038-
var type = decoratedType.type;
1039-
if (!type.isDynamic && !type.isVoid && !type.isDartCoreNull) {
1038+
if (!_typeIsNaturallyNullable(decoratedType.type)) {
10401039
_makeTypeNameNullable(node, decoratedType);
10411040
}
10421041
}
@@ -1073,12 +1072,28 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
10731072
}
10741073

10751074
void _makeTypeNameNullable(TypeAnnotation node, DecoratedType decoratedType) {
1075+
bool makeNullable = decoratedType.node.isNullable;
1076+
if (decoratedType.type.isDartAsyncFutureOr) {
1077+
var typeArguments = decoratedType.typeArguments;
1078+
if (typeArguments.length == 1) {
1079+
var typeArgument = typeArguments[0];
1080+
if ((_typeIsNaturallyNullable(typeArgument.type) ||
1081+
typeArgument.node.isNullable)) {
1082+
// FutureOr<T?>? is equivalent to FutureOr<T?>, so there is no need to
1083+
// make this type nullable.
1084+
makeNullable = false;
1085+
}
1086+
}
1087+
}
10761088
(_fixBuilder._getChange(node) as NodeChangeForTypeAnnotation)
10771089
.recordNullability(
1078-
decoratedType, decoratedType.node.isNullable,
1090+
decoratedType, makeNullable,
10791091
nullabilityHint:
10801092
_fixBuilder._variables.getNullabilityHint(source, node));
10811093
}
1094+
1095+
bool _typeIsNaturallyNullable(DartType type) =>
1096+
type.isDynamic || type.isVoid || type.isDartCoreNull;
10821097
}
10831098

10841099
/// Specialization of [_AssignmentLikeExpressionHandler] for

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,13 +286,16 @@ class Variables {
286286
DartType toFinalType(DecoratedType decoratedType) {
287287
var type = decoratedType.type;
288288
if (type.isVoid || type.isDynamic) return type;
289-
if (type is NeverType || type.isDartCoreNull) {
289+
if (type is NeverType) {
290290
if (decoratedType.node.isNullable) {
291291
return (_typeProvider.nullType as TypeImpl)
292292
.withNullability(NullabilitySuffix.none);
293293
} else {
294294
return NeverTypeImpl.instance;
295295
}
296+
} else if (type.isDartCoreNull) {
297+
return (_typeProvider.nullType as TypeImpl)
298+
.withNullability(NullabilitySuffix.none);
296299
}
297300
var nullabilitySuffix = decoratedType.node.isNullable
298301
? NullabilitySuffix.question

‎pkg/nnbd_migration/test/api_test.dart‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,28 @@ abstract class C {
325325
await _checkSingleFileChanges(content, expected);
326326
}
327327

328+
Future<void> test_avoid_redundant_future_or() async {
329+
// FutureOr<int?> and FutureOr<int?>? are equivalent types; we never insert
330+
// the redundant second `?`.
331+
var content = '''
332+
import 'dart:async';
333+
abstract class C {
334+
FutureOr<int/*?*/> f();
335+
FutureOr<int>/*?*/ g();
336+
FutureOr<int> h(bool b) => b ? f() : g();
337+
}
338+
''';
339+
var expected = '''
340+
import 'dart:async';
341+
abstract class C {
342+
FutureOr<int?> f();
343+
FutureOr<int>? g();
344+
FutureOr<int?> h(bool b) => b ? f() : g();
345+
}
346+
''';
347+
await _checkSingleFileChanges(content, expected);
348+
}
349+
328350
Future<void>
329351
test_back_propagation_stops_at_implicitly_typed_variables() async {
330352
var content = '''
@@ -3018,7 +3040,7 @@ void f(
30183040
FutureOr<int> foi1,
30193041
FutureOr<int?> foi2,
30203042
FutureOr<int>? foi3,
3021-
FutureOr<int?>? foi4
3043+
FutureOr<int?> foi4
30223044
) {
30233045
int i1 = foi1 as int;
30243046
int? i2 = foi2 as int?;

‎pkg/nnbd_migration/test/decorated_type_test.dart‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -540,8 +540,10 @@ class DecoratedTypeTest extends Object
540540
}
541541

542542
void test_toFinalType_null_non_nullable() {
543+
// We never change explicit `Null` types to `Never`, even if we can't find
544+
// any reason they need to be nullable.
543545
var type = _variables.toFinalType(DecoratedType(null_.type, never));
544-
assertDartType(type, 'Never');
546+
assertDartType(type, 'Null');
545547
}
546548

547549
void test_toFinalType_null_nullable() {

‎pkg/nnbd_migration/test/fix_builder_test.dart‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3055,6 +3055,98 @@ void _f() {
30553055
visitTypeAnnotation(findNode.typeAnnotation('dynamic'), 'dynamic');
30563056
}
30573057

3058+
Future<void> test_typeName_futureOr_dynamic_nullable() async {
3059+
await analyze('''
3060+
import 'dart:async';
3061+
void _f() {
3062+
FutureOr<dynamic> x = null;
3063+
}
3064+
''');
3065+
// The type of `x` should be `FutureOr<dynamic>?`, but this is equivalent to
3066+
// `FutureOr<dynamic>`, so we don't add a `?`. Note: expected type is
3067+
// still `FutureOr<dynamic>?`; we don't go to extra effort to remove the
3068+
// redundant `?` from the internal type representation, just from the source
3069+
// code we generate.
3070+
visitTypeAnnotation(
3071+
findNode.typeAnnotation('FutureOr<dynamic> x'), 'FutureOr<dynamic>?',
3072+
changes: {});
3073+
}
3074+
3075+
Future<void> test_typeName_futureOr_inner() async {
3076+
await analyze('''
3077+
import 'dart:async';
3078+
void _f(FutureOr<int/*?*/> x) {
3079+
FutureOr<int> y = x;
3080+
}
3081+
''');
3082+
visitTypeAnnotation(
3083+
findNode.typeAnnotation('FutureOr<int> y'), 'FutureOr<int?>',
3084+
changes: {findNode.typeAnnotation('int> y'): isMakeNullable});
3085+
}
3086+
3087+
Future<void> test_typeName_futureOr_null_nullable() async {
3088+
await analyze('''
3089+
import 'dart:async';
3090+
void _f() {
3091+
FutureOr<Null> x = null;
3092+
}
3093+
''');
3094+
// The type of `x` should be `FutureOr<Null>?`, but this is equivalent to
3095+
// `FutureOr<Null>`, so we don't add a `?`. Note: expected type is
3096+
// still `FutureOr<Null>?`; we don't go to extra effort to remove the
3097+
// redundant `?` from the internal type representation, just from the source
3098+
// code we generate.
3099+
visitTypeAnnotation(
3100+
findNode.typeAnnotation('FutureOr<Null> x'), 'FutureOr<Null>?',
3101+
changes: {});
3102+
}
3103+
3104+
Future<void> test_typeName_futureOr_outer() async {
3105+
await analyze('''
3106+
import 'dart:async';
3107+
void _f(FutureOr<int>/*?*/ x) {
3108+
FutureOr<int> y = x;
3109+
}
3110+
''');
3111+
var typeAnnotation = findNode.typeAnnotation('FutureOr<int> y');
3112+
visitTypeAnnotation(typeAnnotation, 'FutureOr<int>?',
3113+
changes: {typeAnnotation: isMakeNullable});
3114+
}
3115+
3116+
Future<void> test_typeName_futureOr_redundant() async {
3117+
await analyze('''
3118+
import 'dart:async';
3119+
void _f(bool b, FutureOr<int>/*?*/ x, FutureOr<int/*?*/> y) {
3120+
FutureOr<int> z = b ? x : y;
3121+
}
3122+
''');
3123+
// The type of `z` should be `FutureOr<int?>?`, but this is equivalent to
3124+
// `FutureOr<int?>`, so we only add the first `?`. Note: expected type is
3125+
// still `FutureOr<int?>?`; we don't go to extra effort to remove the
3126+
// redundant `?` from the internal type representation, just from the source
3127+
// code we generate.
3128+
visitTypeAnnotation(
3129+
findNode.typeAnnotation('FutureOr<int> z'), 'FutureOr<int?>?',
3130+
changes: {findNode.typeAnnotation('int> z'): isMakeNullable});
3131+
}
3132+
3133+
Future<void> test_typeName_futureOr_void_nullable() async {
3134+
await analyze('''
3135+
import 'dart:async';
3136+
void _f() {
3137+
FutureOr<void> x = null;
3138+
}
3139+
''');
3140+
// The type of `x` should be `FutureOr<void>?`, but this is equivalent to
3141+
// `FutureOr<void>`, so we don't add a `?`. Note: expected type is
3142+
// still `FutureOr<void>?`; we don't go to extra effort to remove the
3143+
// redundant `?` from the internal type representation, just from the source
3144+
// code we generate.
3145+
visitTypeAnnotation(
3146+
findNode.typeAnnotation('FutureOr<void> x'), 'FutureOr<void>?',
3147+
changes: {});
3148+
}
3149+
30583150
Future<void> test_typeName_generic_nonNullable() async {
30593151
await analyze('''
30603152
void _f() {
@@ -3085,6 +3177,19 @@ void _f() {
30853177
changes: {findNode.typeAnnotation('int'): isMakeNullable});
30863178
}
30873179

3180+
Future<void> test_typeName_generic_nullable_arg_and_outer() async {
3181+
await analyze('''
3182+
void _f(bool b) {
3183+
List<int> i = b ? [null] : null;
3184+
}
3185+
''');
3186+
var listInt = findNode.typeAnnotation('List<int>');
3187+
visitTypeAnnotation(listInt, 'List<int?>?', changes: {
3188+
findNode.typeAnnotation('int'): isMakeNullable,
3189+
listInt: isMakeNullable
3190+
});
3191+
}
3192+
30883193
Future<void> test_typeName_simple_nonNullable() async {
30893194
await analyze('''
30903195
void _f() {

0 commit comments

Comments
 (0)