1306701 - Allow initializers in ForInStatement heads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

TC39 has revived this syntax for ES2017.

Comment 1

[André Bargull [:anba]]

Attachment: bug1306701.part01.patch

AFAICT this code in BytecodeEmitter::emitDeclarationList is dead, so we can simply remove it. Can you confirm?

Comment 2

[André Bargull [:anba]]

Attachment: bug1306701.part02.patch

Just inlined and removed emitDestructuringOpsHelper

Comment 3

[André Bargull [:anba]]

Attachment: bug1306701.part1.patch

The actual patch.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8796670 [details] [diff] [review]
bug1306701.part01.patch

Review of attachment 8796670 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think this is right.  But, punting to shu for the quick double-check.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8796673 [details] [diff] [review]
bug1306701.part1.patch

Review of attachment 8796673 [details] [diff] [review]:
-----------------------------------------------------------------

The terrorists have won.  :-(

(Or, more precisely, the people who think that if you can't change something *now*, that's already rare, you'll never ever ever be able to wait it out and remove it eventually.  Bah.)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6019,5 @@
> +            MOZ_ASSERT(forInTarget->isKind(PNK_VAR),
> +                       "for-in initializer expression only valid for |var| declarations");
> +
> +            if (!emitDeclarationList(forInTarget))
> +                return false;

Given what we know, very specifically, about the declaration here, I think I'd prefer more-precise actions be taken, than going through emitDeclarationList.  It also seems valuable to keep this edge-case handling, *in* this edge case, rather than requiring generalized declaration-list handling to deal with it, forevermore.

    // Annex B: Evaluate the var-initializer expression if present.
    // |for (var i = initializer in expr) { ... }|
    ParseNode* forInTarget = forInHead->pn_kid1;
    if (parser->handler.isDeclarationList(forInTarget)) {
        ParseNode* decl = parser->handler.singleBindingFromDeclaration(forInTarget);
        if (decl->isKind(PNK_NAME)) {
            if (ParseNode* initializer = decl->expr()) {
                MOZ_ASSERT(forInTarget->isKind(PNK_VAR),
                           "for-in initializers are only permitted for |var| "
                           "declarations");

                auto emitRhs = [initializer](BytecodeEmitter* bce, const NameLocation&, bool) {
                    return bce->emitTree(initializer);
                };

                if (!emitInitializeName(decl, emitRhs))
                    return false;

                // Pop the initializer.
                if (!emit1(JSOP_POP))
                    return false;
            }
        }
    }

::: js/src/tests/ecma_6/extensions/for-in-with-assignments.js
@@ -1,1 @@
> -/*

Please rerecord this as a move from this location to the other one -- clearly most of it's the same and only a few bits are different, and those differences are the important part.

If you are one of the haters using git instead of Mercurial, know that Mercurial as functional VCS supports proper file moving, and you may have to perform the |hg mv ecma_6/extensions/for-in-with-assignment.js ecma_8/Statements/for-in-with-assignments.js| in a Mercurial repository, not a git repository.

::: js/src/tests/ecma_8/Statements/for-in-with-assignment-syntax.js
@@ +21,5 @@
> +    " {x = 0}",
> +];
> +
> +const invalidSyntax = [
> +    ...destructuring.map(binding => `var {binding}`),

Uh, don't you mean ${binding}?  And in the other two.

Comment 6

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> The terrorists have won.  :-(

https://www.youtube.com/watch?v=v5cBkhYBVt8


> ::: js/src/tests/ecma_6/extensions/for-in-with-assignments.js
> @@ -1,1 @@
> > -/*
> 
> If you are one of the haters using git instead of Mercurial, know that
> Mercurial as functional VCS supports proper file moving, [...].

Lalala, don't hear you! :-D

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8796670 [details] [diff] [review]
bug1306701.part01.patch

Review of attachment 8796670 [details] [diff] [review]:
-----------------------------------------------------------------

Indeed, old SM-style comprehensions are dead.

Comment 8

[André Bargull [:anba]]

Attachment: bug1306701-part01.patch

Update reviewers for patch part 0.1

Comment 9

[André Bargull [:anba]]

Attachment: bug1306701-part02.patch

Update patch part 0.2 to apply cleanly on inbound. Carrying r+ from Waldo.

Comment 10

[André Bargull [:anba]]

Attachment: bug1306701-part1.patch

Update patch part 1 to address review comments. Carrying r+ from Waldo.

Comment 11

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=921e413c5b16a228cfaa65a543ee84381261867e

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d4004121f9c
Part 0.1: Remove dead code in BytecodeEmitter::emitDeclarationList. r=Waldo, shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c1417d3155
Part 0.2: Remove no longer needed destructuring helper method from BytecodeEmitter. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e17a8b40d0
Part 1: Evaluate var-initializer expression in for-in loop per Annex B.3.6 (ES2017). r=Waldo

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d4004121f9c
https://hg.mozilla.org/mozilla-central/rev/d4c1417d3155
https://hg.mozilla.org/mozilla-central/rev/86e17a8b40d0