357754 - Top-level closures don't see let-bound variables

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Brendan pointed this out to me on IRC:

js> function f() { let k = 3; function g() { print(k); } g() }
js> f()
typein:3: ReferenceError: k is not defined

But note that other forms of closures work:
js> function f() { let k = 3; if (1) function g() { print(k); } g() }
js> f()
3

And:
js> function f() { let k = 3; (function() { print(k); })() }
js> f()
3

The problem is that for top-level closures, we don't capture the scope chain (and, in fact, we emit a JSOP_NOP opcode where the closure is defined) this means that we can't find let-declared variables that are captured in the inner function.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

This was easy enough, once I got over the change from prolog to main for JSOP_DEFLOCALFUN.

A latent ugliness: when emitting code, there are two statement info records for the body block: one from the TOK_LEXICALSCOPE node, and one from its TOK_LC kid (pn_expr).  The division of labor follows the AST here, which is clean.  It is only slightly unclean to have two records stacked for one braced body.  I do not want to "fix" this now, or ever.

/be

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 243366 [details] [diff] [review]
fix

- in jsemit.c
>+                JS_ASSERT(stmt->type == STMT_BLOCK &&
>+                          stmt->down &&
>+                          stmt->down->type == STMT_BLOCK &&
>+                          (stmt->down->flags & SIF_SCOPE) &&
>+                          !stmt->down->down);

This assertion is only correct when there's a top-level let declaration. The example |function f() { function g() { } }|botches the assertion because there's only one stmt on the stmt stack.

But there's a further problem, I think: we have to have the prolog bytecode here otherwise, I get things like (with the assertion removed):

js> function f() { g(); function g() { print("hello, world"); } }
js> f()
typein:1: TypeError: g is not a function

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> The
> example |function f() { function g() { } }|botches the assertion because
> there's only one stmt on the stmt stack.

That's easy to fix, thanks.

> But there's a further problem, I think: we have to have the prolog bytecode
> here otherwise, I get things like (with the assertion removed):
> 
> js> function f() { g(); function g() { print("hello, world"); } }
> js> f()
> typein:1: TypeError: g is not a function

It's worse: the prolog can't capture the scope chain.  We need an op after the JSOP_ENTERBLOCK that starts the main bytecode of the script, but not after any other opcode.

/be

Comment 4

[Brendan Eich [:brendan]]

I'm concerned about any reordering of prolog opcodes into the main, but I can't think of a way that reordering would violate ECMA-262 10.1.3 right at the moment.

Say we can reorder to move just the JSOP_DEFLOCALFUN ops into the main bytecode.  Then I see three ways to fix this bug:

1.  Emit the JSOP_DEFLOCALFUN ops right after the JSOP_ENTERBLOCK at the start of the script's main bytecode, with jsinterp.c's JSOP_DEFLOCALFUN case patched as in  attachment 243366 [details] [diff] [review].

2.  Flag the outer function somehow, leaving JSOP_DEFLOCALFUN emitted into the script's prolog, but hacking the interpreter case for that op to check the outer function flag and, if set, get the first block (first atomMap entry) and clone it into the scope chain.

3.  Optimize outer function activation block local access, either by extending JSOP_GETLOCAL, etc., to take a frame number as well as frame-local stack index, or adding JSOP_GETOUTERLOCAL or whatever.

Thinking more, comments welcome.

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> 2.  Flag the outer function somehow, leaving JSOP_DEFLOCALFUN emitted into the
> script's prolog, but hacking the interpreter case for that op to check the
> outer function flag and, if set, get the first block (first atomMap entry) and
> clone it into the scope chain.

This might be the minimal patch.

> 3.  Optimize outer function activation block local access, either by extending
> JSOP_GETLOCAL, etc., to take a frame number as well as frame-local stack index,
> or adding JSOP_GETOUTERLOCAL or whatever.

The "frame" being indexed would not be a JSStackFrame, of course, but a scope chain element (AKA a binding rib).  Option 3 does not relieve the interpreter from cloning a lexical block into the runtime scope chain, therefore.  Which means that option 3 is the same as option 1 or 2, but with further optimization to the way an outer function block-local is found.

So we're back to option 1 vs. 2, and 2 looks simpler, if less general, than 1.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I was thinking that we might be able to emit a prolog JSOP_DEFLOCALFUN *and* a main JSOP_DEFLOCALFUN. In the common case, the version running in the main bytecode would be a noop, but in this case, it would grab the scope chain, and therefore see the let. Is that too wasteful?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> I was thinking that we might be able to emit a prolog JSOP_DEFLOCALFUN *and* a
> main JSOP_DEFLOCALFUN. In the common case, the version running in the main
> bytecode would be a noop, but in this case, it would grab the scope chain, and
> therefore see the let. Is that too wasteful?

It's not sufficient:

function outer() {
  let x = 42;
  print(inner());  // should print 42
  function inner() {
    return x;
  }
}

Unless the main bytecode comes right after the JSOP_ENTERBLOCK (option 1), the scope chain will not include the let-induced block with local x.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: better fix (option 2)

I also cleaned up the pre-existing duplication of JSFUN_HEAVYWEIGHT setting code, consolidating in js_NewScriptFromCG.

/be

Comment 9

[Brendan Eich [:brendan]]

Fixed on trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.126; previous revision: 3.125
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.225; previous revision: 3.224
done
Checking in jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.70; previous revision: 3.69
done
Checking in jsfun.h;
/cvsroot/mozilla/js/src/jsfun.h,v  <--  jsfun.h
new revision: 3.28; previous revision: 3.27
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.301; previous revision: 3.300
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.260; previous revision: 3.259
done
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.121; previous revision: 3.120
done

/be

Comment 10

[Bob Clary [:bc] (inactive)]

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

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061028 windows/linux

Comment 12

[Brendan Eich [:brendan]]

Attachment: 1.8 branch fix, includes fix for bug 358508

Comment 13

[Brendan Eich [:brendan]]

This is a pretty important fix for block-local ("let") scope.  The patch affects only the *_BLOCKLOCALFUN-flagged special cases visible in the new code, plus the cleanup to propagate *_HEAVYWEIGHT in the same way.  I'd like to get this into 1.8.1.1 as well as js1.7src, so we can have a law-of-zeros-correct JS1.7 for 2.0.0.1.

/be

Comment 14

[Jay Patel [:jay]]

Comment on attachment 243963 [details] [diff] [review]
1.8 branch fix, includes fix for bug 358508

Approved for 1.8.1 branch, a=jay for drivers.  Please land asap.  Thanks!

Comment 15

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.93.2.12; previous revision: 3.93.2.11
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.59; previous revision: 3.128.2.58
done
Checking in jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.38.20.19; previous revision: 3.38.20.18
done
Checking in jsfun.h;
/cvsroot/mozilla/js/src/jsfun.h,v  <--  jsfun.h
new revision: 3.22.20.3; previous revision: 3.22.20.2
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.73; previous revision: 3.181.2.72
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.142.2.69; previous revision: 3.142.2.68
done
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.79.2.16; previous revision: 3.79.2.15
done

/be

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 20061122 1.8.1.1 windows/linux/mac*, 1.9 windows/linux