Repository navigation
allow a flag that turns off covariant parameters when checking function assignability #6102
Description
Activity
I checked out your branch and it has hundreds of warnings in lib.d.ts. It would be good to see a demo branch where this isn't the case.
I cannot see how this would be useful in practice. Consider some normal code:
function scan(x: { indexOf(element: {}): number; }) { // .... } scan([1, 2]);
This errors in your branch:
a.ts(5,6): error TS2345: Argument of type 'number[]' is not assignable to parameter of type '{ indexOf(element: {}): number; }'. Types of property 'indexOf' are incompatible. Type '(searchElement: number, fromIndex?: number) => number' is not assignable to type '(element: {}) => number'.So the user is supposed to write...
scan(<{}[]>[1, 2]);? Or something else? It's very confusing that you'd have to write upcasts everywhere you used substitutability of arrays.Have you run this flag on your own code?
Sorry for confusion. A blooper was fixed.
More working code: https://github.com/aleksey-bykov/TypeScript/commit/b5ce252342c26ebef2bde66036e58d6c05b0200b
Lib.d.ts compiles no problem.
Your example compiles too.
Here is a real project that is taking advantage of it as described in #4895
Instructions:
- git clone
- npm install
- cd src
- npm install
- cd ..
- build.cmd
- npm test
I don't see what's being accomplished now
function onEvent(x: (n: HTMLElement) => void) { } onEvent((x: HTMLDivElement) => undefined);
jake local node built\local\tsc.js --noParameterCovariance a.ts (no errors)Same behavior with the initial example you posted
you are right, surprisingly that example does not work, it must be a
different place in the compiler that I have yet to find and fix, which is
confusing because the following example works as expected:interface A { x: number } interface B { x: number, y: number } let foo: (value: A) => void; let bar: (value: B) => void; bar = foo; // works in original, breaks in mine
src/array.ts (17,1): Type '(value: A) => void' is not assignable to type '(value: B) => void'. Type 'A' is not assignable to type 'B'. Property 'y' is missing in type 'A'.does it mean that there is more than one place in the checker that does
matching? not sure how these 2 examples are differentany insights would be welcome
oh crap, you are right, just realized I don't make sense, let me get my shit together
Aleksey-Bykov: the check you actually want is the contravariant one:
isRelatedTo(t, s, reportErrors)I tried checking some code with covariant parameters disallowed, and most errors found were
Subtype[]not being assignable toSupertype[]*, due to the.pushmethod (and similar errors with a few other generic container-ish classes). I think for a --noParameterCovariance flag to be viable we would need something like Java's wildcards, so you could doArray<? extends Supertype>etc.*EDIT: None of the errors were exactly on two array types, one of them was always an interface extending an array. It looks like generics are always assumed to be covariant themselves, but structurally identical types are not.
We chatted about this for a while in the team room.
I was hoping to have a sort of Socratic "But what about ..." inspection of this since I know we've argued about it for a long time, but let me relay some points Anders Hejlsberg (@ahejlsberg) raised that might inform your next direction here.
Your premise here is that it should be invalid to assign
(x: Derived) => voidto(x: Base => void), even thoughDerivedis assignable toBase. Let's chase this to its logical conclusion.First off, the compiler assumes that
X<T>is assignable toX<U>ifTis assignable toU. We could either keep that assumption, or not.If we keep that assumption, then there is a gaping unsoundness hole in the
--noCovarianceflag:interface EventHandler<T> { handle(data: T): void; } let x: EventHandler<Base>; let y: EventHandler<Derived>; // Relationship between x and y is different from the relationship // between a and b, even though they are identical (!) let a: { handle(data: Base): void }; let b: { handle(data: Derived): void };
If we remove that assumption, then you get a) a massive perf hit, because all generic types have to be structurally checked and b) terrible semantics:
let x: Base[] = []; let derived1: Derived; // Error, cannot convert Derived[] to Base[] because types of 'push' do not match x = [derived1];
Second, it's unclear why one should take such umbrage with functions when this problem is omnipresent in the entire type system:
var x: Derived[] = [d1, d2, d3]; var y: Base[] = x; y.push(base); x.some(e => !e.isDerived); // true, wat
You could argue "this is why
Derived[]shouldn't be assignable toBase[]!", but that's not a language anyone wants to use in practice (what's the point of being super-sound if you have to have type assertions everywhere?).I think it's instructive to think about the signatures
containsandpush. It's clearly safe to callcontains(Base)on aDerived[](this happens all the time). And it's clearly safe to callpush(Derived)on aBase[](this happens all the time). But it's unsafe to callpush(Base)on aDerived[].It's easy to convince yourself that you can just think about signatures without thinking about the implications on the types they are embedded in, but this is a fallacy. In a language without extensive const and in/out annotations, this is an unsoundness you have to accept.
It should be possible to write a TSLint rule that can detect most of the cases that humans would consider suspicious. But it's not going to be possible to wire this rule through the entire type system and get sane results.
Reacted by Andrey Tsvetkov, Marin Marinov and Trotyl YuTo me the most important point you made is that
inandoutannotations are a must to be able do it right. I assume they are out of the list of TypeScript features. If so, it's the end of story.But to nourish this idea a bit more.
I see no reason why
Derived[]can not be assigned toBase[]as long as the set of methods ofBase[]is shrunken to read only which is a fair thing to ask, isn't it? Whoever wants the dopushonBase[]has to take care of immutability.It's clearly safe to call
contains(Base)on aDerived[]How come it is? Granted, for arrays it will work because they uses reference equality, but in general case, where equality is structural and hardcoded, it is not safe. To make it safe a comparison function should be passed too.
And it's clearly safe to call push(Derived) on a Base[](this happens all the time).
As long as
pushdoesn't mutate the array it is safe. Which we know it does, so it's not safe.Reacted by Marin Marinovthere was a comment earlier, now it's gone.. anyway, i am glad someone brought it back to life
what i seen from my little exercise is that THE ONLY USE CASE for covariant function parameters are the event handlers
Ryan Cavanaugh (@RyanCavanaugh), am i wrong? where?
if so, it looks like such an overkill to allow the covariant parameters just to make stupid event handlers work
Instead, did you think of making event handlers discriminated by the event name WHOLE DIFFERENT METHODS merely making the event type argument value a CONTINUATION of the METHOD NAME?
so instead of
element.addEventListener('click', handler);we get (not in syntax, but semantically) something like :
element.addEventListenerClick(handler);where
addEventListenerClickis a method that registersclickevent handlers ONLY and has nothing to do with anything elseit looks like the method overloading was a bad idea for allowing specific event handlers on different event types
what i mean is:
if we had specific names for the method that register different event handlers the problem wound have not existed at all
registerClickListener(handler: ClickListenerOnlyAndNothingElse);as apposed to
registerListener('click', covariantHandlerHere);what i seen from my little exercise is that THE ONLY USE CASE for covariant function parameters are the event handlers
Event handlers, and every single generic class that uses its type parameter in a method parameter signature (which is to say, nearly all of them).
Lack of variance control is more problematic than
helpful because it remove complexity. Consider this signature:export function Component<State>(options: { connect: (dom: DomApi) => Property<State>; render: (state: State) => Vnode; }): Vnode;
Here, I will have very poor guarantees that the developer is properly returning a
Property<State>inconnect. For instance, if State is{ a: string, b: number },connectcan return aProperty<{}>and everything compiles fine. This makes no sense.One way to trick it is to force the type Param every time at declaration time with
Component<State>but not forcing it result in poor inference, which will happen for sure as with all conventions.By the way, Flow added covariance/contravariance support a few versions ago.
PS: I assume lack of variance control is the reason why everything is bivariant by default?
By the way, Flow added covariance/contravariance support a few versions ago.
I tracked it down : https://github.com/facebook/flow/blob/master/Changelog.md#v0210 Quote:
Syntax for declaring variance at definition. For example, interface Generator<+Yield,+Return,-Next> {...}. Still pending transpiler support though.
I suspect its not working based on the
still pendingcomment (I don't have flow though).The still pending comment was only about transpiler (Babel) support (though I believe this has been since added to Babel). You can try this out at flowtype.org/try though.
Would love to see something like this land in TypeScript!
Reacted by Basarat Ali Syed and Andrey Tsvetkov- locked and limited conversation to collaborators
on Jun 19, 2018
Currently the following flawed code is permitted:
Thank to the following line: https://github.com/Microsoft/TypeScript/blob/master/src/compiler/checker.ts#L5654
I suggest to give developers a flag that would disable such unsound behavior allowing function subtyping the way it is supposed to be: https://en.wikipedia.org/wiki/Subtyping#Function_types.
An outline of a pull request of how this might look like: https://github.com/aleksey-bykov/TypeScript/commit/34b8f9b5a937cb7bb471771d5c9b9c4d4f629fc8
Related issues: #5961, #3523, #4895, #5741, #3067, #222, #5673