Skip to content

Commit 369900b

Browse files
sandersnsandersn
authored
Emit defineProperty calls before param prop assignments (microsoft#34987)
* Emit defineProperty calls before param prop assignments Note that I restricted this to --useDefineForClassFields is true. Nothing changes when it's off. I think this is the correct fix for a patch release. However, in principal there's nothing wrong with moving parameter property initialisation after property declaration initialisation. It would be Extremely Bad and Wrong to rely on this working: ```ts class C { p = this.q // what is q? constructor(public q: number) { } } ``` But today it does, and probably somebody relies on it without knowing. * Put parameter property initialiser into defineProperty's value * Combine ES5/ESNext into one test
1 parent 2075f74 commit 369900b

14 files changed

+356
-207
lines changed

‎src/compiler/transformers/classFields.ts

+17-18
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ namespace ts {
1010
/**
1111
* Transforms ECMAScript Class Syntax.
1212
* TypeScript parameter property syntax is transformed in the TypeScript transformer.
13-
* For now, this transforms public field declarations using TypeScript class semantics
14-
* (where the declarations get elided and initializers are transformed as assignments in the constructor).
15-
* Eventually, this transform will change to the ECMAScript semantics (with Object.defineProperty).
13+
* For now, this transforms public field declarations using TypeScript class semantics,
14+
* where declarations are elided and initializers are transformed as assignments in the constructor.
15+
* When --useDefineForClassFields is on, this transforms to ECMAScript semantics, with Object.defineProperty.
1616
*/
1717
export function transformClassFields(context: TransformationContext) {
1818
const {
@@ -294,7 +294,8 @@ namespace ts {
294294
}
295295

296296
function transformConstructorBody(node: ClassDeclaration | ClassExpression, constructor: ConstructorDeclaration | undefined, isDerivedClass: boolean) {
297-
const properties = getProperties(node, /*requireInitializer*/ !context.getCompilerOptions().useDefineForClassFields, /*isStatic*/ false);
297+
const useDefineForClassFields = context.getCompilerOptions().useDefineForClassFields;
298+
const properties = getProperties(node, /*requireInitializer*/ !useDefineForClassFields, /*isStatic*/ false);
298299

299300
// Only generate synthetic constructor when there are property initializers to move.
300301
if (!constructor && !some(properties)) {
@@ -325,7 +326,6 @@ namespace ts {
325326
if (constructor) {
326327
indexOfFirstStatement = addPrologueDirectivesAndInitialSuperCall(constructor, statements, visitor);
327328
}
328-
329329
// Add the property initializers. Transforms this:
330330
//
331331
// public x = 1;
@@ -336,19 +336,16 @@ namespace ts {
336336
// this.x = 1;
337337
// }
338338
//
339-
if (constructor && constructor.body) {
340-
let parameterPropertyDeclarationCount = 0;
341-
for (let i = indexOfFirstStatement; i < constructor.body.statements.length; i++) {
342-
if (isParameterPropertyDeclaration(getOriginalNode(constructor.body.statements[i]), constructor)) {
343-
parameterPropertyDeclarationCount++;
344-
}
345-
else {
346-
break;
347-
}
339+
if (constructor?.body) {
340+
let afterParameterProperties = findIndex(constructor.body.statements, s => !isParameterPropertyDeclaration(getOriginalNode(s), constructor), indexOfFirstStatement);
341+
if (afterParameterProperties === -1) {
342+
afterParameterProperties = constructor.body.statements.length;
348343
}
349-
if (parameterPropertyDeclarationCount > 0) {
350-
addRange(statements, visitNodes(constructor.body.statements, visitor, isStatement, indexOfFirstStatement, parameterPropertyDeclarationCount));
351-
indexOfFirstStatement += parameterPropertyDeclarationCount;
344+
if (afterParameterProperties > indexOfFirstStatement) {
345+
if (!useDefineForClassFields) {
346+
addRange(statements, visitNodes(constructor.body.statements, visitor, isStatement, indexOfFirstStatement, afterParameterProperties - indexOfFirstStatement));
347+
}
348+
indexOfFirstStatement = afterParameterProperties;
352349
}
353350
}
354351
addPropertyStatements(statements, properties, createThis());
@@ -421,7 +418,9 @@ namespace ts {
421418
? updateComputedPropertyName(property.name, getGeneratedNameForNode(property.name))
422419
: property.name;
423420

424-
const initializer = property.initializer || emitAssignment ? visitNode(property.initializer, visitor, isExpression) : createVoidZero();
421+
const initializer = property.initializer || emitAssignment ? visitNode(property.initializer, visitor, isExpression)
422+
: hasModifier(getOriginalNode(property), ModifierFlags.ParameterPropertyModifier) && isIdentifier(propertyName) ? propertyName
423+
: createVoidZero();
425424
if (emitAssignment) {
426425
const memberAccess = createMemberAccessForPropertyName(receiver, propertyName, /*location*/ propertyName);
427426
return createAssignment(memberAccess, initializer);

‎src/compiler/transformers/ts.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -900,13 +900,13 @@ namespace ts {
900900
if (parametersWithPropertyAssignments) {
901901
for (const parameter of parametersWithPropertyAssignments) {
902902
if (isIdentifier(parameter.name)) {
903-
members.push(aggregateTransformFlags(createProperty(
903+
members.push(setOriginalNode(aggregateTransformFlags(createProperty(
904904
/*decorators*/ undefined,
905905
/*modifiers*/ undefined,
906906
parameter.name,
907907
/*questionOrExclamationToken*/ undefined,
908908
/*type*/ undefined,
909-
/*initializer*/ undefined)));
909+
/*initializer*/ undefined)), parameter));
910910
}
911911
}
912912
}

‎tests/baselines/reference/defineProperty(target=es5).js

+118
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
//// [defineProperty.ts]
2+
var x: "p" = "p"
3+
class A {
4+
a = this.y
5+
b
6+
["computed"] = 13
7+
;[x] = 14
8+
m() { }
9+
constructor(public readonly y: number) { }
10+
z = this.y
11+
declare notEmitted;
12+
}
13+
class B {
14+
}
15+
class C extends B {
16+
z = this.ka
17+
constructor(public ka: number) {
18+
super()
19+
}
20+
ki = this.ka
21+
}
22+
23+
24+
//// [defineProperty.js]
25+
var __extends = (this && this.__extends) || (function () {
26+
var extendStatics = function (d, b) {
27+
extendStatics = Object.setPrototypeOf ||
28+
({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
29+
function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
30+
return extendStatics(d, b);
31+
};
32+
return function (d, b) {
33+
extendStatics(d, b);
34+
function __() { this.constructor = d; }
35+
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
36+
};
37+
})();
38+
var _a;
39+
var x = "p";
40+
var A = /** @class */ (function () {
41+
function A(y) {
42+
Object.defineProperty(this, "y", {
43+
enumerable: true,
44+
configurable: true,
45+
writable: true,
46+
value: y
47+
});
48+
Object.defineProperty(this, "a", {
49+
enumerable: true,
50+
configurable: true,
51+
writable: true,
52+
value: this.y
53+
});
54+
Object.defineProperty(this, "b", {
55+
enumerable: true,
56+
configurable: true,
57+
writable: true,
58+
value: void 0
59+
});
60+
Object.defineProperty(this, "computed", {
61+
enumerable: true,
62+
configurable: true,
63+
writable: true,
64+
value: 13
65+
});
66+
Object.defineProperty(this, _a, {
67+
enumerable: true,
68+
configurable: true,
69+
writable: true,
70+
value: 14
71+
});
72+
Object.defineProperty(this, "z", {
73+
enumerable: true,
74+
configurable: true,
75+
writable: true,
76+
value: this.y
77+
});
78+
}
79+
Object.defineProperty(A.prototype, "m", {
80+
enumerable: false,
81+
configurable: true,
82+
writable: true,
83+
value: function () { }
84+
});
85+
return A;
86+
}());
87+
_a = x;
88+
var B = /** @class */ (function () {
89+
function B() {
90+
}
91+
return B;
92+
}());
93+
var C = /** @class */ (function (_super) {
94+
__extends(C, _super);
95+
function C(ka) {
96+
var _this = _super.call(this) || this;
97+
Object.defineProperty(_this, "ka", {
98+
enumerable: true,
99+
configurable: true,
100+
writable: true,
101+
value: ka
102+
});
103+
Object.defineProperty(_this, "z", {
104+
enumerable: true,
105+
configurable: true,
106+
writable: true,
107+
value: _this.ka
108+
});
109+
Object.defineProperty(_this, "ki", {
110+
enumerable: true,
111+
configurable: true,
112+
writable: true,
113+
value: _this.ka
114+
});
115+
return _this;
116+
}
117+
return C;
118+
}(B));

‎tests/baselines/reference/defineProperty(target=es5).symbols

+65
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
=== tests/cases/conformance/classes/propertyMemberDeclarations/defineProperty.ts ===
2+
var x: "p" = "p"
3+
>x : Symbol(x, Decl(defineProperty.ts, 0, 3))
4+
5+
class A {
6+
>A : Symbol(A, Decl(defineProperty.ts, 0, 16))
7+
8+
a = this.y
9+
>a : Symbol(A.a, Decl(defineProperty.ts, 1, 9))
10+
>this.y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))
11+
>this : Symbol(A, Decl(defineProperty.ts, 0, 16))
12+
>y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))
13+
14+
b
15+
>b : Symbol(A.b, Decl(defineProperty.ts, 2, 14))
16+
17+
["computed"] = 13
18+
>["computed"] : Symbol(A["computed"], Decl(defineProperty.ts, 3, 5))
19+
>"computed" : Symbol(A["computed"], Decl(defineProperty.ts, 3, 5))
20+
21+
;[x] = 14
22+
>[x] : Symbol(A[x], Decl(defineProperty.ts, 5, 5))
23+
>x : Symbol(x, Decl(defineProperty.ts, 0, 3))
24+
25+
m() { }
26+
>m : Symbol(A.m, Decl(defineProperty.ts, 5, 13))
27+
28+
constructor(public readonly y: number) { }
29+
>y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))
30+
31+
z = this.y
32+
>z : Symbol(A.z, Decl(defineProperty.ts, 7, 46))
33+
>this.y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))
34+
>this : Symbol(A, Decl(defineProperty.ts, 0, 16))
35+
>y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))
36+
37+
declare notEmitted;
38+
>notEmitted : Symbol(A.notEmitted, Decl(defineProperty.ts, 8, 14))
39+
}
40+
class B {
41+
>B : Symbol(B, Decl(defineProperty.ts, 10, 1))
42+
}
43+
class C extends B {
44+
>C : Symbol(C, Decl(defineProperty.ts, 12, 1))
45+
>B : Symbol(B, Decl(defineProperty.ts, 10, 1))
46+
47+
z = this.ka
48+
>z : Symbol(C.z, Decl(defineProperty.ts, 13, 19))
49+
>this.ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))
50+
>this : Symbol(C, Decl(defineProperty.ts, 12, 1))
51+
>ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))
52+
53+
constructor(public ka: number) {
54+
>ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))
55+
56+
super()
57+
>super : Symbol(B, Decl(defineProperty.ts, 10, 1))
58+
}
59+
ki = this.ka
60+
>ki : Symbol(C.ki, Decl(defineProperty.ts, 17, 5))
61+
>this.ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))
62+
>this : Symbol(C, Decl(defineProperty.ts, 12, 1))
63+
>ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))
64+
}
65+

‎tests/baselines/reference/definePropertyESNext.types renamed to ‎tests/baselines/reference/defineProperty(target=es5).types

+11-3
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,16 @@
1-
=== tests/cases/conformance/classes/propertyMemberDeclarations/definePropertyESNext.ts ===
1+
=== tests/cases/conformance/classes/propertyMemberDeclarations/defineProperty.ts ===
22
var x: "p" = "p"
33
>x : "p"
44
>"p" : "p"
55

66
class A {
77
>A : A
88

9-
a = 12
9+
a = this.y
1010
>a : number
11-
>12 : 12
11+
>this.y : number
12+
>this : this
13+
>y : number
1214

1315
b
1416
>b : any
@@ -29,6 +31,12 @@ class A {
2931
constructor(public readonly y: number) { }
3032
>y : number
3133

34+
z = this.y
35+
>z : number
36+
>this.y : number
37+
>this : this
38+
>y : number
39+
3240
declare notEmitted;
3341
>notEmitted : any
3442
}

‎tests/baselines/reference/definePropertyESNext.js renamed to ‎tests/baselines/reference/defineProperty(target=esnext).js

+6-4
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
1-
//// [definePropertyESNext.ts]
1+
//// [defineProperty.ts]
22
var x: "p" = "p"
33
class A {
4-
a = 12
4+
a = this.y
55
b
66
["computed"] = 13
77
;[x] = 14
88
m() { }
99
constructor(public readonly y: number) { }
10+
z = this.y
1011
declare notEmitted;
1112
}
1213
class B {
@@ -20,18 +21,19 @@ class C extends B {
2021
}
2122

2223

23-
//// [definePropertyESNext.js]
24+
//// [defineProperty.js]
2425
var x = "p";
2526
class A {
2627
y;
27-
a = 12;
28+
a = this.y;
2829
b;
2930
["computed"] = 13;
3031
[x] = 14;
3132
m() { }
3233
constructor(y) {
3334
this.y = y;
3435
}
36+
z = this.y;
3537
}
3638
class B {
3739
}

0 commit comments

Comments
 (0)