New AST code throws codebug on certain forms of destructuring assignment

RESOLVED FIXED

Status

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: hannesw, Unassigned)

Tracking

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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.
        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.
(Reporter)

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
(Reporter)

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
(Reporter)

Comment 3

10 years ago
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.
(Reporter)

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
(Reporter)

Comment 5

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.