356106 - Assertion failure: rval[strlen(rval)-1] == '}'

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> function() { return ({x setter: function(){} | 5 }) }
Assertion failure: rval[strlen(rval)-1] == '}', at jsopcode.c:3875

Comment 1

[Brian Crowder]

What is the expected result?

Comment 2

[Brendan Eich [:brendan]]

The value of that setter is not a function, it's a string.  Debug to see what rval contains.

/be

Comment 3

[Brendan Eich [:brendan]]

So this is another case that must fall back on the old setter: syntax.  IOW, as with the first LOCAL_ASSERT, transform into an if that tests opposite of the asserted condition.

/be

Comment 4

[Brian Crowder]

Attachment: another old-school getter/setter decompilation case

I also cleaned up some logic that is unnecessary (pasted from above and added in my last patch in this area without noticing the redundancy).  I'll rip it out, if it's bad form to piggyback.

Comment 5

[Jesse Ruderman]

I don't think converting the assertion to an if creates the correct test.  Consider:

({x setter: function(){} | function(){} })

Comment 6

[Brian Crowder]

I get an invalid setter usage error for that case, which seems fine?

Comment 7

[Jesse Ruderman]

You'd get "invalid setter usage" for the stuff in comment 0 too if you try to evaluate the object naked.  That's why it's wearing a function.

Comment 8

[Brian Crowder]

This decompilation seems okay, though.  No meaning is lost and the code is still broken in the same way as it was before.

js> function() { return ({x setter: function(){} | function(){} }) }
function () {
    return {set x() {} | function () {}};
}

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> This decompilation seems okay, though.  No meaning is lost and the code
> is still broken in the same way as it was before.
> 
> js> function() { return ({x setter: function(){} | function(){} }) }
> function () {
>     return {set x() {} | function () {}};
> }

No, that's a syntax error whereas the old syntax is a runtime error. Different meaning.  See comment 3, use the old syntax here.

/be

Comment 10

[Brian Crowder]

So I should look for the first { of the function match/count braces to the end, and if there is anything after the last }, I should use old getter/setter output?

Comment 11

[Brendan Eich [:brendan]]

Talking to crowder on IRC, thinking for three seconds in a row about this (:-), I think we should make non-function-expression initialisers for old-style getter: and setter: property initialisers be syntax errors.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 241808 [details] [diff] [review]
another old-school getter/setter decompilation case

- for now in light of new thinking.  Hope we can get away with this incompatible change.  It's not just making a runtime error be a syntax error -- up till now you could write o = {p getter: f} where f was a function declared previously.

Not sure how to keep that working and make decompilation simple.

Also not sure we want this incompatibility in js1.7src/1.8.1.1 -- by default, we should *not* make so incompatible (albeit to non-standard API) a change.

/be

Comment 13

[Brian Crowder]

Is this really a critical bug?

Comment 14

[Brian Crowder]

I think this one is beyond my feeble knowledge of the parser, hoping someone else will pick up.

Comment 15

[Brian Crowder]

This bug has either morphed or been fixed:

js> function() { return ({x setter: function(){} | 5 }) }
function () {
    return {x setter: function () {} | 5};
}

Comment 16

[Jesse Ruderman]

What do you mean by "morphed"?  That looks fine to me.

This bug was probably fixed in one of the many other getter/setter decompilation bugs (bug 381101, bug 356083, etc).

Comment 17

[Brian Crowder]

Is the loss of parenthesis an issue here?

Comment 18

[Jesse Ruderman]

The parens around the entire object literal?  Those aren't needed because after "return", a "{" opens an object literal rather than a code block.

Comment 19

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-356106.js,v  <--  regress-356106.js
initial revision: 1.1