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

Referencing this from a static class field #36267

Closed
rauschma opened this issue Jan 17, 2020 · 12 comments · Fixed by #43114
Closed

Referencing this from a static class field #36267

rauschma opened this issue Jan 17, 2020 · 12 comments · Fixed by #43114
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@rauschma
Copy link

TypeScript Version: Nightly Playground

The following code (Playground link) is legal in JavaScript, but produces a static error in TypeScript:

class C {
  static a = 'a';
  static b = this.a;
    // 'this' cannot be referenced in a static property initializer.
}
console.log(C.b); // 'a'

Practical use case – my library Enumify for enums:

class Color extends Enumify {
  static red = new Color();
  static orange = new Color();
  static yellow = new Color();
  static green = new Color();
  static blue = new Color();
  static purple = new Color();
  static _ = this.closeEnum(); // TypeScript: Color.closeEnum()
}
@DanielRosenwasser
Copy link
Member

Sounds like you want #5863, but if I recall correctly, the problem is that we don't assume the static side of one class is substitutable for another because then every subclass construct signature would have to be assignable to the constructor in the superclass.

@rauschma
Copy link
Author

AFAICT, this is simpler. In first example above, this is just a synonym for C.

Another example:

class A {
  // Legal in JS, not allowed in TS:
  static thisIsA = (this === A);
}
class B extends A {}

console.log(B.thisIsA); // true

@DanielRosenwasser
Copy link
Member

So you want "Monomorphic this for static members" 😄

@rauschma
Copy link
Author

Probably? 🤔

@robpalme
Copy link

Thank you for finding this @rauschma!

It's not just the type checking that's wrong. The emit is corrupting legit ESnext code. Playground

class C {
  static self = this;
}

...becomes...

class C {}
C.self = this;

@rauschma
Copy link
Author

Right! Should be done via definition, not assignment.

@robpalme
Copy link

Should be done via definition, not assignment.

Actually that bit is handled fine via the option useDefineForClassFields. The badness comes from using this at top-level scope. The downlevel is just semantically wrong.

Interestingly this bug goes away if target == ESnext && useDefineForClassFields == true.

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jan 21, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.8.1 milestone Jan 21, 2020
@RyanCavanaugh
Copy link
Member

@sandersn can you look at the emit thing (described lower in comments) ?

@sandersn
Copy link
Member

  1. We should remove the error for useDefineForClassFields+ESNext. The emit is correct and the type is correct. So the error is wrong there.
  2. For class field emit, we could just replace this with C outside function and class expressions in static property initialisers. In that case we could remove the error entirely.
  3. I thought that this emit would be incorrect with the semantics proposed in Polymorphic "this" for static members #5863, but that issue requests a this type for the static side of the class. Right now, in a static property of C, this: typeof C, but the emit wouldn't need to change if we changed it to be this: this.

I did (1) in #36781.

@rbuckton can you think of any problems with changing this references to C when emitting static properties?

@sandersn
Copy link
Member

(2) is getting complicated fast -- and I still haven't thought about decorated classes. I'm going to move this to 4.0 since it's unlikely a finished PR would be safe to ship in the RC anyway.

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Dec 11, 2020
@Kingwl
Copy link
Contributor

Kingwl commented Mar 5, 2021

Any progress?

It's affect #42986 and #43092 at least.

Have we considered changing our downlevel emit to support this case? We could either replace this with the class name depending on the context, or just wrap static initializers in something like:

(function() {
Foo.t = this;
Foo.at = () => this;
}).call(Foo);
Not necessary for this PR, but some food for thought.

from #36781 (review)

It's might good for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants