Skip to content

3.5.1 strictFunctionTypes broke code. #31698

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

Closed
deskoh opened this issue May 31, 2019 · 9 comments
Closed

3.5.1 strictFunctionTypes broke code. #31698

deskoh opened this issue May 31, 2019 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@deskoh
Copy link

deskoh commented May 31, 2019

TypeScript Version: 3.6.0-dev.20190531

Search Terms: strictFunctionTypes

Code

Repo: https://github.com/deskoh/sinon-ts-issue

Updated code:

interface MyType<T extends any[]> {
  (...args: T): any;
}

declare let a: MyType<[any]>;
declare let b: MyType<any[]>;

// Following is an error in 3.5.1 but not 3.4.5 with `strictFunctionTypes: true`
// Type 'MyType<[any]>' is not assignable to type 'MyType<any[]>'.
//   Property '0' is missing in type 'any[]' but required in type '[any]'.
b = a;

Original example:

// Original example
import { SinonStub } from 'sinon';

let y: SinonStub<[any], any> = {} as any;

// Following is an error in 3.5.1 but not 3.4.5 with `strictFunctionTypes: true`
// Type 'SinonStub<[any], any>' is not assignable to type 'SinonStub<any[], any>'.
//  Property '0' is missing in type 'any[]' but required in type '[any]'.
let x: SinonStub<any[], any> = y;

Expected behavior:

As described above.

Actual behavior:

As described above.

Playground Link: N.A. (TS 3.5.1 not available yet, see code repo above)

Related Issues: N.A.

@jack-williams
Copy link
Collaborator

This issue is very hard to debug without providing the definition of SinonStub.

It's likely that the first type parameter of SinonStub is being given a contravariant flag, but whether this is correct or not is impossible to tell without the definition of SinonStub.

@deskoh
Copy link
Author

deskoh commented May 31, 2019

@jack-williams Updated example and code repo above.

@jack-williams
Copy link
Collaborator

jack-williams commented May 31, 2019

The new error looks correct to me.

declare let a: MyType<[any]>;
declare let b: MyType<any[]>;

a(); // error, need at least one argument.
b(); // ok, we can have 0 or more arguments.

b = a; // aliasing away `a` should not let us violate the arity of `a`

The function a always needs at least one argument; the function b can have any number of arguments. If you are allowed to assigned a to b then you can violate the contract of a through the alias of b.

@deskoh
Copy link
Author

deskoh commented May 31, 2019

So I take it as the contravariant check for the above example is not working as expected in 3.4.5?

@jack-williams
Copy link
Collaborator

jack-williams commented May 31, 2019

So actually I'm abit confused now...sorry!

The error looks correct to me: assigning a to b is not safe. However the inlined version does not produce an error for me (either in 3.4 or 3.5). I didn't realise this was a legal assignment!

declare let c: (x: any) => any;
declare let d: (...x: any[]) => any;

c(); // error, need at least one argument.
d(); // ok, we can have 0 or more arguments.

d = c; // no error!

This is clearly inconsistent. I'm guessing there is some logic in call signature checking that allows this, but this logic is never reached due to alias/interface variance flags. You can tell this from the following example:

type MyType<T extends any[]> = (...args: T) => any;
type MyType2<T extends any[]> = (...args: T) => any;

declare let a: MyType<[any]>;
declare let b: MyType2<any[]>;

b = a; // no error now because `a` and `b` are different aliases

I think this will need input from someone one the team. IMO assigning c to d should be an error - but this would be (another) breaking change.

@RyanCavanaugh
Copy link
Member

I didn't realise this was a legal assignment!

Variable args in a source signature are assumed to all be present (if requested) in the target signature. A reasonably common pattern is a function that takes a callback and invokes it with a deterministic but finite number of arguments, e.g.

function makeCall(arr: any[], func: (...args: any[]) => void) {
  func.apply(null, arr);
}
makeCall([1, 2, 3], (x, y, z) => undefined);

We tried enforcing this back in 0.9 (0.8 had a bug that unintentionally allowed it) but people complained a lot and we ended up reverting it.

@jack-williams
Copy link
Collaborator

jack-williams commented May 31, 2019

We tried enforcing this back in 0.9 (0.8 had a bug that unintentionally allowed it) but people complained a lot and we ended up reverting it.

The more you know! I guess the fix for this would be to add some more unmeasurable variance positions? (Or just revert the thing that changed it)

@deskoh
Copy link
Author

deskoh commented Jun 1, 2019

Another difference in behaviours between 3.4.5 and 3.5.1 observed with strictFunctionTypes:

type Options = { is: (value: any) => boolean } | object;

// Ok in 3.4.5 but got following error in 3.5.1:
// Types of property 'is' are incompatible.
//   Type '(val1: any, val2: any) => boolean' is not assignable to type '(value: any) => boolean'.
let opt: Options = {
  is: (val1, val2) => true
};

This behaviour in 3.5.1 seemed more correct, but object union type can no longer be a 'catch-all'. I created a PR for yup typings to address this behaviour in 3.5.1 (DefinitelyTyped/DefinitelyTyped/pull/35885).

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jun 13, 2019
@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants