angular / clutz

Closure to TypeScript `.d.ts` generator
MIT License
162 stars 60 forks source link

Type of implicit class property is any. #645

Open bowenni opened 6 years ago

bowenni commented 6 years ago

A very common pattern I saw from the recent migrations:

class A {
  /** @param {boolean}  p */
  constructor(p) {
    /** @private */
    this.p_ = p;
  }
}

This code is converted to:

class A {
  private p_: any;
  constructor(p: boolean) {
    this.p_ = p;
  }
}

Would it make sense to emit this instead?

class A {
  private p_: boolean;
  constructor(p: boolean) {
    this.p_ = p;
  }
}

The renaming the parameter property collapsing is left to TSLint.

mprobst commented 6 years ago

That seems useful!

evmar commented 6 years ago

Clutz only sees the types that Closure sees.

I tried the below program, which suggests that Closure thinks this.p_ really is any.

class A {
  /** @param {boolean}  p */
  constructor(p) {
    /** @private */
    this.p_ = p;
  }

  foo() {
    this.p_ = 'hi';
  }
}

new A(true).foo();
bowenni commented 6 years ago

Yes this.p_ is indeed any.

But we can refer the type from the constructor parameters. Since constructor(p: boolean) and this.p_ = p we can guess that this.p_ is also a boolean.

dpurp commented 6 years ago

iirc, you need @const for Closure to infer this for you.

On Tue, Jan 2, 2018, 1:21 PM Evan Martin notifications@github.com wrote:

Clutz only sees the types that Closure ses.

I tried the below program, which suggests that Closure thinks this.p_ really is any.

class A { /* @param {boolean} p / constructor(p) { /* @private / this.p_ = p; }

foo() { this.p_ = 'hi'; } }

new A(true).foo();

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/angular/clutz/issues/645#issuecomment-354878238, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAn6ACZy-tZOjAiWdJdV-fiebgYDpsMks5tGp3DgaJpZM4RO27Y .

bowenni commented 6 years ago

Adding a @const makes it private readonly p_: any;. The type is still any, I think.

evmar commented 6 years ago

I guess, my point is if we start inferring types beyond what Closure does we're moving into the space of implementing our own impl of a type system. I wonder if we could instead get the user to fix their code to have types. For example, isn't there some noImplicitAny-equivalent flag for Closure code? We could ask users to turn that on before migration.

dpurp commented 6 years ago

There are a few options with js-conformance, and they are suggested as part of the migration workflow. I suspect that most teams want to skip the pre-work and go straight to TypeScript, instead of fixing the errors in Closure-land.

mprobst commented 6 years ago

I think the problem is that gents does not run type checking or type inference at all. That's a conscious choice - you can (could?) only run type checking after es5 down translation, which we didn't want to do for more fidelity with the original code.