336378 - (js1.7let) Support let bindings, blocks, and expressions in JS1.7

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

The proposal, drafted by Lars Hansen after Dave Herman's original proposal in the ECMA TG1 ES4 wiki, looks stable.  The link, which should work by June, is in this bug's URL.  Here are the relevant bits:

''Let'' bindings

This proposal introduces a keyword let that provides local scoping constructs for variables, constants, and functions.

The rationale for the proposal is that tighter scope discipline for all variables is Good.

Let statement

Syntax:

   LetStatement ::= "let" "(" (VarInit ("," VarInit)*)? ")" Block
   VarInit ::= ID (":" Type)? ("=" Expression)?

The let statement binds zero or more variables whose scopes are the following Block (but not the binding clause), and then evaluates Block. The completion value of the statement is the completion value of the Block.

There is nothing magic about the Block. var statements inside the Block are hoisted as they are normally. Function declarations are not allowed here (just as they are not normally allowed inside Blocks).

Observe that the parens following let are required, they could be optional but for symmetry with Let expressions (where they are not optional) they are made mandatory.

Example:

   let (x=x+10, y=12) {
     print(x+y);
   }

Let expression

Syntax:

    LetExpression ::= let "(" (VarInit ("," VarInit)*)? ")" ListExpression

The variables bound by the let expression have as their scope the ListExpression (but not the binding clause). The expression evaluates the initializers in order and then evaluates ListExpression, whose value is the result of the entire expression.

LetExpressions are PrimaryExpressions in the sense of section 14.2 of the draft spec.

Example:

   print( let (x=x+10, y=12) x+y )

Restriction: If the LetExpression is in the statement position then the ListExpression cannot be an object literal, as that would be seen to be the start of a Block (and the statement would be taken to be a LetStatement).
Let definitions

The let keyword can be used to introduce definitions of variables, constants, and functions into a block. For example,

   if (x > y)
   {
      let const k = 37;
      let gamma : int = 12.7 + k;
      let i = 10;
      let function f(n) { return (n/3)+k; }
      return f(gamma) + f(i);
   }

Variables, functions, and constants declared by let, let function, and let const respectively have as their scope the entire Block in which they are defined and any inner Blocks in which they are not redefined.

In programs, functions, and classes let does not create properties on the global and class objects like var does; instead, it creates properties in an implicit block created for the evaluation of statements in those contexts.

In functions, let executed by eval does not create properties on the variable object (activation object or innermost binding rib) like var does; instead, it creates properties in an implicit block created for the evaluation of statements in the program. (This is just a consequence of eval operating on programs coupled with the preceding rule.)

Let-scoped variables in ''for'' loops

let can be used to bind variables locally in the scope of for loops in the same way that var can. For example,

   var i=0;
   for ( let i=i ; i < 10 ; i++ )
     print(i);

   for ( let [name,value] in obj )
     print("Name: " + name + ", Value: " + value);

In loops of the form for (let E1 ; E2 ; E3) S the scope of variables bound in E1 excludes E1. The first example illustrates this case, where a locally bound i takes the value of the outer i as its initial value.

In loops of the form for (let E1 in E2) S and for each (let E1 in E2) the scope of variables bound in E1 excludes both E1 and E2.
-----

I made a small edit to clarify that let at top-level in a function operates on an implicit block within the function body.  It does not operate on the variable object, because that would allow aliasing via arguments: given

  function f(a) {
    let a = 'hi'
    arguments[0] = 42
    return a
  }

f(anything) should return 'hi', not 42.

/be

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Step 1

Bite-size steps: this patch moves away from a source note per variable type and instead uses one source note with many different immediate offsets (var and const now, let variants to come).

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 222959 [details] [diff] [review]
Step 1

>              * In the 'for (var x = i in o) ...' case, the js_EmitTree(...pn3)
>-             * called here will generate the SRC_VAR note for the assignment
>-             * op that sets x = i, hoisting the initialized var declaration
>-             * out of the loop: 'var x = i; for (x in o) ...'.
>+             * called here will generate the proper source note for the
>+             * assignment op that sets x = i, hoisting the initialized var
>+             * declaration out of the loop: 'var x = i; for (x in o) ...'.
>              *
>              * In the 'for (var x in o) ...' case, nothing but the prolog op
>-             * (if needed) should be generated here, we must emit the SRC_VAR
>-             * just before the JSOP_FOR* opcode in the switch on pn3->pn_type
>-             * a bit below, so nothing is hoisted: 'for (var x in o) ...'.
>+             * (if needed) should be generated here, we must emit the source
>+             * note just before the JSOP_FOR* opcode in the switch on
>+             * pn3->pn_type a bit below, so nothing is hoisted:
>+             * 'for (var x in o) ...'.

Sorry to pick comment-only nits, but with just a few standard terms used above you could avoid all the ripple-through affecting multiple lines: respectively s/SRC_VAR/proper/ and s/SRC_VAR/note/.

>@@ -3352,22 +3353,24 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>             if (pn->pn_op != JSOP_NOP && js_Emit1(cx, cg, pn->pn_op) < 0)
>                 return JS_FALSE;
> #endif
> 
>             /* Compile a JSOP_FOR* bytecode based on the left hand side. */
>             emitIFEQ = JS_TRUE;
>             switch (pn3->pn_type) {
>               case TOK_VAR:
>                 pn3 = pn3->pn_head;
>                 JS_ASSERT(pn3->pn_type == TOK_NAME);
>-                if (!pn3->pn_expr && js_NewSrcNote(cx, cg, SRC_VAR) < 0)
>+                if (!pn3->pn_expr && js_NewSrcNote2(cx, cg, SRC_DECLTYPE,
>+                                                    SN_DECL_VAR) < 0) {

Nit: break after && per standard style.

Name nits below.

>+    SRC_DECLTYPE    = 6,        /* type of a declaration (var, const, let*) */

SRC_DECL, all of these are "types".

>+/* Constants for the SRC_DECLTYPE source note. */
>+#define SN_DECL_VAR             0
>+#define SN_DECL_CONST           1

SN_* are note opcode macros, SRC_DECL_VAR etc. are better here as derived names from SRC_DECL.

r=me, new patch if you like.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Step 1, v2

I'm about to check this in.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip

I haven't tested this heavily enough to request review on this yet, but I think this is a good step in the right direction.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 223147 [details] [diff] [review]
wip

>-            if (noteIndex < 0 ||
>-                js_Emit1(cx, cg, JSOP_POP) < 0) {
>+            if (noteIndex < 0 || js_Emit1(cx, cg, JSOP_POP) < 0) {
>                 return JS_FALSE;
>             }

Unbrace if you pull up the condition to fit on one line, and the then part is one line.

>+      case TOK_LET:
>+        tmp = noteIndex = -1;
>+        for (pn2 = pn->pn_head; ; pn2 = pn2->pn_next) {
>+            JS_ASSERT(pn2->pn_type == TOK_NAME);
>+            if (!BindNameToSlot(cx, &cg->treeContext, pn2))
>+                return JS_FALSE;
>+            /* Emit a getlocal for the decopmiler. */

Spelling, and a blank line before the comment (it's a new millennium) to breathe a bit.

>+            JS_ASSERT(pn2->pn_slot >= 0);
>+            if ((pn2 == pn->pn_head &&
>+                 js_NewSrcNote2(cx, cg, SRC_DECL, SRC_DECL_LET) < 0) ||
>+                !js_EmitTree(cx, cg, pn2)) {

Do you need js_EmitTree here?  It would be better to use EMIT_UINT16_IMM_OP, unless (see below) you end up making a subroutine of all this code to share with the TOK_VAR case (in which case what do we need here to handle both cases?).

>+                return JS_FALSE;
>+            }
>+            tmp = CG_OFFSET(cg);
>+            if (noteIndex >= 0) {
>+                if (!js_SetSrcNoteOffset(cx, cg, (uintN)noteIndex, 0, tmp-off))
>+                    return JS_FALSE;
>+            }
>+            if (!pn2->pn_next)
>+                break;
>+            off = tmp;
>+            noteIndex = js_NewSrcNote2(cx, cg, SRC_PCDELTA, 0);
>+            if (noteIndex < 0 || js_Emit1(cx, cg, JSOP_POP) < 0)
>+                return JS_FALSE;
>+        }
>+        if (pn->pn_extra & PNX_POPVAR) {
>+            if (js_Emit1(cx, cg, JSOP_POP) < 0)
>+                return JS_FALSE;
>+        }
>+        break;
> #endif

#if JS_HAS_BLOCK_SCOPE around this and other new let-induced code.

This looks a lot like the TOK_VAR: code, so how about an EmitVariables subroutine that is parameterized appropriately?

>Index: jsemit.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsemit.h,v
>retrieving revision 3.45
>diff -p -U10 -r3.45 jsemit.h
>--- jsemit.h	22 May 2006 23:33:57 -0000	3.45
>+++ jsemit.h	24 May 2006 08:11:55 -0000
>@@ -108,41 +108,42 @@ struct JSTreeContext {              /* t
>     uint16          flags;          /* statement state flags, see below */
>     uint16          numGlobalVars;  /* max. no. of global variables/regexps */
>     uint32          tryCount;       /* total count of try statements parsed */
>     uint32          globalUses;     /* optimizable global var uses in total */
>     uint32          loopyGlobalUses;/* optimizable global var uses in loops */
>     JSStmtInfo      *topStmt;       /* top of statement info stack */
>     JSStmtInfo      *topScopeStmt;  /* top lexical scope statement */
>     JSObject        *blockChain;    /* compile time block scope chain */
>     JSAtomList      decls;          /* function, const, and var declarations */
>     JSParseNode     *nodeList;      /* list of recyclable parse-node structs */
>+    JSParseNode     *blockNode;     /* parse node for a lexical scope */

Put this right under blockChain and maybe note XXX unify or something?

>@@ -1136,20 +1140,25 @@ Statements(JSContext *cx, JSTokenStream 
>                 !js_AllocTryNotes(cx, (JSCodeGenerator *)tc) ||
>                 !js_EmitTree(cx, (JSCodeGenerator *)tc, pn2)) {
>                 tt = TOK_ERROR;
>                 break;
>             }
>             RecycleTree(pn2, tc);
>         } else {
>             PN_APPEND(pn, pn2);
>         }
>     }
>+    if (tc->topStmt != saveStmt)
>+        pn = tc->blockNode;
>+    if (tc->blockNode != pn)
>+        js_PopStatement(tc);
>+    tc->blockNode = saveBlock;

Set off this paragraph with a blank line on either side, and comment it.

>+static JSStmtInfo *
>+FindBlockStatement(JSTreeContext *tc)
>+{
>+    JSStmtInfo *stmt;
>+
>+    stmt = tc->topScopeStmt;

Need to start at tc->topStmt, since any blocks inside a with or catch would be missed by starting at tc->topScopeStmt.

>+static JSBool
>+DeclareLetVar(JSContext *cx, JSAtom *atom, JSObject *blockObj, JSScope *scope,
>+              JSInt32 idx, JSTokenStream *ts, JSBool allowDup)

jsuint index would match the array comprehension code better than JSInt32 idx.  In general we eschew the (later, historically) NSPR PRInt32, etc. names.

>+{
>+    JSScopeProperty *sprop;
>+
>+    if (idx < 0)
>+        idx = OBJ_BLOCK_COUNT(cx, blockObj);

Why do this here, instead of making those callers that need it just pass OBJ_BLOCK_COUNT themselves?  Especially if there is only one caller.

>+    sprop = SCOPE_GET_PROPERTY(scope, ATOM_TO_JSID(atom));
>+    if (sprop) {
>+        const char *name;
>+
>+        JS_ASSERT(sprop->flags & SPROP_HAS_SHORTID);
>+        JS_ASSERT((uint16)sprop->shortid < idx);
>+        OBJ_DROP_PROPERTY(cx, blockObj, (JSProperty *) sprop);
>+        if (allowDup)
>+            return JS_TRUE;
>+
>+        name = js_AtomToPrintableString(cx, atom);
>+        if (name) {
>+            js_ReportCompileErrorNumber(cx, ts, JSREPORT_TS | JSREPORT_ERROR,
>+                                        JSMSG_REDECLARED_VAR, js_var_str,

js_let_str, right?

>+                                        name);
>+        }
>+        return JS_FALSE;
>+    }
>+
>+    if (idx == JS_BIT(16)) {
>+        js_ReportCompileErrorNumber(cx, ts,
>+                            JSREPORT_TS | JSREPORT_ERROR,
>+                            JSMSG_ARRAY_INIT_TOO_BIG);

This is the wrong error number for too many let declarations.  Parameterize.

>+      case TOK_LET:
>+      {

Gratuitous bracing.

>+        /*
>+         * Are we looking at a statement let declaration? If so, we need to

"statement let declaration" seems like one too many words -- "let declaration" (vs. "let block" vs. "let expression") is better.

>+            if (!tc->blockNode || tc->blockNode->pn_type != TOK_LEXICALSCOPE) {
>+                JS_ASSERT(!tc->blockNode ||
>+                          tc->blockNode->pn_type == TOK_LC);
>+                pnlex = NewParseNode(cx, ts, PN_NAME, tc);
>+                if (!pnlex)
>+                    return NULL;
>+
>+                atom = js_AtomizeObject(cx, obj, 0);
>+                if (!atom)
>+                    return NULL;
>+                pnlex->pn_type = TOK_LEXICALSCOPE;
>+                pnlex->pn_atom = atom;
>+                pnlex->pn_expr = tc->blockNode;
>+                tc->blockNode = pnlex;
>+            } else {
>+                pnlex = tc->blockNode;
>+            }

Vestigial else clause should be avoided here by loading pnlex = tc->blockNode before the if and using pnlex in the condition.  Also, any reason not to use pn1 from the top scope in the function?

>+            pn->pn_extra |= PNX_POPVAR;
>+            break;
>+        }
>+      }

Kill gratuitous closing brace, but do add a blank line between cases.

>       case TOK_RETURN:
[
 . . .
]
>+                    if (!DeclareLetVar(cx, atom, obj, scope, atomIndex++,
>+                                       ts, JS_TRUE)) {
>+                        return NULL;

Hrm, I thought I changed to use a jsuint index local in the TOK_LB case here in PrimaryExpr, instead of atomIndex.  Oh, I did -- in the JS_1_7_ALPHA_BRANCH.  Can you merge to that branch, so we can get incremental patches landed even faster?

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: More wip

This is still undertested, but except for a problem with let statements at top level crashing, it seems to work pretty well.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Interdiff to fix top-level crashiness

Here's an interdiff that stops the crashing at top-level by making top-level 'let' statements pretend to be 'var' statements.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Diff to fix let blocks

This is a diff against the JS1.7 branch that I'm about to check in. It fixes let blocks to not assert and makes them work by only hiding the topmost scope when emitting the initializing expression.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Let expressions

I'm not sure if this is the sort of idea that brendan had for JSOP_LEAVEBLOCKEXPR, but it seems to work.

Up next: a real fix for let declarations at the top level.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Expressions need to do the right thing on the stack

Comment 11

[Brendan Eich [:brendan]]

Is this all fixed now, and the patches obsolete?

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, this is fixed.

Comment 13

[Bob Clary [:bc] (inactive)]

js> if (10 > 1) { let const k = 37; }
typein:1: SyntaxError: missing ; before statement:
typein:1: if (10 > 1) { let const k = 37; }
typein:1: ..................^

bug?

Comment 14

[Brendan Eich [:brendan]]

let const and let function did not make js1.7. Need to spec js1.8.

/be