proposal: unnecessary_to_list_in_spreads

[mit-mit]

Lots of Dart UI code operates on lists (e.g. a list of Widgets in Flutter). As some core library functions return iterables, some developers have a habit of calling toList() on those without reflecting on it. One example I often see is doing this in conjunction with the spread operator, where calling toList is not needed, as the spread can be called on the iterable directly.

Examples

BAD:

  children: <Widget>[
    ...['foo', 'bar', 'baz'].map((String s) => Text(s)).toList(),
  ]

GOOD:

  children: <Widget>[
    ...['foo', 'bar', 'baz'].map((String s) => Text(s)),
  ]

[lrhn]

In most places where you immediately iterate the result, calling .toList() is unnecessary (.toSet() is not, it potentially deduplicates). It does indeed do an extra allocation and creates a list which is then iterated once and discarded.

I think "immediately iterates" is currently just spreads and for/in, and it's a little iffy for for/in because you there are situations where it does make sense to make a copy of the iterable: When the later code might (concurrently) change the thing that would otherwise be iterated and break the iteration.

Say, something like:

      for (var value in set.toList()) {
        if (!relevant(value)) set.remove(value);
      }

So, hinting that a .toList on a for/in iteration is unnecessary isn't necessarily true. I don't know if there are easy heuristics to rule out the .toList() being needed, but ... probably not.

I think it'll be safe enough to do it on spreads. Maybe also in a for/in inside a collection literal, since the element expressions of collection literals are usually not having any side effects. And if they do, you should probably reconsider what you're doing. (Then do it anyway because it's cool, but you have to write the //ignore!)

It could probably be extended to immediately calling .forEach on the iterable.toList(), or any other iterable method, but there I'd actually expect the toList() to be deliberate. People are used to doing toList() at the end of an iterable method chain, I don't envision them putting it in the middle by accident very often.

[pq]

@jakemac53 @natebosch @munificent : this feels like a reasonable core lint but I think we need to get the scope just right. Would love to hear your thoughts.

[munificent]

I agree with Lasse. I think linting on toList() inside a spread is safe and a good idea, but linting on toList() inside for-in loop will likely have false positives. (I know I have code that deliberately does toList() inside a for-in loop.)

[mit-mit]

@pq any news about this one? If not, i'd like to nominate this for q1?

[pq]

i'd like to nominate this for q1?

SGTM

[pq]

My thinking is to push ahead on a lint scoped to spreads. Spitballing names, how about unnecessary_to_list_in_spreads?

[mit-mit]

@pq any news about this one?

[pq]

Hey @mit-mit. This is still in the queue.

Maybe we can get some more feedback from core lints folks? If there's a consensus this should be suggested style, it'd help bump the priority.

/cc @natebosch @munificent @jakemac53 @lrhn @goderbauer

[lrhn]

If we had it, I'd make it a recommended lint. doing [ ... iterable.toList() ] is clear unnecessary code if the toList behaves as you'd expect (which means iterate the iterable once and put the elements into a fresh native list in iteration order), because then doing [ ... iterable ] will achieve the same thing, and run the same user code other than toList, without creating an intermediate list.

I'm not going to recommend increasing priority without knowing what it's competing with :)

[goderbauer]

Sounds like a useful lint for "recommended" to me.

[jakemac53]

+1 for linting on toList() in a spread