Fix: make optional chaining expression work #1684
Conversation
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.
andreabergia
force-pushed
the
optional-chaining-expressions-2
branch
from
28c7c84 to
52a1ce2
Compare
|
Great! |
|
I guess you're saying that the previously merged optional chaining operator support didn't properly supported computed properties? Wrt to your 'note': think you filled #1600 for that already, no? |
It didn't support them at all - it didn't even parse the syntax 🙂
Yeah, but I have done something smaller in scope in this PR: I've just made this new implementation work correctly per the spec. I haven't fixed the general problem because, honestly, I can't imagine how to do it without having a big performance impact. |
| cfw.addALoad(contextLocal); | ||
| cfw.addALoad(variableObjectLocal); | ||
| if (node.getIntProp(Node.ISNUMBER_PROP, -1) != -1) { | ||
| addDynamicInvoke("PROP:GETINDEX", Signatures.PROP_GET_INDEX); |
There was a problem hiding this comment.
Awesome -- using the new instructions!
I do see from the coverage report that we don't get to this specific case, and given that we just changed it, I'd feel better if we did -- I have a suggestion below.
There was a problem hiding this comment.
I copy&pasted the code from the existing GETELEM path here:
case Token.GETELEM:
generateExpression(child, node); // object
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);
}
break;However, after trying to add a simple test case for this, I've actually realized that the original code I copied does not work either. I.e. if I do this:
Utils.assertWithAllOptimizationLevelsES6(true, "o = {[0]: true}; o[0]");then the code path for PROP:GETINDEX is not followed. So, this looks like a problem in #1645
rhino/src/test/java/org/mozilla/javascript/tests/OptionalChainingOperatorTests.java
Show resolved
Hide resolved
|
I see one place where I think that the code coverage could be improved. Can you please take a look? Otherwise this looks good! |
|
Going to mark this as draft after the problems I've summarized in #1688 |
|
Closed in favour of #1694 |
I.e. makes this work:
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.
Follow-up from #1593