348904 - Decompiling for (let i = foo ...) loops hoists the let declaration

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Bug 344601 fixed let expressions and let statements to decompile properly, leaving one more problem with let decompilation: given

  for (let i = 3 in {});

We currently decompile (as for the var case):

  let i = 3;
  for (i in {}) {
  }

This is not the same thing, though and will require some work to get right.

Comment 1

[Brendan Eich [:brendan]]

*** Bug 349605 has been marked as a duplicate of this bug. ***

Comment 2

[Brendan Eich [:brendan]]

With let, the initializer can be dropped because there's no way to use the value of the let variable if the loop does not iterate.  Patch next.

/be

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> With let, the initializer can be dropped because there's no way to use the
> value of the let variable if the loop does not iterate.

Unless, of course, the initializer has side effects.  Blech.

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (In reply to comment #2)
> > With let, the initializer can be dropped because there's no way to use the
> > value of the let variable if the loop does not iterate.
> 
> Unless, of course, the initializer has side effects.  Blech.

So 'for (let x = i in o) ...' should translate to 'i; for (let x in o) ...' and the useless expression eliminator should have a crack at i.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix, plus cleanups

Detailed comments, meant to be read side by side with the patch

- Clean up EmitVariables so it does not have to unsynthesize logic from its
  popScope parameter, using knowledge about how its one caller that passes a
  possibly-non-false value, the TOK_LET case in js_EmitTree, synthesizes the
  actual parameter.  Sketch in miniature:

  EmitVariables(popScope) { inLetHead = popScope && !inForInit; ... }
  js_EmitTree() {
    ... case TOK_LET: EmitVariables(popScope = inLetHead || inForInit) ...
  }

  Obviously, inLetHead is correctly computed, but why should EmitVariables
  have to test inForInit again when its significant caller did?  This breaks
  the local abstraction.  The patch refactors to have EmitVariables take an
  inLetHead formal parameter, and synthesize its own popScope, so that the
  TOK_LET case doesn't have to test inForInit at all.

- Re-order var order to match def order in EmitVariables

- Add assertions about the variable list from a 'for (var ... in obj)' loop
  being unit length, and take advantage of that fact (i.e., pn->pn_count == 1)
  instead of a longer test (which moves to an assertion guarded by the shorter
  test) in the destructuring code in EmitVariables.

- Fix bug 348904 by special-casing the awful ECMA-mandated case of a for-in
  loop with an initialized variable declaration to the left of in.  This case
  must be handled by evaluating the initializer before the loop starts, and
  if the loop does not iterate, binding the initializer's value to the loop
  variable.  But for a |for (let x = i in o) ...| loop, the let variable |x|
  must be scoped by the loop body, so there only reason to evaluate |i| is if
  it might have side effects.

  An alterna-patch might avoid hacking the code generator, instead rewriting
  the AST for this case to 'i; for (x in o) ...' -- except that we must not
  set the normal completion result of such a  generated statement-list to the
  value of the |i| expression, which means selecting JSOP_POP instead of the
  usual JSOP_POPV used for an expression statement; and such rewriting must
  preserve well-formedness by wrapping the statement-list in a TOK_LC node,
  which is a pain to do in the current parser (each recursive activation has
  no access to its parent node).

- Some c.s.e. using pn3 to hold NULL for the JSOP_ARGUMENTS special case,
  otherwise to hold pn2->pn_expr, in the TOK_NAME part of the EmitVariables
  main loop body.

- in the TOK_FOR: case of js_EmitTree, add a comment about this mess (similar
  to the more detailed comments in EmitVariables), and lose a nearby redundant
  assignment 'type = pn3->pn_type' (you can see the straight line code from
  the first such assignment to the second, with no pn3 mutation in between).

- Beef up assertions under TOK_LET and TOK_VAR cases in the TOK_FOR/IN main
  case in js_EmitTree.

- Always emit a SRC_DECL note for the for-let-in opcode -- up till now this
  was done only if there was not an initializer for the for-in variable, since
  when there was an initializer, the whole |var x = i| was hoisted out of the
  'for (x in o) ...' loop.

- Fix a broken useless expression elimination attempt further down in the
  main TOK_FOR/IN js_EmitTree case.  The useful in/out parameter passed to
  CheckSideEffects must be initialized to false, not true, or CheckSideEffects
  will just return right away!  This case was trying to avoid the JSOP_FORELEM
  followed by JSOP_ENUMELEM dance, which is required *only* in case the index
  expression in a |for (x[i++].p in o) ...| loop has side effects, optimizing
  |for (x[i].p in o) ...| to use a more efficient bytecode sequence.

- Clean up the parser for case TOK_FOR/IN in Stament with some comments, by
  moving the parenthesized left-hand-side of |in| paren-skipping loop to where
  it is needed, and with a little c.s.e. love using tt.

/be

Comment 6

[Brendan Eich [:brendan]]

This is important to fix for js1.7.

/be

Comment 7

[Brendan Eich [:brendan]]

Argh, I moved the paren-skipping loop to where it was needed, but failed to add a null check to handle syntax error in the expression to the left of IN, or before the first SEMI, in a for loop (this is in jsparse.c of course).

Here's the interdiff against the patch:

diff -u jsparse.c jsparse.c
--- jsparse.c   26 Aug 2006 00:58:04 -0000
+++ jsparse.c   26 Aug 2006 00:55:11 -0000
@@ -2724,8 +2724,10 @@
 #endif
             } else {
                 pn1 = Expr(cx, ts, tc);
-                while (pn1->pn_type == TOK_RP)
-                    pn1 = pn1->pn_kid;
+                if (pn1) {
+                    while (pn1->pn_type == TOK_RP)
+                        pn1 = pn1->pn_kid;
+                }
             }
             tc->flags &= ~TCF_IN_FOR_INIT;
             if (!pn1)

/be

Comment 8

[Brendan Eich [:brendan]]

Need to land this patch before getting on to fixing bug 346642.  Reviewers, start your engines...

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 235506 [details] [diff] [review]
fix, plus cleanups

r=mrbkap with the additional null check that you pointed out.

Comment 10

[Brendan Eich [:brendan]]

Attachment: fix, v2

Firefox works, seems to pass testsuite (still waiting for full suite to finish; passes the js1_7/block suite).

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: fix, v2, updated to merge to trunk head

Need some reviewing here, pronto.

/be

Comment 12

[Brendan Eich [:brendan]]

Checked in on trunk.

/be

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 236144 [details] [diff] [review]
fix, v2, updated to merge to trunk head

This should be ok to land on 1.8 branch after a few days' baking.  It is mainly about the new let declarations, but the code to generate code for all variable declarations is shared, so it touches common control flow paths.  It's mainly adding extra logic for a hard case, as can be seen  from the line deltas (+133 -42 for jsemit.c).  The - lines are mostly comment changes and logic refactoring and factoring.

/be

Comment 14

[Bob Clary [:bc] (inactive)]

Checking in regress-348904.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-348904.js,v  <--  regress-348904.js
initial revision: 1.1

Comment 15

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060901 windows/mac*/linux

Comment 16

[Mike Schroepfer]

Comment on attachment 236144 [details] [diff] [review]
fix, v2, updated to merge to trunk head

a=schrep for drivers.

Comment 17

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060906 windows/mac*/linux
note to self: 1.9 falsely fails due to the difference in decompiling the  (y = 4); between 1.8 and trunk.

Comment 19

[Bob Clary [:bc] (inactive)]

Checking in regress-348904.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-348904.js,v  <--  regress-348904.js
new revision: 1.2; previous revision: 1.1