[vm/aot/tfa/nnbd] Fix handling of non-nullable types in NNBD strong mode

This change contains the following fixes for NNBD strong mode in TFA:

* TypesBuilder.fromStaticType now correctly applies nullability of
  TypeParameterType even if bound is non-nullable.

* UnknownType is introduced to represent a type argument not known at
  compile time. Previously AnyType was used for that purpose, and it
  could pass isSubtypeOfRuntimeType check for non-nullable Object type.

Fixes dart-lang#41452

Change-Id: Ie836c99b38ad288aa68d9aae48e50844f9f949d5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/143310
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>

Author: alexmarkov

pkg/vm/lib/transformations/type_flow/analysis.dart

@@ -230,6 +230,9 @@ class _DirectInvocation extends _Invocation {
           // does not throw exception.
           initializerResult = new Type.nullable(initializerResult);
         }
+        if (kPrintTrace) {
+          tracePrint("Result of ${field} initializer: $initializerResult");
+        }
         fieldValue.setValue(initializerResult, typeFlowAnalysis,
             field.isStatic ? null : args.receiver);
         fieldValue.isInitialized = true;
@@ -989,7 +992,7 @@ class GenericInterfacesInfoImpl implements GenericInterfacesInfo {
     result = new List<Type>(flattenedTypeArgs.length);
     for (int i = 0; i < flattenedTypeArgs.length; ++i) {
       final translated = closedTypeTranslator.translate(flattenedTypeArgs[i]);
-      assertx(translated is RuntimeType || translated is AnyType);
+      assertx(translated is RuntimeType || translated is UnknownType);
       result[i] = translated;
     }
     cachedFlattenedTypeArgsForNonGeneric[klass] = result;

pkg/vm/lib/transformations/type_flow/summary.dart

@@ -339,7 +339,7 @@ class Extract extends Statement {
 
     void extractType(ConcreteType c) {
       if (c.typeArgs == null) {
-        extractedType = const AnyType();
+        extractedType = const UnknownType();
       } else {
         final interfaceOffset = typeHierarchy.genericInterfaceOffsetFor(
             c.cls.classNode, referenceClass);
@@ -364,12 +364,12 @@ class Extract extends Statement {
             }
           }
         } else {
-          assertx(typeArg is AnyType);
+          assertx(typeArg is UnknownType);
         }
         if (extractedType == null || extracted == extractedType) {
           extractedType = extracted;
         } else {
-          extractedType = const AnyType();
+          extractedType = const UnknownType();
         }
       }
     }
@@ -381,7 +381,7 @@ class Extract extends Statement {
       argType.types.forEach(extractType);
     }
 
-    return extractedType ?? const AnyType();
+    return extractedType ?? const UnknownType();
   }
 }
 
@@ -414,7 +414,7 @@ class CreateConcreteType extends Statement {
     final types = new List<Type>(flattenedTypeArgs.length);
     for (int i = 0; i < types.length; ++i) {
       final computed = flattenedTypeArgs[i].getComputedType(computedTypes);
-      assertx(computed is RuntimeType || computed is AnyType);
+      assertx(computed is RuntimeType || computed is UnknownType);
       if (computed is RuntimeType) hasRuntimeType = true;
       types[i] = computed;
     }
@@ -424,7 +424,7 @@ class CreateConcreteType extends Statement {
 
 // Similar to "CreateConcreteType", but creates a "RuntimeType" rather than a
 // "ConcreteType". Unlike a "ConcreteType", none of the type arguments can be
-// missing ("AnyType").
+// missing ("UnknownType").
 class CreateRuntimeType extends Statement {
   final Class klass;
   final Nullability nullability;
@@ -446,8 +446,8 @@ class CreateRuntimeType extends Statement {
     final types = new List<RuntimeType>(flattenedTypeArgs.length);
     for (int i = 0; i < types.length; ++i) {
       final computed = flattenedTypeArgs[i].getComputedType(computedTypes);
-      assertx(computed is RuntimeType || computed is AnyType);
-      if (computed is AnyType) return const AnyType();
+      assertx(computed is RuntimeType || computed is UnknownType);
+      if (computed is UnknownType) return const UnknownType();
       types[i] = computed;
     }
     return new RuntimeType(new InterfaceType(klass, nullability), types);
@@ -501,11 +501,11 @@ class TypeCheck extends Statement {
     Type argType = arg.getComputedType(computedTypes);
     Type checkType = type.getComputedType(computedTypes);
     // TODO(sjindel/tfa): Narrow the result if possible.
-    assertx(checkType is AnyType || checkType is RuntimeType);
+    assertx(checkType is UnknownType || checkType is RuntimeType);
 
     bool canSkip = true; // Can this check be skipped on this invocation.
 
-    if (checkType is AnyType) {
+    if (checkType is UnknownType) {
       // If we don't know what the RHS of the check is going to be, we can't
       // guarantee that it will pass.
       canSkip = false;

pkg/vm/lib/transformations/type_flow/summary_collector.dart

@@ -584,7 +584,8 @@ class SummaryCollector extends RecursiveVisitor<TypeExpr> {
 
   Summary createSummary(Member member,
       {fieldSummaryType: FieldSummaryType.kInitializer}) {
-    debugPrint("===== ${member} =====");
+    debugPrint(
+        "===== ${member}${fieldSummaryType == FieldSummaryType.kFieldGuard ? " (guard)" : ""} =====");
     assertx(!member.isAbstract);
 
     _staticTypeContext = new StaticTypeContext(member, _environment);
@@ -779,7 +780,7 @@ class SummaryCollector extends RecursiveVisitor<TypeExpr> {
 
     final numTypeParameters = numTypeParams(member);
     for (int i = 0; i < numTypeParameters; ++i) {
-      args.add(const AnyType());
+      args.add(const UnknownType());
     }
 
     if (hasReceiverArg(member)) {
@@ -2175,16 +2176,16 @@ class RuntimeTypeTranslatorImpl extends DartTypeVisitor<TypeExpr>
     final flattenedTypeExprs = new List<TypeExpr>(flattenedTypeArgs.length);
 
     bool createConcreteType = true;
-    bool allAnyType = true;
+    bool allUnknown = true;
     for (int i = 0; i < flattenedTypeArgs.length; ++i) {
       final typeExpr =
           translate(substitution.substituteType(flattenedTypeArgs[i]));
-      if (typeExpr != const AnyType()) allAnyType = false;
+      if (typeExpr is! UnknownType) allUnknown = false;
       if (typeExpr is Statement) createConcreteType = false;
       flattenedTypeExprs[i] = typeExpr;
     }
 
-    if (allAnyType) return type;
+    if (allUnknown) return type;
 
     if (createConcreteType) {
       return new ConcreteType(
@@ -2199,7 +2200,7 @@ class RuntimeTypeTranslatorImpl extends DartTypeVisitor<TypeExpr>
   // Creates a TypeExpr representing the set of types which can flow through a
   // given DartType.
   //
-  // Will return AnyType, RuntimeType or Statement.
+  // Will return UnknownType, RuntimeType or Statement.
   TypeExpr translate(DartType type) {
     final cached = typesCache[type];
     if (cached != null) return cached;
@@ -2209,18 +2210,19 @@ class RuntimeTypeTranslatorImpl extends DartTypeVisitor<TypeExpr>
     //   class A<T> extends Comparable<A<T>> {}
     //
     // Creating the factored type arguments of A will lead to an infinite loop.
-    // We break such loops by inserting an 'AnyType' in place of the currently
+    // We break such loops by inserting an 'UnknownType' in place of the currently
     // processed type, ensuring we try to build 'A<T>' in the process of
     // building 'A<T>'.
-    typesCache[type] = const AnyType();
+    typesCache[type] = const UnknownType();
     final result = type.accept(this);
-    assertx(result is AnyType || result is RuntimeType || result is Statement);
+    assertx(
+        result is UnknownType || result is RuntimeType || result is Statement);
     typesCache[type] = result;
     return result;
   }
 
   @override
-  TypeExpr defaultDartType(DartType node) => const AnyType();
+  TypeExpr defaultDartType(DartType node) => const UnknownType();
 
   @override
   TypeExpr visitDynamicType(DynamicType type) => new RuntimeType(type, null);
@@ -2248,7 +2250,7 @@ class RuntimeTypeTranslatorImpl extends DartTypeVisitor<TypeExpr>
     for (var i = 0; i < flattenedTypeArgs.length; ++i) {
       final typeExpr =
           translate(substitution.substituteType(flattenedTypeArgs[i]));
-      if (typeExpr == const AnyType()) return const AnyType();
+      if (typeExpr == const UnknownType()) return const UnknownType();
       if (typeExpr is! RuntimeType) createRuntimeType = false;
       flattenedTypeExprs[i] = typeExpr;
     }
@@ -2271,7 +2273,7 @@ class RuntimeTypeTranslatorImpl extends DartTypeVisitor<TypeExpr>
       final result = functionTypeVariables[type.parameter];
       if (result != null) return result;
     }
-    if (type.parameter.parent is! Class) return const AnyType();
+    if (type.parameter.parent is! Class) return const UnknownType();
     final interfaceClass = type.parameter.parent as Class;
     assertx(receiver != null);
     // Undetermined nullability is equivalent to nonNullable when

pkg/vm/lib/transformations/type_flow/transformer.dart

@@ -190,7 +190,8 @@ class AnnotateKernel extends RecursiveVisitor<Null> {
     if (type is ConcreteType && type.typeArgs != null) {
       typeArgs = type.typeArgs
           .take(type.numImmediateTypeArgs)
-          .map((t) => t is AnyType ? null : (t as RuntimeType).representedType)
+          .map((t) =>
+              t is UnknownType ? null : (t as RuntimeType).representedType)
           .toList();
     }
 
@@ -256,7 +257,7 @@ class AnnotateKernel extends RecursiveVisitor<Null> {
 
     // If the call is not marked as 'isResultUsed', the 'resultType' will
     // not be observed (i.e., it will always be EmptyType). This is the
-    // case even if the result acutally might be used but is not used by
+    // case even if the result actually might be used but is not used by
     // the summary, e.g. if the result is an argument to a closure call.
     // Therefore, we need to pass in 'NullableType(AnyType)' as the
     // inferred result type here (since we don't know what it actually

pkg/vm/lib/transformations/type_flow/types.dart

@@ -93,15 +93,15 @@ abstract class TypesBuilder {
       if (bound is TypeParameterType) {
         result = const AnyType();
       } else {
-        return fromStaticType(bound, canBeNull);
+        result = fromStaticType(bound, canBeNull);
       }
     } else {
       throw 'Unexpected type ${type.runtimeType} $type';
     }
     if (nullSafety && type.nullability == Nullability.nonNullable) {
       canBeNull = false;
     }
-    if (canBeNull) {
+    if (canBeNull && result is! NullableType) {
       result = new Type.nullable(result);
     }
     return result;
@@ -186,6 +186,7 @@ abstract class Type extends TypeExpr {
 /// Order of precedence between types for evaluation of union/intersection.
 enum TypeOrder {
   RuntimeType,
+  Unknown,
   Empty,
   Nullable,
   Any,
@@ -293,7 +294,6 @@ class NullableType extends Type {
 
 /// Type representing any instance except `null`.
 /// Semantically equivalent to ConeType of Object, but more efficient.
-/// Can also represent a set of types, the set of all types.
 class AnyType extends Type {
   const AnyType();
 
@@ -612,7 +612,7 @@ class ConcreteType extends Type implements Comparable<ConcreteType> {
   int _hashCode;
 
   // May be null if there are no type arguments constraints. The type arguments
-  // should represent type sets, i.e. `AnyType` or `RuntimeType`. The type
+  // should represent type sets, i.e. `UnknownType` or `RuntimeType`. The type
   // arguments vector is factored against the generic interfaces implemented by
   // the class (see [TypeHierarchy.flattenedTypeArgumentsFor]).
   //
@@ -693,11 +693,13 @@ class ConcreteType extends Type implements Comparable<ConcreteType> {
           runtimeType.numImmediateTypeArgs);
 
       for (int i = 0; i < runtimeType.numImmediateTypeArgs; ++i) {
-        if (usableTypeArgs[i + interfaceOffset] == const AnyType())
+        final ta = usableTypeArgs[i + interfaceOffset];
+        if (ta is UnknownType) {
           return false;
-        assertx(usableTypeArgs[i + interfaceOffset] is RuntimeType);
-        if (!usableTypeArgs[i + interfaceOffset]
-            .isSubtypeOfRuntimeType(typeHierarchy, runtimeType.typeArgs[i])) {
+        }
+        assertx(ta is RuntimeType);
+        if (!ta.isSubtypeOfRuntimeType(
+            typeHierarchy, runtimeType.typeArgs[i])) {
           return false;
         }
       }
@@ -864,7 +866,7 @@ class ConcreteType extends Type implements Comparable<ConcreteType> {
 //   class A<T> extends Comparable<A<T>> {}
 //
 // To avoid these cycles, we approximate generic super-bounded types (the second
-// case), so the representation for 'A<String>' would be simply 'AnyType'.
+// case), so the representation for 'A<String>' would be simply 'UnknownType'.
 // However, approximating non-generic types like 'int' and 'num' (the first
 // case) would be too coarse, so we leave an null 'typeArgs' field for these
 // types. As a result, when doing an 'isSubtypeOfRuntimeType' against
@@ -963,11 +965,11 @@ class RuntimeType extends Type {
   Type union(Type other, TypeHierarchy typeHierarchy) =>
       throw "ERROR: RuntimeType does not support union.";
 
-  // This only works between "type-set" representations ('AnyType' and
+  // This only works between "type-set" representations ('UnknownType' and
   // 'RuntimeType') and is used when merging type arguments.
   @override
   Type intersection(Type other, TypeHierarchy typeHierarchy) {
-    if (other is AnyType) {
+    if (other is UnknownType) {
       return this;
     } else if (other is RuntimeType) {
       return this == other ? this : const EmptyType();
@@ -1043,3 +1045,52 @@ class RuntimeType extends Type {
     return true;
   }
 }
+
+/// Type which is not known at compile time.
+/// It is used as the right-hand-side of type tests.
+class UnknownType extends Type {
+  const UnknownType();
+
+  @override
+  int get hashCode => 1019;
+
+  @override
+  bool operator ==(other) => (other is UnknownType);
+
+  @override
+  String toString() => "UNKNOWN";
+
+  @override
+  int get order => TypeOrder.Unknown.index;
+
+  @override
+  bool isSubtypeOf(TypeHierarchy typeHierarchy, Class cls) =>
+      throw "ERROR: UnknownType does not support isSubtypeOf.";
+
+  @override
+  Type union(Type other, TypeHierarchy typeHierarchy) {
+    if (other is UnknownType || other is RuntimeType) {
+      return this;
+    }
+    throw "ERROR: UnknownType does not support union with ${other.runtimeType}";
+  }
+
+  // This only works between "type-set" representations ('UnknownType' and
+  // 'RuntimeType') and is used when merging type arguments.
+  @override
+  Type intersection(Type other, TypeHierarchy typeHierarchy) {
+    if (other is UnknownType || other is RuntimeType) {
+      return other;
+    }
+    throw "ERROR: UnknownType does not support intersection with ${other.runtimeType}";
+  }
+
+  bool isSubtypeOfRuntimeType(TypeHierarchy typeHierarchy, RuntimeType other) {
+    final rhs = other._type;
+    return (rhs is DynamicType) ||
+        (rhs is VoidType) ||
+        (rhs is InterfaceType &&
+            rhs.classNode == typeHierarchy.coreTypes.objectClass &&
+            rhs.nullability != Nullability.nonNullable);
+  }
+}

pkg/vm/testcases/transformations/type_flow/transformer/regress_41452_nnbd_strong.dart

@@ -0,0 +1,38 @@
+// Copyright (c) 2020, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Regression test for https://github.com/dart-lang/sdk/issues/41452.
+// Tests handling of null initializer of covariant field.
+// This test requires non-nullable experiment and NNBD strong mode.
+
+// @dart = 2.8
+
+import "package:expect/expect.dart";
+
+class _SplayTreeNode<Node extends _SplayTreeNode<Node>> {
+  Node? left;
+  _SplayTreeNode();
+}
+
+class _SplayTreeMapNode<V> extends _SplayTreeNode<_SplayTreeMapNode<V>> {
+  _SplayTreeMapNode();
+}
+
+class _SplayTree<Node extends _SplayTreeNode<Node>> {
+  Node? _root;
+
+  add(Node n) {
+    Node? root = _root;
+    if (root == null) return;
+    print(root.left); // Should be inferred as nullable.
+  }
+}
+
+class SplayTreeMap<V> extends _SplayTree<_SplayTreeMapNode<V>> {
+  _SplayTreeMapNode<V>? _root = _SplayTreeMapNode<V>();
+}
+
+void main() {
+  SplayTreeMap().add(_SplayTreeMapNode());
+}