Skip to content
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

Callback function type checks don't work with overloads #8457

Closed
falsandtru opened this issue May 4, 2016 · 9 comments
Closed

Callback function type checks don't work with overloads #8457

falsandtru opened this issue May 4, 2016 · 9 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@falsandtru
Copy link
Contributor

falsandtru commented May 4, 2016

TypeScript Version:

master

Code

interface F {
  (g: () => void): number; // if this line is removed, tsc gives a correct error.
  (g: (i: (p: number) => number) => void): number;
}
var f: F;
f(i => h(i));
function h(i: (p: string) => string): string {
  return i('');
}

Expected behavior:

index.ts(6,10): error TS2345: Argument of type '(p: number) => number' is not as signable to parameter of type '(p: string) => string'.
  Types of parameters 'p' and 'p' are incompatible.
    Type 'string' is not assignable to type 'number'.

Actual behavior:

pass

@falsandtru falsandtru changed the title Callback type checks don't work with overloads Callback function type checks don't work with overloads May 4, 2016
@RyanCavanaugh
Copy link
Member

I believe this is a duplicate of #598

@mhegazy mhegazy added the Duplicate An existing issue was already created label May 4, 2016
@falsandtru
Copy link
Contributor Author

falsandtru commented May 4, 2016

I think this is a different case because if finished callback returns a not void type, tsc throws an error correctly.

interface KnockoutElement<T> {
  // Getter
  (): T;
  // Setter
  (value: T): void;
}

function readFile(finished: (contents: string) => string) { /*... */ }
var ko: KnockoutElement<number>;
readFile(ko); // throw an error correctly.

Additionally, my filed case doesn't depend parameters.

interface F {
  (g: () => void): number; // if this line is removed, tsc gives a correct error.
  (g: (i: () => number) => void): number;
}
var f: F;
f(i => h(i));
function h(i: () => string): string {
  return i();
}

So I think TypeScript should add a test of this case when fix this issue.

@RyanCavanaugh RyanCavanaugh removed the Duplicate An existing issue was already created label May 4, 2016
@RyanCavanaugh
Copy link
Member

You're right, this is different. The problem is that this code appears to be valid and it's not at all clear how to make it invalid.

interface F {
  (g: () => void): number;
  (g: (i: (p: number) => number) => void): number;
}
var f: F;
f(g => h(g));
function h(i: (p: string) => string): string {
  return i('');
}

The code says that h takes a parameter i and invokes it with a string. f has two overloads -- either taking a callback of zero arguments, or a callback of one argument. It's valid to ignore parameters (i.e. accept fewer arguments than are provided), so the first overload we try is the one that works -- in fact, we never even typecheck against the second overload at all!

This code should really be written as

interface F {
  (g: (i: (p: number) => number) => void): number;
}

or at the very least

interface F {
  (g: (i: () => number) => void): number;
  (g: () => void): number; // if this line is removed, tsc gives a correct error.
}

but it's unclear how we even detect these problems.

@falsandtru
Copy link
Contributor Author

I fixed my code a little.

- f(g => h(g));
+ f(i => h(i));

@falsandtru
Copy link
Contributor Author

@RyanCavanaugh Could you fix this?

@RyanCavanaugh
Copy link
Member

@falsandtru I'm not sure what you mean?

@falsandtru
Copy link
Contributor Author

@RyanCavanaugh Could you fix this problem? I feel this is a bug.

@RyanCavanaugh
Copy link
Member

Unfortunately it's a design limitation (we choose the first available overload successfully) with no clear fix.

There's no reason to write a set of overloads that way, though, so the fix on your end should just be to do one of the suggested rewrites. Some advanced linting rule might be able to detect a set of "bad" signatures.

@falsandtru
Copy link
Contributor Author

I see, thanks for the answer...

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label May 5, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

3 participants