Skip to content

Commit

Permalink
Cherry-pick PR #34987 into release-3.7
Browse files Browse the repository at this point in the history
Component commits:
5810765 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.

ec79590 Put parameter property initialiser into defineProperty's value

be86355 Merge branch 'master' into fix-defineProperty-parameter-property-emit

8ff59b9 Combine ES5/ESNext into one test
  • Loading branch information
sandersn authored and typescript-bot committed Nov 22, 2019
1 parent d5bcb6f commit 1cb94c9
Show file tree
Hide file tree
Showing 14 changed files with 356 additions and 207 deletions.
35 changes: 17 additions & 18 deletions src/compiler/transformers/classFields.ts
Expand Up @@ -10,9 +10,9 @@ namespace ts {
/**
* Transforms ECMAScript Class Syntax.
* TypeScript parameter property syntax is transformed in the TypeScript transformer.
* For now, this transforms public field declarations using TypeScript class semantics
* (where the declarations get elided and initializers are transformed as assignments in the constructor).
* Eventually, this transform will change to the ECMAScript semantics (with Object.defineProperty).
* For now, this transforms public field declarations using TypeScript class semantics,
* where declarations are elided and initializers are transformed as assignments in the constructor.
* When --useDefineForClassFields is on, this transforms to ECMAScript semantics, with Object.defineProperty.
*/
export function transformClassFields(context: TransformationContext) {
const {
Expand Down Expand Up @@ -294,7 +294,8 @@ namespace ts {
}

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

// Only generate synthetic constructor when there are property initializers to move.
if (!constructor && !some(properties)) {
Expand Down Expand Up @@ -325,7 +326,6 @@ namespace ts {
if (constructor) {
indexOfFirstStatement = addPrologueDirectivesAndInitialSuperCall(constructor, statements, visitor);
}

// Add the property initializers. Transforms this:
//
// public x = 1;
Expand All @@ -336,19 +336,16 @@ namespace ts {
// this.x = 1;
// }
//
if (constructor && constructor.body) {
let parameterPropertyDeclarationCount = 0;
for (let i = indexOfFirstStatement; i < constructor.body.statements.length; i++) {
if (isParameterPropertyDeclaration(getOriginalNode(constructor.body.statements[i]), constructor)) {
parameterPropertyDeclarationCount++;
}
else {
break;
}
if (constructor?.body) {
let afterParameterProperties = findIndex(constructor.body.statements, s => !isParameterPropertyDeclaration(getOriginalNode(s), constructor), indexOfFirstStatement);
if (afterParameterProperties === -1) {
afterParameterProperties = constructor.body.statements.length;
}
if (parameterPropertyDeclarationCount > 0) {
addRange(statements, visitNodes(constructor.body.statements, visitor, isStatement, indexOfFirstStatement, parameterPropertyDeclarationCount));
indexOfFirstStatement += parameterPropertyDeclarationCount;
if (afterParameterProperties > indexOfFirstStatement) {
if (!useDefineForClassFields) {
addRange(statements, visitNodes(constructor.body.statements, visitor, isStatement, indexOfFirstStatement, afterParameterProperties - indexOfFirstStatement));
}
indexOfFirstStatement = afterParameterProperties;
}
}
addPropertyStatements(statements, properties, createThis());
Expand Down Expand Up @@ -421,7 +418,9 @@ namespace ts {
? updateComputedPropertyName(property.name, getGeneratedNameForNode(property.name))
: property.name;

const initializer = property.initializer || emitAssignment ? visitNode(property.initializer, visitor, isExpression) : createVoidZero();
const initializer = property.initializer || emitAssignment ? visitNode(property.initializer, visitor, isExpression)
: hasModifier(getOriginalNode(property), ModifierFlags.ParameterPropertyModifier) && isIdentifier(propertyName) ? propertyName
: createVoidZero();
if (emitAssignment) {
const memberAccess = createMemberAccessForPropertyName(receiver, propertyName, /*location*/ propertyName);
return createAssignment(memberAccess, initializer);
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/transformers/ts.ts
Expand Up @@ -900,13 +900,13 @@ namespace ts {
if (parametersWithPropertyAssignments) {
for (const parameter of parametersWithPropertyAssignments) {
if (isIdentifier(parameter.name)) {
members.push(aggregateTransformFlags(createProperty(
members.push(setOriginalNode(aggregateTransformFlags(createProperty(
/*decorators*/ undefined,
/*modifiers*/ undefined,
parameter.name,
/*questionOrExclamationToken*/ undefined,
/*type*/ undefined,
/*initializer*/ undefined)));
/*initializer*/ undefined)), parameter));
}
}
}
Expand Down
118 changes: 118 additions & 0 deletions tests/baselines/reference/defineProperty(target=es5).js
@@ -0,0 +1,118 @@
//// [defineProperty.ts]
var x: "p" = "p"
class A {
a = this.y
b
["computed"] = 13
;[x] = 14
m() { }
constructor(public readonly y: number) { }
z = this.y
declare notEmitted;
}
class B {
}
class C extends B {
z = this.ka
constructor(public ka: number) {
super()
}
ki = this.ka
}


//// [defineProperty.js]
var __extends = (this && this.__extends) || (function () {
var extendStatics = function (d, b) {
extendStatics = Object.setPrototypeOf ||
({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
return extendStatics(d, b);
};
return function (d, b) {
extendStatics(d, b);
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
})();
var _a;
var x = "p";
var A = /** @class */ (function () {
function A(y) {
Object.defineProperty(this, "y", {
enumerable: true,
configurable: true,
writable: true,
value: y
});
Object.defineProperty(this, "a", {
enumerable: true,
configurable: true,
writable: true,
value: this.y
});
Object.defineProperty(this, "b", {
enumerable: true,
configurable: true,
writable: true,
value: void 0
});
Object.defineProperty(this, "computed", {
enumerable: true,
configurable: true,
writable: true,
value: 13
});
Object.defineProperty(this, _a, {
enumerable: true,
configurable: true,
writable: true,
value: 14
});
Object.defineProperty(this, "z", {
enumerable: true,
configurable: true,
writable: true,
value: this.y
});
}
Object.defineProperty(A.prototype, "m", {
enumerable: false,
configurable: true,
writable: true,
value: function () { }
});
return A;
}());
_a = x;
var B = /** @class */ (function () {
function B() {
}
return B;
}());
var C = /** @class */ (function (_super) {
__extends(C, _super);
function C(ka) {
var _this = _super.call(this) || this;
Object.defineProperty(_this, "ka", {
enumerable: true,
configurable: true,
writable: true,
value: ka
});
Object.defineProperty(_this, "z", {
enumerable: true,
configurable: true,
writable: true,
value: _this.ka
});
Object.defineProperty(_this, "ki", {
enumerable: true,
configurable: true,
writable: true,
value: _this.ka
});
return _this;
}
return C;
}(B));
65 changes: 65 additions & 0 deletions tests/baselines/reference/defineProperty(target=es5).symbols
@@ -0,0 +1,65 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/defineProperty.ts ===
var x: "p" = "p"
>x : Symbol(x, Decl(defineProperty.ts, 0, 3))

class A {
>A : Symbol(A, Decl(defineProperty.ts, 0, 16))

a = this.y
>a : Symbol(A.a, Decl(defineProperty.ts, 1, 9))
>this.y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))
>this : Symbol(A, Decl(defineProperty.ts, 0, 16))
>y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))

b
>b : Symbol(A.b, Decl(defineProperty.ts, 2, 14))

["computed"] = 13
>["computed"] : Symbol(A["computed"], Decl(defineProperty.ts, 3, 5))
>"computed" : Symbol(A["computed"], Decl(defineProperty.ts, 3, 5))

;[x] = 14
>[x] : Symbol(A[x], Decl(defineProperty.ts, 5, 5))
>x : Symbol(x, Decl(defineProperty.ts, 0, 3))

m() { }
>m : Symbol(A.m, Decl(defineProperty.ts, 5, 13))

constructor(public readonly y: number) { }
>y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))

z = this.y
>z : Symbol(A.z, Decl(defineProperty.ts, 7, 46))
>this.y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))
>this : Symbol(A, Decl(defineProperty.ts, 0, 16))
>y : Symbol(A.y, Decl(defineProperty.ts, 7, 16))

declare notEmitted;
>notEmitted : Symbol(A.notEmitted, Decl(defineProperty.ts, 8, 14))
}
class B {
>B : Symbol(B, Decl(defineProperty.ts, 10, 1))
}
class C extends B {
>C : Symbol(C, Decl(defineProperty.ts, 12, 1))
>B : Symbol(B, Decl(defineProperty.ts, 10, 1))

z = this.ka
>z : Symbol(C.z, Decl(defineProperty.ts, 13, 19))
>this.ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))
>this : Symbol(C, Decl(defineProperty.ts, 12, 1))
>ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))

constructor(public ka: number) {
>ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))

super()
>super : Symbol(B, Decl(defineProperty.ts, 10, 1))
}
ki = this.ka
>ki : Symbol(C.ki, Decl(defineProperty.ts, 17, 5))
>this.ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))
>this : Symbol(C, Decl(defineProperty.ts, 12, 1))
>ka : Symbol(C.ka, Decl(defineProperty.ts, 15, 16))
}

@@ -1,14 +1,16 @@
=== tests/cases/conformance/classes/propertyMemberDeclarations/definePropertyESNext.ts ===
=== tests/cases/conformance/classes/propertyMemberDeclarations/defineProperty.ts ===
var x: "p" = "p"
>x : "p"
>"p" : "p"

class A {
>A : A

a = 12
a = this.y
>a : number
>12 : 12
>this.y : number
>this : this
>y : number

b
>b : any
Expand All @@ -29,6 +31,12 @@ class A {
constructor(public readonly y: number) { }
>y : number

z = this.y
>z : number
>this.y : number
>this : this
>y : number

declare notEmitted;
>notEmitted : any
}
Expand Down
@@ -1,12 +1,13 @@
//// [definePropertyESNext.ts]
//// [defineProperty.ts]
var x: "p" = "p"
class A {
a = 12
a = this.y
b
["computed"] = 13
;[x] = 14
m() { }
constructor(public readonly y: number) { }
z = this.y
declare notEmitted;
}
class B {
Expand All @@ -20,18 +21,19 @@ class C extends B {
}


//// [definePropertyESNext.js]
//// [defineProperty.js]
var x = "p";
class A {
y;
a = 12;
a = this.y;
b;
["computed"] = 13;
[x] = 14;
m() { }
constructor(y) {
this.y = y;
}
z = this.y;
}
class B {
}
Expand Down

0 comments on commit 1cb94c9

Please sign in to comment.