350387 - Var declaration in let statement is ignored if they have same name

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[nanto_vi (TOYAMA Nao)]

In global code, var declarations in let statements are ignored if the name of variable declaraed by var is same as the name of variable bound by let.  Referencing the variable outside the let-scope causes ReferenceError.

Actual:
js> let (x = 2) { var x; } print(x);
typein:1: ReferenceError: x is not defined

Expected:
"undefined" is shown.

In a function, the same code works as expected.
js> (function () { let (x = 2) { var x; } print(x); })();
undefined

This bug is found by Hawk's example.
http://blog.hawklab.jp/item-87.html (written in Japanese)

Comment 1

[Brendan Eich [:brendan]]

The behavior at top level is correct, based on the same rule that's in ECMA-262 Edition 3 10.1.3 -- a var declaration does not rebind anything already bound.  The behavior in a function is the bug here.

/be

Comment 2

[nanto_vi (TOYAMA Nao)]

Hm, so why does |{ var x; let x; } print(x);| at top level throw ReferenceError?
Are let declarations processed before var declarations are done?

Comment 3

[nanto_vi (TOYAMA Nao)]

I still think the behavior at top level is the bug.  Consider the following code at top level:

  let (x) { var x; } print(x);

ECMA-262 Edition 3 10.1.3 says "If there is already a property of the variable object with the name of a declared variable, the value of the property and its attributes are not changed."  At top level, the variable object is the global object and the global object has no property named x.  So, "var x" in the let statement should create a property of the global object whose name is x and print(x) should show "undefined", not throw a ReferenceError.

If the variable object in the let block is the let block itself, "var x" doesn't create a property.  In this case,

  let (x) { var y; } print(y);

"var y" in the above code creates a property of the let block and print(y) will throw a ReferenceError.  But the above code doesn't throw an error in the current implementation and shouldn't.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> I still think the behavior at top level is the bug.  Consider the following
> code at top level:
> 
>   let (x) { var x; } print(x);
> 
> ECMA-262 Edition 3 10.1.3 says "If there is already a property of the variable
> object with the name of a declared variable, the value of the property and its
> attributes are not changed."  At top level, the variable object is the global
> object and the global object has no property named x.  So, "var x" in the let
> statement should create a property of the global object whose name is x and
> print(x) should show "undefined", not throw a ReferenceError.

You are right, I was wrong in comment 1.  Sorry about that!

> If the variable object in the let block is the let block itself, "var x"
> doesn't create a property.  In this case,
> 
>   let (x) { var y; } print(y);
> 
> "var y" in the above code creates a property of the let block and print(y) will
> throw a ReferenceError.  But the above code doesn't throw an error in the
> current implementation and shouldn't.

Compare top-level script with function body:

js> let (x) { var x; } print(x);
typein:1: ReferenceError: x is not defined
js> function f(){let (x) { var x; } print(x);}
js> f()
undefined

The JSOP_DEFVAR prolog bytecode is not being emitted to bind x on entry to the global execution context, per ECMA-262.  This because the var x is processed by binding x to the let variable.  Should be fixed for js1.7.  mrbkap, can you take a look at this one?

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix, v1

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 237382 [details] [diff] [review]
Fix, v1

Yuck -- why not SRC_DECL_VAR for a variable?  Or better yet, as I thought we agreed on, a local enum?

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Using local defines

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 237395 [details] [diff] [review]
Using local defines

>+#define LET_DECLARATION 1
>+#define VAR_DECLARATION 2

Move these up above js_LexicalLookup and use LET_DECLARATION vs. 0 there instead of boolean letdecl.

Might also prefer shorter names for local names (LET_DECL, VAR_DECL).

Then you can just say:

    stmt = (decltype != VAR_DECLARATION)
           ? js_LexicalLookup(tc, atom, &slot, decltype)
           : NULL;

instead of:

>-    stmt = js_LexicalLookup(tc, atom, &slot, letdecl);
>+    stmt = (decltype != VAR_DECLARATION)
>+           ? js_LexicalLookup(tc, atom, &slot, decltype == LET_DECLARATION)
>+           : NULL;
>     if (stmt) {

This if (stmt) after a ?: initializes stmt could be

    if (decltype != VAR_DECLARATION &&
        (stmt = js_LexicalLookup(tc, atom, &slot, decltype)) {

instead.

>@@ -3143,18 +3154,22 @@ typedef JSBool
> (*DestructuringDeclEmitter)(JSContext *cx, JSCodeGenerator *cg, JSOp prologOp,
>                             JSParseNode *pn);
> 
> static JSBool
> EmitDestructuringDecl(JSContext *cx, JSCodeGenerator *cg, JSOp prologOp,
>                       JSParseNode *pn)
> {
>     JS_ASSERT(pn->pn_type == TOK_NAME);
>-    if (!BindNameToSlot(cx, &cg->treeContext, pn, prologOp == JSOP_NOP))
>+    if (!BindNameToSlot(cx, &cg->treeContext, pn,
>+                        (prologOp == JSOP_NOP)
>+                        ? LET_DECLARATION
>+                        : VAR_DECLARATION)) {

Have to use ?: here, but not in the next non-zero call:

>-        if (!BindNameToSlot(cx, &cg->treeContext, pn2, let))
>+        if (!BindNameToSlot(cx, &cg->treeContext, pn2,
>+                            let ? LET_DECLARATION : VAR_DECLARATION)) {

Change let to uintN and use LET_DECL vs. 0 here (or not -- I'm cool with super-booleans where 2 is not a value and you can count on passing 0 or 1 to a function taking 0, 1, or 2) to avoid ?:.

In general, too many names for the same value-ideas means translation code in the object file as well as source bloat.  I'm agin' it!

/be

Comment 9

[Brendan Eich [:brendan]]

New patch based on comments coming?

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 237395 [details] [diff] [review]
Using local defines

Ping?

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #8)
> Change let to uintN and use LET_DECL vs. 0 here (or not -- I'm cool with
> super-booleans where 2 is not a value and you can count on passing 0 or 1 to a
> function taking 0, 1, or 2) to avoid ?:.

This gets too wacky, IMO. The non-let case in EmitVariables, which is represented by 0 currently is represented by *2*, which would involve a bunch of changes to EmitVariables. IMO it's simpler to leave the ?: in this one case to help preserve sanity.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Sorry, last patch missed a file.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 255630 [details] [diff] [review]
Updated

Brendan, ping?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 255630 [details] [diff] [review]
Updated

Sorry for the delay -- too much travel lately.

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch to check in

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 18

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/regress/regress-350387.js,v  <--  regress-350387.js
initial revision: 1.1

Comment 19

[Bob Clary [:bc] (inactive)]

verified fixed linux, windows, mac* shell 20070406

Comment 20

[Bob Clary [:bc] (inactive)]

This passes on 1.9.0 but fails on 1.8.1 and 1.9.1 with x is not defined on line 51. The checkin in comment 17 occurred after the merge to hg on Mar 22.