-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Conversation
This only takes into consideration arrays. How do you plan on supporting all iterables? |
This works for arrays and strings. Other iterables will not be supported for ES3/ES5 |
And I'm going to add type checking to only allow arrays in the loop (for ES3/5) and strings (for ES5). |
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't the emit
method set currentSourceFile
? Seems hacky to do it 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.
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 comment
The 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 comment
The 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 emit
should take no parameters and operate on currentSourceFile
. But I prefer the way it was.
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.
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 comment
The 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.
… for-ofES5 Conflicts: tests/baselines/reference/parserES5ForOfStatement18.js tests/baselines/reference/parserES5ForOfStatement21.js
@@ -10891,9 +10891,9 @@ module ts { | |||
getSymbolDisplayBuilder().buildTypeDisplay(getReturnTypeOfSignature(signature), writer, enclosingDeclaration, flags); | |||
} | |||
|
|||
function isUnknownIdentifier(location: Node, name: string): boolean { | |||
function isUnknownIdentifier(location: Node, name: string, sourceFile: SourceFile): boolean { |
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 to resolveName
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:
/**
* If the root has a chance of being a synthesized node, callers should also pass a value for
* lowestNonSynthesizedAncestor. This should be an ancestor of root, it should not be synthesized,
* and there should not be a lower ancestor that introduces a scope. This node will be used as the
* location for ensuring that temporary names are unique.
*/
emitDestructuringAssignment(target, value); | ||
} | ||
else { | ||
function emitAssignmentExpression(root: ExpressionStatement | BinaryExpression) { |
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.
Just take the binary expression, and a 'parent' node.
👍 |
function emitAssignmentExpression(root: ExpressionStatement | BinaryExpression) { | ||
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just take an emitParenthesized
parameter instead of this synthetic business?
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 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 comment
The 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 comment
The 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 comment
The 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.
Emit 'for...of' statements in ES3/ES5
I'd love to see support to Iterators here. In principle, the system is type-aware so the emitter only has to generate a while loop to support this capability in ES3/ES5. |
Code generation for iterators will only work if there is a polyfill for Symbol.iterator, which TS does not provide currently. Additionally, the expectation is that pre-ES6 code authors who want to use for..of are just trying to iterate over an array, though this may become less true over time. |
AngularJs2 currently uses iterator in QueryList which will not work in Typescript. Please support iterator. |
@jamesmoey please log an issue with an example. Old pull requests are not really the place to track these sorts of requests, thanks. |
Not sure if the playground uses the most recent version but there is an inconsistency regarding the emitted ES3/ES5 code: var x = [3, 3, 3];
for (var i of x)
{
x = [2, 2];
console.log(i);
} compiles to var x = [3, 3, 3];
for (var _i = 0; _i < x.length; _i++) {
var i = x[_i];
x = [2, 2];
console.log(i);
} printing
instead of (ES6 semantics)
On the other hand, merely adding parentheses: var x = [3, 3, 3];
for (var i of (x))
{
x = [2, 2];
console.log(i);
} correctly compiles to var x = [3, 3, 3];
for (var _a = 0, _b = (x); _a < _b.length; _a++) {
var i = _b[_a];
x = [2, 2];
console.log(i);
} giving the expected output. |
You're right that's a bug. Please log a new issue for this. |
This is the downlevel emit for 'for...of' loops in ES3/ES5.
The following ES6 code:
will be emitted as:
It works in all forms, including with destructuring.
Still pending is the type check work for this downlevel emit. Also, using a for...of loop below ES6 still errors, but it now emits correctly. I will send out the error removal in another change.