New AST code throws codebug on certain forms of destructuring assignment



10 years ago
10 years ago


(Reporter: hannesw, Unassigned)




(2 attachments, 2 obsolete attachments)



10 years ago
Created attachment 376406 [details]
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.

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:

-                                    NAME obj 2
-                                    NAME foo 2
+                                    NAME obj
+                                    STRING foo
                                     NAME $1
                                 NUMBER 0.0

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

10 years ago
Created attachment 376408 [details] [diff] [review]
simplistic patch

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

Comment 2

10 years ago
Created attachment 376478 [details] [diff] [review]
new patch

Obviously, the string id should only be forced for SETPROP and not for SETELEM operations.
Attachment #376408 - Attachment is obsolete: true

Comment 3

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

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

10 years ago
Created attachment 381303 [details] [diff] [review]
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.
Attachment #376478 - Attachment is obsolete: true

Comment 5

10 years ago
Committed last patch.

Checking in src/org/mozilla/javascript/;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/,v  <--
new revision: 1.141; previous revision: 1.140
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.