Allow 'in' to terminate expressions in for fixes #5203 #5219

rhuanjl · 2018-05-23T18:42:46Z

Attempt at fixing issue #5203

Based on #5216

Not 100% sure I've got this right but it passes the mentioned cases and doesn't regress anything I'm aware of.

cc @zenparsing @akroshg

akroshg · 2018-05-23T20:52:42Z

@rhuanjl the fix seems correct to me. I'll let @aneeshdk to comment/approve this. Thanks for making the change.

Cellule · 2018-06-01T21:31:55Z

I still get a syntax error with

for (var b = () => "" in []) {
  undefined;
}

MSLaguana · 2018-06-01T21:48:05Z

test/es6/generators-syntax.js

+    {
+        name : "yield is allowed before 'in' in a for loop control but not elsewhere bug issue #5203",
+        body : function () {
+            assert.doesNotThrow(function () { eval("function* gf() {for(var a = yield in {});}"); }, "Yield is allowed before 'in' in a for loop");


What is the correct interpretation of for(var a = yield 1 in {}) {…? I think the only way for that to parse is as for( var a = (yield 1) in {}), is that how it behaves with this change?

And going further afield, it looks like v8 parses for(var a = yield 'a' in {} in {a: 1}) as for(var a = (yield 'a') in ({} in {a:1})); does that work out here as well?

Both of these work in that way - I'll compose some functionality tests.

rhuanjl · 2018-06-02T15:02:52Z

I've:

Added some functionality tests cases in response to @MSLaguana's points above
Updated latest

@Cellule that case follows a different path in the Parser and would require a different fix notably it seems that after parsing the lambda expression () => "" the parser has not reached the "in" token and is possibly still on the quotes - I'm afraid I'm not sure how/why this happens.

rhuanjl · 2018-06-14T18:01:20Z

Please let me know if I should do anything further on this.

akroshg · 2018-06-14T19:03:08Z

lib/Parser/Parse.h

@@ -780,7 +780,7 @@ class Parser
    template<bool buildAST> ParseNodePtr ParseMemberList(LPCOLESTR pNameHint, uint32 *pHintLength, tokens declarationType = tkNone);
    template<bool buildAST> IdentPtr ParseSuper(bool fAllowCall);

-    bool IsTerminateToken();
+    bool IsTerminateToken(bool inTerminate);


Nit : would it better to understand if this is called as tkInAsTerminate?

Can't we just reuse the name fAllowIn?

I've renamed it to fAllowIn

akroshg · 2018-06-14T19:06:04Z

LGTM - @aneeshdk can you do the review and validate?
Should this be rebase to release/1.10?

Cellule · 2018-06-14T20:38:30Z

I think it should target release/1.10 yeah

aneeshdk · 2018-06-15T06:46:57Z

LGTM. release/1.10 sounds good.

aneeshdk · 2018-06-15T06:51:05Z

test/es6/forInEdgeCases.js

+                    assert.isTrue(false, "shouldn't reach here");
+                }
+            }
+            testGen(gen3, ['d'], 1);


Can we also add a test case where a value is passed into the next call?

I'm sorry I'm not sure what you're asking for.

The testGen function I'm using a the top uses an assertion to verify that the correct value is yielded - not sure what I should o beyond that?

rhuanjl · 2018-06-23T17:10:36Z

I've:

changed to target 1.10
updated to latest
renamed the inTerminate parameter to fAllowIn per review above

I have not:

Added any more tests - @aneeshdk unsure what additional test case you were asking for
Fixed the Lambda returning a string case mentioned by @Cellule : for (var b = () => "" in []) Possibly should open a separate issue for this - there's something else going on here and I can't work it out - after evaluating the lambda the parser seems to think that the next token is a double quote, not the in.

Sorry that I was a bit slow to respond to feedback on this.

rhuanjl · 2018-06-28T16:43:27Z

This will need updating to be compatible with #5380 as they're touching the same code - there's some overlap both this and #5380 fix:

for (var i = () => {} in {});

This also fixes

for (var a = yield 'd' in {} in {a: 1})

Which #5380 does not fix.

BUT #5380 fixes:

for (var b = () => "" in [])

Which this does not fix.

I assume #5380 will land first then can update this to fit with it.

of a for...in loop fixes chakra-core#5203.

dilijev · 2018-06-30T00:38:27Z

@rhuanjl did you make all the updates you wanted to make after #5380 landed?

rhuanjl · 2018-06-30T08:36:49Z

Yes I think this is good to land now.

aneeshdk · 2018-07-12T07:34:24Z

LGTM

rhuanjl · 2018-07-20T22:10:24Z

Please let me know if there is anything further I need to do on this.

Is it going to be merged? Or should I close it?

dilijev · 2018-07-27T21:00:33Z

@rhuanjl Apologies for letting this fall off my radar. You can leave it open. Right now we are re-evaluating which branch to merge this to.

@zenparsing

#5203 Merge pull request #5219 from rhuanjl:fixYieldIn Attempt at fixing issue #5203 Based on #5216 Not 100% sure I've got this right but it passes the mentioned cases and doesn't regress anything I'm aware of. cc @zenparsing @akroshg

@zenparsing

…ons in for fixes #5203 Merge pull request #5219 from rhuanjl:fixYieldIn Attempt at fixing issue #5203 Based on #5216 Not 100% sure I've got this right but it passes the mentioned cases and doesn't regress anything I'm aware of. cc @zenparsing @akroshg

rhuanjl · 2018-07-27T22:24:41Z

Thanks for merging this.

dilijev · 2018-07-31T16:58:03Z

@rhuanjl thanks for the contribution!

akroshg requested a review from aneeshdk May 23, 2018 19:10

rhuanjl force-pushed the fixYieldIn branch from f095914 to 189396f Compare May 26, 2018 17:58

MSLaguana reviewed Jun 1, 2018

View reviewed changes

rhuanjl force-pushed the fixYieldIn branch from 189396f to 55b8bbf Compare June 2, 2018 13:30

akroshg reviewed Jun 14, 2018

View reviewed changes

akroshg assigned aneeshdk Jun 14, 2018

aneeshdk reviewed Jun 15, 2018

View reviewed changes

aneeshdk assigned rhuanjl and unassigned aneeshdk Jun 15, 2018

dilijev added the External Contributor label Jun 16, 2018

rhuanjl changed the base branch from master to release/1.10 June 23, 2018 13:03

rhuanjl force-pushed the fixYieldIn branch 2 times, most recently from 0ed24a7 to 8410b0d Compare June 23, 2018 13:26

Allow 'in' to terminate expressions in the control

c212f22

of a for...in loop fixes chakra-core#5203.

rhuanjl force-pushed the fixYieldIn branch from 8410b0d to c212f22 Compare June 29, 2018 23:33

dilijev assigned aneeshdk and unassigned rhuanjl Jun 30, 2018

aneeshdk removed their assignment Jul 12, 2018

aneeshdk assigned dilijev Jul 12, 2018

chakrabot merged commit c212f22 into chakra-core:release/1.10 Jul 27, 2018

rhuanjl deleted the fixYieldIn branch July 27, 2018 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow 'in' to terminate expressions in for fixes #5203 #5219

Allow 'in' to terminate expressions in for fixes #5203 #5219

rhuanjl commented May 23, 2018

akroshg commented May 23, 2018

Cellule commented Jun 1, 2018

MSLaguana Jun 1, 2018

MSLaguana Jun 1, 2018

rhuanjl Jun 1, 2018

rhuanjl commented Jun 2, 2018

rhuanjl commented Jun 14, 2018

akroshg Jun 14, 2018

aneeshdk Jun 15, 2018

rhuanjl Jun 23, 2018

akroshg commented Jun 14, 2018

Cellule commented Jun 14, 2018

aneeshdk commented Jun 15, 2018

aneeshdk Jun 15, 2018

rhuanjl Jun 23, 2018

rhuanjl commented Jun 23, 2018 •

edited

Loading

rhuanjl commented Jun 28, 2018

dilijev commented Jun 30, 2018

rhuanjl commented Jun 30, 2018

aneeshdk commented Jul 12, 2018

rhuanjl commented Jul 20, 2018

dilijev commented Jul 27, 2018

rhuanjl commented Jul 27, 2018

dilijev commented Jul 31, 2018

Allow 'in' to terminate expressions in for fixes #5203 #5219

Allow 'in' to terminate expressions in for fixes #5203 #5219

Conversation

rhuanjl commented May 23, 2018

akroshg commented May 23, 2018

Cellule commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Jun 2, 2018

rhuanjl commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akroshg commented Jun 14, 2018

Cellule commented Jun 14, 2018

aneeshdk commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Jun 23, 2018 • edited Loading

rhuanjl commented Jun 28, 2018

dilijev commented Jun 30, 2018

rhuanjl commented Jun 30, 2018

aneeshdk commented Jul 12, 2018

rhuanjl commented Jul 20, 2018

dilijev commented Jul 27, 2018

rhuanjl commented Jul 27, 2018

dilijev commented Jul 31, 2018

rhuanjl commented Jun 23, 2018 •

edited

Loading