Skip to content

collection_methods_unrelated_type does not warn about null keys. #57101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fuzzybinary opened this issue Jun 28, 2024 · 3 comments
Open

collection_methods_unrelated_type does not warn about null keys. #57101

fuzzybinary opened this issue Jun 28, 2024 · 3 comments
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-false-negative linter-set-core P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@fuzzybinary
Copy link

Describe the issue
According to #58427, 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
Copy link
Member

That seems fair.

@srawlins
Copy link
Member

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.

@srawlins srawlins added type-enhancement A request for a change that isn't a bug P3 A lower priority bug or feature request linter-false-negative labels Jul 15, 2024
@fuzzybinary
Copy link
Author

fuzzybinary commented Jul 17, 2024

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....

@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 15, 2024
@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 15, 2024
@bwilkerson bwilkerson added area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. and removed legacy-area-analyzer Use area-devexp instead. labels Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-devexp For issues related to the analysis server, IDE support, linter, `dart fix`, and diagnostic messages. devexp-linter Issues with the analyzer's support for the linter package linter-false-negative linter-set-core P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants