List constructor sentinel must not implement 'int' (extend JSInt).

Having a regular object that 'is' a number was causing horrible problems for type inference and generated code quality.

To get this to work required fixes in type inference and the collection of reified default argument values.

Fixes issue https://github.com/dart-lang/sdk/issues/25566

R=sigmund@google.com

Review URL: https://codereview.chromium.org/1629553002 .

pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart

@@ -1905,7 +1905,10 @@ class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
 
     List<ir.Primitive> arguments = argumentsNode.nodes.mapToList(visit);
     // Use default values from the effective target, not the immediate target.
-    ConstructorElement target = constructor.effectiveTarget;
+    ConstructorElement target = constructor.implementation;
+    while (target.isRedirectingFactory && !target.isCyclicRedirection) {
+      target = target.effectiveTarget.implementation;
+    }
 
     callStructure = normalizeStaticArguments(callStructure, target, arguments);
     TypeMask allocationSiteType;
@@ -1916,10 +1919,11 @@ class IrBuilderVisitor extends ast.Visitor<ir.Primitive>
         Elements.isConstructorOfTypedArraySubclass(constructor, compiler)) {
       allocationSiteType = getAllocationSiteType(send);
     }
+    ConstructorElement constructorImplementation = constructor.implementation;
     return irBuilder.buildConstructorInvocation(
         target,
         callStructure,
-        constructor.computeEffectiveTargetType(type),
+        constructorImplementation.computeEffectiveTargetType(type),
         arguments,
         sourceInformationBuilder.buildNew(node),
         allocationSiteType: allocationSiteType);

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

