Make prefer_collection_literals less suggestive for LinkedHashMap and LinkedHashSet

[jamesderlin]

I have code where insertion order is important, and even though Map's default implementation is a LinkedHashMap, I'd like to emphasize this in code by explicitly using LinkedHashMap:

final lruCache = LinkedHashMap<String, Stuff>();

This triggers the prefer_collection_literals lint.

I thought maybe I instead could do:

final LinkedHashMap<String, Stuff> lruCache = {};

but that runs afoul of --no-implicit-dynamic. I thought that I could give in and be redundant:

final LinkedHashMap<String, Stuff> lruCache = <String, Stuff>{};

but that doesn't work either:

error • A value of type 'Map<String, Stuff>' can't be assigned to a variable of type 'LinkedHashMap<String, Stuff>' at ... • invalid_assignment

So I think that there's no way for me to actually initialize a type declared as a LinkedHashMap while appeasing the linter.

I think that prefer_collection_literals should ignore LinkedHashMap (and LinkedHashSet). In #1425 (comment), @bwilkerson asked:

I'd like to drop the flagging of:

1. LinkedHashSet constructors altogether, and

I'm not sure why. As I understand it, a set literal creates an instance of LinkedHashSet, so the two should be consistent.

I claim that if someone is explicitly using LinkedHashMap/LinkedHashSet instead of just Map or Set, then they care about being explicit. (And if we don't think that's important, then I'd argue that LinkedHashMap and LinkedHashSet shouldn't even exist as separate classes.)

[jamesderlin]

I thought maybe I instead could do:

final LinkedHashMap<String, Stuff> lruCache = {};

but that runs afoul of --no-implicit-dynamic. I thought that I could give in and be redundant:

And that doesn't work because {} (a Map) isn't assignable to LinkedHashMap (the invalid_assignment error from the third attempt) and therefore the type isn't inferred.

So the fact that Map isn't assignable to LinkedHashMap suggests to me that the linter should not be suggesting that map literals replace a default construction of LinkedHashMap.

[bwilkerson]

The situation is a bit strange, and I'm not sure what the right answer is here.

On the one hand, the specification states that a set or map literal will preserve the order in which the elements or entries appear in the code, so you're guaranteed to get exactly the behavior you're looking for without explicitly referencing LinkedHashMap or LinkedHashSet.

On the other hand, wanting to be explicit about the fact that the set or map preserves order seems perfectly reasonable.

Unfortunately, the specification only says that the static type of a set or map literal is either Set or Map, which means that we loose information that the type system could have used to help you.

It seems to me that we have two equally valid use cases:

• a desire to use literals everywhere that doing so would not change the runtime semantics, and
• a desire to use literals everywhere that doing so would not change the declared or inferred types of variables.

I expect that the tension we're running into here is from trying to make a single lint serve both use cases. The alternative is to have two lint rules, but that runs into the problem of making it harder for users to decide which rule is the one they want to use.

[pq]

Thanks for the analysis Brian!

My gut is that the second use case is the most common:

• a desire to use literals everywhere that doing so would not change the declared or inferred types of variables.

and that loosening this lint is a reasonable compromise.

If this frustrates users down the road we could consider the ultimate solution of 2 lints but I'm hoping it won't come to that...

WDYT?

[pq]

FWIW: if we do poke this hole, I'd like to add a nice bit to the docs explaining the intent.

[natebosch]

This lint is now included in package:pedantic which is widely used and the fact that {} is not assignable to a field which is statically a LinkedHashMap forces use to add // ignore comments.

Loosening this check should be a high priority.

[pq]

Thanks @natebosch. I agree. I still lean towards adding the exception (with an explanatory doc). Unless there are any strong objections, I'll make the change.

[pq]

Looking a little at our tests, we do allow these cases:

  LinkedHashSet<int> ss6 = LinkedHashSet<int>(); // OK
  LinkedHashMap hashMap = LinkedHashMap(); // OK

Of course this then disagrees w/ omit_local_variable_types...

[pq]

Thinking about this some more, I'm not convinced that

final lruCache = LinkedHashMap<String, Stuff>();

is sufficient to emphasize ordering. If I were to be reviewing that code, I'd probably ask for an explanatory comment. Of course in that case the explicit type is redundant.

I guess I'm now firmly back on the fence. 😬

[jamesderlin]

Thinking about this some more, I'm not convinced that

final lruCache = LinkedHashMap<String, Stuff>();

is sufficient to emphasize ordering. If I were to be reviewing that code, I'd probably ask for an explanatory comment. Of course in that case the explicit type is redundant.

I guess I'm now firmly back on the fence. grimacing

I don't entirely agree. Using LinkedHashMap instead of Map should raise some suspicion in the reader. Possibly that suspicion doesn't convey intent, but it at least should give enough pause that discourages someone from haphazardly changing the type to HashMap.

Even with a comment, I think that using a Map warrants even more verbiage in the comments. For example, I would prefer:

// Ordering matters, so we must use a [LinkedHashMap].
final lruCache = LinkedHashMap<String, Stuff>();

instead of:

// Ordering matters, so we must use a [LinkedHashMap], which is the default [Map] implementation.
final lruCache = <String, Stuff>{};

[jamesderlin]

Also, given that:

  LinkedHashSet<int> ss6 = LinkedHashSet<int>(); // OK
  LinkedHashMap hashMap = LinkedHashMap(); // OK

works, then I claim that:

  var ss6 = LinkedHashSet<int>();
  var hashMap = LinkedHashMap();

should work too, at least for consistency.

[natebosch]

My primary concern right now is that the following code is impossible (AFAICT) to write while using this lint:

class Foo {
  LinkedHashMap m;
  void foo() {
    m = {}; // The map literal type 'Map<dynamic, dynamic>' isn't of expected type 'LinkedHashMap<dynamic, dynamic>'.
    m = LinkedHashMap(); // Use collection literals when possible.
  }
}

[dkwingsmt]

This has brought some (visual) mess to the Flutter code as well, such as in flutter/flutter#61190 (comment).

The following code will report "invalid_assignment",

    final LinkedHashMap<MouseTrackerAnnotation, Matrix4> annotations = <MouseTrackerAnnotation, Matrix4>{};

The following code will report "prefer_collection_literals",

    final LinkedHashMap<MouseTrackerAnnotation, Matrix4> annotations =
        LinkedHashMap<MouseTrackerAnnotation, Matrix4>();

And this is what we ended up having to use, which is pretty clumsy,

    final LinkedHashMap<MouseTrackerAnnotation, Matrix4> annotations = <MouseTrackerAnnotation, Matrix4>{}
        as LinkedHashMap<MouseTrackerAnnotation, Matrix4>;

And it appears a lot.

I doubt if making "prefer_collection_literals" less suggestive will work, since the Flutter repo is pretty strict on lint errors. I would really hope that the literal {} is considered of type LinkedHashMap, so that the assignment is legal in the first place.

[bwilkerson]

I would really hope that the literal {} is considered of type LinkedHashMap ...

The reason that it isn't is that the Dart Language Specification explicitly states that

The static type of a map literal of the form <K, V >e is Map<K, V >.

The fact that it's a LinkedHashMap in practice is an implementation detail that the implementors are free to change at will. Baking that implementation detail into the lint rule seems wrong.

There's probably a good reason for not doing so, but out of curiosity, have you considered writing

final Map<MouseTrackerAnnotation, Matrix4> annotations = {};

The specification also states that

A map is ordered: iteration over the keys, values, or key/value pairs occurs in the order in which the keys were added to the set.

So you don't need to give it a type of LinkedHashMap to ensure the ordered-ness of the map, and if you write it as above the type parameters will be inferred automatically, which reduces a lot of duplication. (I'm assuming you can't omit the type annotation because you're writing in the Flutter repo, but that would also be an alternative for some readers of this issue.)

[dkwingsmt]

The LinkedHashMap is used for a variable whose order matters, the same case as the issue owner's. I'm aware that the doc of Map says the default constructor gives an ordered map, but HashMap, which is unordered, also inherits Map, which means a Map parameter does not guarantee receiving an ordered map.

[natebosch]

I doubt if making "prefer_collection_literals" less suggestive will work, since the Flutter repo is pretty strict on lint errors. I would really hope that the literal {} is considered of type LinkedHashMap, so that the assignment is legal in the first place.

With the change suggested in this issue you won't get a lint error. We won't make {} have a static type of LinkedHashMap since it would break a bunch of existing code which assigns a map literal and uses inference.

I'm not sure if the change requested here is difficult. @pq - is this something we can prioritize?

[natebosch]

#2340 should fix some of the false positives.

After that the only decision left will be whether to allow var s = LinkedHashSet() since the type is impacting inference in that case.

[ghost]

final orderedMap = LinkedHashMap.of(<String, String>{});

doesn't trigger this rule, and orderedMap type is LinkedHashMap<String, String>. That's I need.

[robert-j-webb]

Any update on this? I don't think the PR to fix this has been touched since 2021

[pq]

There are some unaddressed comments on that PR. Either we need to fix them or start fresh...

/fyi @natebosch @bwilkerson

[natebosch]

There were some comments that I think I got stuck on but I can't recall which, I haven't looked at this in a long time.