dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
627 stars 172 forks source link

proposal: `list_dot_empty` #3756

Open srawlins opened 1 year ago

srawlins commented 1 year ago

list_dot_empty

Or without the dot_, whatever.

Description

Avoid using List.empty, introduced in Dart 2.9.

Details

This constructor has one bool parameter, growable.

It appears the existing prefer_collection_literals lint does not flag List.empty (at least there are no tests).

Kind

Style and performance.

Good Examples

var x = [];
var y = const [];

Bad Examples

var x = List.empty(growable: true);
var y  = List.empty();

Discussion

This came up in an internal readability discussion.

Discussion checklist

srawlins commented 1 year ago

Alternatively we could call this a false negative on prefer_collection_literals. That is a lints:recommended lint rule, enforced internally as well, so that would require cleanup before landing.

bwilkerson commented 1 year ago

Given that prefer_collection_literals is intended to enforce the Effective Dart guideline, I'd much rather see it enhanced to cover this case than to see a new lint rule.

bwilkerson commented 1 year ago

If there's a lot of cleanup that needs to be done, then I'd have to ask whether List.empty ought to be covered by any lint at all.

srawlins commented 1 year ago

And I would answer, 'yes'. 🤣

rakudrama commented 1 year ago

List.empty was added because it allows you to do something that is otherwise difficult.

List.empty is most useful in contexts where the type parameter depends on the environment, or the growable: argument is not a constant. In other cases, the suggestion seems reasonable.

Counterexamples:

The growable parameter may be e.g. a variable rather than a constant. In this case you can't choose between the suggested replacements.

srawlins commented 1 year ago

Ah that's really good context, @rakudrama . So maybe don't report if the constructor has a type variable as an explicit type argument, or when growable is not a true or false literal.

rakudrama commented 1 year ago

When growable: is false, it is not whether the constructor call has an explicit type argument, but whether the type argument, explicit or inferred, contains type variables.

Consider

List<Set<T>>  noSets<T>() => List.empty();

The inferred type argument to the constructor call is Set<T>, which contains a type variable reference T that makes the const rewrite impossible.

But these would be a reasonable candidates because the inferred or explicit type arguments contain no type arguments:

List<Set<int>> noIntSets1() => List.empty();
List<Set<int>> noIntSets2() => List<Iterable<num>>.empty();

If growable: is always true, you can replace the constructor call with [] or <T>[] regardless of whether there are type variables in the type argument.

lrhn commented 1 year ago

Yep, that!

The List.empty was introduced because I needed it during null-safety migration of the platform libraries, precisely for situations where growable was variable.

There are other ways to do everything List.empty does. Basically <T>[] for growable and List<T>.generate(0, () => throw "unreachable", growable: false) for non-growable. (Can't use List.filled since it requires an actual value of the type, which you won't have.)

Using const is not a replacement, for a number of reasons, the main one being that it doesn't work with type variables, the second one is that it creates a list with a different underlying runtime type. That can introduce polymorphism in code which would otherwise be monomorphic.

I'm all for recommending you use [] or <T>[] instead of List.empty(growable: true). For growable:false (the default), you should not recommend a replacement, using List.empty should be the recommended way to get an empty fixed-length list. Using const, even when there is no type variable included, is not the recommened way.

I don't want users to start thinking "ooh, my type here is constant, so I can use const <T>[] instead of List<T>.empty(), but over here I cannot."