Add type-asserting wrapper constructors.

Add type-asserting wrapper constructors.

Closes #25

R=lrn@google.com

Committed: https://github.com/dart-lang/collection/commit/16fae6cf8afd57b03a3f08c59a3a9556c307caa3

[nweiz, 2016-03-26 00:02:42]

nweiz@google.com changed reviewers:
+ leafp@google.com

[nweiz, 2016-03-28 22:33:37]

PTAL

I switched this over to using static methods instead of constructors so that it
could transparently return the original object if it already has the correct
reified type.

[nweiz, 2016-03-28 22:39:51]

+cc lrn

[Lasse Reichstein Nielsen, 2016-03-29 12:56:23]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2016-03-29 12:56:25]

LGTM with comments.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
File lib/src/typed_wrappers.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:20: const _TypedIterableBase();
Don't make a const constructor unless you intend to use it - you can always make
an existing constructor const (if at all possible), but going the other
direction is a breaking change.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:30: Iterable/*<T>*/ expand/*<T>*/(Iterable/*<T>*/
f(E element)) => _base.expand(_assert(f));
Long line

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:91: _UnaryFunction/*<dynamic, F>*/ _assert/*<F>*/(
Consider renaming to _validate or _ensureType. The name "assert" is loaded when
we have an assert statement in the language.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:92: _UnaryFunction/*<E, F>*/ function) =>
Just (F function(E argument)), no need for the typedef in the parameter.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:96: /// An [Iterable] that asserts the types of
values in a base iterable.
Consider referring to where the class is used from (DelegatingIterable.typed).

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:97: class TypedIterable<E> extends
_TypedIterableBase<E>
The name TypedIterable is a little too close to the unrelated typed-data for my
taste.
Maybe TypeValidatingIterable or TypeSafeIterable?

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:144: void insertAll(int index, Iterable<E> iterable)
{
Does this mean that the input iterable isn't checked, and could potentially
contain a non-E value?

https://codereview.chromium.org/1840573002/diff/20001/lib/src/wrappers.dart
File lib/src/wrappers.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/lib/src/wrappers.dart#n...
lib/src/wrappers.dart:104: /// unmodified.
So, if it's an Iterable<E>, then it's assumed to only contain E (or null)
elements - which is likely true, but probably not guaranteed. The typed version
adds a (lazy) check that all the elements are actually E instances (or null).

I think "typeChecked" is a better name than "typed" - the latter is too vague to
say what it really does, and it could mean any number of things (including being
typed-data related).

https://codereview.chromium.org/1840573002/diff/20001/test/typed_wrapper/iter...
File test/typed_wrapper/iterable_test.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/test/typed_wrapper/iter...
test/typed_wrapper/iterable_test.dart:200: <Object>["foo", "bar",
"baz"].map((element) => element));
Consider having some valid types, like
   <Object>[1, "foo', "baz"]
That would succeed at doing .first but not .last

https://codereview.chromium.org/1840573002/diff/20001/test/typed_wrapper/list...
File test/typed_wrapper/list_test.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/test/typed_wrapper/list...
test/typed_wrapper/list_test.dart:341: expect(inner, equals([15, 16, 17, "foo",
"bar", "baz"]));
Is there some way to insert a non-integer using insertAll?

https://codereview.chromium.org/1840573002/diff/20001/test/typed_wrapper/list...
test/typed_wrapper/list_test.dart:420: }, skip: "Re-enable this when test can
run DDC (test#414).");
Would it make sense to do a test where the base is mutated from the outside,
making it invalid after running correctly first. Something like:
   expect(wrapped.first, 42);
   base.insertAt(0, "foo");
   expect(wrapped#first, throwsCastError);

(Or is that just irrelevant?)

https://codereview.chromium.org/1840573002/diff/20001/test/utils.dart
File test/utils.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/test/utils.dart#newcode7
test/utils.dart:7: final Matcher throwsCastError = throwsA(new
isInstanceOf<CastError>());
Does "isInstanceOf" really start with a lower-case letter?

[nweiz, 2016-03-29 20:17:55]

Code review changes

[nweiz, 2016-03-29 20:18:05]

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
File lib/src/typed_wrappers.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:20: const _TypedIterableBase();
On 2016/03/29 12:56:24, Lasse Reichstein Nielsen wrote:
> Don't make a const constructor unless you intend to use it - you can always
make
> an existing constructor const (if at all possible), but going the other
> direction is a breaking change.

Done.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:30: Iterable/*<T>*/ expand/*<T>*/(Iterable/*<T>*/
f(E element)) => _base.expand(_assert(f));
On 2016/03/29 12:56:24, Lasse Reichstein Nielsen wrote:
> Long line

Done.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:91: _UnaryFunction/*<dynamic, F>*/ _assert/*<F>*/(
On 2016/03/29 12:56:24, Lasse Reichstein Nielsen wrote:
> Consider renaming to _validate or _ensureType. The name "assert" is loaded
when
> we have an assert statement in the language.

Done.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:92: _UnaryFunction/*<E, F>*/ function) =>
On 2016/03/29 12:56:24, Lasse Reichstein Nielsen wrote:
> Just (F function(E argument)), no need for the typedef in the parameter.

Done.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:96: /// An [Iterable] that asserts the types of
values in a base iterable.
On 2016/03/29 12:56:24, Lasse Reichstein Nielsen wrote:
> Consider referring to where the class is used from (DelegatingIterable.typed).

Done.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:97: class TypedIterable<E> extends
_TypedIterableBase<E>
On 2016/03/29 12:56:24, Lasse Reichstein Nielsen wrote:
> The name TypedIterable is a little too close to the unrelated typed-data for
my
> taste.
> Maybe TypeValidatingIterable or TypeSafeIterable?

Done.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/typed_wrappers....
lib/src/typed_wrappers.dart:144: void insertAll(int index, Iterable<E> iterable)
{
On 2016/03/29 12:56:24, Lasse Reichstein Nielsen wrote:
> Does this mean that the input iterable isn't checked, and could potentially
> contain a non-E value? 

In strong mode, I believe that's not possible. Otherwise, yes, it could, but I
think that's temporary enough and enough of an edge case that it's not worth
worrying about.

https://codereview.chromium.org/1840573002/diff/20001/lib/src/wrappers.dart
File lib/src/wrappers.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/lib/src/wrappers.dart#n...
lib/src/wrappers.dart:104: /// unmodified.
On 2016/03/29 12:56:25, Lasse Reichstein Nielsen wrote:
> So, if it's an Iterable<E>, then it's assumed to only contain E (or null)
> elements - which is likely true, but probably not guaranteed. The typed
version
> adds a (lazy) check that all the elements are actually E instances (or null).

I believe in strong mode it's guaranteed that "is Iterable<E>" implies that
methods only emit values that are "is E".

> I think "typeChecked" is a better name than "typed" - the latter is too vague
to
> say what it really does, and it could mean any number of things (including
being
> typed-data related).

I'd really like to avoid a longer name than necessary here. When it comes to
naming functions, I generally prefer a shorter name even if it's more
ambiguous—we can always disambiguate in the documentation, but the price of a
long unambiguous name must be paid over and over.

https://codereview.chromium.org/1840573002/diff/20001/test/typed_wrapper/iter...
File test/typed_wrapper/iterable_test.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/test/typed_wrapper/iter...
test/typed_wrapper/iterable_test.dart:200: <Object>["foo", "bar",
"baz"].map((element) => element));
On 2016/03/29 12:56:25, Lasse Reichstein Nielsen wrote:
> Consider having some valid types, like
>    <Object>[1, "foo', "baz"]
> That would succeed at doing .first but not .last

I think it's pretty clear from the existing tests how this would work, and I
want to avoid exponentiating them unnecessarily.

[nweiz, 2016-03-29 20:20:34]

Description was changed from

==========
Add type-asserting wrapper constructors.

Closes #25
==========

to

==========
Add type-asserting wrapper constructors.

Closes #25

R=lrn@google.com

Committed:
https://github.com/dart-lang/collection/commit/16fae6cf8afd57b03a3f08c59a3a95...
==========

[nweiz, 2016-03-29 20:20:35]

Committed patchset #3 (id:40001) manually as
16fae6cf8afd57b03a3f08c59a3a9556c307caa3 (presubmit successful).

[Lasse Reichstein Nielsen, 2016-03-29 20:49:58]

LGTM

https://codereview.chromium.org/1840573002/diff/20001/lib/src/wrappers.dart
File lib/src/wrappers.dart (right):

https://codereview.chromium.org/1840573002/diff/20001/lib/src/wrappers.dart#n...
lib/src/wrappers.dart:104: /// unmodified.
Then keep "typed". As long as it's described what it does, I guess it's ok.

The return type of `operator[]` in `List<E>` is `E` or `first` in `Iterable<E>`,
and it should allow returning null. I don't think strong mode prevents that
(unless they have incorporated non-null types too, I don't know strong mode well
enough to know for sure).

The "as E" casts will allow null, so if you can get null then the typed wrapper
won't catch it.