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]>

Author: stereotype441

pkg/nnbd_migration/lib/src/fix_builder.dart

@@ -1035,8 +1035,7 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
     var decoratedType = _fixBuilder._variables
         .decoratedTypeAnnotation(_fixBuilder.source, node);
     if (!typeIsNonNullableByContext(node)) {
-      var type = decoratedType.type;
-      if (!type.isDynamic && !type.isVoid && !type.isDartCoreNull) {
+      if (!_typeIsNaturallyNullable(decoratedType.type)) {
         _makeTypeNameNullable(node, decoratedType);
       }
     }
@@ -1073,12 +1072,28 @@ class _FixBuilderPreVisitor extends GeneralizingAstVisitor<void>
   }
 
   void _makeTypeNameNullable(TypeAnnotation node, DecoratedType decoratedType) {
+    bool makeNullable = decoratedType.node.isNullable;
+    if (decoratedType.type.isDartAsyncFutureOr) {
+      var typeArguments = decoratedType.typeArguments;
+      if (typeArguments.length == 1) {
+        var typeArgument = typeArguments[0];
+        if ((_typeIsNaturallyNullable(typeArgument.type) ||
+            typeArgument.node.isNullable)) {
+          // FutureOr<T?>? is equivalent to FutureOr<T?>, so there is no need to
+          // make this type nullable.
+          makeNullable = false;
+        }
+      }
+    }
     (_fixBuilder._getChange(node) as NodeChangeForTypeAnnotation)
         .recordNullability(
-            decoratedType, decoratedType.node.isNullable,
+            decoratedType, makeNullable,
             nullabilityHint:
                 _fixBuilder._variables.getNullabilityHint(source, node));
   }
+
+  bool _typeIsNaturallyNullable(DartType type) =>
+      type.isDynamic || type.isVoid || type.isDartCoreNull;
 }
 
 /// Specialization of [_AssignmentLikeExpressionHandler] for

pkg/nnbd_migration/lib/src/variables.dart

@@ -286,13 +286,16 @@ class Variables {
   DartType toFinalType(DecoratedType decoratedType) {
     var type = decoratedType.type;
     if (type.isVoid || type.isDynamic) return type;
-    if (type is NeverType || type.isDartCoreNull) {
+    if (type is NeverType) {
       if (decoratedType.node.isNullable) {
         return (_typeProvider.nullType as TypeImpl)
             .withNullability(NullabilitySuffix.none);
       } else {
         return NeverTypeImpl.instance;
       }
+    } else if (type.isDartCoreNull) {
+      return (_typeProvider.nullType as TypeImpl)
+          .withNullability(NullabilitySuffix.none);
     }
     var nullabilitySuffix = decoratedType.node.isNullable
         ? NullabilitySuffix.question

pkg/nnbd_migration/test/api_test.dart

@@ -325,6 +325,28 @@ abstract class C {
     await _checkSingleFileChanges(content, expected);
   }
 
+  Future<void> test_avoid_redundant_future_or() async {
+    // FutureOr<int?> and FutureOr<int?>? are equivalent types; we never insert
+    // the redundant second `?`.
+    var content = '''
+import 'dart:async';
+abstract class C {
+  FutureOr<int/*?*/> f();
+  FutureOr<int>/*?*/ g();
+  FutureOr<int> h(bool b) => b ? f() : g();
+}
+''';
+    var expected = '''
+import 'dart:async';
+abstract class C {
+  FutureOr<int?> f();
+  FutureOr<int>? g();
+  FutureOr<int?> h(bool b) => b ? f() : g();
+}
+''';
+    await _checkSingleFileChanges(content, expected);
+  }
+
   Future<void>
       test_back_propagation_stops_at_implicitly_typed_variables() async {
     var content = '''
@@ -3018,7 +3040,7 @@ void f(
     FutureOr<int> foi1,
     FutureOr<int?> foi2,
     FutureOr<int>? foi3,
-    FutureOr<int?>? foi4
+    FutureOr<int?> foi4
 ) {
   int i1 = foi1 as int;
   int? i2 = foi2 as int?;

pkg/nnbd_migration/test/decorated_type_test.dart

@@ -540,8 +540,10 @@ class DecoratedTypeTest extends Object
   }
 
   void test_toFinalType_null_non_nullable() {
+    // We never change explicit `Null` types to `Never`, even if we can't find
+    // any reason they need to be nullable.
     var type = _variables.toFinalType(DecoratedType(null_.type, never));
-    assertDartType(type, 'Never');
+    assertDartType(type, 'Null');
   }
 
   void test_toFinalType_null_nullable() {

pkg/nnbd_migration/test/fix_builder_test.dart

@@ -3055,6 +3055,98 @@ void _f() {
     visitTypeAnnotation(findNode.typeAnnotation('dynamic'), 'dynamic');
   }
 
+  Future<void> test_typeName_futureOr_dynamic_nullable() async {
+    await analyze('''
+import 'dart:async';
+void _f() {
+  FutureOr<dynamic> x = null;
+}
+''');
+    // The type of `x` should be `FutureOr<dynamic>?`, but this is equivalent to
+    // `FutureOr<dynamic>`, so we don't add a `?`.  Note: expected type is
+    // still `FutureOr<dynamic>?`; we don't go to extra effort to remove the
+    // redundant `?` from the internal type representation, just from the source
+    // code we generate.
+    visitTypeAnnotation(
+        findNode.typeAnnotation('FutureOr<dynamic> x'), 'FutureOr<dynamic>?',
+        changes: {});
+  }
+
+  Future<void> test_typeName_futureOr_inner() async {
+    await analyze('''
+import 'dart:async';
+void _f(FutureOr<int/*?*/> x) {
+  FutureOr<int> y = x;
+}
+''');
+    visitTypeAnnotation(
+        findNode.typeAnnotation('FutureOr<int> y'), 'FutureOr<int?>',
+        changes: {findNode.typeAnnotation('int> y'): isMakeNullable});
+  }
+
+  Future<void> test_typeName_futureOr_null_nullable() async {
+    await analyze('''
+import 'dart:async';
+void _f() {
+  FutureOr<Null> x = null;
+}
+''');
+    // The type of `x` should be `FutureOr<Null>?`, but this is equivalent to
+    // `FutureOr<Null>`, so we don't add a `?`.  Note: expected type is
+    // still `FutureOr<Null>?`; we don't go to extra effort to remove the
+    // redundant `?` from the internal type representation, just from the source
+    // code we generate.
+    visitTypeAnnotation(
+        findNode.typeAnnotation('FutureOr<Null> x'), 'FutureOr<Null>?',
+        changes: {});
+  }
+
+  Future<void> test_typeName_futureOr_outer() async {
+    await analyze('''
+import 'dart:async';
+void _f(FutureOr<int>/*?*/ x) {
+  FutureOr<int> y = x;
+}
+''');
+    var typeAnnotation = findNode.typeAnnotation('FutureOr<int> y');
+    visitTypeAnnotation(typeAnnotation, 'FutureOr<int>?',
+        changes: {typeAnnotation: isMakeNullable});
+  }
+
+  Future<void> test_typeName_futureOr_redundant() async {
+    await analyze('''
+import 'dart:async';
+void _f(bool b, FutureOr<int>/*?*/ x, FutureOr<int/*?*/> y) {
+  FutureOr<int> z = b ? x : y;
+}
+''');
+    // The type of `z` should be `FutureOr<int?>?`, but this is equivalent to
+    // `FutureOr<int?>`, so we only add the first `?`.  Note: expected type is
+    // still `FutureOr<int?>?`; we don't go to extra effort to remove the
+    // redundant `?` from the internal type representation, just from the source
+    // code we generate.
+    visitTypeAnnotation(
+        findNode.typeAnnotation('FutureOr<int> z'), 'FutureOr<int?>?',
+        changes: {findNode.typeAnnotation('int> z'): isMakeNullable});
+  }
+
+  Future<void> test_typeName_futureOr_void_nullable() async {
+    await analyze('''
+import 'dart:async';
+void _f() {
+  FutureOr<void> x = null;
+}
+''');
+    // The type of `x` should be `FutureOr<void>?`, but this is equivalent to
+    // `FutureOr<void>`, so we don't add a `?`.  Note: expected type is
+    // still `FutureOr<void>?`; we don't go to extra effort to remove the
+    // redundant `?` from the internal type representation, just from the source
+    // code we generate.
+    visitTypeAnnotation(
+        findNode.typeAnnotation('FutureOr<void> x'), 'FutureOr<void>?',
+        changes: {});
+  }
+
   Future<void> test_typeName_generic_nonNullable() async {
     await analyze('''
 void _f() {
@@ -3085,6 +3177,19 @@ void _f() {
         changes: {findNode.typeAnnotation('int'): isMakeNullable});
   }
 
+  Future<void> test_typeName_generic_nullable_arg_and_outer() async {
+    await analyze('''
+void _f(bool b) {
+  List<int> i = b ? [null] : null;
+}
+''');
+    var listInt = findNode.typeAnnotation('List<int>');
+    visitTypeAnnotation(listInt, 'List<int?>?', changes: {
+      findNode.typeAnnotation('int'): isMakeNullable,
+      listInt: isMakeNullable
+    });
+  }
+
   Future<void> test_typeName_simple_nonNullable() async {
     await analyze('''
 void _f() {