dart-lang / linter

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

`collection_methods_unrelated_type` does not warn about `null` keys. #5004

Open fuzzybinary opened 4 months ago

fuzzybinary commented 4 months ago

Describe the issue According to #3049, collection_methods_unrelated_type should warn about using null as a key to a non-nullable collection. However, the following code does not generate the lint:

final v = <String, String>{ “somekey” : “somevalue” };
final nv = v[null];

If this is expected, I propose another lint that will warn about indexing into collections with non-null keys with potentially null values.

To Reproduce

The following code:

final v = <String, String>{ “somekey” : “somevalue” };
final nv = v[null];

Expected behavior Warn on line 2 about null being used to on a map with non-null keys.

srawlins commented 4 months ago

That seems fair.

srawlins commented 3 months ago

Sorry, to clarify a bit: I think it is fair to warn in this specific case of a literal null (or Null-typed value) being used where a non-null is expected. But if you wrote final nv = v[nullableValue]; where nullableValue has type String?, I don't think we should report that. It seems excessive to make developers get the type exactly, where assignability (and I think reverse assignability) will do.

fuzzybinary commented 3 months ago

I would disagree a bit here, if you think of String and String? as different types.

By typing the map as Map<String, String> you are saying null is an invalid value as null is not in the type space of String. Personally, I'd rather have the warning and either be required to perform a force de-reference to get to the right type than have it fail silently like it does now.

I don't find it that excessive to have to either:

final nv = v[nullableValue!];

or

String? nullableValue;
//...
if (nullableValue != null) { 
  final nv = v[nullableValue];
}

Especially if it prevents weird bugs.

So, yes I'm glad we clarified. But, having the null literal trigger a warning would at least be a start....