Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error with destructuring array assignment(?) #1187

Closed
ndeuma opened this issue Feb 22, 2022 · 3 comments · Fixed by #1529
Closed

Error with destructuring array assignment(?) #1187

ndeuma opened this issue Feb 22, 2022 · 3 comments · Fixed by #1529
Labels
bug Issues considered a bug Regression

Comments

@ndeuma
Copy link

ndeuma commented Feb 22, 2022

The following cannot be evaluated and leads to an exception with 1.7R3 and later
var a = [1, 2]; var b = 0; [a[b+1], a[b]] = [a[b], a[b+1]]; a[0]

Exception:

java.lang.IllegalStateException: FAILED ASSERTION: unexpected token: ADD
	at org.mozilla.javascript.Kit.codeBug(Kit.java:366)
	at org.mozilla.javascript.IRFactory.decompile(IRFactory.java:2498)
	at org.mozilla.javascript.IRFactory.decompileElementGet(IRFactory.java:2549)
	at org.mozilla.javascript.IRFactory.decompile(IRFactory.java:2492)
	at org.mozilla.javascript.IRFactory.decompileArrayLiteral(IRFactory.java:2509)
	at org.mozilla.javascript.IRFactory.decompile(IRFactory.java:2469)
	at org.mozilla.javascript.IRFactory.transformAssignment(IRFactory.java:441)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:222)
	at org.mozilla.javascript.IRFactory.transformExprStmt(IRFactory.java:559)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:219)
	at org.mozilla.javascript.IRFactory.transformScript(IRFactory.java:1133)
	at org.mozilla.javascript.IRFactory.transform(IRFactory.java:201)
	at org.mozilla.javascript.IRFactory.transformTree(IRFactory.java:121)
	at org.mozilla.javascript.Context.parse(Context.java:2484)
	at org.mozilla.javascript.Context.compileImpl(Context.java:2401)
	at org.mozilla.javascript.Context.compileString(Context.java:1369)
	at org.mozilla.javascript.Context.compileString(Context.java:1357)
	at org.mozilla.javascript.Context.evaluateString(Context.java:1135)
	at de.ndn.labs.RhinoBugTest.main(RhinoBugTest.java:16)

But it is evaluated with 1.7R2 (to "2"). I really don't understand why this works in a very old version and fails in all newer versions.

The only thing that works is removing the "+" in the array access:
var a = [1, 2]; var b = 0; var c = 1; [a[c], a[b]] = [a[b], a[c]]; a[0]

Minimal Java example here (and attached)
https://github.com/ndeuma/rhino-bugtest

rhino-bugtest.zip

@p-bakker p-bakker added the bug Issues considered a bug label Mar 21, 2022
@p-bakker
Copy link
Collaborator

I really don't understand why this works in a very old version and fails in all newer versions.

I assume changes were made to implement new features or fix bugs and that those had an unexpected side effect

@tuchida
Copy link
Contributor

tuchida commented Jul 16, 2024

ref. #1519, #1520
After applying these, I got the following error.

$ ./gradlew run -q --console=plain --args="-version 200"
Rhino 1.7.16-SNAPSHOT
js> var a = [1, 2]; var b = 0; [a[b+1], a[b]] = [a[b], a[b+1]]; a[0]
Exception in thread "main" java.lang.NullPointerException
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:967)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:1233)
	at org.mozilla.javascript.optimizer.BodyCodegen.visitSetElem(BodyCodegen.java:4143)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:1426)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:1114)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:1627)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:1114)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateExpression(BodyCodegen.java:1627)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateStatement(BodyCodegen.java:860)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateStatement(BodyCodegen.java:666)
	at org.mozilla.javascript.optimizer.BodyCodegen.generateBodyCode(BodyCodegen.java:61)
	at org.mozilla.javascript.optimizer.Codegen.generateCode(Codegen.java:289)
	at org.mozilla.javascript.optimizer.Codegen.compileToClassFile(Codegen.java:175)
	at org.mozilla.javascript.optimizer.Codegen.compile(Codegen.java:95)
	at org.mozilla.javascript.Context.compileImpl(Context.java:2457)
	at org.mozilla.javascript.Context.compileString(Context.java:1405)
	at org.mozilla.javascript.Context.compileString(Context.java:1393)
	at org.mozilla.javascript.tools.shell.Main.processSource(Main.java:480)
	at org.mozilla.javascript.tools.shell.Main.processFiles(Main.java:180)
	at org.mozilla.javascript.tools.shell.Main$IProxy.run(Main.java:98)
	at org.mozilla.javascript.Context.call(Context.java:549)
	at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:475)
	at org.mozilla.javascript.tools.shell.Main.exec(Main.java:164)
	at org.mozilla.javascript.tools.shell.Main.main(Main.java:142)

