Skip to content

Implicit any issue with overloaded methods #11936

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
tinganho opened this issue Oct 29, 2016 · 10 comments
Closed

Implicit any issue with overloaded methods #11936

tinganho opened this issue Oct 29, 2016 · 10 comments
Assignees
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@tinganho
Copy link
Contributor

This code was working for me before but broken in the latest:

class A {
    forEach(callback: (a: number) => void): void
    forEach(callback: (a: number, b: number) => void): void
    forEach(callback: (a: number, b?: number) => void): void {

    }
}

const a = new A();
a.forEach((a, b)=> { // Parameter 'a' and 'b' implicitly has an 'any' type
})
@alitaheri
Copy link

I can confirm with 2.1.0-dev.20161029. Seems like it tries to match with only the first overload.

Removing the first overload fixes it. Changing the callback to (a) => {} also fixes this.

@HerringtonDarkholme
Copy link
Contributor

Probably caused by #11905

@mhegazy mhegazy added the In Discussion Not yet reached consensus label Oct 31, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Oct 31, 2016

this indeed is caused by the change #11905. we should reconsider our fix there.

@mhegazy mhegazy added the Bug A bug in TypeScript label Oct 31, 2016
@mhegazy mhegazy added this to the TypeScript 2.1.2 milestone Oct 31, 2016
@sandersn sandersn changed the title Implicit any issue with overflowed methods Implicit any issue with overloaded methods Oct 31, 2016
@sandersn
Copy link
Member

This is actually an improvement — b was (mostly) an unreported implicit any before. In 2.0 (or earlier, all the way back to 1.1) try compiling b.blah() inside the body of the lambda. You won't get an error because b is implicitly any.

Note that

  1. The playground gives the type of b as number, so there's an additional bug in the language service.
  2. The anys are a limitation of the current compiler design, which fixes types while resolving the first overload, which in this case is too short. Previously the types were incorrect, now they're not available at all.

The correct fix is to reverse the order of the overloads or to get rid of the overloads entirely. They are technically redundant but are probably useful as documentation.

@sandersn sandersn added Design Limitation Constraints of the existing architecture prevent this from being fixed Breaking Change Would introduce errors in existing code and removed In Discussion Not yet reached consensus labels Oct 31, 2016
@tinganho
Copy link
Contributor Author

which fixes types while resolving the first overload, which in this case is too short

@sandersn Why do you need to fix the type on the first overload? And not just choose the overload that "fits"? In my example, the second overload should clearly be used for 2 argument callbacks.

I'm pretty sure though, that previous types where not typed any, at least not on my real world application. I'm also getting the error, when defining a test with Mocha testing framework(this one I'm pretty sure it was not typed as any):

it(testName, (done) => { // error 'done' is implicitly typed as any
})

@sandersn
Copy link
Member

@Andy-MS actually fixed jasmine.d.ts this morning to work with the updated compiler. The core limitation, since TypeScript 1.1, is that the compiler will only assign types to any expression once. This gives, unfortunately, extremely subtle semantics.

Normally this is fine, but overload resolution changes the contextual type as it tries each overload in order, so you can observe the effect of this limitation. I don't think there's a reasonable way to check overloads besides checking the arguments against each overload in turn. For example, these single-argument functions:

declare function f((x: number, y: number, z: number) => string): void;
declare function f((x: string, y: string) => string): void;
f(x => x);

Even when there's something that clearly makes an overload unsuitable for an argument, it may be nested deep in the argument or parameter type. So I think it's better to use normal type-checking machinery when possible: just ask whether all arguments are assignable to their respective parameters.

In the future, we might go back and rewrite overload resolution but right now we'd prefer to correctly report implicit-any errors from our limited algorithm instead of silently hiding them.

@HerringtonDarkholme
Copy link
Contributor

Thank you for detailed comments. It's really enlightening.

I'm thinking about doing overload inference better by choosing overload by their argument length.
The reasoning behind is that at runtime we cannot tell apart two functions from their signatures other than their length properties, as pointed out here.

For example

var use: Overload;
use((req: string, res) => {});

interface Overload {
    (handler1: (req1: string) => void): void;
    (handler2: (req2: number, res2: number) => void): void;
}

the argument function will always be contextual inferred against handler2 that has length of 2. And at runtime, an Overload implementation can only inspect the function by their length property, so even if user manually annotate req as string, it gets number at runtime.

Also, the paramter annotation of string is incompatible with number, it is desirable compiler can detect this and report an error.

(theoretically users can inspect function type by inspecting function's source string, Angular1 actually have done this. But that isn't common in JS)

@sandersn
Copy link
Member

sandersn commented Nov 1, 2016

Well, the top-level overload resolution does check function arity. But in your example, both signatures have arity=1 and the parameters themselves are functions that differ in arity. And this difference in arity could occur at any level, eg

class Foo1 {
  foo: Bar1;
}
class Foo2 {
  foo: Bar2;
}
class Bar1 {
  nested(req1: string) => void): void;
}
class Bar2 {
 nested(req2: number, res2: number) => void): void;
}
interface Overload {
    (handler1: Foo1): void;
    (handler2: Foo2): void;
}
let use: Overload
use({ 
  foo: { 
    nested: (req2, res2) => { 
      console.log("req2: string, oh no!"); 
    } 
  } 
})

An instance of Foo2 looks like it could match handler1 until we reach Bar2. That level of nesting is why the check should rely on normal assignability rules to check each overload. But checking assignability requires the compiler to know the types of each parameter and argument. And asking for the type the argument will fix the types for later overloads.

So I think we need to remove the limitation that once checked, types are fixed, and let type checking operate in a non-permanent fashion.

@ArtemAvramenko
Copy link

ArtemAvramenko commented Mar 13, 2017

@sandersn, why it is not merged to (req2: string | number, res2?: number)?

@sandersn
Copy link
Member

@ArtemAvramenko this merging is too permissive in many cases. If the two overload signatures are (s: string, t: string) and (n: number, m: number) then merging them to two string | number parameters incorrectly allows me to pass a string and a number.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

6 participants