SkipIterable - ensure field is only initialized to a known int.

sdk/lib/internal/iterable.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.
 
 part of dart._internal;
 
 /**
  * Marker interface for [Iterable] subclasses that have an efficient
  * [length] implementation.
  */
@@ skipping 569 matching lines @@
   final Iterable<E> _iterable;
   final int _skipCount;
 
   factory SkipIterable(Iterable<E> iterable, int count) {
     if (iterable is EfficientLengthIterable) {
       return new EfficientLengthSkipIterable<E>(iterable, count);
     }
     return new SkipIterable<E>._(iterable, count);
   }
 
-  SkipIterable._(this._iterable, this._skipCount) {
-    if (_skipCount is! int) {
-      throw new ArgumentError.value(_skipCount, "count is not an integer");
+  SkipIterable._(this._iterable, int skipCount)
+      : _skipCount = _checkSkip(skipCount);

[Lasse Reichstein Nielsen, 2017/04/28 06:25:23]

How about inlining the check:

 : _skipCount = ((skipCount is int) ? skipCount
     : throw new ArgumentError.value(skipCount, "skipCount", 
           "not an integer")) {
  RangeError.checkNotNegative(_skipCount, "skipCount");
}

or combined:

 : _skipCount = ((skipCount is int && skipCount >= 0) ? skipCount
     : throw new ArgumentError.value(skipCount, "skipCount", 
           "not a non-negative integer"));

[sra1, 2017/04/28 18:38:36]

The trouble with combined conditions (&&, ||) is that they are compiled as
expressions yielding a value, which makes it difficult for dart2js to understand
that all the conditions apply to the final value in the non-throw branch.

I want to fix this problem with dart2js but it is harder than is should be.

+
+  static int _checkSkip(int skipCount) {

[Lasse Reichstein Nielsen, 2017/04/28 06:25:23]

If not inlined, maybe name this _checkCount and remove references to "skip".
It's generic enough that it can be reused.

[sra1, 2017/04/28 18:38:37]

Done.

+    if (skipCount is! int) {
+      throw new ArgumentError.value(skipCount, "count is not an integer");

[Lasse Reichstein Nielsen, 2017/04/28 06:25:23]

Second argument is the argument name, not a message, so:

throw new ArgumentError.value(skipCount, "skipCount", "not an integer");

[sra1, 2017/04/28 18:38:36]

Acknowledged.

     }
-    RangeError.checkNotNegative(_skipCount, "count");
+    RangeError.checkNotNegative(skipCount, "count");

[Lasse Reichstein Nielsen, 2017/04/28 06:25:23]

Could we combine the two:

if (skipCount is! int || skipCount < 0) {
  throw new ArgumentError.value(skipCount, "skipCount",
      "not a non-negative integer");
}

[sra1, 2017/04/28 18:38:37]

See above. dart2js does much better at understanding code that goes one if-test
at a time.

[Lasse Reichstein Nielsen, 2017/05/01 05:32:00]

I'm a little concerned about writing code to match the quirks of a single
implementation. It works, but you improve dart2js, will we go back and fix the
code? I haven't been told about any improvements to dart2js that means I can
change things back to the more immediate style for changes we have done in the
past. And are we sure it won't make things worse for the VM?

IOW, it is, or should be, a short-term hack to fix the library, not the compiler
(because it only affects our library code, not user code), but I'm seeing it
being used as a "solution" and never turned back.

That worry aside, let's do this now, but please, make it a priority to fix the
real problem.

+    return skipCount;
   }
 
   Iterable<E> skip(int count) {
     if (_skipCount is! int) {

[sra1, 2017/04/28 18:38:37]

Why is this checking _skipCount instead of count?

[Lasse Reichstein Nielsen, 2017/05/01 05:32:00]

That looks like a bug. Good catch.

       throw new ArgumentError.value(_skipCount, "count is not an integer");
     }
     RangeError.checkNotNegative(_skipCount, "count");
     return new SkipIterable<E>._(_iterable, _skipCount + count);

[sra1, 2017/04/28 18:38:36]

.skip(1).skip(-1) 'works'

[Lasse Reichstein Nielsen, 2017/05/01 05:32:00]

Iterable.skip is documented as requiring a non-negative argument, so "working"
isn't a good thing, it's just a way to hide a bug.

   }
 
   Iterator<E> get iterator {
     return new SkipIterator<E>(_iterable.iterator, _skipCount);
   }
 }
 
 class EfficientLengthSkipIterable<E> extends SkipIterable<E>
     implements EfficientLengthIterable<E> {
   EfficientLengthSkipIterable(Iterable<E> iterable, int skipCount)
@@ skipping 149 matching lines @@
  * Creates errors throw by [Iterable] when the element count is wrong.
  */
 abstract class IterableElementError {
   /** Error thrown thrown by, e.g., [Iterable.first] when there is no result. */
   static StateError noElement() => new StateError("No element");
   /** Error thrown by, e.g., [Iterable.single] if there are too many results. */
   static StateError tooMany() => new StateError("Too many elements");
   /** Error thrown by, e.g., [List.setRange] if there are too few elements. */
   static StateError tooFew() => new StateError("Too few elements");
 }