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 #57905, @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.
  }
}