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

prefer_spread_collections trigger on .toIterable() #5092

Open
FMorschel opened this issue Sep 6, 2024 · 3 comments
Open

prefer_spread_collections trigger on .toIterable() #5092

FMorschel opened this issue Sep 6, 2024 · 3 comments
Labels
P3 A lower priority bug or feature request set-recommended Affects a rule in the recommended Dart rule set type-enhancement A request for a change that isn't a bug

Comments

@FMorschel
Copy link
Contributor

Describe the issue
The current description or prefer_spread_collections:

Collection literals are excellent when you want to create a new collection out of individual items. But, when existing items are already stored in another collection, spread collection syntax leads to simpler code.

I'd like to bring focus to the end:

... spread collection syntax leads to simpler code.

So I'd like to propose for it to trigger on methods like toSet and toList.

To Reproduce

final list = <int>[];
final set = list.toSet(); // <-- no-trigger

Expected behavior

For it to trigger prefer_spread_collections in the second line.

@github-actions github-actions bot added the set-recommended Affects a rule in the recommended Dart rule set label Sep 6, 2024
@lrhn
Copy link
Member

lrhn commented Sep 8, 2024

I'm not sure the difference between {...list} vs list.toSet() is big enough to encourage either over.

Which one is more readable depends on what you're more used to. The toSet() is definitely more explicit.

I'm not sure I'd want a recommendation for spreads over .toSet to be in the recommended set.

@pq pq added the P3 A lower priority bug or feature request label Sep 9, 2024
@FMorschel
Copy link
Contributor Author

The only reason I lean over using the spread operator is that for non-nullable fields/variables, it is easier to use it over the .toSet() call since it would remain null if any call before it was null. In this case, I prefer {...?list} than list?.toSet() ?? {} since (I think may be related to dart-lang/language#3527 or dart-lang/language#3009, not sure) some of the time the type inference can't solve the right side of ?? and I need to add the generics there, whereas in the first example, I don't have to. Also, less writing when using the spread operator in this case.

@lrhn
Copy link
Member

lrhn commented Sep 17, 2024

I prefer {...?list} than list?.toSet() ?? {}

Ack, me too. The former makes it easier to read that the set is created no matter what, and the elements are conditioned on list not being null or empty. The latter has to be read to conclusion to recognize that a set is created.

(Do consider not having nullable lists at all, and always use an empty list instead of null, so there is only one way to represent zero elements. Unless you need to distinguish an empty list from no list, which this operation doesn't.)

On the other hand, I don't prefer {...list} over list.toSet().

The two expressions are probably mostly equivalent in most cases.
The list.toSet() has a (very low) chance to be slightly more efficient if the iterable knows something more about the elements than how to iterate them. It doesn't have to create an Iterator and iterate through it.
A List.toSet can return {for (var i = 0; i < this.length; i++) this[i]} which is a more efficient iteration than going through an iterator.
(The ... might not use the iterator either, if it can recognize the list as a platform List, but that's significantly harder than having the list itself know that it's a list.)
That concern also applies to list?.toSet() ?? {}, but that expression has the extra risk of creating two different kinds of sets, and making the value polymorphic. (It's "but"s all the way down, there is no one-size-fits-all!)

All considerations included, there is no clear winner in either performance or readability that applies to all cases, so I wouldn't want to mandate, or even recommend, one of {...list} and list.toSet() over the other.

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 A lower priority bug or feature request set-recommended Affects a rule in the recommended Dart rule set type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants