facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.08k stars 1.86k forks source link

Why are the new $ReadOnly{Map,WeakMap,Set,Weakset} types invariant? #7198

Closed STRML closed 1 year ago

STRML commented 5 years ago

https://github.com/facebook/flow/blob/master/Changelog.md#0860 indicates that the new $ReadOnly types are invariant on their type parameter, unlike $ReadOnlyArray<T>. Due to Flow's closed development style there is no explanation of this ideological change. Is there a technical reason for this or a shift in thinking for Flow's future support of variance?

jbrown215 commented 5 years ago

Yes. For the maps, the key value must be invariant because it appears as a function input. We don't check variance on declare classes today, but we ought to, so adding type declarations for types that would not be valid in the future didn't seem like a good idea.

For sets, they also have the element value in a contravariant position (function parameter in has), which is why it could not be covariant.

The philosophical question to answer is should we allow types in our libdefs that would be rejected by flow if it wasn't a libdef? Because that question requires a lot more thought, we chose to be conservative here until demand forces us to change our minds. Internally, it fit the engineer's use case to keep these type parameters invariant. They were primarily concerned about the interface being exposed to a function, not the subtyping behavior in the type parameters.

I would accept a PR to change the value type on the Maps to be covariant, but am unsure about the other parameters.

wchargin commented 5 years ago

For the maps, the key value must be invariant because it appears as a function input. […]

For sets, they also have the element value in a contravariant position (function parameter in has), which is why it could not be covariant.

These are interesting points. There is a reasonable argument that the arguments to get and has should be of type mixed, not T: it would be totally sound, with respect to the semantics of Map and Set, to do so. (Java, for instance, takes this approach.) The arguments to get and has are never stored anywhere or returned from any other methods, and in fact the implementations of these methods need not even know that these arguments are of any particular type.

There are also reasonable arguments that having the type parameters strictly specified (as is) is the right thing to do, as violations of these strict semantics in simple cases will usually indicate a programmer error even if they would not immediately lead to a runtime error.

It is also interesting to note that we might conceive of map/set implementations for which we do not have the luxury of this choice. For instance, a TreeSet<T> that requires a total ordering on T to be specified at construction time might be genuinely unable to efficiently implement has(mixed).

The philosophical question to answer is should we allow types in our libdefs that would be rejected by flow if it wasn't a libdef?

This question seems very simple and easily answered to me. :-)

SamChou19815 commented 1 year ago

The V type params are made covariant now