Add static ensure() methods to unmodifiable views.

sdk/lib/collection/collections.dart

Index: sdk/lib/collection/collections.dart
diff --git a/sdk/lib/collection/collections.dart b/sdk/lib/collection/collections.dart
index fb9a94b8832dcbfb685b08ca6d39ab0750444a8e..6f0d42692527977a809ca31c6bda463266fcd7a6 100644
--- a/sdk/lib/collection/collections.dart
+++ b/sdk/lib/collection/collections.dart
@@ -12,6 +12,18 @@ part of dart.collection;
  */
 class UnmodifiableListView<E> extends UnmodifiableListBase<E> {
   final Iterable<E> _source;
+
+  /**
+   * Returns a view of [source] that's guaranteed to be unmodifiable.
+   *
+   * If [source] already extends [UnmodifiableListBase] or
+   * [UnmodifiableListView], this avoids re-wrapping it.

[Lasse Reichstein Nielsen, 2015/08/27 07:59:04]

-> , it is returned without re-wrapping it.

(to specify what actually happens instead of re-wrapping).

[nweiz, 2015/08/27 19:25:37]

Done.

+   */
+  static List ensure(Iterable source) =>

[Lasse Reichstein Nielsen, 2015/08/27 07:59:04]

I'm not sure I like the name. It's too ... non-descriptive. It's a verb without
a subject (ensure what?).

On the other hand, UnmodifiableListView.ensureUnmodifiable is quite long. I
still think I'd prefer that, though, but I'm open to other suggestions.

[Lasse Reichstein Nielsen, 2015/08/27 07:59:04]

Move static method below constructor.

[nweiz, 2015/08/27 19:25:37]

My intention was that it would be clear that it would be ensuring
unmodifiability, because the word "Unmodifiable" always comes before "ensure"
anyway.

If that doesn't resonate with you, what about
"UnmodifiableListView.unmodifiable"?
Done.

[sra1, 2015/08/27 20:24:52]

How does this type-check in DDC?

[nweiz, 2015/08/28 00:41:29]

I'm not sure how to test that... "dartanalyzer --strong
sdk/lib/collection/collection.dart" reports a whole bunch of independent errors.

[Lasse Reichstein Nielsen, 2015/08/31 09:08:23]

UmodifiableListView.unmodifiable isn't very clear about what it does, and seems
redundant.
UnmidifiableListView.ensure is a verb and it's not clear what it's ensuring (not
even when Unmidifiable is before it, so is List and View, and it's still
hanging).
I think I prefer ensureUnmodifiable.
Alterantives like unmodifiableIfNot or ifModifiable are not great either, with
the latter *maybe* being reasonable - it's just too far from everything else we
do.

[Bob Nystrom, 2015/08/31 20:42:45]

Drive-by comment! A 39-character method name is pretty rough for a call that
often appears inside constructor initialization lists.

Personally, I think "ensure" is pretty unambiguous because of the class where it
appears. Given that it's on UnmodifiableListView, it takes a list, and returns
an UnmodifiableListView, what else could it be ensuring?

Another option would to make a class like this in package:collection:

class Unmodifiable {
  static UnmodifiableListView list(List list) { ... }
  static UnmodifiableMapView list(Map map) { ... }
  static UnmodifiableSetView list(Set set) { ... }
}

Letting you do:

_foo = Unmodifiable.list(foo);

You do lose the type argument, though. :(

It's a shame the constructors for UnmodifiableListView et. al. can't just be
factory constructors that always do this.

+      source is UnmodifiableListBase

[Lasse Reichstein Nielsen, 2015/08/27 07:59:04]

This is slightly dangerous because someone might have extended
UnmodifiableListBase, and this won't hide those extensions.
On the other hand, the conditions for not being wrapped is clearly specified, so
I think it's OK.

[sra1, 2015/10/06 19:19:22]

I don't think it is clear because it is not clear which classes are implemented
using UnmodifiableListBase.

VM runtime and JS runtime don't use UnmodifiableListBase the same way.
The VM's private _ImmutableList, used for constants, extends
UnmodifiableListBase.
dart2js const lists to not satisfy 'x is UnmodifiableListBase' and it is not
easily to make then do so because there is really only one class for const and
non-const lists - JavaScript Array.

    UnmodifiableListView.ensure(new List.unmodifiable(source))
    UnmodifiableListView.ensure(const [])

will be wrapped unpredictably (currently on one implementation and not in the
other.)

If the test was for UnmodifiableListView, the constructor could be changed to a
factory constructor (and keep the type arguments) rather than a separate ensure
method.  This would solve the DDC problems.

+          ? source
+          : new UnmodifiableListView(source);
+
   /** Create an unmodifiable list backed by [source]. */
   UnmodifiableListView(Iterable<E> source) : _source = source;

[sra1, 2015/10/06 19:19:22]

Question: why is source Iterable?

[Lasse Reichstein Nielsen, 2015/10/07 07:40:40]

Because the UnmodifiableListView is also a way to make an Iterable with
efficient length and elementAt accessible as a List without calling toList on
it.

   int get length => _source.length;