Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QueueList's cast and retype do not adhere to the List methods' documentation #590

Closed
srawlins opened this issue Mar 16, 2018 · 3 comments · Fixed by dart-archive/collection#88
Assignees
Labels
package:collection type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@srawlins
Copy link
Member

srawlins commented Mar 16, 2018

I'd like to say that QueueList's cast and retype do not adhere to the contract laid out by List.cast and List.retype but that may be up for debate. In any case, List.cast's documentation says:

Returns a view of this list as a list of R instances, if necessary.

List.retype's documentation says, similarly:

Returns a view of this list as a list of R instances.

(Ignoring for now what "if necessary" must mean...) I believe it is intended that the return value be the same list (just a different view of it), not a new list. For example, let's play with List:

void main() {
  var a = new List<num>()..addAll([1,2,3]);
  var b = a.retype<int>();
  b.add(4); // add 4 to the view.
  a.add(5); // add 5 to the original.
  b.remove(3); // remove 3 from the view.
  print('a: $a'); //=> [1, 2, 4, 5]
  print('b: $b'); //=> [1, 2, 4, 5]
}

They are the same list. Now let's do the same for QueueList:

import 'package:collection/collection.dart';

void main() {
  var c = new QueueList<num>()..addAll([1,2,3]);
  var d = c.retype<int>();
  d.add(4);
  print('c: $c'); //=> {1, 2, 3} // Not the same list!
  print('d: $d'); //=> {1, 2, 3, null, null, null, null, null, 4} // Duck?
  c.add(5);
  print('c: $c'); //=> {1, 2, 3, 5}
  print('d: $d'); //=> {1, 2, 3, null, null, null, null, null, 4}
}

I have not checked the other cast and retype implementations yet, but when I do, I presume I'll just add on to this bug, rather than filing a bug for each class.

@matanlurey
Copy link
Contributor

I believe it is intended that the return value be the same list (just a different view of it), not a new list.

I don't think so (@leafpetersen @lrhn), even though I wish that was the case - I think .cast tries to re-use the same list, and basically invokes .retype if not possible, making this OK.

I.e. this is just a convenience for calling List.castFrom:

https://github.com/dart-lang/sdk/blob/f1ebe2bd5cfcb6b522e5b4fd406cdabb1a2d2091/sdk/lib/collection/list.dart#L332-L335

Let's look at the implementation of DoubleLinkedQueue.cast:

https://github.com/dart-lang/sdk/blob/f1ebe2bd5cfcb6b522e5b4fd406cdabb1a2d2091/sdk/lib/collection/queue.dart#L359-L362

Unfortunately there is no way in Dart to really "re-type" something, so unlike Dart 1's as List<String>, list.cast<String>() may return a new list if necessary to fulfill that cast.

@lrhn lrhn changed the title ListQueue's cast and retype do not adhere to the List methods' documentation QueueList's cast and retype do not adhere to the List methods' documentation Mar 16, 2018
@lrhn
Copy link
Member

lrhn commented Mar 16, 2018

Both cast or retype are really intended to return a view of the original collection, one that also forwards mutating methods. The "if necessary" means that cast returns the original list if it already implements the desired interface, possibly by implementing a subtype, where retype always wraps so the return type is exact. It should not create a new collection that isn't linked to the original.

The QueueList class has the extra issue of needing to implement both List.cast and Queue.cast, so it needs to return something that is both a List and a Queue. I have a class ready for that in the private parts of the SDK, because one of the plans for Dart 2 was to make ListQueue implement List as well, but that doesn't help this package.

@matanlurey matanlurey added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Mar 19, 2018
@matanlurey
Copy link
Contributor

Let me give this a shot, thanks @srawlins and @lrhn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:collection type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants