1227677 - Disentangle comprehension for-in/of handling from for-in/of loop handling

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Quarantine the contagion!

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Some minor renaming in for-in stuffs

This might/might not belong in this bug, but I happened to develop the rest of these atop this, and it's a minimal patch.
<none>

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Extract PNK_COMPREHENSIONFOR from PNK_FOR so that comprehension gunk doesn't taint for-loop emitting logic

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Emit code for PNK_COMPREHENSIONFOR using separate code from that used for for-loops

Lots of copypasta, alas.  Aside from removing the code definitely not relevant to comprehensions (the spreading cases, mostly), and adding stack-related comments, I didn't make actual changes.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Rename the misnomer |letDecl| variable associated with for-in/of loop variables to |letBlockScope|

It matches the variable's used meaning more (used to determine when to push/pop a block scope).  And |letDecl| implies relevance to |let| declarations, but |for (let x of []);| doesn't actually trigger |*letDecl|.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Simplify code in light of the fact that for-in/of loops never have a PNK_LEXICALSCOPE declaration node, ergo no letBlockScope

Parser::forStatement never produces PNK_FOR{IN,OF} with first kid not either null or a declaration -- only comprehensions can have PNK_LEXICALSCOPE.  Cleaning up the comprehension code as the complement to this is left to a separate patch.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Simplify emitting of comprehension variables

The code/comments as they exist now, for for-in/of regardless of comprehensions or not, are a bit wrong.  Rewrite some, and simplify the code too.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8691517 [details] [diff] [review]
Some minor renaming in for-in stuffs

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

::: js/src/frontend/Parser.cpp
@@ -5371,5 @@
>                      bool constDecl = tt == TOK_CONST;
>                      isForDecl = true;
> -                    blockObj = StaticBlockObject::create(context);
> -                    if (!blockObj)
> -                        return null();

lol I think jorendorff also removed this recently.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8691575 [details] [diff] [review]
Extract PNK_COMPREHENSIONFOR from PNK_FOR so that comprehension gunk doesn't taint for-loop emitting logic

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

In itself the patch doesn't do anything. I'm assuming the juicy stuff is in the later patches.

::: js/src/frontend/Parser.cpp
@@ +7910,1 @@
>                                                    nullptr, nullptr);

Why is this not switched to handler.newComprehensionFor? I suppose it's because the for-head and the body aren't yet parsed at this point, and are assigned later. Indeed, legacyComprehensionTail is a terrifying function, so it's fine to leave this as is.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8691607 [details] [diff] [review]
Emit code for PNK_COMPREHENSIONFOR using separate code from that used for for-loops

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

This is literally copied emitForIn, emitForOf, and emitForInOrOfVariables, but renamed, right? I did not read them super closely.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5905,5 @@
> +    //
> +    // In `for (let x in/of EXPR)`, ES6 specifies that EXPR is evaluated in a
> +    // scope containing an uninitialized `x`. If EXPR accesses `x`, we should
> +    // get a ReferenceError due to the TDZ violation. This is not yet
> +    // implemented. See bug 1069480.

Does this comment apply to the comprehension case? Doesn't seem to.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/945a33faaac9

Comment 11

[Jeff Walden [:Waldo]]

Horrifically bad try-results that I don't really believe at all (I think it's just a steady stream of unrelated intermittent-orange) have me pushing this in some increments.  That's just the first one, rest to come whenever I manage to convince myself there's not real bustage in them.  Somehow.  Seeing as I don't think there is now but fear the tree-gods.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/945a33faaac9

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/748c27d6775f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17be37dbd32
https://hg.mozilla.org/integration/mozilla-inbound/rev/781b2dc8f216
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d30a3fecc3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/68a9e4279f60

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/748c27d6775f
https://hg.mozilla.org/mozilla-central/rev/e17be37dbd32
https://hg.mozilla.org/mozilla-central/rev/781b2dc8f216
https://hg.mozilla.org/mozilla-central/rev/1d30a3fecc3a
https://hg.mozilla.org/mozilla-central/rev/68a9e4279f60