350730 - let in with causes "Assertion failure: pn2->pn_slot >= 0 || varOrConst" [@ EmitVariables]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

javascript:with({}) let y;

Assertion failure: pn2->pn_slot >= 0 || varOrConst,
at jsemit.c:3478 [@ EmitVariables]

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

A with statement will never be a scope statement -- if it is, then it will have a child STMT_BLOCK that will be converted into a scope.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 236092 [details] [diff] [review]
Fix

It's odd that for a statement type t, STMT_TYPE_IS_SCOPE(t) does not imply STMT_TYPE_MAYBE_SCOPE(t).  Would STMT_TYPE_HAS_SCOPE (STMT_HAS_SCOPE in parallel) be a better name for the former?

r=me for the fix.

/be

Comment 3

[Brendan Eich [:brendan]]

Or since STMT_IS_SCOPE is used only in js_PushStatement and js_PopStatement (and STMT_TYPE_IS_SCOPE is used only in STMT_IS_SCOPE for parallel factoring to STMT_TYPE_MAYBE_SCOPE/STMT_MAYBE_SCOPE), would STMT_(TYPE_|)LINKS_SCOPE be even better?

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better fix

This brute-force patch fixes the problems with the STMT_TYPE macros. I didn't change the names to make this a smaller patch.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 236184 [details] [diff] [review]
Better fix

>Index: jsemit.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsemit.c,v
>retrieving revision 3.190
>diff -p -U8 -r3.190 jsemit.c
>--- jsemit.c	29 Aug 2006 23:15:21 -0000	3.190
>+++ jsemit.c	31 Aug 2006 03:49:45 -0000
>@@ -236,18 +236,18 @@ const char js_with_statement_str[] = "wi
> const char js_finally_block_str[]  = "finally block";
> const char js_script_str[]         = "script";
> 
> static const char *statementName[] = {
>     "label statement",       /* LABEL */
>     "if statement",          /* IF */
>     "else statement",        /* ELSE */
>     "switch statement",      /* SWITCH */
>-    "block",                 /* BLOCK */
>     js_with_statement_str,   /* WITH */

You reverted the enum reordering, but not this table change -- that can't be right.

>+    "block",                 /* BLOCK */
> #define STMT_TYPE_MAYBE_SCOPE(type)                                           \
>-    STMT_TYPE_IN_RANGE(type, STMT_BLOCK, STMT_SUBROUTINE)
>+    (type != STMT_WITH &&                                                     \
>+     STMT_TYPE_IN_RANGE(type, STMT_BLOCK, STMT_SUBROUTINE))

Given the unchanged enum with block before with, this part seems ok -- but why not reorder and use the right ranges for MAYBE and IS?

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better still

This leaves an assertion with the testcase in a function:
js> function f() { with ({}) let k; }
Assertion failure: pn2->pn_slot >= 0 || !let, at jsemit.c:3524

Which I'll file a new bug on, because it's a symptom of a larger misbehavior:
js> function f() { with({k : ''}) var k = 3; print(k); }
js> f()
undefined

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Which I'll file a new bug on, because it's a symptom of a larger misbehavior:
> js> function f() { with({k : ''}) var k = 3; print(k); }
> js> f()
> undefined

That's correct behavior.  See ECMA 10.1.3 and 12.2.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Created an attachment (id=236354) [edit]
> Better still
> 
> This leaves an assertion with the testcase in a function:
> js> function f() { with ({}) let k; }
> Assertion failure: pn2->pn_slot >= 0 || !let, at jsemit.c:3524

We need BindNameToSlot when called for a name being declared to use the precise scope in which the declaration should bind.  That would be the body of f, here, and the assertion would be satisfied.

Your flag idea is looking good at this point ;-).

/be

Comment 9

[Brendan Eich [:brendan]]

Need action here this week, sooner the better -- or else.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Even better fix

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 237062 [details] [diff] [review]
Even better fix


>+js_LexicalLookup(JSTreeContext *tc, JSAtom *atom, jsint *slotp, JSBool letdecl)
> {
>     JSStmtInfo *stmt;
>     JSObject *obj;
>     JSScope *scope;
>     JSScopeProperty *sprop;
>     jsval v;
> 
>     for (stmt = tc->topScopeStmt; stmt; stmt = stmt->downScope) {
>-        if (stmt->type == STMT_WITH)
>-            break;
>+        if (stmt->type == STMT_WITH) {
>+            /* Ignore with statements in let declarations. */
>+            if (!letdecl)
>+                break;
>+            continue;
>+        }

It would be more natural, and match the comment, to write if (letdecl) continue; break instead.

Also, how about let or letting?  Just a thought.

>Index: jsinterp.c

Stray file in patch.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Even better-er fix

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 237068 [details] [diff] [review]
Even better-er fix


> JSStmtInfo *
>-js_LexicalLookup(JSTreeContext *tc, JSAtom *atom, jsint *slotp)
>+js_LexicalLookup(JSTreeContext *tc, JSAtom *atom, jsint *slotp, JSBool letdecl)
> {
>     JSStmtInfo *stmt;
>     JSObject *obj;
>     JSScope *scope;
>     JSScopeProperty *sprop;
>     jsval v;
> 
>     for (stmt = tc->topScopeStmt; stmt; stmt = stmt->downScope) {
>-        if (stmt->type == STMT_WITH)
>+        if (stmt->type == STMT_WITH) {
>+            /* Ignore with statements in let declarations. */
>+            if (letdecl)
>+                continue;
>             break;
>+        }

Nit: "Ignore with statements enclosing a single let declaration."

r=me with that.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 15

[Bob Clary [:bc] (inactive)]

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

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 237068 [details] [diff] [review]
Even better-er fix

This needs to land ASAP.

/be

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060909 windows/mac*/linux

Comment 18

[Mike Schroepfer]

Comment on attachment 237068 [details] [diff] [review]
Even better-er fix

a=schrep

Comment 19

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux