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

Parameter signatures of functions in object literals are not always inferred correctly. #4241

Closed
rjamesnw opened this issue Aug 8, 2015 · 21 comments
Labels
Breaking Change Would introduce errors in existing code Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@rjamesnw
Copy link

rjamesnw commented Aug 8, 2015

Hi, I ran into an issue where the types don't seem to be inferred correctly. Consider this:

interface ITestHandler { (a: string, ...args: any[]) }
interface ITestHandlers { [index: string]: ITestHandler; }
class Test { Events: ITestHandlers; }
var t = new Test();
t.Events = {
     handler1: (a) => { }, // <= this works fine, 'a' is 'string'
     handler2: (a, b: string) => { } // <= 'a' is 'any', but 'b' is optional in the interface!
};

I expect "a" for "handler2" to be "string", but instead it is "any". Shouldn't "a" be interred from the signature, since the "args" are of type "any", which should match any type?

Thanks.

@rjamesnw rjamesnw changed the title Types not inferred correctly Parameter signatures of functions in object literals are not inferred correctly. Aug 8, 2015
@rjamesnw rjamesnw changed the title Parameter signatures of functions in object literals are not inferred correctly. Parameter signatures of functions in object literals are not always inferred correctly. Aug 8, 2015
@DanielRosenwasser
Copy link
Member

This is actual per spec. From section 4.10 Function Expressions:

When a function expression with no type parameters and no parameter type annotations is contextually typed (section 4.23) by a type T and a contextual signature S can be extracted from T, the function expression is processed as if it had explicitly specified parameter type annotations as they exist in S.

The rule is effectively that a function expression isn't contextually typed if it has any type annotations on its parameters.

@rjamesnw
Copy link
Author

rjamesnw commented Aug 9, 2015

Just because it says that in the spec doesn't mean that it makes any sense. The compiler should infer missing types at the same positions when a type is missing on the function expression. Just doesn't make any sense really. At the very least, it should see the given parameter type "string" as compatible with "any" and continue to infer the other types (especially if the given parameter type is optional!). It's really annoying to have a list of functions on an object all of the same supporting signatures, and for some, having to explicitly specify the missing parameter types that don't infer.

Bottom line, since b is optional (based on the interface signature), and has a type that can be assigned to the optional parameters, the missing type(s) should infer (in fact, with "no implicit any", I'm sure it would err out in the case above).

@zpdDG4gta8XKpMCd
Copy link

wow...

guys, you keep making people really pissed off with all your specs and the way you do you work, you need to double your effort and stop letting good developers down, jeez..

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Aug 10, 2015
@JsonFreeman
Copy link
Contributor

I believe the rationale here is that you either want the function's entire parameter list explicitly typed, or the entire parameter list inferred. It has been spec'd this way because the thinking was, if the user put a type annotation on any parameter, it means the user wants the type system to back off and let them do the typing.

In this case, it looks like the intent of the contextual signature was to be exact/specific about the type of one parameter, and vague about others (in this case the rest). Then when contextually typing a function expression, the specific type should be automatically assigned to the corresponding parameter in the function expression, but the "vague type" should be overridden by the explicit type annotation in the function expression.

The compiler would need some way to know whether the explicit type annotations in the function expression correspond to positions of "vague types" in the contextual signature. If so, the remaining parameters should have their types assigned by contextual typing. If not, then it is a signal that the user wants these remaining parameters to be of type any. Is there any meaningful heuristic for whether a parameter in the contextual signature is "vague"?

@rjamesnw
Copy link
Author

Sounds about right. I expected that if a parameter type of a function expression in context is missing, and the other parameters are of a same or super type (or 'any') to the expected signature in the context, that it is quite obvious the developer wants the missing parameter types inferred.

@DanielRosenwasser
Copy link
Member

@Aleksey-Bykov @rjamesnw my apologies. When I said it's in the spec, I didn't mean to shoot you down - that's why I didn't close the issue as "by design". I mostly wanted to just give context that this isn't a compiler bug, but rather, a potential improvement for the language.

I didn't realize this was the case either, and indeed, I expected the un-annotated parameters to be contextually typed as well.

@JsonFreeman
Copy link
Contributor

It would be a breaking change though, if we suddenly contextually typed all missing parameters. It's just a question of how many partially annotated contextually type-able parameter lists there are out there.

@rjamesnw
Copy link
Author

Only for those not using "no implicit any". ;) And even then, it would be of type "any", so I'm guessing it wouldn't break much. In fact, it'll probably help people find bugs and push them to add proper types for the rare cases. I think there are more benefits than not.

@zpdDG4gta8XKpMCd
Copy link

@DanielRosenwasser I was kidding when I said that (wasn't too funny I know), but @rjamesnw seemed seriously upset

@rjamesnw
Copy link
Author

Annoyed, but not upset, lol. ;)

@DanielRosenwasser
Copy link
Member

It'd be a breaking change, but the workaround would be casting the function to any, so I don't think it would be terrible.

@Aleksey-Bykov tone sometimes doesn't transfer online all that well, but I can sorta see it now - @rjamesnw awesome, as long as you're not upset! 😄

@rjamesnw
Copy link
Author

I'd rather give types for the parameters missing the types, since that's the better point of TypeScript I think (and was what I was doing anyhow). ;) It's not a huge issue, just an annoying one. This only happens on the "special" functions added where there may be extra optional parameters added. If I created an API for this part of my project, it would suck for others to end up having to do the same thing.

@JsonFreeman
Copy link
Contributor

Only for those not using "no implicit any". ;) And even then, it would be of type "any", so I'm guessing it wouldn't break much.

That is not quite true. The proposal to let the types flow through would reduce the number of anys, so you'd get fewer "implicit any" errors, but stricter types. That is what I meant by breaking change.

I think this is worth considering, I'm just noting it is a breaking change. To reduce the damage of the breaking change, we may have to come up with a heuristic, but no obvious one comes to mind, and it would add to the complexity of contextual typing. My belief is that either we keep the current behavior, or always fill in the missing types from the contextual signature. I do not know if there is any meaningful middle ground.

@rjamesnw
Copy link
Author

I think you misunderstood my point. TODAY, if "no implicit any" is enabled, it WILL error out, so for THOSE people, it's not an issue. ;) It would, however, be much better and safer IMHO for everyone to have those types come through. Consider that the interface index signature shows what the developer IS expecting - a very specific signature. In fact, I don't see how this would be a big breaking change at all! In my example, you CAN'T type the parameters ANY OTHER way (outside of using 'any'), or there will be errors anyhow. Consider this:

t.Events = {
     handler1: (a) => { }, // <= this works fine, 'a' is 'string'
     handler2: (a:number, b: string) => { } // <= ERROR, 'a' MUST be 'ANY' or expected type.
};

The types ARE expected because of the INDEX signature enforced by 'ITestHandlers'. Why on earth would this be a breaking change when the compiler is enforcing the types anyway. ;)

@JsonFreeman
Copy link
Contributor

Sorry, I had indeed read you wrong on the noImplicitAny thing.

It is a breaking change because let's say your original code did something like this (I just changed the body of handler2):

interface ITestHandler { (a: string, ...args: any[]) }
interface ITestHandlers { [index: string]: ITestHandler; }
class Test { Events: ITestHandlers; }
var t = new Test();
t.Events = {
     handler1: (a) => { },
     handler2: (a, b: string) => {
         a.bop(); // <= 'a' is 'any' today, but with the proposed change would be string
     }
};

Today you would get no error for bop because a is any, but with the proposed change you'd get an error because a is a string. So it is a breaking change.

@rjamesnw
Copy link
Author

I think my point was it's not a BIG breaking change - who on earth would do such a stupid thing (when 'string' is clearly the expectation)? lol :P ;) (very few, or they should be fired, lol)

... and I may add, that ".bop()" may also be an error, and you just helped someone solve a bug. ;)

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Good First Issue Well scoped, documented and has the green light and removed In Discussion Not yet reached consensus labels Oct 5, 2015
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Oct 5, 2015
@RyanCavanaugh RyanCavanaugh added the Breaking Change Would introduce errors in existing code label Oct 5, 2015
@RyanCavanaugh
Copy link
Member

Approved for PRs. This should be an easyish fix for anyone who's interested in trying out a compiler change.

@sandersn
Copy link
Member

Actually, I don't think it will be that easy because contextual typing interacts with type parameter inference. And our type inference is not easy. Here's an example based on discussion with @mhegazy and @DanielRosenwasser yesterday:

declare function makeSorter<T>(gt: (t1: T, t2: T) => boolean): (array: T[]) => T[];
makeSorter((n: number, m) => n > m); // T=any, n: number, m: any
makeSorter((n, m: number) => n > m); // T=any, n: number, m: any

What you actually want is T=number, n: number, m: number for both calls. Unfortunately, type inference currently skips contextually typed functions entirely and tries them in a second round, hoping that information gathered from other parameters will finish up inference first. This change would require inference to extract information from partially annotated functions in the first round.

Here's a way that might work: in the first round of inference,

  1. Use only annotated parameters of function expressions for inference
  2. Come up with some kind of Skip type for unannotated parameters (any, maybe?) so that inference can skip single parameters instead of the whole function.

Now in the second round of inference,

  1. Handle partially annotated functions as well as completely unannotated ones.
  2. Only infer from unannotated parameters (the ones we skipped in the first round).

I haven't tried this so of course it might not work.

@sandersn sandersn added the Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". label Jul 13, 2016
@sandersn sandersn removed the Good First Issue Well scoped, documented and has the green light label Jul 13, 2016
@JsonFreeman
Copy link
Contributor

One other way you could do it is:

  1. In the first pass, you skip a function if any of its parameters would be contextually typed. In the example above, gt would be skipped.
  2. In the second pass, when you visit a function, process the annotated parameters first (to add inference candidates), and then fix the type parameters for any bare parameters they align with (which I believe happens automatically when contextually typing those parameters).

@yortus
Copy link
Contributor

yortus commented Sep 16, 2016

Dang, I just got bitten by this. It took a long session of reducing ~200 lines of fairly complex generic types to get down to the following simplified repro, where the annotation issue is finally obvious. I couldn't make any sense of the error message in the original code. But once I worked out it was this issue, the first line of the error message was a giveaway.

declare function foo<T>(cb: Callback<T>): void;

interface Callback<T> {
    (val: T, captures: {[name: string]: string}, next: Function): void;
}

foo<string>((val, {n}, next) => {}); // OK, val is string, n is string, next is Function

foo((val: string, {n}, next) => {}); // ERROR
//  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// src/index.ts(14,5): error TS2345: Argument of type '(val: string, {n}: { n: any; }, next: any) => void' is not assignable to parameter of type 'Callback<string>'.
//   Types of parameters '__1' and 'captures' are incompatible.
//     Type '{ [name: string]: string; }' is not assignable to type '{ n: any; }'.
//       Property 'n' is missing in type '{ [name: string]: string; }'.

@HerringtonDarkholme
Copy link
Contributor

HerringtonDarkholme commented Oct 11, 2016

I have a proposal for this. For background and example please see #11475 . TypeScript needs this change even more than before because of this type and library paradigm shift (flux like library).

The key difference from @sandersn 's proposal is how type parameter is inferred. I choose to avoid two rounds inference: type parameter is fixed if preceding parameter is unannotated. And following parameter annotation has no effect on fixed type parameter.

Changed mind. I think double passes are better ergonomics.

Proposal

Partially annotated function is contextually inferred. We need first define what a partially annotated function.

A partially annotated function is a function expression that has no parameter or has at least one parameter missing type annotation.

No parameter is for compatibility like below.

interface A { contextualTypeThis(this: {a: number}): void }
var a: A = { contextualTypeThis: function() { this.a } }

To enable contextual typing on partially annotated function requires several modification to spec.

In #4.10

When a function expression with no type parameters and no parameter type annotations is contextually typed.

is changed to

When a partially annotated function expression with no type parameters is contextually typed.

And in #4.15.2, Type parameter inference is not necessarily fixed when function expression argument is inferred.

When a function expression is inferentially typed (section 4.10), all annotated parameters are used to produce candidates for type parameters referenced in types assigned to corresponding parameter. After that, for unannotated parameters, if a type is assigned to one parameter in that expression references type parameters for which inferences are being made, the corresponding inferred type arguments to become fixed and no further candidate inferences are made for them.

This is basically two round inference.

Compatiblity

This will introduce breaking changes. But I have no idea how will it impact real world code

A parameter used to be inferred as any will become inferred to some type. This is a breaking change.

The above example is also a breaking change, the return type is currently inferred as any.

Overloading resolution probably is not influenced because contextual typing will succeed for a unannotated argument.

Implementaion

I have a experimental branch for partial annotation, and some new tests (still needs a lot more). It seems modification to existing code is not massive.

HerringtonDarkholme added a commit to HerringtonDarkholme/TypeScript that referenced this issue Oct 11, 2016
@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Oct 18, 2016
@mhegazy mhegazy modified the milestones: TypeScript 2.1, Community Oct 18, 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
Breaking Change Would introduce errors in existing code Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants