Closed Bug 355736 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
minor

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: igor)

References

Details

(Keywords: testcase)

Attachments

(1 file, 4 obsolete files)

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: .....^
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: ................^
Summary: Decompilation "[super] = q;" has quotes around "super" → Decompilation of "[super] = q;" has quotes around "super"
Same with the "goto" keyword/identifier:

js> function() { [goto] = a }   
function () {
    ["goto"] = a;
}
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.
OS: Mac OS X → All
Hardware: Macintosh → All
(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.
Assignee: general → igor
Duplicate of this bug: 353026
Attached patch Fix v1 (obsolete) — Splinter Review
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.
Attachment #258362 - Flags: review?(mrbkap)
Attachment #258362 - Flags: review?(Seno.Aiko)
Attachment #258362 - Flags: review?(mrbkap) → review+
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 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.
Attachment #258362 - Flags: review?(Seno.Aiko) → review+
(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.
> 

(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?
(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.
(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
Attached patch Fix v2 (obsolete) — Splinter Review
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.
Attachment #258362 - Attachment is obsolete: true
Attachment #258484 - Flags: review?(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
(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.
Attached patch Fiix v3 (obsolete) — Splinter Review
This is the previous patch without the optimization in jsinterp.c.
Attachment #258484 - Attachment is obsolete: true
Attachment #258512 - Flags: review?(brendan)
Attachment #258484 - Flags: review?(brendan)
Attached patch Fix v3 for real (obsolete) — Splinter Review
Attachment #258512 - Attachment is obsolete: true
Attachment #258513 - Flags: review?(brendan)
Attachment #258512 - Flags: review?(brendan)
Attachment #258513 - Attachment is patch: true
Attachment #258513 - Attachment mime type: text/x-patch → text/plain
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
Attachment #258513 - Flags: review?(brendan) → review+
Blocks: 350681
Attached patch Fix v4Splinter Review
Patch to commit that replaces as suggested 
  for (++chars; chars != end; ++chars) 
by simpler 
  while (++chars != end)
Attachment #258513 - Attachment is obsolete: true
Attachment #259369 - Flags: review+
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-355736.js,v  <--  regress-355736.js
initial revision: 1.1
Flags: in-testsuite+
verified fixed linux, windows, mac* shell 20070406
Status: RESOLVED → VERIFIED
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

(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.
You need to log in before you can comment on or make changes to this bug.