@tuchida
Copy link
Contributor

tuchida commented Jul 17, 2024

This code will reproduce it.

// ElementGet.getElement() is not transformable.
var a = []; var b = 0; [a[b+1]] = [123];

// ElementGet.getTarget() is not transformable.
var a = []; [(NaN, a)[0]] = [123];

// PropertyGet.getTarget() is not transformable.
var a = {}; [(NaN, a).b] = [123];

This will fix it.

diff --git a/rhino/src/main/java/org/mozilla/javascript/IRFactory.java b/rhino/src/main/java/org/mozilla/javascript/IRFactory.java
index 723f7d1..4841f3f 100644
--- a/rhino/src/main/java/org/mozilla/javascript/IRFactory.java
+++ b/rhino/src/main/java/org/mozilla/javascript/IRFactory.java
@@ -102,6 +102,7 @@ public final class IRFactory {
         parser = new Parser(env, errorReporter);
         astNodePos = new AstNodePosition(sourceString);
         parser.currentPos = astNodePos;
+        parser.irFactory = this;
     }
 
     /** Transforms the tree into a lower-level IR suitable for codegen. */
@@ -129,7 +130,7 @@ public final class IRFactory {
     // functions into the AstNode subclasses.  OTOH that would make
     // IR transformation part of the public AST API - desirable?
     // Another possibility:  create AstTransformer interface and adapter.
-    private Node transform(AstNode node) {
+    Node transform(AstNode node) {
         switch (node.getType()) {
             case Token.ARRAYCOMP:
                 return transformArrayComp((ArrayComprehension) node);
diff --git a/rhino/src/main/java/org/mozilla/javascript/Parser.java b/rhino/src/main/java/org/mozilla/javascript/Parser.java
index 754dc43..e616f85 100644
--- a/rhino/src/main/java/org/mozilla/javascript/Parser.java
+++ b/rhino/src/main/java/org/mozilla/javascript/Parser.java
@@ -157,6 +157,8 @@ public class Parser {
 
     private boolean defaultUseStrictDirective;
 
+    IRFactory irFactory;
+
     // Exception to unwind
     public static class ParserException extends RuntimeException {
         private static final long serialVersionUID = 5882582646773765630L;
@@ -4236,11 +4238,11 @@ public class Parser {
                     // override getFirstChild/getLastChild and return the appropriate
                     // field, but that seems just as ugly as this casting.
                     if (left instanceof PropertyGet) {
-                        obj = ((PropertyGet) left).getTarget();
+                        obj = irFactory.transform(((PropertyGet) left).getTarget());
                         id = ((PropertyGet) left).getProperty();
                     } else if (left instanceof ElementGet) {
-                        obj = ((ElementGet) left).getTarget();
-                        id = ((ElementGet) left).getElement();
+                        obj = irFactory.transform(((ElementGet) left).getTarget());
+                        id = irFactory.transform(((ElementGet) left).getElement());
                     } else {
                         // This branch is called during IRFactory transform pass.
                         obj = left.getFirstChild();

However, this is too tightly coupled to the Parser and IRFactory. In order to make them loosely coupled, I will think about it some more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues considered a bug Regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants