Fix cast and retype implementations. #88

matanlurey · 2018-03-19T04:50:17Z

I had to be a little creative for QueueList. The others seem OK enough.

lrhn · 2018-03-19T06:32:31Z

lib/src/wrappers.dart

+  /// all elements stored in the returned  are actually instance of [S],
+  /// then the returned set can be used as a `MapKeySet<T>`.
+  static MapKeySet<T> castFrom<S, T>(MapKeySet<S> source) {
+    return new MapKeySet(source._baseMap.cast<T, dynamic>());


Too creative here. A MapKeySet is just as Set, so use Set.castFrom and don't introduce a new castFrom for this implementation of Set.

lrhn · 2018-03-19T06:33:47Z

lib/src/wrappers.dart

+  /// If all accessed elements of [source] are actually instances of [T] and if
+  /// all elements stored in the returned  are actually instance of [S],
+  /// then the returned set can be used as a `MapKeySet<T>`.
+  static MapValueSet<K2, V2> castFrom<K, V, K2, V2>(MapValueSet<K, V> source) {


Again, this class is just an implementation of Set, so just use Set.castFrom.

lrhn · 2018-03-19T06:38:15Z

lib/src/queue_list.dart

@@ -84,10 +102,10 @@ class QueueList<E> extends Object with ListMixin<E> implements Queue<E> {
    if (this is QueueList<T>) {


While you are here, please rewrite this as:

QueueList<Object> self = this; return self is QueueList<T> ? self : this.retype<T>();

It generates better code for dart2js to not have an explicit cast. By up-casting first. we make sure that a successful test promotes the variable.

Ditto for the cast methods below.

lrhn · 2018-03-19T06:39:06Z

CHANGELOG.md

+
+* Added `QueueList.castFrom<S, T>` (similar to `List.castFrom`, `Queue.castFrom`).
+
+* Added `MapKeySet.castFrom<S, T>` (similar to `Set.castFrom`).


No need for the last two castFrom's.

lrhn · 2018-03-19T06:43:26Z

lib/src/wrappers.dart

  final Map<E, dynamic> _baseMap;

  MapKeySet(Map<E, dynamic> base) : _baseMap = base;

  Iterable<E> get _base => _baseMap.keys;

-  Set<T> cast<T>() => _baseMap.keys.toSet().cast<T>();
+  MapKeySet<T> cast<T>() {


Just return Set<T> and use Set.castFrom.

lrhn · 2018-03-19T06:44:05Z

lib/src/wrappers.dart

@@ -513,7 +533,7 @@ class MapKeySet<E> extends _DelegatingIterableBase<E>
  E lookup(Object element) =>
      throw new UnsupportedError("MapKeySet doesn't support lookup().");

-  Set<T> retype<T>() => _baseMap.keys.toSet().retype<T>();
+  Set<T> retype<T>() => MapKeySet.castFrom<E, T>(this);


MapKeySet -> Set

lrhn · 2018-03-19T06:45:20Z

lib/src/queue_list.dart

@@ -230,3 +248,17 @@ class QueueList<E> extends Object with ListMixin<E> implements Queue<E> {
    _head = 0;
  }
 }
+
+class _CastQueueList<S, T> extends QueueList<T> {


This is creative. Tests, or it doesn't work. :)

I was hoping to extend ThingImAddingRetypeTo as well 😄

Done. Take a look - tests are basic.

lrhn · 2018-03-19T18:09:31Z

test/queue_list_test.dart

+    expect(
+      () => numQueue.add(1),
+      throwsCastError,
+      skip: isDart2 ? false : 'In checked mode (VM), CastError is not thrown',


The real reason is that cast only works in Dart 2 because it depends on the type parameter of a generic method. It's not VM specific in any way.

(In checked mode, the add(1) would throw because you are adding an int to a QueueList<String>, but it's more likely to be a TypeError than a CastError).

Thanks. Done!

Looks like the type thrown is CastErrorImplementation, which is not a TypeError.

lrhn · 2018-03-19T18:12:43Z

lib/src/queue_list.dart

    }
-    return QueueList.castFrom<E, T>(this);
+    return retype<T>();
  }

  QueueList<T> retype<T>() => QueueList.castFrom<E, T>(this);


TBH, after some more consideration, I'd remove QueueList.castFrom again.
It's a very specific implementation class, not a general interface that others might implement, so you don't need to make the wrapping available to to others.
The reason for Iterable.castFrom is that it can be used to implement Iterable.cast for other classes that implement Iterable, but nobody should implement QueueList, they should just implement Queue and List if they want a class that is both.

So, consider making castFrom private or just inline it here.

Made it private. Thanks!

lrhn · 2018-03-19T18:16:50Z

test/queue_list_test.dart

@@ -258,6 +258,25 @@ void main() {
    expect(queue.cast<Pattern>(), same(queue));
  }, skip: isDart2 ? false : 'Requires a Dart2 runtime');

+  test("cast does not throw on mutation when the type is valid", () {
+    var patternQueue = new QueueList<Pattern>()..addAll(['a', 'b']);
+    QueueList<Object> untypedQueue = patternQueue;


Why this line? Changing the static type has no effect on cast, you can just write patternQueue.cast<String>() below.

lrhn · 2018-03-19T18:18:16Z

test/queue_list_test.dart

+    var patternQueue = new QueueList<Pattern>()..addAll(['a', 'b']);
+    QueueList<Object> untypedQueue = patternQueue;
+    var stringQueue = untypedQueue.cast<String>();
+    stringQueue.addAll(['c', 'd']);


If you forgot to do the cast, the test would still succeed (either because it infers <Pattern>['c', 'd'], or because List<String> is-a List<Pattern>).

At least add expect(stringQueue is List<String>, true);, which shows that the cast actually did something (and skip in non-dart2 mode because there cast actually doesn't do anything).

lrhn · 2018-03-19T18:19:19Z

test/queue_list_test.dart

+    var stringQueue = untypedQueue.cast<String>();
+    stringQueue.addAll(['c', 'd']);
+
+    expect(stringQueue, ['a', 'b', 'c', 'd']);


Check that patternQueue was also modified.

lrhn

LGTM if you fix the type that you check against.

lrhn · 2018-03-20T07:27:54Z

test/queue_list_test.dart

    stringQueue.addAll(['c', 'd']);
+    expect(
+      stringQueue,
+      const isInstanceOf<QueueList<Pattern>>(),


Check for QueueList<String> here. That's the type you are casting to, and a type that patternQueue doesn't have, so it checks that something did happen.
(You could throw in a check that stringQueue and patternQueue are not identical, but it's not important after checking that stringQueue has a type that patternQueue doesn't).

This is the expect that needs the skip parameter, because in Dart 1 mode, the type check won't be true since cast returns patternQueue unmodified. The two tests below will still succeed when stringQueue and patternQueue are the same object.

Done. Thanks @lrhn!

* Fix cast and retype implementations. * Fix and add a few tests. * Address feedbakc.

Fix cast and retype implementations.

d826a5e

matanlurey requested review from srawlins and lrhn March 19, 2018 04:50

googlebot added the cla: yes label Mar 19, 2018

lrhn reviewed Mar 19, 2018

View reviewed changes

Fix and add a few tests.

2bf2cd6

lrhn reviewed Mar 19, 2018

View reviewed changes

Address feedbakc.

0a9c9ac

lrhn approved these changes Mar 20, 2018

View reviewed changes

Address feedback.

6c0d286

matanlurey merged commit 07bb52e into dart-archive:master Mar 20, 2018

matanlurey deleted the fix-cast-retype branch March 20, 2018 15:49

mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 18, 2024

Fix cast and retype implementations. (dart-archive/collection#88)

0ed5796

* Fix cast and retype implementations. * Fix and add a few tests. * Address feedbakc.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cast and retype implementations. #88

Fix cast and retype implementations. #88

matanlurey commented Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

srawlins Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn Mar 19, 2018

matanlurey Mar 19, 2018

lrhn left a comment

lrhn Mar 20, 2018

matanlurey Mar 20, 2018

		@@ -84,10 +102,10 @@ class QueueList<E> extends Object with ListMixin<E> implements Queue<E> {
		if (this is QueueList<T>) {


		* Added `QueueList.castFrom<S, T>` (similar to `List.castFrom`, `Queue.castFrom`).

		* Added `MapKeySet.castFrom<S, T>` (similar to `Set.castFrom`).

Fix cast and retype implementations. #88

Fix cast and retype implementations. #88

Conversation

matanlurey commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment