355736 - Decompilation of "[super] = q;" has quotes around "super"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> f = function() { [super] = q; }
function () {
    ["super"] = q;
}
js> eval("" + f)
typein:6: SyntaxError: invalid assignment left-hand side:
typein:6:     ["super"] = q;
typein:6: .....^

Comment 1

[Jesse Ruderman]

Also happens for an object literal with a getter:

js> f = function() { return { get super() { } } }
function () {
    return {get 'super'() {}};
}

js> eval("" + f)
typein:4: SyntaxError: missing : after property id:
typein:4:     return {get 'super'() {}};
typein:4: ................^

Comment 2

[Jesse Ruderman]

Same with the "goto" keyword/identifier:

js> function() { [goto] = a }   
function () {
    ["goto"] = a;
}

Comment 3

[Aiko]

Same with all words that are marked as reserved in jskeyword.tbl, really. Also 'let' and 'yield' in JS versions < 1.7.

Seems like js_IsIdentifier isn't up to the job any more because since bug 336376 we now have three kinds of strings:
1. some are never allowed as identifiers (starting with a number, for example)
2. some are allowed as property names
3. some are allowed as variable / argument names.
With its boolean return type js_IsIdentifer obviously can't be used to seperate these three cases.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> With its boolean return type js_IsIdentifer obviously can't be used to seperate
> these three cases.

The boolean return is ok, but an extra arg has to be passed to js_IsIdentifier describing the context.

Comment 6

[Igor Bukanov]

Attachment: Fix v1

It turned out that all users of js_IsIdentifier are from context when SpiderMonkey  allows to use keywords. This is a natural consequence of the fact the when the keywords are special, they would not end as names.

So the patch just removes the keyword check from js_IsIdentifier, adds comments to the prototype on that and moved JSOP_QNAMEPART case in jsinterp.c to share its code with JSOP_STRING to emphasis that now we got 4 different bytecodes with the same code just to make the decompiler happy.

Comment 7

[Brendan Eich [:brendan]]

Truly those could all become JSOP_CONST -- ancient history (at least in the case of JSOP_NUMBER and JSOP_STRING; JSOP_OBJECT is pretty old too).

Want to file a followup bug asking to combine these? There are potentially many srcnote codes available, since they are context-sensitive now (but not well-supported by the js_SrcNoteSpec array -- it should have bytecode/name pairs in vectors for each row, and it could minimize the number of "shapes" that constitute each row).

/be

Comment 8

[Aiko]

Comment on attachment 258362 [details] [diff] [review]
Fix v1

This also changes the result of Object.prototype.toSource, for example:

Without the patch
 js> ({'do':2}).toSource() 
 ({'do':2})
 
 With the patch
 js> ({'do':2}).toSource() 
 ({do:2})
 
 That could cause trouble if SpiderMonkey's output is fed to another engine which doesn't allow keywords as property names. But I guess this is a rather hypothetical problem, so r+.
 
 I'm just wondering why js_IsIdentifier is in jsfun.c? Looks really out of place there.

Comment 9

[Igor Bukanov]

