Why is using a wrongly typed index into a Map not an error but just a warning

[escamoteur]

I got bitten by this here

_imageCache is defined

  final Map<ReactionType, ui.Image> _imageCache = {};

where ReatcionType is an enum. The warning says: The argument type 'String' isn't related to 'ReactionType'

even with

analyzer:
  language:
    # strict-casts: true
    # strict-inference: true

it stays a warning that throws an exception at runtime. I think this should always be treated as an error. Which use case would make sense to allow code with this to compile?

[dart-github-bot]

Summary: The user is reporting that accessing a Map with a wrongly typed index (e.g., using a String key when the Map expects a ReactionType enum) only generates a warning, not an error, even with strict type checking enabled. They believe this should be an error to prevent runtime exceptions and are asking for justification for allowing this behavior.

[lrhn]

The API of Map gives the operator [] the signature K operator[](Object? key).

It does that for a number of reasons, but mainly that you can cast a of, fx, Map<int, String> to, fx, Map<num, String> and still use it, and, fx, do numMap[3.14] without getting a type error.

The same argument applies to a number of operations:

• Iterable.contains
• List.remove
• Set.remove
• Set.lookup
• Set.intersection
• Set.difference
• Set.containsAll
• Set.removeAll
• Set.retainAll
• Map.containsKey
• Map.containsValue
• Map.operator[]
• Map.remove

All of these operations can work on objects that are a supertype of the element/key/value type, because they don't require the input to become an element/key/value. They just ask if there is one that's equal to it, and "no" is a fine answer.

It's deliberate and not something that is easy to change at this point, even if there was something good to change it to.

[escamoteur]

But as the Linzer is able to detect it, why not default it to error? Am 15. Nov. 2024, 21:29 +0100 schrieb Lasse R.H. Nielsen @.***>:

The API of Map gives the operator [] the signature K operator[](Object? key). It does that for a number of reasons, but mainly that you can cast a of, fx, Map<int, String> to, fx, Map<num, String> and still use it, and, fx, do numMap[3.14] without getting a type error. The same argument applies to a number of operations:

• Iterable.contains • List.remove • Set.remove • Set.lookup • Set.intersection • Set.difference • Set.containsAll • Set.removeAll • Set.retainAll • Map.containsKey • Map.containsValue • Map.operator[] • Map.remove

All of these operations can work on objects that are a supertype of the element/key/value type, because they don't require the input to become an element/key/value. They just ask if there is one that's equal to it, and "no" is a fine answer. It's deliberate and not something that is easy to change at this point, even if there was something good to change it to. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[lrhn]

Because we don't want to make it an error. That would require making a breaking change to some of the most implemented interfaces in the SDK (Iterable, List, Set and Map, because fixing only Map.operator[] isn't enough), which is probably not possible even if we wanted to. And again, that would require there to be something better to change it to.

It is not an error that the object's operator[] accepts Object?, it has to do so for the up-cast to be safe. And it works.

That is:

Map<int, String> imap = {1: "One"};
Map<num, String> nmap = imap; // Valid up-cast.
print(nmap[3.14]); // Works, prints `null`.
print(imap{3.14]); // Does *exactly the same thing*.
print(imap{1.0]); // Works, prints "One".

The warning is there because of code that recognizes Map.operator[] and knows that it will probably always return null if the key's static type isn't a subtype of the map's key-type. That's not always true, but it's a good rule of thumb, and worth warning you about. It's not an error, but it is suspicious enough that you should check that you're doing what you intend.

The language doesn't care. It's a type-valid invocation, and all is well.

Having it as a warning is perfectly reasonable, and that's why there is a lint that warns.

[escamoteur]

Hmm, but why can I define that lint's severity to error and it still works? I understand the reasoning why to choose that approach butike in my example when there isn't any relation between the used types I don't see what's the advantage of not reporting this as error. We had a bigger refactoring replacing previously Strings with enums and we only found this via Sentry. Am 15. Nov. 2024, 23:18 +0100 schrieb Lasse R.H. Nielsen @.***>:

Because we don't want to make it an error. That would require making a breaking change to some of the most implemented interfaces in the SDK (Iterable, List, Set and Map, because fixing only Map.operator[] isn't enough), which is probably not possible even if we wanted to. And again, that would require there to be something better to change it to. It is not an error that the object's operator[] accepts Object?, it has to do so for the up-cast to be safe. And it works. That is: Map<int, String> imap = {1: "One"}; Map<num, String> nmap = imap; // Valid up-cast. print(nmap[3.14]); // Works, prints null. print(imap{3.14]); // Does exactly the same thing. print(imap{1.0]); // Works, prints "One". The warning is there because of code that recognizes Map.operator[] and knows that it will probably always return null if the key's static type isn't a subtype of the map's key-type. That's not always true, but it's a good rule of thumb, and worth warning you about. It's not an error, but it is suspicious enough that you should check that you're doing what you intend. The language doesn't care. It's a type-valid invocation, and all is well. Having it as a warning is perfectly reasonable, and that's why there is a lint that warns. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[lrhn]

The lint that warns about an unrelated type is in the recommended and core sets. Being in the recommended set means that the Dart team wants everybody to get that warning. Being in the core set means that we consider code which get the warning as potentially dangerously flawed. It's not that we don't want you to get warned. We just don't want to make it a language error.

The language doesn't want to special-case Map.operator[]. It's just a method like any other, and it has a signature that allows Object? as argument. It doesn't have to know what the method does, it does whatever the implementation says it does, within the allowed signature.

Hmm, but why can I define that lint's severity to error and it still works?

You can turn the lint warning into an error because you can do anything you want for your own program, in your own editor, by enabling lints or using other source checks. Lints do nothing other than reporting warnings or errors for perfectly valid Dart programs, because you have opted in to having those warnings or errors.

The warning about unrelated types is not there because it expects the program to crash at runtime, but because it expects the [] invocation to always return null. (And it might not return null in some cases.) It suggests that you're doing something wrong, but not that the API itself is wrong.

Warnings are there to notify you that you are doing something that looks wrong, but might be right. You can either rewrite it to not look wrong, or add something (a comment or extra code) to say that you're doing this deliberately, and then the warning will go away.

Compile-time errors are there to tell you that your program is not a valid Dart program. The compiler refuses to compile it to something that can run, because it cannot give a valid Dart semantics to it. Real errors are strict. You can't ignore them, because the compiler will still not compile the program.

There is no desire to making unrelated collection arguments into a compile-time language error that cannot be ignored.

There is no real difference between being a linter warning or a linter error (which can still be ignored), unless the author chooses to overlook warnings.

[eernstg]

@escamoteur, do you already have the following in your analysis_options.yaml?:

analyzer:
  errors:
    collection_methods_unrelated_type: error

linter:
  rules:
    - collection_methods_unrelated_type

[escamoteur]

Hi Erick,

I did not have the setting to make the lint an error and because in our pretty big project we have quite some warnings so I didn't notice that problem and it just felt  so wrong that this isn't an error.

With the setting now it is shown as error. It might be  good thing to have this set to an error at least for application projects until you intentionally change it to warning? Am 18. Nov. 2024, 15:51 +0100 schrieb Erik Ernst @.***>:

@escamoteur, do you already have the following in your analysis_options.yaml?: analyzer: errors: collection_methods_unrelated_type: error

linter: rules:

• collection_methods_unrelated_type — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[bwilkerson]

We use 'error' to denote something that will prevent the code from being executed, and this problem doesn't have that characteristic.

I did not have the setting to make the lint an error and because in our pretty big project we have quite some warnings so I didn't notice that problem ...

I'd be interested to know which warnings being produced that you don't consider to be essential to fix. The intent is for all warnings to be actual problems in the code, typically something that will produce a run-time failure but that we can warn you about in advance. If some of our warnings don't fit that criteria, then we ought to change that.

On a practical note, you can use the mechanism from above to downgrade or disable the warnings you don't intend to fix in order to reduce the amount of noise and help you see the ones you do care about fixing. For example:

analyzer:
  errors:
    warning_to_downgrade: info
    warning_to_disable: ignore

[escamoteur]

@bwilkerson I just found out something interesting. If I don't add

    collection_methods_unrelated_type: warning

The problem is only listed as info in VS codes "problems" view and because we have many infos like unused_elements and other stuff I didn't see it. If I add the above with level "warning" it gets sorted higher up with the yellow warning sign.

Kind of weird that unsused_import is a warning compared to that.

[eernstg]

@escamoteur wrote:

It might be good thing to have this set to an error at least for application projects until you intentionally change it to warning?

I think this kind of thing would have to be done "manually" (e.g., an organization could have a rule saying "if you run dart create ... then the next action should be to run our configuration customization script").

An SDK issue (this repo) labeled with area-dart-cli and dart-cli-create could be used to request some amount of dart create configurability. On the other hand, it might not be much better than running a script. ;-)

[escamoteur]

@eernstg I understand that but as I wrote in the post above it seems that the default level of this lint isn't even "warning" but only "info"

[eernstg]

Right! @bwilkerson, do we provide any kind of support for an organization that wishes to have certain standard modifications of their analysis configuration (e.g., they always want collection_methods_unrelated_type to have level warning rather than info ), or do they need to use ad-hoc techniques like the ones I mentioned here?