Closed Bug 231518 Opened 21 years ago Closed 21 years ago

Decompiler must quote keywords and non-identifier property names


(Core :: JavaScript Engine, defect)

Not set





(Reporter: brendan, Assigned: brendan)





(4 files)

Adrian Klein and Martin Honnen have reported several cases where the decompiler
does not quote reserved identifiers used to name properties.  The first case was

    function do_something() {
      var obj = new Object();

      obj['name']      = 'Just a name';
      obj['if']        = false;
      obj['some text'] = 'correct';

which decompiles with obj['if'] rendered as the illegal obj.if.  Martin Honnen's
followup pointed out a case with object literals:

    function f() { return {'if': "then"}; }

I already fixed the first case, but Martin's post turned over a virtual rock,
under which crawled a number of bugs, including footprint bloat in Decompile
dating from the introduction of ECMA Edition 3 Unicode identifiers.

Patch soon.

Attached patch proposed fixSplinter Review
- Save footprint by eliminating old (pre-ECMA-Edition-3) IsASCIIIDentifier
  static helper and its calls.	This was used to decompile o.p\uBC19rop as
  o['p\uBC19rop'] before the Edition 3 support for Unicode identifiers went in.

  But when Unicode id support was done, IsASCIIIdentifier was not removed.

  Now, we need only test ATOM_KEYWORD in order to decide whether bytecode such
  as JSOP_GETPROP<obj, 'if'>, which comes from an input expression of the form
  obj["if"], should be decompiled as obj['if'].  If the property identifier is
  not a reserved id, we can decompile using dot notation -- provided we call
  QuoteString(..., 0) to escape any non-ASCII characters in the id.

  The Decompile cases had redundant QuoteString(..., 0) calls before
  goto do_for*in* jumps, which could be commoned at the jump target(s).  So I
  consolidated all such calls.	See the code that goes to do_fornameinloop, and

  code below that label that depends on lval, atom, xval, and rval being set
  correctly for the current JSOP_FOR* bytecode.

- Share a bunch of similar Sprint calls using a |const char *fmt| local.  Also
  fused the JSOP_SETPROP += vs. = (SRC_ASSIGNOP vs. no such note) cases via %s=

  and a conditionally empty assignment-operator-string Sprint argument.

- Fix JSOP_ENUMELEM (again), it was not conspiring with JSOP_FORELEM correctly
  at all.  It ends with a goto do_forinbody, which code depends on the |tail|
  local variable, but |tail| was not set by JSOP_ENUMELEM -- instead, it was
  hacked in JSOP_FORELEM, with a random 1 subtracted from its value reckoned
  from the ifeq at the top of the (for example) |for (o[p] in q);| loop.  But
  the bytecodes needed to execute o[p] come between the forelem;ifeq and the
  enumelem ops, and these indexing expression bytecodes have arbitrary length
  in general, so there's no way JSOP_FORELEM can compute |tail|.

  The fix is to propagate the tail (loop-closing goto) bytecode pointer via a
  forelem_tail local, analogous to forelem_done, from JSOP_FORELEM's case in
  Decompile to JSOP_ENUMELM's case.  The latter case can then subtract the
  current pc to compute the prper |tail| value, before the goto do_forinbody.

- Clean up some unnecessarily nested assignments in conditions, etc.

Comment on attachment 139454 [details] [diff] [review]
proposed fix

Passes the JS testsuite.

Attachment #139454 - Flags: review?(shaver)
One tiny change, here's the diff-diff (old patch on left, new on right):

< +++ jsopcode.c        20 Jan 2004 00:06:46 -0000
> +++ jsopcode.c        22 Jan 2004 23:48:02 -0000
< +                    js_printf(jp, "%s%s", *lval ? "." : "", xval);
> +                    js_printf(jp, *lval ? ".%s" : "%s", xval);

To illuminate this further, the code here is part of the do_forinloop jazz that
wants to handle for (x in o), for (x.y in o), and for (x[z] in o), where y may
be a keyword (in which case we want for (x['y'] in o) as output).

So lval, atom, xval, and rval are all set, and if (atom), we have for (x in o)
or for (x.y in o) where y is known not to be a keyword, and lval == "" tells
whethe we have for (x in o) or for (x.y in o); else if (xval), we have the
indexed case: for (x[z] in o).

The above tweak just reduces by one the somewhat carefree %s format mania in the

Hope this is all clear.

Comment on attachment 139454 [details] [diff] [review]
proposed fix

We clearly need some of these bits to be added to the test suite.  And for my
birthday, I would like a copy of O'Reilly's "Mastering the Spidermonkey
Decompiler", thanks.

The duplication of the 

ATOM_KEYWORD ? (quote, o[p]) : (0, o.p);

logic makes me wonder if we should have a macro to unify, but it's not a

Attachment #139454 - Flags: review?(shaver) → review+
Attached patch like this?Splinter Review
I'll check this in after a handshake with shaver, here or in irc.


Closed: 21 years ago
Resolution: --- → FIXED
I'd like to get this in straight away.

Attachment #140223 - Flags: review?(shaver)
Generalizing bug as I should have done originally.

Resolution: FIXED → ---
Summary: Decompiler must quote keywords → Decompiler must quote keywords and non-identifier property names
Followup fix is in.

Closed: 21 years ago21 years ago
Resolution: --- → FIXED
*** Bug 232736 has been marked as a duplicate of this bug. ***
Does your fix also catch this variation?

js> a = new RegExp;
js> b=eval(uneval(a));
js> b.toSource();
3: TypeError: b has no properties
In short, "no":
js> build()
built on Jan 25 2004 at 09:14:22
js> a = new RegExp;
js>  b=eval(uneval(a));
js>  b.toSource();

That was fixed by bug 225550 a while ago
Re: comment 12: Please don't spam closed bugs with clearly unrelated issues.

I'll write to Adrian and Martin.
js1_5/Regress/regress-231518.js checked in.
Flags: testcase+
verified fixed.
You need to log in before you can comment on or make changes to this bug.