Skip to content

Commit

Permalink
Emit computed properties in ES3/ES5 properly.
Browse files Browse the repository at this point in the history
  • Loading branch information
@DanielRosenwasser
DanielRosenwasser committed Feb 5, 2015
1 parent d6b2c6d commit ddb63d2
Show file tree
Hide file tree
Showing 47 changed files with 545 additions and 253 deletions.
173 changes: 162 additions & 11 deletions src/compiler/emitter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ module ts {
});
}

function getAllAccessorDeclarations(node: ClassDeclaration, accessor: AccessorDeclaration) {
function getAllAccessorDeclarations(declarations: NodeArray<Declaration>, accessor: AccessorDeclaration) {

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

classElementDeclarations

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

But why do this in the first place? I feel like the class declaration is better

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Oh I see, you want to do it for object literals too, and the parent is going to be a different kind
var firstAccessor: AccessorDeclaration;
var getAccessor: AccessorDeclaration;
var setAccessor: AccessorDeclaration;
Expand All @@ -288,7 +288,7 @@ module ts {
}
}
else {
forEach(node.members,(member: Declaration) => {
forEach(declarations, (member: Declaration) => {
if ((member.kind === SyntaxKind.GetAccessor || member.kind === SyntaxKind.SetAccessor) &&
(<Identifier>member.name).text === (<Identifier>accessor.name).text &&
(member.flags & NodeFlags.Static) === (accessor.flags & NodeFlags.Static)) {
Expand Down Expand Up @@ -1116,8 +1116,8 @@ module ts {
if (hasDynamicName(node)) {
return;
}
var accessors = getAllAccessorDeclarations(<ClassDeclaration>node.parent, node);

var accessors = getAllAccessorDeclarations((<ClassDeclaration>node.parent).members, node);
if (node === accessors.firstAccessor) {
emitJsDocComments(accessors.getAccessor);
emitJsDocComments(accessors.setAccessor);
Expand Down Expand Up @@ -2220,7 +2220,10 @@ module ts {

// This function specifically handles numeric/string literals for enum and accessor 'identifiers'.
// In a sense, it does not actually emit identifiers as much as it declares a name for a specific property.
// For example, this is utilized when feeding in a result to Object.defineProperty.

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Good comment
function emitExpressionForPropertyName(node: DeclarationName) {
Debug.assert(node.kind !== SyntaxKind.BindingElement);

if (node.kind === SyntaxKind.StringLiteral) {
emitLiteral(<LiteralExpression>node);
}
Expand Down Expand Up @@ -2417,22 +2420,167 @@ module ts {
}
}

function emitObjectLiteral(node: ObjectLiteralExpression) {
function emitObjectLiteralBody(node: ObjectLiteralExpression, numElements: number) {
write("{");
var properties = node.properties;
if (properties.length) {

if (numElements > 0) {
var properties = node.properties;
var multiLine = (node.flags & NodeFlags.MultiLine) !== 0;
if (!multiLine) {
write(" ");
}
emitList(properties, 0, properties.length, /*multiLine*/ multiLine,
emitList(properties, 0, numElements, /*multiLine*/ multiLine,
/*trailingComma*/ properties.hasTrailingComma && languageVersion >= ScriptTarget.ES5);
if (!multiLine) {
write(" ");
}
}

write("}");
}

function emitDownlevelObjectLiteralWithComputedProperties(node: ObjectLiteralExpression, firstComputedPropertyIndex: number) {
var multiLine = (node.flags & NodeFlags.MultiLine) !== 0;
var properties = node.properties;

write("(");

// For computed properties, we need to create a unique handle to the object
// literal so we can modify it without risking internal assignments tainting the object.

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Not sure what you mean by 'internal assignments tainting the object'

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 9, 2015

Author Member

Any assignments that take place in an expression have access to the original variable. In ES6, the variable will never allow access to the object literal during its construction - only after each declaration is processed will the variable be assigned the value of the obj literal. Downlevel, you'd actually be able to modify the object literal while properties are being tacked on. So you need a unique handle.

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 10, 2015

Contributor

Oh, I get it!
var tempVar = createTempVariable(node);
recordTempDeclaration(tempVar);

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Why isn't this just one step?

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 9, 2015

Author Member

Because we need to keep a handle to the tempVar.

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 10, 2015

Contributor

But why does createTempVariable not also record it, and then return it?

// Write out the first non-computed properties
// (or all properties if none of them are computed),
// then emit the rest through indexing on the temp variable.
emit(tempVar)
write(" = ");
emitObjectLiteralBody(node, firstComputedPropertyIndex);

if (multiLine) {
increaseIndent();
}

for (var i = firstComputedPropertyIndex, n = properties.length; i < n; i++) {
writeSeparator();

var property = properties[i];

emitStart(property)
if (property.kind === SyntaxKind.GetAccessor || property.kind === SyntaxKind.SetAccessor) {

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Isn't there an emitAccessor kind of thing that already does this?

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 9, 2015

Author Member

There's something like it, its use differs depending on the context.

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 10, 2015

Contributor

Can we use it?
// TODO (drosen): Reconcile with 'emitMemberFunctions'.
var accessors = getAllAccessorDeclarations(node.properties, <AccessorDeclaration>property);
write("Object.defineProperty(");
emit(tempVar);
write(", ");
emitStart(node.name);
emitExpressionForPropertyName(property.name);
emitEnd(property.name);
write(", {");
increaseIndent();
if (accessors.getAccessor) {
writeLine()

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Doesn't this put an awkward space between the { and the get?
emitLeadingComments(accessors.getAccessor);
write("get: ");
emitStart(accessors.getAccessor);

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Isn't accessors.getAccessor potentially the same as property? You already did emitStart on the property, so what does it mean if you now emitStart to the same property again?
write("function ");
emitSignatureAndBody(accessors.getAccessor);
emitEnd(accessors.getAccessor);
emitTrailingComments(accessors.getAccessor);
write(",");
}
if (accessors.setAccessor) {
writeLine();
emitLeadingComments(accessors.setAccessor);
write("set: ");
emitStart(accessors.setAccessor);
write("function ");
emitSignatureAndBody(accessors.setAccessor);
emitEnd(accessors.setAccessor);
emitTrailingComments(accessors.setAccessor);
write(",");
}
writeLine();
write("enumerable: true,");
writeLine();
write("configurable: true");
decreaseIndent();
writeSeparator();
write("})");
emitEnd(property);
}
else {
emitLeadingComments(property);
emitStart(property.name);

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

For accessors, emitStart was after the temp var. But here it's before. Why?

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 18, 2015

Author Member

I don't have a good answer to this to be honest. Maybe @sheetalkamat can weigh in on how to do this correctly.
emit(tempVar);
emitMemberAccessForPropertyName(property.name);
emitEnd(property.name);

write(" = ");

if (property.kind === SyntaxKind.PropertyAssignment) {
emit((<PropertyAssignment>property).initializer);
}
else if (property.kind === SyntaxKind.ShorthandPropertyAssignment) {
emitExpressionIdentifier((<ShorthandPropertyAssignment>property).name);
}
else if (property.kind === SyntaxKind.MethodDeclaration) {
emitFunctionDeclaration(<MethodDeclaration>property);
}
else {
Debug.fail("ObjectLiteralElement type not accounted for: " + property.kind);
}
}

emitEnd(property);
}

writeSeparator();
emit(tempVar);

write(")");

if (multiLine) {
decreaseIndent();
}

function writeSeparator() {

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

writeComma
if (multiLine) {
write(",");
writeLine();
}
else {
write(", ");
}
}
}

function emitObjectLiteral(node: ObjectLiteralExpression) {
if (languageVersion >= ScriptTarget.ES6) {

This comment has been minimized.

Sign in to view

Copy link
@CyrusNajmabadi

CyrusNajmabadi Feb 5, 2015

Contributor

Another way to write this is to have emitObjectLIteralBody(node, node.properties.length) be the last line of this mehtod. That way you emit in that fashion for >= ES6, or if you have no computed props. Then check for computed props and do the special emit if you have them. i.e.

if (languageVersion < Scripttarget.es6) {
  if (checkForComputed) { 
    emitDownLevel(...);
  }
}
emitNormal(...);

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Yes, I like that better too.

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 18, 2015

Author Member

Address'd
emitObjectLiteralBody(node, node.properties.length);
return;
}

var properties = node.properties;

// Find the first computed property.
// Everything until that point can be emitted as part of the initial object literal.
var numInitialNonComputedProperties = properties.length;

This comment has been minimized.

Sign in to view

Copy link
@CyrusNajmabadi

CyrusNajmabadi Feb 5, 2015

Contributor

fix name for numInitialNonComputedProperties

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 18, 2015

Author Member

Addressed.
forEach(properties, (property, i) => {

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Do we have indexOf? Can we just use that?

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 18, 2015

Author Member

I guess I could do indexOf(map(properties, property => property.kind, SyntaxKind.ComputedProperty)) but it'd be a little overkill.

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 18, 2015

Author Member

I'm just turning this into a for-loop.
if (hasDynamicName(properties[i])) {

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

This won't work for symbols. Use properties[i].name.kind === SyntaxKind.ComputedPropertyName

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 9, 2015

Author Member

Why won't this work for symbols?

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 10, 2015

Contributor

See #1978. hasDynamicName will return false for something like Symbol.iterator

This comment has been minimized.

Sign in to view

Copy link
@DanielRosenwasser

DanielRosenwasser Feb 18, 2015

Author Member

Addressed.
numInitialNonComputedProperties = i;
return true;
}
});

var hasComputedProperty = numInitialNonComputedProperties !== properties.length;

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

I prefer using < instead of !==
if (hasComputedProperty) {
emitDownlevelObjectLiteralWithComputedProperties(node, numInitialNonComputedProperties);
}
else {
emitObjectLiteralBody(node, properties.length);
}
}

function emitComputedPropertyName(node: ComputedPropertyName) {
write("[");
Expand Down Expand Up @@ -2464,7 +2612,7 @@ module ts {
// export var obj = { y };
// }
// The short-hand property in obj need to emit as such ... = { y : m.y } regardless of the TargetScript version
if (languageVersion < ScriptTarget.ES6 || resolver.getExpressionNamePrefix(node.name)) {
if (languageVersion <= ScriptTarget.ES5 || resolver.getExpressionNamePrefix(node.name)) {
// Emit identifier as an identifier
write(": ");
// Even though this is stored as identifier treat it as an expression
Expand Down Expand Up @@ -3446,6 +3594,7 @@ module ts {
}

function emitMemberAccessForPropertyName(memberName: DeclarationName) {
// TODO: (jfreeman,drosen): comment on why this is emitNode instead of emit here.
if (memberName.kind === SyntaxKind.StringLiteral || memberName.kind === SyntaxKind.NumericLiteral) {
write("[");
emitNode(memberName);
Expand Down Expand Up @@ -3495,13 +3644,14 @@ module ts {
emitLeadingComments(member);
emitStart(member);
emitStart((<MethodDeclaration>member).name);
emitNode(node.name);
emitNode(node.name); // TODO (shkamat,drosen): comment for why emitNode instead of emit.
if (!(member.flags & NodeFlags.Static)) {
write(".prototype");
}
emitMemberAccessForPropertyName((<MethodDeclaration>member).name);
emitEnd((<MethodDeclaration>member).name);
write(" = ");
// TODO (drosen): Should we performing emitStart twice on emitStart(member)?
emitStart(member);
emitFunctionDeclaration(<MethodDeclaration>member);
emitEnd(member);
Expand All @@ -3510,7 +3660,7 @@ module ts {
emitTrailingComments(member);
}
else if (member.kind === SyntaxKind.GetAccessor || member.kind === SyntaxKind.SetAccessor) {
var accessors = getAllAccessorDeclarations(node, <AccessorDeclaration>member);
var accessors = getAllAccessorDeclarations(node.members, <AccessorDeclaration>member);
if (member === accessors.firstAccessor) {
writeLine();
emitStart(member);
Expand All @@ -3521,6 +3671,7 @@ module ts {
write(".prototype");
}
write(", ");
// TODO: Shouldn't emitStart on name occur *here*?

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

I think you're right. Before computed properties, it did not matter because there was nothing to execute, but now there is.
emitExpressionForPropertyName((<AccessorDeclaration>member).name);
emitEnd((<AccessorDeclaration>member).name);
write(", {");
Expand Down
3 changes: 2 additions & 1 deletion tests/baselines/reference/FunctionDeclaration8_es6.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@
var v = { [yield]: foo }

//// [FunctionDeclaration8_es6.js]
var v = { [yield]: foo };
var v = (_a = {}, _a[yield] = foo, _a);
var _a;
3 changes: 2 additions & 1 deletion tests/baselines/reference/FunctionDeclaration9_es6.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ function * foo() {

//// [FunctionDeclaration9_es6.js]
function foo() {
var v = { []: foo };
var v = (_a = {}, _a[] = foo, _a);
var _a;
}
5 changes: 3 additions & 2 deletions tests/baselines/reference/FunctionPropertyAssignments5_es6.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
var v = { *[foo()]() { } }

//// [FunctionPropertyAssignments5_es6.js]
var v = { [foo()]: function () {
} };
var v = (_a = {}, _a[foo()] = function () {
}, _a);
var _a;
29 changes: 15 additions & 14 deletions tests/baselines/reference/computedPropertyNames10_ES5.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,27 +20,28 @@ var v = {
var s;
var n;
var a;
var v = {
[s]: function () {
var v = (_a = {},
_a[s] = function () {
},
[n]: function () {

This comment has been minimized.

Sign in to view

Copy link
@JsonFreeman

JsonFreeman Feb 9, 2015

Contributor

Wow, we actually emitted this before? I guess that's what happens when the syntax checks become type checks.
_a[n] = function () {
},
[s + s]: function () {
_a[s + s] = function () {
},
[s + n]: function () {
_a[s + n] = function () {
},
[+s]: function () {
_a[+s] = function () {
},
[""]: function () {
_a[""] = function () {
},
[0]: function () {
_a[0] = function () {
},
[a]: function () {
_a[a] = function () {
},
[true]: function () {
_a[true] = function () {
},
["hello bye"]: function () {
_a["hello bye"] = function () {
},
["hello " + a + " bye"]: function () {
}
};
_a["hello " + a + " bye"] = function () {
},
_a);
var _a;
Loading

0 comments on commit ddb63d2

Please sign in to comment.