Change static fields emits #43114
Change static fields emits #43114
Conversation
typescript-bot
added
the
For Milestone Bug
|
@rbuckton Sorry for bothering. But take a look please. |
|
Before I look at the PR, I remember talking to @rbuckton about this last year and getting confused about how to make decorators work after he pointed out that they could be troublesome. Can you add some tests with decorated classes? |
|
Added a basic test. It's really confused.... tc39/proposal-decorators#329 |
|
I think we could just replace this to some variable, the same as the class name reference: @foo
class C {
static a = 1
static b = 1 + C.a
static c = () => C.b
static d = () => this.b
}to var C_1;
let C = C_1 = class C {
};
C.a = 1;
C.b = 1 + C_1.a;
C.c = () => C_1.b;
C.d = () => C_1.b;
C = C_1 = __decorate([
foo
], C);It's seems does not break anything. |
|
let C = @decorator class {
static x = 1;
static y = this.x; // refers to undecorated class
static z = C.y; // TDZ error since C hasn't been initialized yet.
static w = () => C.y; // Ok if called after C is initialized.
} |
|
So I guess the correct emit should be: @foo
class C {
static a = 1
static b = 1 + C.a
static c = () => C.b
static d = () => this.b
}var C_1, C_2; // a variable to keep undecorated class
let C = C_1 = C_2 = class C {
};
C.a = 1;
C.b = 1 + C_1.a;
C.c = () => C_1.b;
C.d = () => C_2.b; // this reference to undecorated class
C = C_1 = __decorate([
foo
], C);Or we just denied this one because the decorator proposal is not stable yet? BTW: Seems we does not support decorator on class expression (#42198)... |
|
I discussed this with the TC39 Decorator stakeholders group. There's no final answer on how class decorators should interact with |
|
Thanks! |
|
By the way, static private fields and static methods and accessors just landed in master, so you will want to rebase. |
|
Oh. I just realized. Should we error and avoid the |
|
class A { static x = 1 }
class B extends A { static y = super.x }
B.y; // 1 |
|
Oh wow. Thanks for your hard working. |
|
@sandersn, @weswigham can either of you review my changes to this PR? In general I think this is fairly close to ready. I'd like to merge tomorrow morning for the 4.4 beta if possible. |
|
Well... A huge update. |
# Conflicts: # src/compiler/checker.ts # src/compiler/transformers/classFields.ts
Probably, yes. Once I get |
|
I've just about finished adding |
|
Something that will need a bit more time to consider after this has merged is how control flow should affect the types of static properties: class C {
static x; // infers type through control-flow
static {
this.x = 1;
}
}This is similar to what we do for class C {
static x;
static { this.x = 1; }
static y = this.x; // y should have type `number`
static { this.x++; } // this.x should have type `number`
} |
|
The last update fixes some use-before-def errors. I also added support for strict property initialization for statics, but I think I will actually revert that since it would be a possible breaking change and we haven't had the opportunity to discuss it in design meeting. |
rbuckton
force-pushed
the
change_static_fields_emits
branch
from
28c870d to
9850dc3
Compare
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 9850dc3. You can monitor the build here. Update: The results are in! |
|
@RyanCavanaugh, depending on the test results this should be ready to merge shortly. |
Currently I'm just waiting on the perf results. |
|
@rbuckton Here they are:Comparison Report - main..43114
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf tests seem like a negligible change. There's an increase in emit time, but I think that can be addressed next week. |
|
I will also follow up next week with the reserved name error. |
|
Thanks! |
Fixes #36267
TODO:
thisin static contextsuperin static context