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

Parenthize "in" in for-loop init, even when init has nested for-loop #3324

Merged
merged 1 commit into from Feb 5, 2016

Conversation

Projects
None yet
4 participants
@zjmiller
Contributor

zjmiller commented Feb 5, 2016

When transpiling the following valid JS:

for (function(){for(;;);} && (a in b);;);

Babel returned the following invalid JS:

for (function () {
  for (;;);
} && a in b;;);

Note the dropped parens around the in expression. This results in a syntax error.

This happened because Babel's code generator kept track of whether it was in a for-loop init with a single boolean (see 6caaf68). Exiting a for-loop init toggled this variable to false. So exiting the nested for-loop's init made the code generator think it wasn't in any for-loop init, even though it was still in the enclosing one. Since it didn't think it was in a for-loop init, it didn't bother to wrap the in expression in parens.

This was fixed by changing this._inForStatementInit from a boolean to a counter. The counter starts at 0. Every time we enter a for-loop init, we increment the counter. Every time we exit a for-loop init, we decrement the counter. When it comes to for-loop init nesting, if we've exited as many as we've entered, we get back to 0, and we know that we're not in a for-loop init. If we've exited fewer than we've entered, the counter is positive, so we know that we're still in a for-loop init, and we wrap in expressions in parens.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Feb 5, 2016

Current coverage is 85.14%

Merging #3324 into master will decrease coverage by -0.23% as of a8f103d

@@            master   #3324   diff @@
======================================
  Files          215     215       
  Stmts        15762   15768     +6
  Branches      3385    3385       
  Methods          0       0       
======================================
- Hit          13457   13425    -32
- Partial        684     722    +38
  Missed        1621    1621       

Review entire Coverage Diff as of a8f103d

Powered by Codecov. Updated on successful CI builds.

codecov-io commented Feb 5, 2016

Current coverage is 85.14%

Merging #3324 into master will decrease coverage by -0.23% as of a8f103d

@@            master   #3324   diff @@
======================================
  Files          215     215       
  Stmts        15762   15768     +6
  Branches      3385    3385       
  Methods          0       0       
======================================
- Hit          13457   13425    -32
- Partial        684     722    +38
  Missed        1621    1621       

Review entire Coverage Diff as of a8f103d

Powered by Codecov. Updated on successful CI builds.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Feb 5, 2016

Member

Awesome - maybe we want a comment to explain the need for an array (nested loops)

cc @amasad

Member

hzoo commented Feb 5, 2016

Awesome - maybe we want a comment to explain the need for an array (nested loops)

cc @amasad

@zjmiller

This comment has been minimized.

Show comment
Hide comment
@zjmiller

zjmiller Feb 5, 2016

Contributor

Sure I'll edit my original comment.

Contributor

zjmiller commented Feb 5, 2016

Sure I'll edit my original comment.

@amasad

This comment has been minimized.

Show comment
Hide comment
@amasad

amasad Feb 5, 2016

Member

Great, thanks! Would it be simpler if we moved to a counter rather than a stack? Because we're not really trying to keep data

Member

amasad commented Feb 5, 2016

Great, thanks! Would it be simpler if we moved to a counter rather than a stack? Because we're not really trying to keep data

@zjmiller

This comment has been minimized.

Show comment
Hide comment
@zjmiller

zjmiller Feb 5, 2016

Contributor

Yeah I think it's a toss up. The stack seems a little easier to understand (maybe). Especially if you're not that familiar with the whole issue of "in" needing parens here. But I agree that technically the stack is a bit of overkill. I'd be happy to amend the commit to use a counter if that seems better.

Contributor

zjmiller commented Feb 5, 2016

Yeah I think it's a toss up. The stack seems a little easier to understand (maybe). Especially if you're not that familiar with the whole issue of "in" needing parens here. But I agree that technically the stack is a bit of overkill. I'd be happy to amend the commit to use a counter if that seems better.

@amasad

This comment has been minimized.

Show comment
Hide comment
@amasad

amasad Feb 5, 2016

Member

yeah I actually think a counter would be better if that's not too much trouble 💃

Member

amasad commented Feb 5, 2016

yeah I actually think a counter would be better if that's not too much trouble 💃

@zjmiller

This comment has been minimized.

Show comment
Hide comment
@zjmiller

zjmiller Feb 5, 2016

Contributor

Cool let me know what you think. I'll edit my original comment to reflect the difference.

Contributor

zjmiller commented Feb 5, 2016

Cool let me know what you think. I'll edit my original comment to reflect the difference.

@amasad

This comment has been minimized.

Show comment
Hide comment
@amasad

amasad Feb 5, 2016

Member

awesome, thanks! will let the CI run and then merge

Member

amasad commented Feb 5, 2016

awesome, thanks! will let the CI run and then merge

hzoo added a commit that referenced this pull request Feb 5, 2016

Merge pull request #3324 from zjmiller/master
Parenthize "in" in for-loop init, even when init has nested for-loop

@hzoo hzoo merged commit 4e619db into babel:master Feb 5, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Feb 5, 2016

Member

Thanks @zjmiller!

Member

hzoo commented Feb 5, 2016

Thanks @zjmiller!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment