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

This-function parameter inference fails in object with other typing features #10461

Closed
HerringtonDarkholme opened this issue Aug 21, 2016 · 7 comments
Labels
Fixed A PR has been merged for this issue

Comments

@HerringtonDarkholme
Copy link
Contributor

HerringtonDarkholme commented Aug 21, 2016

This may relate to #10288

TypeScript Version: 2.0.0, installed via typescript@beta

Code
All code under --noImplicitThis flag.

function plainThisBound(m: {[k:string]: (this: string) => string}) {}

plainThisBound({
  method() {
    return this.substr(0) // no error
  }
})

function polymorphicThisBound(m: {[k:string]: <V>(this: string) => V}) {}
polymorphicThisBound({
  method() {
    return this.substr(0) // error in noImplicitThis
  }
})

function unionThisBound(m: {[k:string]: (this: string) => string} & {method(): string}) {}
unionThisBound({
  method() {
    return this.substr(0) // error in noImplicitThis
  }
})

Expected behavior:
All three example should compile, if this parameter is inferred on call site
Or no example should compile, if this must be explicitly annotated

Actual behavior:
Only the first example compiles.

@HerringtonDarkholme
Copy link
Contributor Author

I think some background story will be helpful.
I come up with this when trying to write a more type-safe API for vue.js

Annotating this in vue's API is difficult, so I tried this:

declare class VueTyped<T> {
  data<D>(d: D): VueTyped<T & D>;
  method<M>(m: M & { [k:string]: (this: T) => any }): VueTyped<T & typeof m>
  static new(): VueTyped<{}>
  get(): T
}


var b = VueTyped.new()
  .data({msg: '123'})
  .method({
    method() {
      return this.msgs // expected to have an error for typo
    }
  })
  .get()

on the line this.msgs I expect to have an error. But it does not.

@DanielRosenwasser
Copy link
Member

For your polymorphicThisBound, your signature really reads to me as "each of the methods in this object is generic" (see #5737 for something related) whereas I think you really meant to just return {} or any.

In unionThisBound, I believe the issue is that we don't look to the index signature if we have a member of a certain name in the originating type.

Maybe @ahejlsberg has some insights.

@HerringtonDarkholme
Copy link
Contributor Author

HerringtonDarkholme commented Aug 22, 2016

Thanks for pointing out the spec.

I understand polymorphicThisBound is quite contrived and not inferred according to the spec.

It seems unionThisBound should be inferred as it has an apparent member of index signature?

For Vue specific issue, I found constraints on type parameter is not ignored. So it can be used to annotate method

declare class VueTyped<T> {
  method<M extends { [k:string]: (this: T) => {} }>(m: M): VueTyped<T & M>
}

@ahejlsberg
Copy link
Member

The issue here is fixed by #9746. Your example works in that branch, using either a union type for the m parameter or a type constraint for the M type parameter.

This is a pretty neat trick. It is kind of magic how the correct type just pops out at the end.

One thing I noticed is that you get errors if you add additional parameters to the methods you define with method because they aren't assignable to the indexer type. But you can fix that like so:

declare class VueTyped<T> {
  data<D>(d: D): VueTyped<T & D>;
  method<M>(m: M & { [k:string]: (this: T, ...args: any[]) => any }): VueTyped<T & M>
  static new(): VueTyped<{}>
  get(): T
}

One problem that remains is that when you add explicit parameter type annotations to methods in the method argument, the contextual type disappears. We've discussed having contextual types survive in those cases and continue to affect those parameters that don't have type annotations. We should probably look at that again.

Adding @sandersn.

@yuit yuit added this to the Community milestone Aug 22, 2016
@sandersn
Copy link
Member

Should be fixed now that #9746 is merged.

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Aug 24, 2016
@mhegazy mhegazy modified the milestones: TypeScript 2.0.1, Community Aug 24, 2016
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 7, 2016

@HerringtonDarkholme have you thought about porting this into to the Vue declaration file at all?

@HerringtonDarkholme
Copy link
Contributor Author

@DanielRosenwasser @kaorun343 has already updated declaration file for Vue 2.0 and that declaration file has used a lot of TS2.0 features to give a description for Vue.

My approach needs many API changes which are unsuitable for official Vue to adopt.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

6 participants