linter icon indicating copy to clipboard operation
linter copied to clipboard

Hint and/or lint for changing `List.from` to `List.of`

Open rakudrama opened this issue 4 years ago • 13 comments

Many uses of List.from can be changed to List.of. The reasons to do so:

  • List.of is nicer to use because the extra constraint helps type inference and editor suggestions
  • List.of is more efficient
  • There is an education problem that since List.of is newer, there are lots of poor examples of List.from that could have been migrated to List.of.

Examples that could be hints (i.e. no false positives):

  • The explicit or inferred type argument E in List<E>.from(iterable) matches the static type of the iterable argument
  • The explicit type argument E in List<E>.from(iterable) is a supertype of the static type of the iterable argument

More complicated is where dynamic creeps in. I have seen examples like:

void foo(Map<String, String> map) {
  for (final key in List.from(map.keys)) {
    print(key.isEmpty);
...

If we change List.from to List.of, the inferred type of key moves from dynamic to String. This would be helpful to the developer for completion suggestion, since key.isE• can now only complete to key.isEmpty, and not key.isEven. We would want to suggest the replacement if the new inferred types do not lead to new or different errors or warnings or a different resolution of elements (e.g. a dynamic instance method call becoming a static extension method call).

rakudrama avatar Mar 23 '21 19:03 rakudrama

Does the same advice extend to Set.of?

pq avatar Mar 24 '21 14:03 pq

Currious : is List<E>.of(iterableOfE) better than iterableOfE.toList() ?

If you look at https://api.dart.dev/stable/2.8.4/dart-core/Iterable/toList.html you can see that the implementation relies on List.from. Should it be changed to use List.of ?

a14n avatar Mar 24 '21 15:03 a14n

Does the same advice extend to Set.of?

And Map.of?

a14n avatar Mar 24 '21 15:03 a14n

The advice extends to Set.of and Map.of.

@a14n: Yes, we should change that List.from to List.of. That code dates back to Dart 1 before there was a List.of, and the proposed hint would help find it.


@a14n: Regarding iterableOfE, less clear cut, so I would not suggest a similar hint.

They are different in that the run time type value E for toList comes from the object, and for List<E>.of from the context of the call. I would use List.of if I wanted to update an element of the list; for lists that are not modified, toList is fine.

Reasons to use iterableOfE.toList():

  • It is more concise
  • Plays well with type infererence
  • It is in the right place when thinking of a chain like where-map-where-toList.
  • It can be a custom implementation, bound to the receiver, which can be more efficient that List.of. For this reason sometimes I wish List.of was implemented in terms of toList, but we can't because toList is less trustworthy (see below).

Reasons to use List.of:

  • You want to modify the list (e.g. assign a new element). toList might return a list with too narrow a type.
  • The compilers can do a more consistent good job with List.of because there is one implementation that is known to the compiler and the compiler knows the return type is one of the system lists, and how that depends on growable.
  • There are many implementations of toList, and some of them might be confusing for the compilers
    • The return type of toList has unconstrained variance, so it can return a subtype, e.g. => <Never>[];
    • The compilers can't always tell the exact implementation type(s) returned by toList, making handling of the returned list potentially polymorphic and less efficient.

rakudrama avatar Mar 24 '21 16:03 rakudrama

We might also consider folding this advice into Effective Dart?

See: https://dart.dev/guides/language/effective-dart/usage#dont-use-listfrom-unless-you-intend-to-change-the-type-of-the-result

pq avatar Mar 24 '21 21:03 pq

Isn't one of the main benefits of using .from that you can cast the types more precisely?

void parse(Map<String, dynamic> json) {
  for (final MapEntry<String, dynamic> entry in json.entries) {
    print("${entry.key}: ${entry.value}");
  }
}

void main() {
  final Map json= {"hello": 1, "world": true};  // typed as Map<dynamic, dynamic>
  parse(Map<String, dynamic>.of(json));  // Error: Map<dynamic, dynamic> is not Map<String, dynamic>
  parse(Map<String, dynamic>.from(json));  // okay
}

Levi-Lesches avatar Jul 13 '21 01:07 Levi-Lesches

Seems like a duplicate of https://github.com/dart-lang/linter/issues/2066?

jamesderlin avatar Jul 17 '21 20:07 jamesderlin

Well that was created later than this, and the discussion is here, so technically #2066 is a duplicate of this

Levi-Lesches avatar Jul 18 '21 06:07 Levi-Lesches

Well that was created later than this

Huh? This issue seems to have been created in March 2021 (unless transferring the issue modified the timestamp?). #2066 was filed last year. =/

jamesderlin avatar Jul 18 '21 07:07 jamesderlin

Oh sorry, I didn't see the years, just "March" and "April"

Levi-Lesches avatar Jul 18 '21 16:07 Levi-Lesches

I'm seeing this issue in Flutter framework code - using from where we reallys hould use of because of extra type checking that is incurred by from.

dnfield avatar Dec 07 '21 00:12 dnfield

And Queue.from => Queue.of.

srawlins avatar Aug 03 '22 14:08 srawlins

@Levi-Lesches to answer your question, which holds with @rakudrama 's initial suggestion, is that we would not report a List.from if the explicit type argument E in List<E>.from(iterable) is not a supertype of F where iterable as an instance of Iterable is Iterable<F> .

srawlins avatar Aug 03 '22 17:08 srawlins