336379 - Support destructuring assignment in JS1.7

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Destructuring assignment has been proposed for inclusion in ES4/JS2, based on array destructuring implemented in Opera, and instead of a group assignment proposal meant to address the same use-cases.  These cases include [key, value] pair return from iterators (see bug 326466), the proposed Date.nanotime() method which returns a pair of numbers, and permuting variables (e.g., [a, b] = [b, a]).

More details soon, I want to get some syntax nailed down before copying proposal content here.

/be

Comment 1

[Brendan Eich [:brendan]]

Destructuring assignment

The idea is to allow for simple destructuring of array and object data using a syntax that resembles array and object literals.

Proposed concrete syntax

   pattern ::= lhs
   lhs     ::= "{" (field ("," field)*)? "}"  |  "[" ((lhs | lvalue)? ",")* "]"
   field   ::= ident ":" (lhs | lvalue)
   lvalue  ::= <any lvalue expression allowed in a normal assignment expression> 

A <pattern> can only appear on the left-hand-side of “=” in the following contexts:

    *
      in a plain assignment expression
    *
      in a variable initializer in a var, let or const definition
    *
      in a variable initializer in a let expression or let statement
    *
      in a variable initializer in a for statement

If preceded by a var, let, or const the <pattern> must contain <lvalue>s that are all identifiers. If not, the compiler must throw a SyntaxError for the phrase.

Note that object literals cannot appear in statement positions, so a plain object destructuring assignment statement { x } = y must be parenthesized either as ({ x } = y) or ({ x }) = y.

Proposed semantics

The meaning is given separately for the four contexts in which destructuring may appear.

Assignments

The meaning of the assignment expression P “=” E where P is a pattern and E is an expression is:

   1. Evaluate E yielding a value V
   2. Assign V to a fresh temporary T
   3. If P is an array pattern then
      1. For each lvalue N at the top level of P, with its ordinal position I in P
         1. Perform N = T[I]
      2. For each pattern P’ at the top level of P, with its ordinal position I in P
         1. Destructure P’ and T[I] according to this algorithm (from step 2)
   4. If P is an object pattern then
      1. For each field F with name Q at the top level of P where the right-hand-side of the colon is an lvalue N
         1. Perform N = T.Q
      2. For each field F with name Q at the top level of P where the right-hand-side of the colon is a pattern P’
         1. Destructure P’ and T.Q according to this algorithm (from step 2)
   5. Return T

As can be seen, the destructuring assignment is simple syntactic sugar.

Variable definitions

The meaning of

   <defining-keyword> <pattern> = expr;

where the <defining-keyword> can be var, let, or const and all <lvalue>s in <pattern> are simple identifiers, the set of which is i1, i2, ..., is

   <defining-keyword> i1, i2, ...;
   <pattern> = expr

''let'' statements and ''let'' expressions

The meaning of

   let (b0, <pattern> = expr, b1, ...) { ... }

where all <lvalue>s in <pattern> are simple identifiers i1, i2, ..., is

   let (b0 ..., tmp = expr, i1, i2, ... b1, ...) { 
      <pattern> = tmp;
      ... 
   }

where tmp is a fresh temporary variable.

Similarly, the meaning of

   let (b0, <pattern> = expr, b1, ...) E

where all <lvalue>s in <pattern> are simple identifiers i1, i2, ..., is

   let (b0 ..., tmp = expr, i1, i2, ... b1, ...) ( <pattern> = tmp, E )

where tmp is a fresh temporary variable.

For statements

Below a <var-keyword> is var or let.

If a <var-keyword> is present in the for loops, then all the <lvalue>s in the <pattern> must be simple identifiers.

Plain ''for'' statement

The meaning of

   for ( <var-keyword>? b0, ..., <pattern> = expr, b1, ... ; e1 ; e2 ) { ... }

is that the b0 ... are evaluated, then expr is evaluated and destructured into the <lvalue>s of <lhs>, then the b1 .... are evaluated, and then the loop proceeds by normal rules.

For-in statements

The meaning of

   for ( <var-keyword>? <pattern> = expr in e0 ) { ... }

where <pattern> has the form “[” i1 “,” i2 “]” where both i1 and i2 may be omitted, is that expr is evalulated and destructured into i1 and i2 before the loop begins; then before each loop iteration i1 receives the property name and i2 receives the property value extracted from the object expression e0.

If <pattern> has any other form then the compiler must throw a SyntaxError.

For-each statements

The meaning of

   for each ( <var-keyword>? <pattern> = expr in e0 ) { ... }

where <lvalue>s in <pattern> are identifiers i1, i2, ... is that expr is evaluated and destructured into i1, i2, ... before the loop begins. Then before at each iteration the value extracted from the object e0 is destructured into the variables i1, i2 ....

Examples

Multiple-value returns:

   function f() { return [1,2] }
   var a, b;
   [a,b] = f();

Multiple-value returns, some values are not interesting:

   function f() { return [1,2,3] }
   var [a,,b] = f();

Going deeper into the array:

   [a,,[b,,[c]]] = f();

Object destructuring:

   var { op: a, lhs: b, rhs: c } = getASTNode()

Digging deeper into an object:

   var { op: a, lhs: { op: b }, rhs: c } = getASTNode()

Looping across an object:

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

Looping across values in an object:

   for each ( let { name: n, family: { father: f } } in obj )
     print("Name: " + n + ", Father: " + f);

[This is Lars T. Hansen of Opera's work, iterated with feedback from ECMA TG1 members.  Array destructuring is actually implemented in Opera.  It's my belief that this proposal will be adopted more or less as-is.  /be]

Comment 2

[Brendan Eich [:brendan]]

One point about which I'm seeking a change: the semantics proposed require two traversals of each level of destructuring initialiser, one to process lvalues, the second to recursively deal with nested destructuring initialisers.  Unless there is a good reason for this, a single left-to-right traversal (a DFS of the entire destructuring left-hand side, in other words) seems better.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: unoptimized implementation, _sans_ decompilation

Reviewers, start your engines.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: patch merged to JS_1_7_ALPHA_BRANCH

Comment 5

[Erik Arvidsson]

This may be too much off topic but what about pattern matching in general? It would be really nice to clean this up and allow patterns to be used in switch-case statements and mabe even in ifs and other places. I remember with fondness my early days using pattern matching in Haskell.

Comment 6

[Brendan Eich [:brendan]]

We have some thoughts on destructuring typecase (type switch), but this ain't no static lazy functional language here!  Seriously, destructuring is not matching in the OCaml or Haskell sense.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: updated patch against updated-to-trunk JS_1_7_ALPHA_BRANCH

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 223133 [details] [diff] [review]
updated patch against updated-to-trunk JS_1_7_ALPHA_BRANCH

I landed this on the JS_1_7_ALPHA_BRANCH to aid wider testing.

/be

Comment 9

[Eric Shepherd [:sheppy]]

I've noticed since last night's checkins that in the JS shell, you can no longer type:

js> function f() {
/* function body
}
js>

Now when you hit enter after the first line, you get a syntax error.  Is this intentional or a glitch introduced by these checkins?

Comment 10

[Eric Shepherd [:sheppy]]

Also, is it intentional that this works (it does, but I'm curious if it's on purpose):

[,,,] = f()

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #9)
> I've noticed since last night's checkins

which were to the JS_1_7_ALPHA_BRANCH, not the trunk -- just checking/noting.

> that in the JS shell, you can no
> longer type:
> 
> js> function f() {
> /* function body
> }
> js>
> 
> Now when you hit enter after the first line, you get a syntax error.  Is this
> intentional or a glitch introduced by these checkins?

Bug, fixed just now in jsparse.c 3.170.2.5.

(In reply to comment #10)
> Also, is it intentional that this works (it does, but I'm curious if it's on
> purpose):
> 
> [,,,] = f()

Allowed by the proposed Edition 4 spec grammar.  But the proposed spec disallows empty object initialisers.  We could add a semantic check to avoid complicating the BNF unduly.  I'll raise the issue.

/be

Comment 12

[Edward Kmett]

[]=[] segfaults

Comment 13

[Brendan Eich [:brendan]]

Empty initialisers must be prohibited, and are now -- update JS_1_7_ALPHA_BRANCH to get this fix, along with a fix to error reporting for non-lvalue expressions in the "value" positions of destructuring initialisers, and some group assignment optimizations.

/be

Comment 14

[nanto_vi (TOYAMA Nao)]

Will destructuring assignment be supported in an array comprehension and an arguments list?
e.g.
[key + " = " + value for ([key, value] in obj)];
[[1,2],[3,4],[5,6]].map(function ([a, b]) { return a + b; });

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> Will destructuring assignment be supported in an array comprehension and an
> arguments list?
> e.g.
> [key + " = " + value for ([key, value] in obj)];
> [[1,2],[3,4],[5,6]].map(function ([a, b]) { return a + b; });
> 

Yes, and also in catch block head -- but not immediately.  I want to do this for js1.7, but it won't make Firefox 2 beta 1.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: destructuring formal parameters

I would like to land this patch and then patch catch blocks to use block scope, and *then* patch destructuring catch variable binding.

/be

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 232635 [details] [diff] [review]
destructuring formal parameters

This patch looks bigger than it is due to renaming and some refactoring.  I thought I might minimize the patch size with a sed script to unrename, but it doesn't help that much -- the refactoring, often just movement of code, dwarfs the name change froth.

The BindVarArgs struct is now BindData, and local variables of that type or pointing to that type are now named "data", not "args", to avoid confusion.

BumpFormalCount, which checks for overflow and increments fun->nargs, is factored.

BindLocalVariable is factored too, for use from BindVariable and the new BindDestructuringArg.

The change to FunctionDef follows the pattern of previous JS_HAS_DESTRUCTURING changes: instead of matching a TOK_NAME only, either a TOK_LB, TOK_LC, or TOK_NAME is allowed.  The first two lead to destructuring "lvalue" parsing via PrimaryExpr, followed by CheckDestructuring and code specific to the destructuring form.  In this case, the formal destructuring parameters must be turned into a comma expression of, one operand per param, that gets the actual positional parameter and destructures it according to each lvalue in turn.  This is done by prepending a synthesized statement-expression to the function body.

The BIND_DATA_REPORT_ARGS macro is a bad, bad boy: it expands to two actual parameters to js_ReportCompileErrorNumber, so that when destructuring, the error's source coordinate comes from the already-parsed real lvalue JSParseNode * without the larger destructuring "lvalue"; but when parsing a simple variable binding form, the current token in ts is used.

To avoid confusion, all instances and pointers-to-instances of the FindPropertyValue parameter block type, FindPropValData, were renamed from "data" to "fpvd".

SetupLexicalBlock was renamed to PushLexicalScope and made more efficient by returning a JSParseNode *, as all parser functions in jsparse.c do.  Its unnecessary objp out param was eliminated.

The following #if JS_HAS_BLOCK_SCOPE was simplified:

>-            } else {
> #if JS_HAS_BLOCK_SCOPE
>-                if (tt == TOK_LET) {
>-                    (void) js_GetToken(cx, ts);
>-                    if (!SetupLexicalBlock(cx, ts, tc, &block, &pnlet, &obj))
>-                        return NULL;
>-                    pn1 = Variables(cx, ts, tc);
>-                } else
>+            } else if (tt == TOK_LET) {
>+                (void) js_GetToken(cx, ts);
>+                pnlet = PushLexicalScope(cx, ts, tc, &blockInfo);
>+                if (!pnlet)
>+                    return NULL;
>+                pn1 = Variables(cx, ts, tc);
> #endif
>-                    pn1 = Expr(cx, ts, tc);
>+            } else {
>+                pn1 = Expr(cx, ts, tc);
>             }

Catch and finally parsing was cleaned up to avoid peeking and then getting.

An overweight if/else (where the then clause is significantly larger than the else clause) was prettified by inverting if condition and transposing then and else clauses.

/be

Comment 18

[Igor Bukanov]

With the patch the following test case prints

function test([a, [b0, b1]],  {prop: c, prop2: [d]}, e)
{
	print("a="+a+" b0="+b0+" b1="+b1+" c="+c+" d="+d+" e="+e);
}

function test2([a, [b0, b1]],  {prop: c, prop2: [d]}, e)
{
	print(Array.prototype.toSource.call(arguments));
}

test([0, [1, 2]], {prop: 3, prop2: [4]}, 5);
test2([0, [1, 2]], {prop: 3, prop2: [4]}, 5);


a=0 b0=1 b1=2 c=3 d=4 e=5
[[0, [1, 2]], {prop:3, prop2:[4]}, 5]

Comment 19

[Igor Bukanov]

The patch does not detect duplicated parameter names and asserts later:

js> function f([a, a]) { print (a); }
js> f()
Assertion failure: atom, at jsopcode.c:2335
Trace/breakpoint trap

Note that duplicated names in let bindings currently are detected only partially:

js> { let [a, a] = [1, 2]; print(a); }
typein:10: TypeError: redeclaration of variable a:
typein:10: { let [a, a] = [1, 2]; print(a); }
typein:10: ..........^

while
js> let [c, c] = [1, 2]; print(c)      
2

Comment 20

[Igor Bukanov]

Note that the following does not detect duplicated names but at least it does not assert:
 
js> function f([a], [a]) { print(a); }
js> f([1], [2]) 
2

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #19)
> The patch does not detect duplicated parameter names

Duplicate parameter names are required to be supported by ECMA-262, for no good reason that I can recall (MS's rep wanted them).  They are a botch and a blight. We give a strict warning, in all cases including destructuring, but you need to use -sw with the shell, or otherwise set the prefs or options.strict, etc. properties (in a prior script tag).

> and asserts later:
> 
> js> function f([a, a]) { print (a); }
> js> f()
> Assertion failure: atom, at jsopcode.c:2335
> Trace/breakpoint trap

This is in the course of throwing a type error due to dereferencing undefined.  It's known that the decompiler is lagging, see bug 346642.

> Note that duplicated names in let bindings currently are detected only
> partially:
> 
> js> { let [a, a] = [1, 2]; print(a); }
> typein:10: TypeError: redeclaration of variable a:
> typein:10: { let [a, a] = [1, 2]; print(a); }
> typein:10: ..........^
> 
> while
> js> let [c, c] = [1, 2]; print(c)      
> 2

That's a known bug too, but probably not one on file yet (top-level let is turned into var).  Blake, can you file this and link it to the js1.7 tracking bug?  Thanks,

/be

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #20)
> Note that the following does not detect duplicated names but at least it does
> not assert:
> 
> js> function f([a], [a]) { print(a); }
> js> f([1], [2]) 
> 2

This works in the same way that function f(a, a) {print(a)} works per ECMA-262, all editions.  I don't intend to deviate without an explicit change for Edition 4, and may defer that at this rate (I'll poke the destructuring proposer in ECMA TG1 about this).

/be

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #21)
> That's a known bug too, but probably not one on file yet (top-level let is
> turned into var).

This is already filed as bug 346749.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 232635 [details] [diff] [review]
destructuring formal parameters

Igor, will you reconsider in light of separate decompiler bug?  This patch works for non-error cases, and I'm trying to avoid making a giant patch of all the work, risking conflicts and failing to land partial progress.

/be

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 232635 [details] [diff] [review]
destructuring formal parameters


>+static JSBool
>+BindDestructuringArg(JSContext *cx, BindData *data, JSAtom *atom,
>+                     JSTreeContext *tc)
>+{
>+    JSAtomListElement *ale;
>+    JSFunction *fun;
>+    JSObject *obj, *pobj;
>+    JSProperty *prop;
>+    JSScopeProperty *sprop;

This was set but never used, I had already eliminated it from my tree yesterday.

/be

Comment 26

[Brendan Eich [:brendan]]

Attachment: destructuring formal parameters, v2

With useless sprop local removed from BindDestructuringArg, and with a fix to FunctionDef to pass &funtc, not tc, to CheckDestructuring (this requires init'ing funtc early, but otherwise has the desired effect of capturing TCF_FUN_HEAVYWEIGHT in funtc.flags for cases such as function ([arguments, b]){print(arguments, b)}, where the arguments built-in local variable name is being overridden).

/be

Comment 27

[Igor Bukanov]

Comment on attachment 232803 [details] [diff] [review]
destructuring formal parameters, v2

Assuming that duplicated parameter name crash would be resolved in bug 346642.

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 232803 [details] [diff] [review]
destructuring formal parameters, v2

I landed this patch on the trunk.

/be

Comment 29

[Brendan Eich [:brendan]]

Attachment: rename vars, shrink JSStmtInfo, preparing way for catch block scope

Trying to make small patches here.

Next comes a bigger patch that will move catch block code generation out of line, into a TOK_CATCH case in js_EmitTree, so that TOK_LEXICALSCOPE can be composed to get lexical catch variable scope.

/be

Comment 30

[Brendan Eich [:brendan]]

Attachment: rename vars, shrink JSStmtInfo, preparing way for catch block scope

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 233196 [details] [diff] [review]
rename vars, shrink JSStmtInfo, preparing way for catch block scope

This patch is checked into the trunk now.

/be

Comment 32

[Brendan Eich [:brendan]]

Attachment: next step: change try-catch-finally AST

As documented in jsparse.[ch], the old tree structure singly-linked ternary catch nodes via pn_kid2, and pointed from the ternary TOK_TRY node to the first catch node via pn_kid2 as well; for try-finally, pn_kid2 was null.  The new AST uses the same no-catch case (pn_kid2 null), but if there are catch clauses, we burn a list node to hold them, for greater regularity (code sharing) and to allow O(1) access to the last catch clause.

This patch also renameds JSStmtInfo.label to JSStmtInfo.atom, and eliminates JSStmtInfo.blockObj by saving the atomized block object's atom in stmt->atom.

A drive-by style infraction cleanup on jspubtd.h tags along here.

A diff -w version of this patch is next, for easier reviewing.

/be

Comment 33

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 233293 [details] [diff] [review]
next step: change try-catch-finally AST

>Index: jsparse.c
>+                if (js_PeekToken(cx, ts) == TOK_IF) {
>+                    (void)js_GetToken(cx, ts); /* eat 'if' */

Nit: Replace this with js_MatchToken.

Comment 35

[Brendan Eich [:brendan]]

Attachment: with that old shaver-nit picked

I landed this on the trunk.

/be

Comment 36

[Brendan Eich [:brendan]]

Attachment: lexical scope for catch clauses

- Switch catch clauses to use lexical scope (JSOP_ENTER/LEAVEBLOCK) instead of
  new Object followed by JSOP_ENTER/LEAVEWITH.  JSOP_INITCATCHVAR is now a
  specialized version of JSOP_SETLOCAL.  SRC_CATCH changes incompatibly.  This
  patch does not include a XUL_FASTLOAD_FILE_VERSION change, but I will make
  that change before landing.

- LexicalLookup changed to use SCOPE_GET_PROPERTY, so is infallible, removing
  LL_NOT_FOUND as the static whose address was returned for no-such-variable.
  This change caused js_InCatchBlock, which was used only by the parser along
  with js_InWithStatement, to become specialized into js_IsGlobalReference,
  avoiding two or three walks up the statement stack.

- Fix latent bug in js_DefineCompileTimeConstant where the current frame/cg is
  skipped if a lexical binding is found -- this would make global constants
  "shine through" local let variables!  Fixed by returning true forthwith.

- Moved the JSOpLength enum (generated via #include "jsopcode.tbl") from
  jsinterp.c to jsopcode.c.

- Fixed JSOP_SETLOCAL not to have JOF_ASSIGNING or JOF_DETECTING -- these are
  obviously not needed given the lexical nature of all local ops.

- Changed XXX to FIXME: https://bugzilla.mozilla.org/show_bug.cgi?id=346749.

/be

Comment 37

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 233658 [details] [diff] [review]
lexical scope for catch clauses


> JSBool
>-js_InCatchBlock(JSTreeContext *tc, JSAtom *atom)
>+js_IsGlobalReference(JSTreeContext *tc, JSAtom *atom, JSBool *loopyp)
> {
>     JSStmtInfo *stmt;
>+    JSObject *obj;
>+    JSScope *scope;
> 
>+    *loopyp = JS_FALSE;
>     for (stmt = tc->topStmt; stmt; stmt = stmt->down) {
>-        if (stmt->type == STMT_CATCH && stmt->atom == atom)
>-            return JS_TRUE;
>+        if (stmt->type == STMT_WITH)
>+            return JS_FALSE;

Talk me through this: can't we have global references within with statements?

I'm working through the catch-related codegen changes in another window, but I
figured I'd start asking questions early.

Comment 38

[Brendan Eich [:brendan]]

(In reply to comment #37)
> Talk me through this: can't we have global references within with statements?

We can, of course -- this predicate is just answering whether a reference is known to be global (or else a reference error).  If with lies along the statement stack then we just don't know.

/be

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 233658 [details] [diff] [review]
lexical scope for catch clauses

Looks good.

Comment 40

[Brendan Eich [:brendan]]

Attachment: What I just checked in

Comment 41

[Brendan Eich [:brendan]]

Orange tinderboxen on second start after fastload file regeneration => latent bug in block object XDR code, exposed by this change (which results in many more block objects at compile time, one per catch clause, than before).  I'm backing out the js/src/* changes and debugging harder.  Sorry for the flames.

/be

Comment 42

[Brendan Eich [:brendan]]

Of course, I have to change the XUL_FASTLOAD_FILE_VERSION again, to avoid bad file bits pre-backout mixing with good code, post-backout.

/be

Comment 43

[Brendan Eich [:brendan]]

(In reply to comment #41)
> Orange tinderboxen on second start after fastload file regeneration => latent
> bug in block object XDR code, exposed by this change

Filed at bug 348696 and patched.

/be

Comment 44

[Brendan Eich [:brendan]]

Attachment: fix, take 2

This is the same patch that I landed, but with these changes:

- XUL_FASTLOAD_FILE_VERSION changed yet again
- Beefy assertion in JSOP_LEAVEBLOCK to catch any non-blocks on fp->blockChain.
- GUARDJUMPS renamed GUARDJUMP -- it's not a backpatch chain cursor, it's the index of a single jump needing targeting.

/be

Comment 45

[Brendan Eich [:brendan]]

Attachment: fix, take 3

Oops, didn't mean to revert the fix for bug 348685.

/be

Comment 46

[Brendan Eich [:brendan]]

Attachment: plain old diff of fix, take 3 against what i landed before the backout

For reviewing ease.

/be

Comment 47

[Brendan Eich [:brendan]]

I landed "fix, take 3" on the trunk.  All should be well now, since bug 348696 has already been fixed (just a little while ago).

/be

Comment 48

[Brendan Eich [:brendan]]

Argh, latent bug #2 bites:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x230658cd in js_XDRObject (xdr=0x1b746f00, objp=0xbfffdccc) at jsobj.c:4598
4598                clasp = OBJ_GET_CLASS(cx, proto);
(gdb) p cx.globalObject
$8 = (JSObject *) 0x18dc028
(gdb) p *(JSClass*)($.slots[2]-1)
$9 = {
  name = 0x2da7bc1c "nsXULPrototypeScript compilation scope", 
  flags = 9, 
  addProperty = 0x2300a09e <JS_PropertyStub>, 
  delProperty = 0x2300a09e <JS_PropertyStub>, 
  getProperty = 0x2300a09e <JS_PropertyStub>, 
  setProperty = 0x2300a09e <JS_PropertyStub>, 
  enumerate = 0x2300a0a8 <JS_EnumerateStub>, 
  resolve = 0x2d816208 <nsXULPDGlobalObject_resolve(JSContext*, JSObject*, long)>, 
  convert = 0x2300a0bc <JS_ConvertStub>, 
  finalize = 0x2d8184e0 <nsXULPDGlobalObject_finalize(JSContext*, JSObject*)>, 
  getObjectOps = 0, 
  checkAccess = 0, 
  call = 0, 
  construct = 0, 
  xdrObject = 0, 
  hasInstance = 0, 
  mark = 0, 
  reserveSlots = 0
}

This is bug 346144.  I'm going to back out yet again, and mark the dependency.

/be

Comment 49

[Brendan Eich [:brendan]]

(In reply to comment #48)
> Argh, latent bug #2 bites: ...
> This is bug 346144.

Wrong bug, I meant (or mean, now that we've diagnosed bug 346144 as INVALID) bug 348786.  Fixing that momentarily.

/be

Comment 50

[Brendan Eich [:brendan]]

(In reply to comment #49)
> Wrong bug, I meant (or mean, now that we've diagnosed bug 346144 as INVALID)
> bug 348786.  Fixing that momentarily.

With that bug fixed, the patch for this bug works for me on any number of restarts (which use the FastLoad file, deserializing block objects created for catch clause lexical scope).

/be

Comment 51

[Brendan Eich [:brendan]]

Attachment: fix, take 3a

What I'm about to land.  Same as last time except the jsint count = -1 in the TOK_TRY js_EmitTree case uses 0 as the initializer now, and the assertion that it's non-negative now asserts that it's positive.  Empty blocks should never be generated.

/be

Comment 52

[Brendan Eich [:brendan]]

Comment on attachment 233939 [details] [diff] [review]
fix, take 3a

This has been checked into the trunk (btw, in spite of the warning from bugzilla, it bugzilla-interdiffs against the "take 3" patch).

/be

Comment 53

[Brendan Eich [:brendan]]

Attachment: destructuring catch variables

After this, some much deferred decompiler love, and then rest and recovery for me.

/be

Comment 54

[Robert Sayre]

It's looking likely that this patch broke the feed parser. 

I used MOZ_CO_DATE to narrow the regression to between 15 Aug 2006 19:08 PDT and 15 Aug 2006 22:11 PDT.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-08-15+19%3A08%3A00&maxdate=2006-08-15+22%3A11%3A00&cvsroot=%2Fcvsroot

I don't know what exactly is broken. I will try to find a smaller test case than 1000 lines of SAX callbacks.

Comment 55

[Robert Sayre]

(In reply to comment #54)
> It's looking likely that this patch broke the feed parser. 

OK, patch 3a definitely did it. I checked with patch -R. I will reduce this.


Test results in XPCShell:

Before
tests:  156
ran:    156
passed: 156

After
tests:  156
ran:    156
passed: 58

Comment 56

[Robert Sayre]

Attachment: 1st reduction: exceptions from XPConnect object inside loops

Comment 57

[Robert Sayre]

Expected Output:
----------------------------------------

The Bag: [xpconnect wrapped nsIPropertyBag2 @ 0x525dd0 (native @ 0x5185a0)]

fields to obj
-- open outer
-- number1
-- foo,bar,baz
---- open inner
---- Exception: container.fields has no properties
---- prop: null
---- close inner
---- open inner
---- Exception: container.fields has no properties
---- prop: null
---- close inner
---- open inner
---- Exception: container.fields has no properties
---- prop: null
---- close inner
-- close outer
-- open outer
-- number2
-- qux,quux,quuux
---- open inner
---- Exception: container.fields has no properties
---- prop: null
---- close inner
---- open inner
---- Exception: container.fields has no properties
---- prop: null
---- close inner
---- open inner
---- Exception: container.fields has no properties
---- prop: null
---- close inner
-- close outer
end fields to obj






Output After Patch
----------------------------------------

The Bag: [xpconnect wrapped nsIPropertyBag2 @ 0x53a560 (native @ 0x539fb0)]

fields to obj
-- open outer
-- number1
-- foo,bar,baz
---- open inner
/Users/sayrer/bonecho/mozilla/fb-debug/dist/bin/loopcatch.js:22: TypeError: e has no properties

Comment 58

[Robert Sayre]

Attachment: 2nd pass: replace variables left over from my app

this one should be easier to follow

Comment 59

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Mightily reduced testcase

This testcase shows the essence of the problem.

Comment 60

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for the regressions

While we do know the slot at parse-time, we need to adjust it at compile-time to account for the block depth, just like BindNameToSlot via LexicalLookup does.

Comment 61

[Brendan Eich [:brendan]]

Attachment: comment and c.s.e.

Thanks to mrbkap, he says r=him.

/be

Comment 62

[Brendan Eich [:brendan]]

(In reply to comment #58)
> Created an attachment (id=234068) [edit]
> 2nd pass: replace variables left over from my app
> 
> this one should be easier to follow

Thanks, and sorry for the regression.

/be

Comment 63

[Brendan Eich [:brendan]]

Attachment: destructuring catch variables, updated to track trunk and fix pn_extra abusage

This also renames JSOP_INITCATCHVAR to JSOP_SETLOCALPOP (same functionality, stack arity, etc.).  It turns out we want popping setters for destructuring at least, and we would be faster and more ECMA-compliant in general if we had such opcodes.

/be

Comment 64

[Brendan Eich [:brendan]]

Attachment: destructuring catch variables, with one more tweak

Pre-existing code in LetBlock was not combining consequents under one if test, making a gratuitous else (gratuitous jumps).

/be

Comment 65

[Brendan Eich [:brendan]]

Attachment: destructuring catch variables, with one final tweak

What I'm checking in later tonight (have to run now).

/be

Comment 66

[Brendan Eich [:brendan]]

Attachment: destructuring catch variables, what I just checked in

Comment 67

[Brendan Eich [:brendan]]

Attachment: 1.8 branch patch of lexical/destructuring catch

This has been baking for too long.  It's required for js1.7, and needed as a prerequisite other approved patches.  Sorry I didn't pull it together till now.

/be

Comment 68

[Mike Schroepfer]

Comment on attachment 237431 [details] [diff] [review]
1.8 branch patch of lexical/destructuring catch

a=schrep for drivers.

Comment 69

[Brendan Eich [:brendan]]

Fixed by the final patches for bug 346642.

/be