[dart2js] Update inferrer with current List constructors

Use the 'growable:' argument or its absence to pick a better type for
List.of, List.from, List.generate, List.empty and List.filled.

Fixes a bug where it was assumed that List.filled was always fixed length.

Slightly better type analysis on some apps, but nothing that is measurable on benchmarks.

Change-Id: Ibe029a93488412956273fc981f626d33bd295386
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/163132
Commit-Queue: Stephen Adams <[email protected]>
Reviewed-by: Mayank Patke <[email protected]>

Author: rakudrama

pkg/compiler/lib/src/common_elements.dart

@@ -586,11 +586,12 @@ abstract class JCommonElements implements CommonElements {
   /// compilation.
   bool isUnnamedListConstructor(ConstructorEntity element);
 
-  /// Returns `true` if [element] is the 'filled' constructor of `List`.
+  /// Returns `true` if [element] is the named constructor of `List`,
+  /// e.g. `List.of`.
   ///
   /// This will not resolve the constructor if it hasn't been seen yet during
   /// compilation.
-  bool isFilledListConstructor(ConstructorEntity element);
+  bool isNamedListConstructor(String name, ConstructorEntity element);
 
   bool isDefaultEqualityImplementation(MemberEntity element);
 
@@ -879,8 +880,8 @@ class CommonElementsImpl
   /// This will not resolve the constructor if it hasn't been seen yet during
   /// compilation.
   @override
-  bool isFilledListConstructor(ConstructorEntity element) =>
-      element.name == 'filled' && element.enclosingClass == listClass;
+  bool isNamedListConstructor(String name, ConstructorEntity element) =>
+      element.name == name && element.enclosingClass == listClass;
 
   @override
   DynamicType get dynamicType => _env.dynamicType;

pkg/compiler/lib/src/inferrer/abstract_value_domain.dart

@@ -161,6 +161,11 @@ abstract class AbstractValueDomain {
   /// at runtime.
   AbstractValue get fixedListType;
 
+  /// The [AbstractValue] that represents the union of [growableListType] and
+  /// [fixedListType], i.e. JavaScript arrays that may have their elements
+  /// assigned.
+  AbstractValue get mutableArrayType;
+
   /// The [AbstractValue] that represents a non-null 31-bit unsigned integer at
   /// runtime.
   AbstractValue get uint31Type;

pkg/compiler/lib/src/inferrer/builder_kernel.dart

@@ -1188,7 +1188,34 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
     return null;
   }
 
-  /// Returns `true` if
+  /// Find the base type for a system List constructor from the value passed to
+  /// the 'growable' argument. [defaultGrowable] is the default value of the
+  /// 'growable' parameter.
+  TypeInformation _listBaseType(ir.Arguments arguments,
+      {bool defaultGrowable}) {
+    TypeInformation finish(bool /*?*/ growable) {
+      if (growable == true) return _types.growableListType;
+      if (growable == false) return _types.fixedListType;
+      return _types.mutableArrayType;
+    }
+
+    for (ir.NamedExpression named in arguments.named) {
+      if (named.name == 'growable') {
+        ir.Expression argument = named.value;
+        if (argument is ir.BoolLiteral) return finish(argument.value);
+        if (argument is ir.ConstantExpression) {
+          ir.Constant constant = argument.constant;
+          if (constant is ir.BoolConstant) return finish(constant.value);
+        }
+        // 'growable' is present, but indeterminate.
+        return finish(null);
+      }
+    }
+    // 'growable' is missing.
+    return finish(defaultGrowable);
+  }
+
+  /// Returns `true` for constructors of typed arrays.
   bool _isConstructorOfTypedArraySubclass(ConstructorEntity constructor) {
     ClassEntity cls = constructor.enclosingClass;
     return cls.library.canonicalUri == Uris.dart__native_typed_data &&
@@ -1208,7 +1235,15 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
       ArgumentsTypes argumentsTypes) {
     TypeInformation returnType =
         handleStaticInvoke(node, selector, constructor, argumentsTypes);
-    if (_elementMap.commonElements.isUnnamedListConstructor(constructor)) {
+
+    // See if we can replace the returned type with one that better describes
+    // the operation. For system List constructors we can treat this as the
+    // allocation point of a new collection. The static invoke above ensures
+    // that the implementation of the constructor sees the arguments.
+
+    var commonElements = _elementMap.commonElements;
+
+    if (commonElements.isUnnamedListConstructor(constructor)) {
       // We have `new List(...)`.
       if (arguments.positional.isEmpty && arguments.named.isEmpty) {
         // We have `new List()`.
@@ -1224,18 +1259,52 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
             () => _types.allocateList(_types.fixedListType, node,
                 _analyzedMember, _types.nullType, length));
       }
-    } else if (_elementMap.commonElements
-        .isFilledListConstructor(constructor)) {
-      // We have `new Uint32List(len, fill)`.
+    }
+
+    if (commonElements.isNamedListConstructor('filled', constructor)) {
+      // We have something like `List.filled(len, fill)`.
       int length = _findLength(arguments);
       TypeInformation elementType = argumentsTypes.positional[1];
+      TypeInformation baseType =
+          _listBaseType(arguments, defaultGrowable: false);
+      return _inferrer.concreteTypes.putIfAbsent(
+          node,
+          () => _types.allocateList(
+              baseType, node, _analyzedMember, elementType, length));
+    }
+
+    if (commonElements.isNamedListConstructor('generate', constructor)) {
+      // We have something like `List.generate(len, generator)`.
+      int length = _findLength(arguments);
+      // TODO(sra): What we really want here is the result of calling the
+      // `generator` parameter with a non-negative integer.
+      TypeInformation elementType = _types.dynamicType;
+      TypeInformation baseType =
+          _listBaseType(arguments, defaultGrowable: true);
+      return _inferrer.concreteTypes.putIfAbsent(
+          node,
+          () => _types.allocateList(
+              baseType, node, _analyzedMember, elementType, length));
+    }
 
+    if (commonElements.isNamedListConstructor('empty', constructor)) {
+      // We have something like `List.empty(growable: true)`.
+      TypeInformation baseType =
+          _listBaseType(arguments, defaultGrowable: false);
       return _inferrer.concreteTypes.putIfAbsent(
           node,
-          () => _types.allocateList(_types.fixedListType, node, _analyzedMember,
-              elementType, length));
-    } else if (_isConstructorOfTypedArraySubclass(constructor)) {
-      // We have something like `new List.filled(len, fill)`.
+          () => _types.allocateList(
+              baseType, node, _analyzedMember, _types.nonNullEmpty(), 0));
+    }
+    if (commonElements.isNamedListConstructor('of', constructor) ||
+        commonElements.isNamedListConstructor('from', constructor)) {
+      // We have something like `List.of(elements)`.
+      TypeInformation baseType =
+          _listBaseType(arguments, defaultGrowable: true);
+      return baseType;
+    }
+    if (_isConstructorOfTypedArraySubclass(constructor)) {
+      // We have something like `Uint32List(len)`.
       int length = _findLength(arguments);
       MemberEntity member = _elementMap.elementEnvironment
           .lookupClassMember(constructor.enclosingClass, '[]');
@@ -1248,9 +1317,9 @@ class KernelTypeGraphBuilder extends ir.Visitor<TypeInformation> {
               _analyzedMember,
               elementType,
               length));
-    } else {
-      return returnType;
     }
+
+    return returnType;
   }
 
   TypeInformation handleStaticInvoke(ir.Node node, Selector selector,

pkg/compiler/lib/src/inferrer/powersets/powerset_bits.dart

@@ -589,6 +589,10 @@ class PowersetBitsDomain {
   int get growableListType => _growableListType ??=
       createNonNullExact(commonElements.jsExtendableArrayClass);
 
+  int _mutableArrayType;
+  int get mutableArrayType => _mutableArrayType ??=
+      createNonNullSubtype(commonElements.jsMutableArrayClass);
+
   int get nullType => nullValue;
 
   int get nonNullType => powersetTop & ~nullValue;

pkg/compiler/lib/src/inferrer/powersets/powersets.dart

@@ -744,6 +744,11 @@ class PowersetDomain implements AbstractValueDomain {
       _abstractValueDomain.growableListType,
       _powersetBitsDomain.growableListType);
 
+  @override
+  AbstractValue get mutableArrayType => PowersetValue(
+      _abstractValueDomain.mutableArrayType,
+      _powersetBitsDomain.mutableArrayType);
+
   @override
   AbstractValue get nullType => PowersetValue(
       _abstractValueDomain.nullType, _powersetBitsDomain.nullType);

pkg/compiler/lib/src/inferrer/powersets/wrapped.dart

@@ -541,6 +541,10 @@ class WrappedAbstractValueDomain implements AbstractValueDomain {
   AbstractValue get growableListType =>
       WrappedAbstractValue(_abstractValueDomain.growableListType);
 
+  @override
+  AbstractValue get mutableArrayType =>
+      WrappedAbstractValue(_abstractValueDomain.mutableArrayType);
+
   @override
   AbstractValue get nullType =>
       WrappedAbstractValue(_abstractValueDomain.nullType);

pkg/compiler/lib/src/inferrer/trivial.dart

@@ -401,6 +401,9 @@ class TrivialAbstractValueDomain implements AbstractValueDomain {
   @override
   AbstractValue get growableListType => const TrivialAbstractValue();
 
+  @override
+  AbstractValue get mutableArrayType => const TrivialAbstractValue();
+
   @override
   AbstractValue get nullType => const TrivialAbstractValue();
 

pkg/compiler/lib/src/inferrer/type_graph_inferrer.dart

@@ -154,9 +154,8 @@ class TypeGraphInferrer implements TypesInferrer {
       parameterResults[parameter] = type;
     });
 
-    Map<ir.TreeNode, AbstractValue> allocatedLists =
-        <ir.TreeNode, AbstractValue>{};
-    Set<ir.TreeNode> checkedForGrowableLists = new Set<ir.TreeNode>();
+    Map<ir.TreeNode, AbstractValue> allocatedLists = {};
+    Set<ir.TreeNode> checkedForGrowableLists = {};
     inferrer.types.allocatedLists
         .forEach((ir.TreeNode node, ListTypeInformation typeInformation) {
       ListTypeInformation info = inferrer.types.allocatedLists[node];

pkg/compiler/lib/src/inferrer/type_system.dart

@@ -218,6 +218,10 @@ class TypeSystem {
         getConcreteTypeFor(_abstractValueDomain.growableListType);
   }
 
+  TypeInformation _mutableArrayType;
+  TypeInformation get mutableArrayType => _mutableArrayType ??=
+      getConcreteTypeFor(_abstractValueDomain.mutableArrayType);
+
   TypeInformation setTypeCache;
   TypeInformation get setType =>
       setTypeCache ??= getConcreteTypeFor(_abstractValueDomain.setType);

pkg/compiler/lib/src/inferrer/typemasks/masks.dart

@@ -131,7 +131,7 @@ class CommonMasks implements AbstractValueDomain {
 
   @override
   TypeMask get listType => _listType ??=
-      new TypeMask.nonNullExact(commonElements.jsArrayClass, _closedWorld);
+      new TypeMask.nonNullSubtype(commonElements.jsArrayClass, _closedWorld);
 
   @override
   TypeMask get constListType => _constListType ??= new TypeMask.nonNullExact(
@@ -197,6 +197,7 @@ class CommonMasks implements AbstractValueDomain {
   TypeMask get readableArrayType => _readableArrayType ??=
       new TypeMask.nonNullSubclass(commonElements.jsArrayClass, _closedWorld);
 
+  @override
   TypeMask get mutableArrayType =>
       _mutableArrayType ??= new TypeMask.nonNullSubclass(
           commonElements.jsMutableArrayClass, _closedWorld);
@@ -999,7 +1000,11 @@ String formatType(DartTypes dartTypes, TypeMask type) {
     // a null value we accidentally printed out.
     if (type.isEmptyOrNull) return type.isNullable ? 'Null' : 'Empty';
     String nullFlag = type.isNullable ? '?' : '';
-    String subFlag = type.isExact ? '' : type.isSubclass ? '+' : '*';
+    String subFlag = type.isExact
+        ? ''
+        : type.isSubclass
+            ? '+'
+            : '*';
     return '${type.base.name}$nullFlag$subFlag';
   }
   if (type is UnionTypeMask) {

pkg/compiler/lib/src/ssa/builder_kernel.dart

@@ -3935,9 +3935,8 @@ class KernelSsaGraphBuilder extends ir.Visitor {
     } else if (isJSArrayTypedConstructor) {
       // TODO(sra): Instead of calling the identity-like factory constructor,
       // simply select the single argument.
+
       // Factory constructors take type parameters.
-      if (closedWorld.rtiNeed
-          .classNeedsTypeArguments(function.enclosingClass)) {}
       List<DartType> typeArguments =
           _getConstructorTypeArguments(function, invocation.arguments);
       // TODO(johnniwinther): Remove this when type arguments are passed to
@@ -3962,6 +3961,9 @@ class KernelSsaGraphBuilder extends ir.Visitor {
       _addImplicitInstantiation(instanceType);
       _pushStaticInvocation(function, arguments, typeMask, typeArguments,
           sourceInformation: sourceInformation, instanceType: instanceType);
+
+      // TODO(sra): Special handling of List.filled, List.generate, List.of and
+      // other list constructors where 'growable' is false.
     }
 
     HInstruction newInstance = stack.last;

pkg/compiler/lib/src/ssa/optimize.dart

@@ -1309,6 +1309,9 @@ class SsaInstructionSimplifier extends HBaseVisitor
   HInstruction visitGetLength(HGetLength node) {
     HInstruction receiver = node.receiver;
     if (_graph.allocatedFixedLists.contains(receiver)) {
+      // TODO(sra): How do we keep this working if we lower/inline the receiver
+      // in an optimization?
+
       // TODO(ngeoffray): checking if the second input is an integer
       // should not be necessary but it currently makes it easier for
       // other optimizations to reason about a fixed length constructor