410571 - Yield-expression disappears in decompilation of object literal

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

$ ./js -v 170
js> (function() { var t = { x: 3, y: yield 4 }; })
function () {
    var t = {x: 3, y:  };
}

This doesn't compile because the "yield 4" after "y:" is missing.

Comment 1

[Brendan Eich [:brendan]]

Regression from patch for bug 376957 -- I should have noticed the dependency on rval that cannot survive the split Sprint.

/be

Comment 2

[Brendan Eich [:brendan]]

Waldo, can you take this one? I'll do a better job reviewing.

/be

Comment 3

[Jesse Ruderman]

Similar issue with |let|:

js> (function() { return { x: 1, y: let (z) z } })
function () {
    return {x: 1, y:  };
}

Comment 4

[Brendan Eich [:brendan]]

This bug is in the code that decompiles property initialisers, so it does not really depend on yield, let, etc. It depends on how strings popped from the decompiler's SprintStack get overwritten by result strings pushed in more than one Sprint call (no memory unsafety, just bad output).

/be

Comment 5

[Jeff Walden [:Waldo]]

Attachment: JS_strdup and free

This would be simpler (not necessary) if we just pushed a >=1-sized string in the place where the numerical index for array literals usually goes ("#" or "##" or similar protects the ", " from overwriting the start of |rval|), but I don't see a simple way to do that in a smaller-sized change without tweaking bytecode.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 295572 [details] [diff] [review]
JS_strdup and free

>                         const char *end = rval + strlen(rval);
> 
>                         if (*rval == '(')
>                             ++rval, --end;
>                         LOCAL_ASSERT(strncmp(rval, js_function_str, 8) == 0);
>                         LOCAL_ASSERT(rval[8] == ' ');
>                         rval += 8 + 1;
>                         LOCAL_ASSERT(*end ? *end == ')' : end[-1] == '}');
>                         if (Sprint(&ss->sprinter, "%s %s%s%.*s",
>                                    (lastop == JSOP_GETTER)
>                                    ? js_get_str : js_set_str,
>                                    xval,
>                                    (rval[0] != '(') ? " " : "",
>                                    end - rval, rval) < 0) {
>-                            return NULL;
>+                            goto initprop_error;

[snip]

>+              initprop_error:
>+                JS_free(cx, (char *) rval);

rval is not a pointer to a separate malloc block if the goto was taken.

Would prefer to go back to old code, avoid strdup/free, somehow just Sprint all at once. Too hard inline? Make a helper function to do it.

/be

Comment 7

[Brendan Eich [:brendan]]

Waldo: new patch coming?

/be

Comment 8

[Jeff Walden [:Waldo]]

Yes, "soon-ish".

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Combine codepaths a bit

1) Are you a fan of my code alignment in this patch?  I think I'm forging new ground, style-wise.  :-D
2) Are you a fan of non-toplevel-in-switch case statements?  We avoid a needless comparison this way, and we more or less have to do this to share the |isFirst| binding with the shared sprinting code.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Combine codepaths a bit

1) Are you a fan of my code alignment in this patch?  I think I'm forging new ground, style-wise.  :-D
2) Are you a fan of non-toplevel-in-switch case statements?  We avoid a needless comparison this way, and we more or less have to do this to share the |isFirst| binding with the shared sprinting code.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 301620 [details] [diff] [review]
Combine codepaths a bit

As a fan of Duff's Device I have no problem with putting case labels inside outer blocks, and in fact have done so in Decompile. But please don't overindent the labels or their code:

>               {
>                 JSBool isFirst;
> 
>+                case JSOP_INITELEM:

This should be indented to the same level as other cases.

>+                    /* Turn off most parens (all if there's only one initialiser). */

and this and subsequent lines should be four spaces to the left.

>+                    LOCAL_ASSERT(pc + len < endpc);
>+                    isFirst = (ss->opcodes[ss->top - 3] == JSOP_NEWINIT);
>+                    op = (isFirst &&
>+                          GetStr(ss, ss->top - 2)[0] == '0' &&
>+                          (JSOp) pc[len] == JSOP_ENDINIT)
>+                         ? JSOP_NOP
>+                         : JSOP_SETNAME;

The remaining isFirst uses all select "" or ", ", so you could common that into a const char *maybeComma block-local.

I mentally executed some of the control flow graph, then got tired. Do we have tests covering all paths? Did you flip the #ifdefs to test the old code? If it's hard to maintain we should consider getting rid of it instead of sort of kind of maintaining it.

r/a=me with nits and assuming good test coverage and results.

/be

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Fix whitespace (?), maybeComma, add set of tests

I couldn't find any problems in my testing, although it's a bit weird that that the rhs for an old-school getter (die!) can't be a let-expression.

Last question before committing and fixing: did I interpret the comments on indentation correctly?

Comment 13

[Jeff Walden [:Waldo]]

By the way, if old-school getters are to die (yes!  can we kill sharps too?  shaver will r+!), they shouldn't die in response to the non-existent difficulty of making this patch accommodate them.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 302292 [details] [diff] [review]
Fix whitespace (?), maybeComma, add set of tests

Given the indentation (which is what I wanted, thanks for asking), why not put JSOP_INITELEM's case down where it was? More minimal diff that way, if I'm squinting right.

Let's get rid of old-style getters for js1.9 (after mozilla1.9).

/be

Comment 15

[Jeff Walden [:Waldo]]

(In reply to comment #14)
> Given the indentation (which is what I wanted, thanks for asking), why not put
> JSOP_INITELEM's case down where it was? More minimal diff that way, if I'm
> squinting right.

Possibly; I don't think it helped for readability that way, tho.  It made it hard to compare INITELEM code vs. INITPROP, and it meant you had to scroll down, then back up to parse the shared part of INITELEM.

> Let's get rid of old-style getters for js1.9 (after mozilla1.9).

Whee!

In on trunk, fixed.

Comment 16

[Bob Clary [:bc] (inactive)]

Jeff, I get the following error when running the test for this bug in the browser.

Error: syntax error
Source File: http://test.mozilla.com/tests/mozilla.org/js/js1_7/decompilation/regress-410571.js
Line: 153, Column: 9
Source Code:
function () { 

do you need to wrap the source in '()' before calling eval?

Comment 17

[Jeff Walden [:Waldo]]

I don't think so; |function () { ... }| is a Program which is a Statement which is an ExpressionStatement.  For example:

javascript:eval("function%20()%20{\n}")

What's the contents of |dec| that are being eval'd there?

Comment 18

[Bob Clary [:bc] (inactive)]

dec ~ anonymous function... |function () { ... }| See bug 376052. I changed it locally to wrap in () before the eval and it passes in the browser.

Comment 19

[Brendan Eich [:brendan]]

Waldo: ES3 doesn't produce an anonymous function expression as a Program.

/be

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: patch to js1_7/decompilation/regress-410571.js

change to fail by compare rather than fail by uncaught exception and wrap source to be eval'd in '()'

Checking in regress-410571.js;
/cvsroot/mozilla/js/tests/js1_7/decompilation/regress-410571.js,v  <--  regress-410571.js
new revision: 1.2; previous revision: 1.1

Comment 21

[Bob Clary [:bc] (inactive)]

v