492036 - New AST code throws codebug on certain forms of destructuring assignment

Status: RESOLVED FIXED

(Rhino Graveyard :: Compiler, defect)

Description

[Hannes Wallnoefer]

Attachment: simple test script

From IRFactory.transformAssignment: (line 397)

        // TODO(stevey)
        // Bug: for code like "var obj={p:3};[obj.p]=[9];", "left" will
        // be ARRAYLITERAL with an embedded GETPROP. This causes errors
        // at codegen.
        Kit.codeBug();

Looking at dumps of the parse tree, I noticed that the difference between rhino 1.7R2 and cvs head is middle child node of the SETPROP is of type STRING in 1.7R2 and of type NAME in HEAD:

                         WITH
                             COMMA
                                 SETPROP
-                                    NAME obj 2
-                                    NAME foo 2
+                                    NAME obj
+                                    STRING foo
                                     NAME $1
                                 NUMBER 0.0
                         LEAVEWITH

So I came up with this really simple patch that sets the node type where it is created in Parser.simpleAssignment(). It seems to work, but it's a simple hack of course, so somebody who knows the parser/classgen should definitely look into this for a real solution.

Comment 1

[Hannes Wallnoefer]

Attachment: simplistic patch

set the node type of second SETPROP child to STRING to mirror pre-AST-refactoring output

Comment 2

[Hannes Wallnoefer]

Attachment: new patch

Obviously, the string id should only be forced for SETPROP and not for SETELEM operations.

Comment 3

[Hannes Wallnoefer]

I noted that Norris recently removed the comment and Kit.codeBug():

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=mozilla%2Fjs%2Frhino%2F&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2009-05-27+05%3A23&maxdate=2009-05-27+05%3A25&cvsroot=%2Fcvsroot

Unfortunately the bug is still there, it just doesn't show in the Rhino shell because this is a codegen-only bug and the shell runs in interpreter mode, which is probably why Norris assumed the bug had been fixed. 

If nobody else comes up with a better solution, I suggest I'll commit my workaround patch with a big fat TODO comment including some explanation and a link to this bug.

Comment 4

[Hannes Wallnoefer]

Attachment: patch with workaround and TODO comment with link to this bug

Patch with the workaround that sets the GETPROP id type to STRING and a revised TODO comment with link to this bug.

Comment 5

[Hannes Wallnoefer]

Committed last patch.

Checking in src/org/mozilla/javascript/Parser.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/Parser.java,v  <--  Parser.java
new revision: 1.141; previous revision: 1.140
done