Make List constructor give better error messages for non-int arguments

tests/corelib/growable_list_test.dart

 // Copyright (c) 2011, 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.
 
 // Sanity check on the growing behavior of a growable list.
 
 import "package:expect/expect.dart";
-import "dart:collection" show IterableBase;
+
+void main() {
+  testConstructor();
+
+  bool checked = false;
+  assert((checked = true));
+  // Concurrent modification checks are only guaranteed in checked mode.
+  if (checked) testConcurrentModification();
+}
 
 // Iterable generating numbers in range [0..count).
 // May perform callback at some point underways.
-class TestIterableBase extends IterableBase<int> {
+class TestIterableBase extends Iterable<int> {
   final int length;
   final int count;
   // call [callback] if generating callbackIndex.
   final int callbackIndex;
   final Function callback;
   TestIterableBase(this.length, this.count,
                    this.callbackIndex, this.callback);
   Iterator<int> get iterator => new CallbackIterator(this);
 }
 
@@ skipping 25 matching lines @@
     _current = _nextIndex;
     _nextIndex++;
     if (_current == _iterable.callbackIndex) {
       _iterable.callback();
     }
     return true;
   }
   int get current => _current;
 }
 
+void testConstructor() {
+  // Constructor can make both growable and fixed-length lists.
+  testGrowable(list) {
+    Expect.isTrue(list is List<int>);
+    Expect.isFalse(list is List<String>);
+    int length = list.length;
+    list.add(42);
+    Expect.equals(list.length, length + 1);
+  }
 
-void main() {
+  testFixedLength(list) {
+    Expect.isTrue(list is List<int>);
+    int length = list.length;
+    Expect.throws(() { list.add(42); }, null, "adding to fixed-length list");
+  }
+
+  bool checked = false;
+  assert((checked = true));
+  testThrowsOrTypeError(fn, test, [name]) {
+    Expect.throws(fn, checked ? null : test,

[Lasse Reichstein Nielsen, 2015/07/07 13:08:02]

null should be ((e)=>e is TypeError), but dartj2s seems to bypass the type check
and call the constructor directly in some cases.

[herhut, 2015/07/08 08:59:58]

Yes, this happens early on in handleNewSend in the SSABuilder. That code is
wrong and should be fixed.

[Lasse Reichstein Nielsen, 2015/07/08 09:03:39]

I'm not sure it's necessarily wrong, it's throwing in all the right places, just
not as informative an error as possible. That's still acceptable.

I won't force dart2js to throw the more precise error if it makes the code
shorter to not do so (and if it would make the code shorter to go through the
constructor, it will probably happen anyway).

+                      checked ? name : "$name w/ TypeError");
+  }
+  testFixedLength(new List<int>(0));
+  testFixedLength(new List<int>(5));
+  testGrowable(new List<int>());
+  testGrowable(new List<int>()..length = 5);
+  Expect.throws(() => new List<int>(-1), (e) => e is ArgumentError, "-1");
+  // There must be limits. Fix this test if we ever allow 10^30 elements.
+  Expect.throws(() => new List<int>(0x1000000000000000000000000000000),
+                (e) => e is ArgumentError, "bignum");
+  Expect.throws(() => new List<int>(null), (e) => e is ArgumentError, "null");
+  testThrowsOrTypeError(() => new List([]), (e) => e is ArgumentError, 'list');

[sra1, 2015/07/08 09:57:01]

Have two versions of each of these:

    new List(x)
    new List(confuse(x))

Can you split the test so that constructor argument errors should be in a
different test file to concurrent modification errors?

+  testThrowsOrTypeError(() => new List([42]),
+                        (e) => e is ArgumentError, "list2");

[Lasse Reichstein Nielsen, 2015/07/07 13:08:02]

I wanted to check for RangeError here, but it seems some parts of dart2js
bypasses the js_array code and throws the JS exception directly (which is
converted to ArgumentError).

[herhut, 2015/07/08 08:59:57]

See above.

+}
+
+void testConcurrentModification() {
   // Without EfficientLength interface
   {
-    // Change length of list after 20 additions.
+    // Change length of list after 200 additions.
     var l = [];
     var ci = new TestIterable(257, 200, () {
       l.add("X");
     });
     Expect.throws(() {
       l.addAll(ci);
-    }, (e) => e is ConcurrentModificationError);
+    }, (e) => e is ConcurrentModificationError, "cm1");
   }
 
   {
-    // Change length of list after 20 additions.
+    // Change length of list after 200 additions.
     var l = [];
     var ci = new TestIterable(257, 200, () {
       l.length = 0;
     });
     Expect.throws(() {
       l.addAll(ci);
-    }, (e) => e is ConcurrentModificationError);
+    }, (e) => e is ConcurrentModificationError, "cm2");
   }
 
   // With EfficientLength interface (uses length).
   {
     // Change length of list after 20 additions.
     var l = [];
     var ci = new EfficientTestIterable(257, 257, 20, () {
       l.add("X");
     });
     Expect.throws(() {
       l.addAll(ci);
-    }, (e) => e is ConcurrentModificationError);
+    }, (e) => e is ConcurrentModificationError, "cm3");
   }
 
   {
     var l = [];
     var ci = new EfficientTestIterable(257, 257, 20, () {
       l.length = 0;
     });
     Expect.throws(() {
       l.addAll(ci);
-    }, (e) => e is ConcurrentModificationError);
+    }, (e) => e is ConcurrentModificationError, "cm4");
   }
 
   {
-    // Length 50, only 25 elements.
+    // Length 500, only 250 elements.
     var l = [];
     var ci = new EfficientTestIterable(500, 250);
     l.addAll(ci);
-    Expect.listEquals(new List.generate(250, (x)=>x), l);
+    Expect.listEquals(new List.generate(250, (x)=>x), l, "cm5");
   }
 
   {
-    // Length 25, but 50 elements.
+    // Length 250, but 500 elements.
     var l = [];
     var ci = new EfficientTestIterable(250, 500);
     l.addAll(ci);
-    Expect.listEquals(new List.generate(500, (x)=>x), l);
+    Expect.listEquals(new List.generate(500, (x)=>x), l, "cm6");
   }
 
   {
     // Adding to yourself.
     var l = [1];
-    Expect.throws(() { l.addAll(l); }, (e) => e is ConcurrentModificationError);
+    Expect.throws(() { l.addAll(l); },
+                  (e) => e is ConcurrentModificationError, "cm7");
   }
 
   {
     // Adding to yourself.
     var l = [1, 2, 3];
-    Expect.throws(() { l.addAll(l); }, (e) => e is ConcurrentModificationError);
+    Expect.throws(() { l.addAll(l); },
+                  (e) => e is ConcurrentModificationError, "cm8");
   }
 }