(In reply to comment #8)
>  That could cause trouble if SpiderMonkey's output is fed to another engine
> which doesn't allow keywords as property names. But I guess this is a rather
> hypothetical problem, so r+.

Since MSIE does not support toSource, such compatibility issue in practice can only happen when one would feed a decompiled function, not the object itself. I suspect a probability of such feed is rather low so I suggest to go with this change.



> 
>  I'm just wondering why js_IsIdentifier is in jsfun.c? Looks really out of
> place there.
> 

Comment 10

[Igor Bukanov]

(In reply to comment #8)
>  I'm just wondering why js_IsIdentifier is in jsfun.c? Looks really out of
> place there.

Yep, perhaps it is time to move it to jsscan.h?

Comment 11

[Igor Bukanov]

(In reply to comment #7)
> Truly those could all become JSOP_CONST -- ancient history (at least in the
> case of JSOP_NUMBER and JSOP_STRING; JSOP_OBJECT is pretty old too).
> 
> Want to file a followup bug asking to combine these?


See bug 373772.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #8)
> >  I'm just wondering why js_IsIdentifier is in jsfun.c? Looks really out of
> > place there.
> 
> Yep, perhaps it is time to move it to jsscan.h?

Please do move to jsscan.[ch]. This began life in jsfun.c for Function, IIRC, but it's better off in the scanner.

/be

Comment 13

[Igor Bukanov]

Attachment: Fix v2

This is a previous patch with js_Identifier (slightly altered to remove no longer necessary variable s) moved to jsscan.[ch] and no longer used js_IsKeyword macro removed.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 258484 [details] [diff] [review]
Fix v2


>+#if JS_HAS_XML_SUPPORT
>+          BEGIN_CASE(JSOP_QNAMEPART)
>+#endif

JSOP_QNAMEPART is used even if !JS_HAS_XML_SUPPORT.

/be

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> (From update of attachment 258484 [details] [diff] [review])
> 
> >+#if JS_HAS_XML_SUPPORT
> >+          BEGIN_CASE(JSOP_QNAMEPART)
> >+#endif
> 
> JSOP_QNAMEPART is used even if !JS_HAS_XML_SUPPORT.

The patch does not change that, this is an exercise for bug 373772.

Comment 16

[Igor Bukanov]

Attachment: Fiix v3

This is the previous patch without the optimization in jsinterp.c.

Comment 17

[Igor Bukanov]

Attachment: Fix v3 for real

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 258513 [details] [diff] [review]
Fix v3 for real

>+JSBool
>+js_IsIdentifier(JSString *str)
>+{
>+    size_t length;
>+    jschar c, *chars, *end;
>+
>+    length = JSSTRING_LENGTH(str);
>+    if (length == 0)
>+        return JS_FALSE;
>+    chars = JSSTRING_CHARS(str);
>+    c = *chars;
>+    if (!JS_ISIDSTART(c))
>+        return JS_FALSE;
>+    end = chars + length;
>+    for (++chars; chars != end; ++chars) {

Nit: better written as a while (++cahrs != end) loop.

r=me with that.

/be

Comment 19

[Igor Bukanov]

Attachment: Fix v4

Patch to commit that replaces as suggested 
  for (++chars; chars != end; ++chars) 
by simpler 
  while (++chars != end)

Comment 20

[Igor Bukanov]

I committed the patch from comment 19 to the trunk:

Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.178; previous revision: 3.177
done
Checking in jsfun.h;
/cvsroot/mozilla/js/src/jsfun.h,v  <--  jsfun.h
new revision: 3.30; previous revision: 3.29
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.124; previous revision: 3.123
done
Checking in jsscan.h;
/cvsroot/mozilla/js/src/jsscan.h,v  <--  jsscan.h
new revision: 3.48; previous revision: 3.47
done

Comment 21

[Bob Clary [:bc] (inactive)]

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

Comment 22

[Bob Clary [:bc] (inactive)]

verified fixed linux, windows, mac* shell 20070406

Comment 23

[Bob Clary [:bc] (inactive)]

Bug 356083 changed the decompilation of test :2.
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-355736.js,v  <--  regress-355736.js
new revision: 1.2; previous revision: 1.1

Comment 24

[Sebastiaan Visser]

(In reply to comment #8) 
> (From update of attachment 258362 [details] [diff] [review])
> This also changes the result of Object.prototype.toSource, for example:
> 
> Without the patch
>  js> ({'do':2}).toSource() 
>  ({'do':2})
> 
>  With the patch
>  js> ({'do':2}).toSource() 
>  ({do:2})
> 
>  That could cause trouble if SpiderMonkey's output is fed to another engine
> which doesn't allow keywords as property names. But I guess this is a rather
> hypothetical problem, so r+. 
> 
> ... 
 
Somehow I managed to encounter this 'rather hypothetical' case. I am using SpiderMonkey to compile/convert a JS DSL into a 'flattened' version which sould also be readable by other browsers. For this purpose it is essential to quote object properties that 'seem' like keywords.

For our own usage I patched the Decompile function in jsopcode.c . In the case 'JSOP_INITPROP' I changed the check:
     ATOM_IS_IDENTIFIER(atom)
to
     !ATOM_IS_KEYWORD(atom) && ATOM_IS_IDENTIFIER
This causes keywords to get quoted when used as properties in object literals.

I also patched the obj_toSource function in jsobj.h with a similar check for keyword-like properties.

Do you guys think it will be a good idea to somehow merge these changes back into SpiderMonkey? Feeding toSource'ed code to other browsers can actual be quite useful.