Sdk: Add generics to new Map.fromIterable()

[nex3]

Currently, new Map.fromIterable() is very difficult to use in strong mode, because it doesn't support type propagation from the Iterable argument to the callbacks:

Map.fromIterable(Iterable elements, {K key(element), V value(element)});

We should fix this, either using #26391 to make the constructor generic, or by making it a static method instead of a constructor.

[jacehensley-wf]

I just ran into a case where this would be great, when adding some new lints.

[kevmoo]

@floitschG @lrhn – would be great to consider for v2

[lrhn]

Definitely. It's one of the clear use-cases for #26391.

[matanlurey]

Uh oh - this is now a warning in the latest dev released of the SDK:

class Foo {
  String bar;
}

void doThing(Iterable<Foo> foos) {
  new Map<Foo, String>.fromIterable(foos, value: (Foo foo) => foo.bar);
}

This is a hint, soon to be an error, because the definition dynamic -> String is expected:

value: (/* Dynamic as bottom */ foo) => (foo as Foo).bar

[natebosch]

With implicit-casts: false does this make it impossible to implement a fallback for these functions?

The current implementation has a default of the identify function for both key and value - I don't think it could do that if we had stricter generic types.

The tradeoff is probably worth it, but if no-implicit-casts becomes the default it's a bit harder to do.

[natebosch]

Ah, you can use a runtime cast in that case

[matanlurey]

+1. I would really _really_ like to see this either get fixed, or a new method introduced, i.e.:

abstract class Map<K, V> {
  static Null _null() => null;
  static Map<K, V, E> ofIterable<K, V, E>(Iterable<E> elements, {
    K Function(E) value: _null,
    V Function(E) value: _null,
  })
}

I ran into a particularly nasty set of cases internally here:
https://dartpad.dartlang.org/6cb0fc63842837afa14ecf7e65e0642a

[lrhn]

Adding the ofIterable is the least intrusive change.

Since fromIterable is a constructor, and constructors cannot (yet) be generic, we can't fix it - we're blocked on the language feature of generic constructors. We also can't make it a non-constructor since someone might be calling it using new. I wish we could fix it, but currently there really is no good solution.
Maybe we could fix the argument types of key and value to be Null, then do dynamic invocations inside the body, but the call will type-check with (Foo foo) => foo.bar as argument.
... Except that that doesn't actually work either, because writing K key(Null element) as the parameter means that some function literal arguments will infer Null or Object as the argument type, not dynamic like now, and then (id) => id.name is no longer valid and hilarity ensues.
So, I really can't fix Map.fromIterable at all without breaking code.

Even the ofIterable described above is not safe. You can omit the key and value arguments, and that only works if E is a subtype of K or V. The type system doesn't catch that, you will get a run-time error if you do:

Map<int, String> m = Map.fromIterable(<Object>["1"]); 

Everything type-checks, but the omitted key function is effectively replaced by (Object o) => o as int.

Also, inference might not do what you expect in all cases.

var map = Map.ofIterable([1, 2, 3], key: (x) => x + 1, value: (x) => x * 2);

You'd probably expect this to be inferred as:

Map<int, int> map = Map.ofIterable<int, int, int>(<int>[1, 2, 3], 
    key: (int x) => x + 1, value: (int x) => x * 2);

It isn't. There is no downward information at all, and the arguments are inferred independently, so first we get to:

var map = Map.ofIterable(<int>[1, 2, 3], 
    key: (dynamic x) => x + 1, value: (dynamic x) => x * 2);

Then the type of the map is derived from the arguments as:

Map<dynamic, dynamic> map = Map.ofIterable<dynamic, dynamic, int>(<int>[1, 2, 3], 
    key: (dynamic x) => x + 1, value: (dynamic x) => x * 2);

If you want this function, I can add it. I already have the CL. I'm just afraid that it will cause problems when it still doesn't work as you would expect.

[bergwerf]

I hope this is planned somewhere not too far down the road ;). I keep running into cases where this causes some frustration/unexpected problems (for example using a typed argument in the key or value closure will throw a run-time type error now that is not predicted by the analyzer), and then I end up removing types or adding a utility function similar to ofIterable. I would say that, to prevent everyone from diverging here, it is relatively important that an 'official' workaround is added soon.

[lrhn]

The official workaround is to use Map.fromEntries and map the iterable to a MapEntry iterable manually.

  var myMap = Map.fromEntries(iterable.map((i) => MapEntry(key(i), value(i)));

All the other functions are error prone because they do some things implicitly that might not always work.

We can't make constructors generic, so there is very little we can do for something structured like fromIterable that is significantly better than what we already have.
I think the best solution in that direction that we can get to, which isn't perfect, is a static function:

static Map<K, V> ofIterable<K, V, I>(Iterable<I> elements, K key(I element), V value(I element)) ...

By not making the key/value arguments optional, we avoid the case where omitting them introduces an unlisted run-time cast from I to K or V. On the other hand, we now have two unnamed function parameters that are easy to forget the order of.
It's still not great, and we still have fromIterable around.
If we get generic constructors (and we will), then we can make the constructor at least take the iterable element type as type argument. Until then, things are annoying, so let's take this as an incentive to get those generic named constructors.

[Hixie]

Just ran into this. It makes this constructor quite awkward to use.

[om-ha]

As of Jan 2020, this is still an issue.

final List<MyItem> itemsList = getItemsIterableList();

final Map<int, MyItem> itemsMap = Map.fromIterable(
  itemsList,
  key: (item) => item.Id,
  value: (item) => item,
);

In this case, item is of dynamic type in both key and value parameters.

[lrhn]

At least in 2020, we have map literals like:

final itemsMap = {for (var item in itemsList) item.id: item};

:smile:

(I'm never going to use Map.fromIterable again!)

[szepeshazi]

Just another awkward situtation with Map.fromIterable and extension methods.

class Person {
  final String firstName;
  final String lastName;

  const Person(this.firstName, this.lastName);
}

extension FullName on Person {
  String get fullName => "$firstName $lastName";
}

void main() {
  const List<Person> persons = [Person("John", "Doe"), Person("Jane", "Doe")];
  Map<String, Person> byFullName = Map.fromIterable(persons, key: (person) => person.fullName);
}

The Map.fromIterable constructor will throw runtime, Uncaught TypeError: person.get$fullName is not a function.

[gothamgasworks]

I think another solutoin could be to create a toMap extension method on Iterable.

[lrhn]

The Map.fromIterable constructor suffers from constructors not being generic and type inference not pushing information between arguments. That's why the person variable is typed as dynamic, and it would probably stay that way even if constructors could be generic.

I recommend forgetting that Map.fromIterable exists, it really doesn't work that well in Dart 2, and just use a map literal instead:

Map<String, Person> byFullName = {for (var person in persons) person.fullName: person};

[bergwerf]

@lrhn I missed that this kind of map building notation was introduced. Where can I find more about it? After a quick search I couldn't find it mentioned anywhere else (including the language tour and the specification document section 16.10).

[natebosch]

@bergwerf

The language tour section is: https://dart.dev/guides/language/language-tour#collection-operators

There is also a really nice writeup that @munificent wrote about these features at https://medium.com/dartlang/making-dart-a-better-language-for-ui-f1ccaf9f546c