Consider changing context type for index expressions

[stereotype441]

The current front end rule for the context of index expressions (that is, the argument to []) is currently:

• In a simple indexed read (target[index]), the context type for index is the type of the index parameter to the target's operator [].
• In a simple indexed write (target[index] = value), the context type for index is the type of the index to the target's operator []=.
• In a compound read/write (e.g. target[index] += value, target[index] ??= value, ++target[index], target[index]++, the context type for index is the type of the index parameter to the target's operator [].

The first two bullets make a lot of sense. The third bullet, not so much. There's been a lot of discussion about this at https://github.com/dart-lang/sdk/issues/49106, but since that issue is primarily about a problem with the migration tool, I'm going to try to capture the rough consensus (as I understand it).

There seemed to be general support for the idea of using the GLB of the index parameter types from both operator [] and operator []=. This would work well for maps, where the index type of operator []= is a subtype of the index type of operator []. Probably a lot of classes that implement these two operators are providing map-like functionality, so this choice will probably work well for them too.

There was also some discussion about whether we should emit a diagnostic if the user doesn't give the two operators compatible index types. It's not immediately clear to me what "compatible index types" means, though. Do we require the same relationship that Map has (index type of operator []= is a subtype of the index type of operator [])? Or do we just require that either of them be a subtype of the other? Also, should the diagnostic be an error or a lint?

Similarly, do we want a diagnostic relating the return type of operator [] to the type of the "value" parameter of operator []=? This would be similar to the diagnostic we already have for how getter and setter types must relate. And if we do want to do this, should it be an error or a lint?

[stereotype441]

CC @eernstg @lrhn @munificent @kallentu @leafpetersen @mit-mit @natebosch @jakemac53

[eernstg]

using the GLB

Sounds good!

index type of operator []= is a subtype of the index type of operator []

This makes sense because (1) it allows the same approach as Map as well as a plain "same type" approach, and (2) insofar as operator [] is a lookup and operator []= introduces something that is similar to a mapping (from a key to a value, where the index is the key and the 3rd argument is the value), it makes sense to ensure, at least, that operator [] allows every key to be specified, that is, the allowable keys for a new mapping should be a subset of the allowable keys for a lookup.

[jakemac53]

Fwiw, I don't think that [] on map should actually have the parameter type that it does. It is just really confusing and a foot gun. I think the only reason it isn't just K is because it would have been insanely breaking.

In general, I think [] and []= should be treated the same as a getter/setter pair. The typical usage falls into that pattern.

I am not sure the language should enforce that (there may be edge cases where it is inconvenient, just like there are with getters and setters), but if I were going to make a diagnostic I would make it exactly consistent with a getter/setter pair.

[munificent]

Fwiw, I don't think that [] on map should actually have the parameter type that it does. It is just really confusing and a foot gun. I think the only reason it isn't just K is because it would have been insanely breaking.

It was a deliberate choice. It makes some uses of covariant maps work that would throw runtime exceptions otherwise:

Map<String, String> stringKeys = {'key': 'value'};
Map<Object, String> objectKeys = stringKeys; // <-- Covariant upcast.
print(things[123]);

This prints false, as expected, because indeed stringKeys does not contain the key 123. If we made operator [] take K, then things[123] would do a runtime cast of 123 to String (because covariance is unsound and checked at runtime) and that cast would fail and throw an exception.

So loosening the type makes some covariant uses of collections work correctly. That's why List.contains(), etc. all take Object? and not K or E. But it also leads to a lot of bugs.

I think it was a reasonable trade-off in Dart 1, but not in Dart 2. But it's so breaking that it's hard to fix. :( Maybe if we do sound variance controls, we can figure out a way to move to collection types that are more tightly typed here.

[lrhn]

My best suggestion is to change the parameter type of Map.operator[]/Map.containsKey to K, Iterable.contains to E, etc. and then make sure that every implementation class keeps the Object? parameter type.

That will allow up-casts to keep working, but still warn you if you do something weird at the current static type. It will also allow every existing subtype to stay interface valid, since widening the parameter type is allowed in a subclass. (If it forwards to another map, it won't work, though).

What might break is code which uses an Object? typed key for lookup without downcasting it first (because it doesn't need to). A dynamic typed key will get an implicit downcast, and should still work, just a little slower.

(The Set removeAll/retainAll/intersection/difference methods are a little harder because it makes sense to use a supertype there, the two sets just need a common element supertype.)

I have tried doing this, a few times, but it's still a horribly breaking change. :)