Fix: make optional chaining expression work

I.e. code such as: ```js o = {a: true}; o?.['a'] ``` Note that the spec says that the expression on the right-hand side should _not_ be evaluated if the left-hand side object is null or undefined.
mozilla · Oct 7, 2024 · 28c7c84 · 28c7c84
1 parent 53e49ac
commit 28c7c84
Show file tree

Hide file tree

Showing 7 changed files with 138 additions and 7 deletions.
diff --git a/rhino/src/main/java/org/mozilla/javascript/CodeGenerator.java b/rhino/src/main/java/org/mozilla/javascript/CodeGenerator.java
@@ -742,6 +742,34 @@ private void visitExpression(Node node, int contextFlags) {
                 stackChange(-1);
                 break;
 
+            case Token.GETELEM_OPTIONAL:
+                visitExpression(child, 0);
+
+                // If object == null (shallow eq)
+                addIcode(Icode_DUP);
+                stackChange(1);
+                addToken(Token.NULL);
+                stackChange(1);
+                addToken(Token.EQ);
+                stackChange(-1);
+                int getExpr = iCodeTop;
+                addGotoOp(Token.IFNE);
+                stackChange(-1);
+
+                // Put undefined
+                addStringOp(Token.NAME, "undefined");
+                int after = iCodeTop;
+                addGotoOp(Token.GOTO);
+
+                // Else: get property key and do a GETELEM
+                resolveForwardGoto(getExpr);
+                child = child.getNext();
+                visitExpression(child, 0);
+                addToken(Token.GETELEM);
+                stackChange(-1);
+                resolveForwardGoto(after);
+                break;
+
             case Token.POS:
             case Token.NEG:
             case Token.NOT:

diff --git a/rhino/src/main/java/org/mozilla/javascript/IRFactory.java b/rhino/src/main/java/org/mozilla/javascript/IRFactory.java
@@ -163,6 +163,11 @@ private Node transform(AstNode node) {
             case Token.GETELEM:
                 return transformElementGet((ElementGet) node);
             case Token.QUESTION_DOT:
+                if (node instanceof ElementGet) {
+                    return transformElementGet((ElementGet) node);
+                } else {
+                    return transformPropertyGet((PropertyGet) node);
+                }
             case Token.GETPROP:
                 return transformPropertyGet((PropertyGet) node);
             case Token.HOOK:
@@ -521,7 +526,10 @@ private Node transformElementGet(ElementGet node) {
         // iff elem is string that can not be number
         Node target = transform(node.getTarget());
         Node element = transform(node.getElement());
-        return new Node(Token.GETELEM, target, element);
+        return new Node(
+                node.type == Token.QUESTION_DOT ? Token.GETELEM_OPTIONAL : Token.GETELEM,
+                target,
+                element);
     }
 
     private Node transformExprStmt(ExpressionStatement node) {

diff --git a/rhino/src/main/java/org/mozilla/javascript/Parser.java b/rhino/src/main/java/org/mozilla/javascript/Parser.java
@@ -3078,6 +3078,29 @@ private AstNode propertyAccess(int tt, AstNode pn) throws IOException {
                     ref = propertyName(-1, memberTypeFlags);
                     break;
                 }
+
+            case Token.LB:
+                {
+                    if (tt == Token.QUESTION_DOT) {
+                        consumeToken();
+                        ref = expr(true);
+                        mustMatchToken(Token.RB, "msg.no.bracket.index", true);
+
+                        ElementGet result = new ElementGet();
+                        int pos = pn.getPosition();
+                        result.setPosition(pos);
+                        result.setLength(getNodeEnd(ref) - pos);
+                        result.setLineno(pn.getLineno());
+                        result.setTarget(pn);
+                        result.setElement(ref);
+                        result.setType(Token.QUESTION_DOT);
+                        return result;
+                    } else {
+                        reportError("msg.no.name.after.dot");
+                        return makeErrorNode();
+                    }
+                }
+
             default:
                 if (compilerEnv.isReservedKeywordAsIdentifier()) {
                     // allow keywords as property names, e.g. ({if: 1})

diff --git a/rhino/src/main/java/org/mozilla/javascript/Token.java b/rhino/src/main/java/org/mozilla/javascript/Token.java
@@ -124,10 +124,11 @@ public static enum CommentType {
             BIGINT = REF_NS_NAME + 1, // ES2020 BigInt
             GETPROP_OPTIONAL = BIGINT + 1,
             REF_SPECIAL_OPTIONAL = GETPROP_OPTIONAL + 1,
-            CALL_OPTIONAL = REF_SPECIAL_OPTIONAL + 1;
+            CALL_OPTIONAL = REF_SPECIAL_OPTIONAL + 1,
+            GETELEM_OPTIONAL = CALL_OPTIONAL + 1;
 
     // End of interpreter bytecodes
-    public static final int LAST_BYTECODE_TOKEN = CALL_OPTIONAL,
+    public static final int LAST_BYTECODE_TOKEN = GETELEM_OPTIONAL,
             TRY = LAST_BYTECODE_TOKEN + 1,
             SEMI = TRY + 1, // semicolon
             LB = SEMI + 1, // left and right brackets
@@ -615,6 +616,8 @@ public static String typeToName(int token) {
                 return "REF_SPECIAL_OPTIONAL";
             case CALL_OPTIONAL:
                 return "CALL_OPTIONAL";
+            case GETELEM_OPTIONAL:
+                return "GETELEM_OPTIONAL";
             case TEMPLATE_LITERAL:
                 return "TEMPLATE_LITERAL";
             case TEMPLATE_CHARS:

diff --git a/rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java b/rhino/src/main/java/org/mozilla/javascript/optimizer/BodyCodegen.java
@@ -1370,6 +1370,50 @@ private void generateExpression(Node node, Node parent) {
                 }
                 break;
 
+            case Token.GETELEM_OPTIONAL:
+                {
+                    generateExpression(child, node); // object
+
+                    int getExpr = cfw.acquireLabel();
+                    int putUndefined = cfw.acquireLabel();
+                    int after = cfw.acquireLabel();
+
+                    // if (object == null) goto putUndefined
+                    cfw.add(ByteCode.DUP);
+                    ++maxStack;
+                    cfw.add(ByteCode.IFNULL, putUndefined);
+
+                    // if (!Undefined.isUndefined(object)) goto getExpr
+                    cfw.add(ByteCode.DUP); // No need to increase maxStack again!
+                    cfw.addInvoke(
+                            ByteCode.INVOKESTATIC,
+                            "org.mozilla.javascript.Undefined",
+                            "isUndefined",
+                            "(Ljava/lang/Object;)Z");
+                    cfw.add(ByteCode.IFEQ, getExpr);
+
+                    cfw.markLabel(putUndefined);
+                    cfw.add(ByteCode.POP); // Remove object
+                    cfw.add(
+                            ByteCode.GETSTATIC,
+                            "org/mozilla/javascript/Undefined",
+                            "instance",
+                            "Ljava/lang/Object;");
+                    cfw.add(ByteCode.GOTO, after);
+
+                    cfw.markLabel(getExpr);
+                    generateExpression(child.getNext(), node); // id
+                    cfw.addALoad(contextLocal);
+                    cfw.addALoad(variableObjectLocal);
+                    if (node.getIntProp(Node.ISNUMBER_PROP, -1) != -1) {
+                        addDynamicInvoke("PROP:GETINDEX", Signatures.PROP_GET_INDEX);
+                    } else {
+                        addDynamicInvoke("PROP:GETELEMENT", Signatures.PROP_GET_ELEMENT);
+                    }
+                    cfw.markLabel(after);
+                    break;
+                }
+
             case Token.GET_REF:
                 generateExpression(child, node); // reference
                 cfw.addALoad(contextLocal);

diff --git a/rhino/src/test/java/org/mozilla/javascript/tests/OptionalChainingOperatorTests.java b/rhino/src/test/java/org/mozilla/javascript/tests/OptionalChainingOperatorTests.java
@@ -126,4 +126,31 @@ public void testOptionalChainingOperator() {
                     return null;
                 });
     }
+
+    @Test
+    public void expressionsInOptionalChaining() {
+        Utils.assertWithAllOptimizationLevelsES6(true, "o = {a: true}; o?.['a']");
+        Utils.assertWithAllOptimizationLevelsES6(Undefined.instance, "o = null; o?.['a']");
+        Utils.assertWithAllOptimizationLevelsES6(Undefined.instance, "o = undefined; o?.['a']");
+    }
+
+    @Test
+    public void expressionsInOptionalChainingAreNotEvaluatedIfUnnecessary() {
+        Utils.assertWithAllOptimizationLevelsES6(1,
+                "c = 0;\n" +
+                        "function f() { ++c; return 0; }\n" +
+                        "o = {}\n" +
+                        "o?.[f()];\n" +
+                        "c\n");
+        Utils.assertWithAllOptimizationLevelsES6(0, 
+                "c = 0;\n" +
+                        "function f() { ++c; return 0; }\n" +
+                        "null?.[f()];\n" +
+                        "c\n");
+        Utils.assertWithAllOptimizationLevelsES6(0,
+                "c = 0;\n" +
+                        "function f() { ++c; return 0; }\n" +
+                        "undefined?.[f()];\n" +
+                        "c\n");
+    }
 }
diff --git a/tests/testsrc/test262.properties b/tests/testsrc/test262.properties
@@ -5893,14 +5893,13 @@ language/expressions/object 809/1169 (69.2%)
     yield-non-strict-access.js non-strict
     yield-non-strict-syntax.js non-strict
 
-language/expressions/optional-chaining 25/38 (65.79%)
+language/expressions/optional-chaining 23/38 (60.53%)
     call-expression.js
     early-errors-tail-position-null-optchain-template-string.js
     early-errors-tail-position-null-optchain-template-string-esi.js
     early-errors-tail-position-optchain-template-string.js
     early-errors-tail-position-optchain-template-string-esi.js
     eval-optional-call.js
-    iteration-statement-for.js
     iteration-statement-for-await-of.js {unsupported: [async]}
     iteration-statement-for-in.js
     iteration-statement-for-of-type-error.js
@@ -5913,9 +5912,8 @@ language/expressions/optional-chaining 25/38 (65.79%)
     optional-chain.js
     optional-chain-async-optional-chain-square-brackets.js {unsupported: [async]}
     optional-chain-async-square-brackets.js {unsupported: [async]}
-    optional-chain-expression-optional-expression.js
+    optional-chain-expression-optional-expression.js {strict: [-1], non-strict: [-1]}
     optional-chain-prod-arguments.js
-    optional-chain-prod-expression.js
     punctuator-decimal-lookahead.js
     short-circuiting.js
     super-property-optional-call.js