@@ -413,6 +413,9 @@ class SimpleTypeInferrerVisitor<T>
     FunctionSignature signature = function.functionSignature;
     signature.forEachOptionalParameter((ParameterElement element) {
       ast.Expression defaultValue = element.initializer;
+      // TODO(25566): The default value of a parameter of a redirecting factory
+      // constructor comes from the corresponding parameter of the target.
+
       // If this is a default value from a different context (because
       // the current function is synthetic, e.g., a constructor from
       // a mixin application), we have to start a new inferrer visitor
@@ -1482,54 +1485,50 @@ class SimpleTypeInferrerVisitor<T>
     return super.handleTypeLiteralInvoke(arguments);
   }
 
-  /// Handle constructor invocation of [element].
-  T handleConstructorSend(ast.Send node, ConstructorElement element) {
+  /// Handle constructor invocation of [constructor].
+  T handleConstructorSend(ast.Send node, ConstructorElement constructor) {
+    ConstructorElement target = constructor.implementation;
     ArgumentsTypes arguments = analyzeArguments(node.arguments);
     if (visitingInitializers) {
       if (ast.Initializers.isConstructorRedirect(node)) {
         isConstructorRedirect = true;
       } else if (ast.Initializers.isSuperConstructorCall(node)) {
         seenSuperConstructorCall = true;
-        analyzeSuperConstructorCall(element, arguments);
+        analyzeSuperConstructorCall(constructor, arguments);
       }
     }
-    // If we are looking at a new expression on a forwarding factory,
-    // we have to forward the call to the effective target of the
-    // factory.
-    if (element.isFactoryConstructor) {
-      // TODO(herhut): Remove the while loop once effectiveTarget forwards to
-      //               patches.
-      while (element.isFactoryConstructor) {
-        ConstructorElement constructor = element;
-        if (!constructor.isRedirectingFactory) break;
-        element = constructor.effectiveTarget.implementation;
-      }
+    // If we are looking at a new expression on a forwarding factory, we have to
+    // forward the call to the effective target of the factory.
+    // TODO(herhut): Remove the loop once effectiveTarget forwards to patches.
+    while (target.isFactoryConstructor) {
+      if (!target.isRedirectingFactory) break;
+      target = target.effectiveTarget.implementation;
     }
-    if (compiler.backend.isForeign(element)) {
-      return handleForeignSend(node, element);
+    if (compiler.backend.isForeign(target)) {
+      return handleForeignSend(node, target);
     }
     Selector selector = elements.getSelector(node);
     TypeMask mask = elements.getTypeMask(node);
     // In erroneous code the number of arguments in the selector might not
     // match the function element.
     // TODO(polux): return nonNullEmpty and check it doesn't break anything
-    if (!selector.applies(element, compiler.world) ||
-        (mask != null && !mask.canHit(element, selector, compiler.world))) {
+    if (!selector.applies(target, compiler.world) ||
+        (mask != null && !mask.canHit(target, selector, compiler.world))) {
       return types.dynamicType;
     }
 
-    T returnType = handleStaticSend(node, selector, mask, element, arguments);
-    if (Elements.isGrowableListConstructorCall(element, node, compiler)) {
+    T returnType = handleStaticSend(node, selector, mask, target, arguments);
+    if (Elements.isGrowableListConstructorCall(constructor, node, compiler)) {
       return inferrer.concreteTypes.putIfAbsent(
           node, () => types.allocateList(
               types.growableListType, node, outermostElement,
               types.nonNullEmpty(), 0));
-    } else if (Elements.isFixedListConstructorCall(element, node, compiler)
-        || Elements.isFilledListConstructorCall(element, node, compiler)) {
+    } else if (Elements.isFixedListConstructorCall(constructor, node, compiler)
+        || Elements.isFilledListConstructorCall(constructor, node, compiler)) {
 
       int length = findLength(node);
       T elementType =
-          Elements.isFixedListConstructorCall(element, node, compiler)
+          Elements.isFixedListConstructorCall(constructor, node, compiler)
               ? types.nullType
               : arguments.positional[1];
 
@@ -1537,15 +1536,14 @@ class SimpleTypeInferrerVisitor<T>
           node, () => types.allocateList(
               types.fixedListType, node, outermostElement,
               elementType, length));
-    } else if (Elements.isConstructorOfTypedArraySubclass(element, compiler)) {
+    } else if (
+        Elements.isConstructorOfTypedArraySubclass(constructor, compiler)) {
       int length = findLength(node);
-      ConstructorElement constructor = element.implementation;
-      constructor = constructor.effectiveTarget;
       T elementType = inferrer.returnTypeOfElement(
-          constructor.enclosingClass.lookupMember('[]'));
+          target.enclosingClass.lookupMember('[]'));
       return inferrer.concreteTypes.putIfAbsent(
         node, () => types.allocateList(
-          types.nonNullExact(constructor.enclosingClass), node,
+          types.nonNullExact(target.enclosingClass), node,
           outermostElement, elementType, length));
     } else {
       return returnType;

pkg/compiler/lib/src/js_emitter/js_emitter.dart

@@ -34,6 +34,7 @@ import '../deferred_load.dart' show
 import '../elements/elements.dart' show
     ClassElement,
     ConstructorBodyElement,
+    ConstructorElement,
     Element,
     Elements,
     ElementKind,

pkg/compiler/lib/src/js_emitter/metadata_collector.dart

@@ -178,12 +178,42 @@ class MetadataCollector implements jsAst.TokenFinalizer {
   }
 
   List<jsAst.DeferredNumber> reifyDefaultArguments(FunctionElement function) {
+    function = function.implementation;
     FunctionSignature signature = function.functionSignature;
     if (signature.optionalParameterCount == 0) return const [];
+
+    // Optional parameters of redirecting factory constructors take their
+    // defaults from the corresponding parameters of the redirection target.
+    Map<ParameterElement, ParameterElement> targetParameterMap;
+    if (function is ConstructorElement) {
+      // TODO(sra): dart2js generates a redirecting factory constructor body
+      // that has the signature of the redirecting constructor that calls the
+      // redirection target. This is wrong - it should have the signature of the
+      // target. This would make the reified default arguments trivial.
+
+      ConstructorElement constructor = function;
+      while (constructor.isRedirectingFactory &&
+             !constructor.isCyclicRedirection) {
+        // TODO(sra): Remove the loop once effectiveTarget forwards to patches.
+        constructor = constructor.effectiveTarget.implementation;
+      }
+
+      if (constructor != function) {
+        if (signature.hasOptionalParameters) {
+          targetParameterMap =
+              mapRedirectingFactoryConstructorOptionalParameters(
+                  signature, constructor.functionSignature);
+        }
+      }
+    }
+
     List<jsAst.DeferredNumber> defaultValues = <jsAst.DeferredNumber>[];
     for (ParameterElement element in signature.optionalParameters) {
-      ConstantValue constant =
-          _backend.constants.getConstantValueForVariable(element);
+      ParameterElement parameter =
+          (targetParameterMap == null) ? element : targetParameterMap[element];
+      ConstantValue constant = (parameter == null)
+          ? null
+          : _backend.constants.getConstantValueForVariable(parameter);
       jsAst.Expression expression = (constant == null)
           ? new jsAst.LiteralNull()
           : _emitter.constantReference(constant);
@@ -192,6 +222,40 @@ class MetadataCollector implements jsAst.TokenFinalizer {
     return defaultValues;
   }
 
+  Map<ParameterElement, ParameterElement>
+  mapRedirectingFactoryConstructorOptionalParameters(
+      FunctionSignature source, FunctionSignature target) {
+    var map = <ParameterElement, ParameterElement>{};
+
+    if (source.optionalParametersAreNamed !=
+        target.optionalParametersAreNamed) {
+      // No legal optional arguments due to mismatch between named vs positional
+      // optional arguments.
+      return map;
+    }
+
+    if (source.optionalParametersAreNamed) {
+      for (ParameterElement element in source.optionalParameters) {
+        for (ParameterElement redirectedElement in target.optionalParameters) {
+          if (element.name == redirectedElement.name) {
+            map[element] = redirectedElement;
+            break;
+          }
+        }
+      }
+    } else {
+      int i = source.requiredParameterCount;
+      for (ParameterElement element in source.orderedOptionalParameters) {
+        if (i >= target.requiredParameterCount && i < target.parameterCount) {
+          map[element] =
+              target.orderedOptionalParameters[i - target.requiredParameterCount];
+        }
+        ++i;
+      }
+    }
+    return map;
+  }
+
   jsAst.Expression reifyMetadata(MetadataAnnotation annotation) {
     ConstantValue constant =
         _backend.constants.getConstantValueForMetadata(annotation);
@@ -365,4 +429,4 @@ class UnBoundDebugger extends jsAst.BaseVisitor {
     node.accept(this);
     return _foundUnboundToken;
   }
-}
+}

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

@@ -7150,21 +7150,64 @@ class SsaBuilder extends ast.Visitor
     FunctionSignature targetSignature = targetConstructor.functionSignature;
     FunctionSignature redirectingSignature =
         redirectingConstructor.functionSignature;
-    redirectingSignature.forEachRequiredParameter((ParameterElement element) {
-      inputs.add(localsHandler.readLocal(element));
-    });
+
+    List<Element> targetRequireds = targetSignature.requiredParameters;
+    List<Element> redirectingRequireds
+        = redirectingSignature.requiredParameters;
+
     List<Element> targetOptionals =
         targetSignature.orderedOptionalParameters;
     List<Element> redirectingOptionals =
         redirectingSignature.orderedOptionalParameters;
-    int i = 0;
-    for (; i < redirectingOptionals.length; i++) {
-      ParameterElement parameter = redirectingOptionals[i];
+
+    // TODO(25579): This code can do the wrong thing redirecting constructor and
+    // the target do not correspond. It is correct if there is no
+    // warning. Ideally the redirecting constructor and the target would be the
+    // same function.
+
+    void loadLocal(ParameterElement parameter) {
       inputs.add(localsHandler.readLocal(parameter));
     }
-    for (; i < targetOptionals.length; i++) {
-      inputs.add(handleConstantForOptionalParameter(targetOptionals[i]));
+    void loadPosition(int position, ParameterElement optionalParameter) {
+      if (position < redirectingRequireds.length) {
+        loadLocal(redirectingRequireds[position]);
+      } else if (position < redirectingSignature.parameterCount &&
+                 !redirectingSignature.optionalParametersAreNamed) {
+        loadLocal(redirectingOptionals[position - redirectingRequireds.length]);
+      } else if (optionalParameter != null) {
+        inputs.add(handleConstantForOptionalParameter(optionalParameter));
+      } else {
+        // Wrong.
+        inputs.add(graph.addConstantNull(compiler));
+      }
     }
+
+    int position = 0;
+
+    for (ParameterElement targetParameter in targetRequireds) {
+      loadPosition(position++, null);
+    }
+
+    if (targetOptionals.isNotEmpty) {
+      if (targetSignature.optionalParametersAreNamed) {
+        for (ParameterElement parameter in targetOptionals) {
+          ParameterElement redirectingParameter =
+              redirectingOptionals.firstWhere(
+                  (p) => p.name == parameter.name,
+                  orElse: () => null);
+          if (redirectingParameter == null) {
+            inputs.add(handleConstantForOptionalParameter(parameter));
+          } else {
+            inputs.add(localsHandler.readLocal(redirectingParameter));
+          }
+        }
+      } else {
+        for (ParameterElement parameter in targetOptionals) {
+          loadPosition(position++, parameter);
+        }
+      }
+    }
+
     ClassElement targetClass = targetConstructor.enclosingClass;
     if (backend.classNeedsRti(targetClass)) {
       ClassElement cls = redirectingConstructor.enclosingClass;

sdk/lib/_internal/js_runtime/lib/core_patch.dart

@@ -340,20 +340,11 @@ class Stopwatch {
   static int _now() => Primitives.timerTicks();
 }
 
-class _ListConstructorSentinel extends JSInt {
-  const _ListConstructorSentinel();
-}
-
 // Patch for List implementation.
 @patch
 class List<E> {
   @patch
-  factory List([int length = const _ListConstructorSentinel()]) {
-    if (length == const _ListConstructorSentinel()) {
-      return new JSArray<E>.emptyGrowable();
-    }
-    return new JSArray<E>.fixed(length);
-  }
+  factory List([int length]) = JSArray<E>.list;
 
   @patch
   factory List.filled(int length, E fill, {bool growable: false}) {

sdk/lib/_internal/js_runtime/lib/js_array.dart

@@ -4,6 +4,11 @@
 
 part of _interceptors;
 
+class _Growable {
+  const _Growable();
+}
+const _ListConstructorSentinel = const _Growable();
+
 /**
  * The interceptor class for [List]. The compiler recognizes this
  * class as an interceptor, and changes references to [:this:] to
@@ -14,6 +19,16 @@ class JSArray<E> extends Interceptor implements List<E>, JSIndexable {
 
   const JSArray();
 
+  // This factory constructor is the redirection target of the List() factory
+  // constructor. [length] has no type to permit the sentinel value.
+  factory JSArray.list([length = _ListConstructorSentinel]) {
+    if (_ListConstructorSentinel == length) {
+      return new JSArray<E>.emptyGrowable();
+    }
+    return new JSArray<E>.fixed(length);
+  }
+
+
   /**
    * Returns a fresh JavaScript Array, marked as fixed-length.
    *
@@ -21,7 +36,8 @@ class JSArray<E> extends Interceptor implements List<E>, JSIndexable {
    */
   factory JSArray.fixed(int length)  {
     // Explicit type test is necessary to guard against JavaScript conversions
-    // in unchecked mode.
+    // in unchecked mode, and against `new Array(null)` which creates a single
+    // element Array containing `null`.
     if (length is !int) {
       throw new ArgumentError.value(length, "length", "is not an integer");
     }

tests/lib/mirrors/constructor_list_test.dart

@@ -0,0 +1,23 @@
+// Copyright (c) 2016, 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.
+
+library test.constructor_lsit_test;
+
+@MirrorsUsed(targets: const [List])
+import 'dart:mirrors';
+import 'package:expect/expect.dart';
+
+main() {
+  ClassMirror cm;
+  MethodMirror mm;
+
+  cm = reflectClass(List);
+  print(cm);
+
+  var list1 = cm.newInstance(const Symbol(''), []).reflectee;
+  var list2 = cm.newInstance(const Symbol(''), [10]).reflectee;
+
+  Expect.equals(0, list1.length);
+  Expect.equals(10, list2.length);
+}

tests/lib/mirrors/constructor_optional_args_test.dart

@@ -0,0 +1,58 @@
+// Copyright (c) 2016, 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.
+
+library test.constructor_test;
+
+@MirrorsUsed(targets: const [A])
+import 'dart:mirrors';
+import 'package:expect/expect.dart';
+
+class A {
+  factory A([x, y]) = B;
+  factory A.more([x, y]) = B.more;
+  factory A.oneMore(x, [y]) = B.more;
+}
+
+class B implements A {
+  final _x, _y, _z;
+
+  B([x = 'x', y = 'y']) : _x = x, _y = y;
+
+  B.more([x = 'x', y = 'y', z = 'z']) : _x = x, _y = y, _z = z;
+
+  toString() => 'B(x=$_x, y=$_y, z=$_z)';
+}
+
+
+main() {
+  var d1 = new A(1);
+  Expect.equals('B(x=1, y=y, z=null)', '$d1', 'direct 1');
+
+  var d2 = new A.more(1);
+  Expect.equals('B(x=1, y=y, z=z)', '$d2', 'direct 2');
+
+  ClassMirror cm = reflectClass(A);
+
+  var v1 = cm.newInstance(const Symbol(''), []).reflectee;
+  var v2 = cm.newInstance(const Symbol(''), [1]).reflectee;
+  var v3 = cm.newInstance(const Symbol(''), [2, 3]).reflectee;
+
+  Expect.equals('B(x=x, y=y, z=null)', '$v1', 'unnamed 1');
+  Expect.equals('B(x=1, y=y, z=null)', '$v2', 'unnamed 2');
+  Expect.equals('B(x=2, y=3, z=null)', '$v3', 'unnamed 3');
+
+  var m1 = cm.newInstance(const Symbol('more'), []).reflectee;
+  var m2 = cm.newInstance(const Symbol('more'), [1]).reflectee;
+  var m3 = cm.newInstance(const Symbol('more'), [2, 3]).reflectee;
+
+  Expect.equals('B(x=x, y=y, z=z)', '$m1', 'more 1');
+  Expect.equals('B(x=1, y=y, z=z)', '$m2', 'more 2');
+  Expect.equals('B(x=2, y=3, z=z)', '$m3', 'more 3');
+
+  var o1 = cm.newInstance(const Symbol('oneMore'), [1]).reflectee;
+  var o2 = cm.newInstance(const Symbol('oneMore'), [2, 3]).reflectee;
+
+  Expect.equals('B(x=1, y=y, z=z)', '$o1', 'oneMore one arg');
+  Expect.equals('B(x=2, y=3, z=z)', '$o2', 'oneMore two args');
+}