simc / dartx

Superpowers for Dart. Collection of useful static extension methods.
https://pub.dev/packages/dartx
Apache License 2.0
1.08k stars 88 forks source link

zip can only zip iterables of the same type #91

Open mreichelt opened 4 years ago

mreichelt commented 4 years ago

The zip method is especially useful when we can zip items of different types - but zip in dartx is implemented to only accept a right-hand iterable of the same type. So things like this work:

final numbers = [1, 2, 3];
final multipliers = [3, 4, 5];

final multiplied = numbers.zip(multipliers, (number, multiplier) => number * multiplier).toList();
print(multiplied); // prints [3, 8, 15]

But this doesn't:

final countOfFruits = [1, 2, 3];
final fruits = ['bananas', 'apples', 'oranges'];
final combined = countOfFruits.zip(fruits, (count, fruit) => '$count $fruit').toList();

image

We can try to change the implementation of the original zip, but then we must make sure we don't break any calling code. So let's test this thoroughly. 🤓

mreichelt commented 4 years ago

Rough new implementation that works for me:

extension IterableZip<E> on Iterable<E> {
  Iterable<V> zipWith<R, V>(Iterable<R> other, V Function(E a, R b) transform) sync* {
    final it1 = iterator;
    final it2 = other.iterator;
    while (it1.moveNext() && it2.moveNext()) {
      yield transform(it1.current, it2.current);
    }
  }
}
mreichelt commented 4 years ago

Yep - this works, but unfortunately it also breaks consumer code:

image

The second parameter will now become dynamic - which is sad, because every info for Dart to find out the generic R is available at build time. It just won't pass that as generic for parameter b in transform. And Dart doesn't have operator overloading, so that doesn't work as well.

@leisim @passsy it would be great if you can add some guidance on how you would like this to be handled. I basically see two options:

  1. Keep the existing implementation, and add the new one under a different name (i.e. zipWith). That works, but then we'll have two methods where one is more broad than the other. And we'll loose the nice naming, which I would like to stay similar with Kotlin. We could deprecate zip, but still we would not be able to keep the name.
  2. Leverage the fact that dartx is still a 0.x version, and as such, can add breaking changes. Maybe we should ask the community first before we do this. I added a PR for this, just to show how this would look like and to start the discussion.
simc commented 4 years ago

@mreichelt Thanks for the detailed proposal.

Is it only me or is this a bug in Dart? In my opinion Dart should be able to infer the types correctly without manual annotations.

I don't think we should add a zipWith() method that basically does the same as the zip() method. It is not great that type annotations will be necessary in the future but I think we should replace the existing zip() method with the proposed one.

mreichelt commented 4 years ago

Ok, thanks! Then, at least I'll add some docs to the zip method later so people know how to make their code working again :)

mreichelt commented 4 years ago

@leisim I added an example in the .zip() documentation as stated. It's ready to be merged now IMO :-) https://github.com/leisim/dartx/pull/94

passsy commented 4 years ago

That's a problem I've also ran into when building kt.dart.

Screen-Shot-2020-07-09-20-25-33 80

Definitely a dart issue

I think right now would be a good time to open a dart-lang/sdk issue about this. I searched for quite a while but haven't found anything related.

details First guesses: It seems like Kotlin evaluates generic type parameters (`T`) in the arguments starting with the first argument. If types are provided, it will assume those types for all following arguments, also when an argument is a lambda of type `(T) -> Any` In Dart the generic types in the arguments don't seem to have precedence. The compiler doesn't forward previously defined generic types (`T`) to lambdas which are defined as `dynamic Function(dynamic)`. TODO: come up with an easier example than `zip()` for the bug report

Split zip in two methods

Since we're doing a breaking API change, I was thinking about aligning the zip functionality from kt_dart with dartx.

In kt_dart I've split zip into two methods

I decided against it because Dart doesn't offer tuples in the Dart SDK. Therefore we are left with a single zip method and don't have to think about the naming conflict.

Going forward