-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Emit 'for...of' statements in ES3/ES5 #2207
Changes from 16 commits
e417e1d
76e9b6a
61224e9
5a87864
9b76a02
f915efa
a0f108c
a99449a
9288424
4d32650
4bb0587
905f350
ed3ab96
946dc0e
fecd20a
835c84f
a27fbff
b15d8aa
1349a19
f389aef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1639,10 +1639,12 @@ module ts { | |
} | ||
|
||
if (root) { | ||
currentSourceFile = root; | ||
emit(root); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why doesn't the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It used to. This change was @CyrusNajmabadi's idea, but the purpose is to set it earlier, so that it's easy to see it. In other words, there is no reason to wait until emit to set currentSourceFile. If you feel strongly, I can change it back. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would also mean that the 'emit' method needs to check what type of node it has. Something it doesn't do today. It would also need to do this for the sourcemap/non-sourcemap case. This is a simpler way to accomplish all that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like it. If anything, you should set it here and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with the previous form was that setting current would happen far down the line. i.e. only once we actually got around to calling 'emitSourceFile'. That meant that between the call to 'emit' and the eventual call to emitSourceFile, you had a time when there was no 'currentSourceFile', which could break anyone who tried to use it for any purpose. This approach ensures that currentSourceFile is set hte moment we start processing an actual source file. The alternative is to move this check/set into 'emit'. However, that means that ever call to emit need to immediately check what type of node it is emitting, and then set currentSourceFile if it's a sourceFile. Given how often emit is called, it seems better to just set the sourceFile the moment we know we're emitting a file, and hten have it correctly set for all code that runs from that point on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I added emitSourceFile, which sets the currentSourceFile, and then calls emit on the source file. The old emitSourceFile has been renamed to emitSourceFileNode. |
||
} | ||
else { | ||
forEach(host.getSourceFiles(), sourceFile => { | ||
currentSourceFile = sourceFile; | ||
if (!isExternalModuleOrDeclarationFile(sourceFile)) { | ||
emit(sourceFile); | ||
} | ||
|
@@ -1684,7 +1686,11 @@ module ts { | |
else { | ||
name = generateUniqueName(baseName, n => isExistingName(location, n)); | ||
} | ||
|
||
|
||
return putNameInCurrentScopeNames(name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename to recordNameInCurrentScope? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay |
||
} | ||
|
||
function putNameInCurrentScopeNames(name: string): string { | ||
if (!currentScopeNames) { | ||
currentScopeNames = {}; | ||
} | ||
|
@@ -1694,7 +1700,7 @@ module ts { | |
|
||
function isExistingName(location: Node, name: string) { | ||
// check if resolver is aware of this name (if name was seen during the typecheck) | ||
if (!resolver.isUnknownIdentifier(location, name)) { | ||
if (!resolver.isUnknownIdentifier(location, name, currentSourceFile)) { | ||
return true; | ||
} | ||
|
||
|
@@ -2061,6 +2067,9 @@ module ts { | |
|
||
function emitNodeWithMap(node: Node) { | ||
if (node) { | ||
if (nodeIsSynthesized(node)) { | ||
return emitNode(node); | ||
} | ||
if (node.kind != SyntaxKind.SourceFile) { | ||
recordEmitNodeStartSpan(node); | ||
emitNode(node); | ||
|
@@ -2099,6 +2108,11 @@ module ts { | |
name = "_" + (tempCount < 25 ? String.fromCharCode(tempCount + (tempCount < 8 ? 0 : 1) + CharacterCodes.a) : tempCount - 25); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add a comment |
||
tempCount++; | ||
} | ||
|
||
// This is necessary so that a name generated via renameNonTopLevelLetAndConst will see the name | ||
// we just generated. | ||
putNameInCurrentScopeNames(name); | ||
|
||
var result = <Identifier>createSynthesizedNode(SyntaxKind.Identifier); | ||
result.text = name; | ||
return result; | ||
|
@@ -2882,6 +2896,12 @@ module ts { | |
|
||
return result; | ||
} | ||
|
||
function createExpressionStatement(expression: Expression): ExpressionStatement { | ||
var result = <ExpressionStatement>createSynthesizedNode(SyntaxKind.ExpressionStatement); | ||
result.expression = expression; | ||
return result; | ||
} | ||
|
||
function createMemberAccessForPropertyName(expression: LeftHandSideExpression, memberName: DeclarationName): PropertyAccessExpression | ElementAccessExpression { | ||
if (memberName.kind === SyntaxKind.Identifier) { | ||
|
@@ -3444,6 +3464,10 @@ module ts { | |
} | ||
|
||
function emitForInOrForOfStatement(node: ForInStatement | ForOfStatement) { | ||
if (languageVersion < ScriptTarget.ES6 && node.kind === SyntaxKind.ForOfStatement) { | ||
return emitDownLevelForOfStatement(node); | ||
} | ||
|
||
var endPos = emitToken(SyntaxKind.ForKeyword, node.pos); | ||
write(" "); | ||
endPos = emitToken(SyntaxKind.OpenParenToken, endPos); | ||
|
@@ -3470,6 +3494,148 @@ module ts { | |
emitToken(SyntaxKind.CloseParenToken, node.expression.end); | ||
emitEmbeddedStatement(node.statement); | ||
} | ||
|
||
function emitDownLevelForOfStatement(node: ForOfStatement) { | ||
// The following ES6 code: | ||
// | ||
// for (var v of expr) { } | ||
// | ||
// should be emitted as | ||
// | ||
// for (var _i = 0, _a = expr; _i < _a.length; _i++) { | ||
// var v = _a[_i]; | ||
// } | ||
// | ||
// where _a and _i are temps emitted to capture the RHS and the counter, | ||
// respectively. | ||
// When the left hand side is an expression instead of a var declaration, | ||
// the "var v" is not emitted. | ||
// When the left hand side is a let/const, the v is renamed if there is | ||
// another v in scope. | ||
// Note that all assignments to the LHS are emitted in the body, including | ||
// all destructuring. | ||
// Note also that because an extra statement is needed to assign to the LHS, | ||
// for-of bodies are always emitted as blocks. | ||
|
||
var endPos = emitToken(SyntaxKind.ForKeyword, node.pos); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just copied it from emitForInOrForOfStatement. If source maps are requested, it emits the token with its source map spans, as opposed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to say I find this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Me too. Particularly because it's a function pointer that can be set to emitTokenText or writeTextWithSpanRecord. One thing I don't understand is why it's important to emit a source map span for a token. The other thing I don't understand is why it takes a callback to override the default emit behavior. |
||
write(" "); | ||
endPos = emitToken(SyntaxKind.OpenParenToken, endPos); | ||
|
||
// Do not emit the LHS var declaration yet, because it might contain destructuring. | ||
|
||
// Do not call create recordTempDeclaration because we are declaring the temps | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reminder to remove "create" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
// right here. Recording means they will be declared later. | ||
// In the case where the user wrote an identifier as the RHS, like this: | ||
// | ||
// for (var v of arr) { } | ||
// | ||
// we don't want to emit a temporary variable for the RHS, just use it directly. | ||
var rhsIsIdentifier = node.expression.kind === SyntaxKind.Identifier; | ||
var counter = createTempVariable(node, /*forLoopVariable*/ true); | ||
var rhsReference = rhsIsIdentifier ? <Identifier>node.expression : createTempVariable(node, /*forLoopVariable*/ false); | ||
|
||
// This is the var keyword for the counter and rhsReference. The var keyword for | ||
// the LHS will be emitted inside the body. | ||
emitStart(node.expression); | ||
write("var "); | ||
|
||
// _i = 0 | ||
emitNode(counter); | ||
write(" = 0"); | ||
emitEnd(node.expression); | ||
|
||
if (!rhsIsIdentifier) { | ||
// , _a = expr | ||
write(", "); | ||
emitStart(node.expression); | ||
emitNode(rhsReference); | ||
write(" = "); | ||
emitNode(node.expression); | ||
emitEnd(node.expression); | ||
} | ||
write("; "); | ||
|
||
// _i < _a.length; | ||
emitStart(node.initializer); | ||
emitNode(counter); | ||
write(" < "); | ||
emitNode(rhsReference); | ||
write(".length"); | ||
emitEnd(node.initializer); | ||
write("; "); | ||
|
||
// _i++) | ||
emitStart(node.initializer); | ||
emitNode(counter); | ||
write("++"); | ||
emitEnd(node.initializer); | ||
emitToken(SyntaxKind.CloseParenToken, node.expression.end); | ||
|
||
// Body | ||
write(" {"); | ||
writeLine(); | ||
increaseIndent(); | ||
|
||
// Initialize LHS | ||
// var v = _a[_i]; | ||
var rhsIterationValue = createElementAccessExpression(rhsReference, counter); | ||
emitStart(node.initializer); | ||
if (node.initializer.kind === SyntaxKind.VariableDeclarationList) { | ||
write("var "); | ||
var variableDeclarationList = <VariableDeclarationList>node.initializer; | ||
if (variableDeclarationList.declarations.length >= 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My preference is |
||
var declaration = variableDeclarationList.declarations[0]; | ||
if (isBindingPattern(declaration.name)) { | ||
// This works whether the declaration is a var, let, or const. | ||
// It will use rhsIterationValue _a[_i] as the initializer. | ||
emitDestructuring(declaration, rhsIterationValue); | ||
} | ||
else { | ||
// The following call does not include the initializer, so we have | ||
// to emit it separately. | ||
emitNode(declaration); | ||
write(" = "); | ||
emitNode(rhsIterationValue); | ||
} | ||
} | ||
else { | ||
// It's an empty declaration list. This can only happen in an error case, if the user wrote | ||
// for (var of []) {} | ||
var emptyDeclarationListTemp = createTempVariable(node, /*forLoopVariable*/ false); | ||
emitNode(emptyDeclarationListTemp); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay |
||
write(" = "); | ||
emitNode(rhsIterationValue); | ||
} | ||
} | ||
else { | ||
// Initializer is an expression. Emit the expression in the body, so that it's | ||
// evaluated on every iteration. | ||
var assignmentExpression = createBinaryExpression(<Expression>node.initializer, SyntaxKind.EqualsToken, rhsIterationValue, /*startsOnNewLine*/ false); | ||
var assignmentExpressionStatement = createExpressionStatement(assignmentExpression); | ||
if (node.initializer.kind === SyntaxKind.ArrayLiteralExpression || node.initializer.kind === SyntaxKind.ObjectLiteralExpression) { | ||
// This is a destructuring pattern, so call emitDestructuring instead of emit. Calling emit will not work, because it will cause | ||
// the BinaryExpression to be passed in instead of the expression statement, which will cause emitDestructuring to crash. | ||
emitDestructuring(assignmentExpressionStatement); | ||
} | ||
else { | ||
emitNode(assignmentExpression); | ||
} | ||
} | ||
emitEnd(node.initializer); | ||
write(";"); | ||
|
||
if (node.statement.kind === SyntaxKind.Block) { | ||
emitLines((<Block>node.statement).statements); | ||
} | ||
else { | ||
writeLine(); | ||
emit(node.statement); | ||
} | ||
|
||
writeLine(); | ||
decreaseIndent(); | ||
write("}"); | ||
} | ||
|
||
function emitBreakOrContinueStatement(node: BreakOrContinueStatement) { | ||
emitToken(node.kind === SyntaxKind.BreakStatement ? SyntaxKind.BreakKeyword : SyntaxKind.ContinueKeyword, node.pos); | ||
|
@@ -3625,13 +3791,14 @@ module ts { | |
} | ||
} | ||
|
||
function emitDestructuring(root: BinaryExpression | VariableDeclaration | ParameterDeclaration, value?: Expression) { | ||
// Note that a destructuring assignment can be either an ExpressionStatement or a BinaryExpression. | ||
function emitDestructuring(root: ExpressionStatement | BinaryExpression | VariableDeclaration | ParameterDeclaration, value?: Expression) { | ||
var emitCount = 0; | ||
// An exported declaration is actually emitted as an assignment (to a property on the module object), so | ||
// temporary variables in an exported declaration need to have real declarations elsewhere | ||
var isDeclaration = (root.kind === SyntaxKind.VariableDeclaration && !(getCombinedNodeFlags(root) & NodeFlags.Export)) || root.kind === SyntaxKind.Parameter; | ||
if (root.kind === SyntaxKind.BinaryExpression) { | ||
emitAssignmentExpression(<BinaryExpression>root); | ||
if (root.kind === SyntaxKind.ExpressionStatement || root.kind === SyntaxKind.BinaryExpression) { | ||
emitAssignmentExpression(<ExpressionStatement | BinaryExpression>root); | ||
} | ||
else { | ||
emitBindingElement(<BindingElement>root, value); | ||
|
@@ -3770,13 +3937,14 @@ module ts { | |
} | ||
} | ||
|
||
function emitAssignmentExpression(root: BinaryExpression) { | ||
var target = root.left; | ||
var value = root.right; | ||
if (root.parent.kind === SyntaxKind.ExpressionStatement) { | ||
emitDestructuringAssignment(target, value); | ||
} | ||
else { | ||
function emitAssignmentExpression(root: ExpressionStatement | BinaryExpression) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just take the binary expression, and a 'parent' node. |
||
// Synthesized nodes will not have parents, so the ExpressionStatements will have to be passed | ||
// in directly. Otherwise, it will crash when we access the parent of a synthesized binary expression. | ||
var emitParenthesized = root.kind !== SyntaxKind.ExpressionStatement && root.parent.kind !== SyntaxKind.ExpressionStatement; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you just take an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will take a look at doing that. An alternative is what Cyrus mentioned, which is to pass in the parent if the node is synthesized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think the parent is used for anything other than determining whether to parenthesize. No reason to make it more cryptic than need be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think it can work with just a boolean to say whether it's parenthesized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I added a required boolean parameter called isAssignmentExpressionStatement. And I am checking this instead of checking the parent. |
||
var expression = <BinaryExpression>(root.kind === SyntaxKind.ExpressionStatement ? (<ExpressionStatement>root).expression : <BinaryExpression>root); | ||
var target = expression.left; | ||
var value = expression.right; | ||
if (emitParenthesized) { | ||
if (root.parent.kind !== SyntaxKind.ParenthesizedExpression) { | ||
write("("); | ||
} | ||
|
@@ -3788,6 +3956,9 @@ module ts { | |
write(")"); | ||
} | ||
} | ||
else { | ||
emitDestructuringAssignment(target, value); | ||
} | ||
} | ||
|
||
function emitBindingElement(target: BindingElement, value: Expression) { | ||
|
@@ -3846,7 +4017,7 @@ module ts { | |
} | ||
} | ||
else { | ||
var isLet = renameNonTopLevelLetAndConst(<Identifier>node.name); | ||
renameNonTopLevelLetAndConst(<Identifier>node.name); | ||
emitModuleMemberName(node); | ||
|
||
var initializer = node.initializer; | ||
|
@@ -5037,7 +5208,6 @@ module ts { | |
} | ||
|
||
function emitSourceFile(node: SourceFile) { | ||
currentSourceFile = node; | ||
// Start new file on new line | ||
writeLine(); | ||
emitDetachedComments(node); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
sourceFile
just an optimization? Also, the order of arguments should be sourceFile, location, name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sourceFile is not just an optimization. It is necessary. The reason is that location may be a synthesized node, in which case it will not have a proper parent chain with which to obtain the source file.
I will change the argument order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But
location
is used as a parameter toresolveName
which crawls up the parent chain. How's that going to work if it has no parent?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point. It is possible that there is a bug here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the loop in resolveName, if the location at any point does not have a parent, the the function will assume that it's at the global scope. It will try to find the name at the global scope, and will return whether it found it. So I'm not sure it's possible to observe a problem because of this behavior.
That said, I agree with you that it's appropriate for the caller to rely on this behavior. So I think we should discuss what should happen for synthesized nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this so it no longer takes a source file. It now asserts that the location passed in is not synthesized.
In the emitter, I've changed emitDestructuring to take an optional parameter lowestNonSynthesizedAncestor, and I've added a comment as to how it should be used: