Optional Chaining Operator ?.

[nabacg]

Optional chaining operator by transforming into conditional expression in IRFactory, similar #1591

Closes #937

[p-bakker]

Please uncomment ~language/expressions/optional-chaining in test262.priperties and then run the Test262TestSuite with the update flag

[nabacg]

Please uncomment ~language/expressions/optional-chaining in test262.priperties and then run the Test262TestSuite with the update flag

That's done and the results are part of the last commit. Sadly troubleshooting through those tests has exposed a problem with this approach to implementing this operator. The core idea was to replace optional chaining operator expression tree with a conditional expression like below

target?.property => (target == null || target == undefined) ? undefined : target.property

but this means that we're rewriting the tree into something like this

conditionalNode
    (target == null || target == undefined)
    undefined
    target.property

and crucially the actual ASTNode for target above is used 3 times in the same tree and it just doesn't work without cloning that expression to have a unique instance for each subtree. If the same node is shared between subtrees, we get all sort of interesting errors after IRFactory transforms this tree, usually due to how Node ctor overwrites next fields..

So, I don't think this works without cloning ASTNodes, which turns out is not that easy as one could expect. I ended up finding a hacky way to do this ( I don't even want to think about the performance hit.. 🙄 ) but I really hope someone knows a better way..

[p-bakker]

Thnx for trying!

Not an expert at all in this area, but maybe an approach like was taken in the exponential operator implementation could help

That implementation implements it's logic for interpreted and compiled mode in two different locations (see separate commits for each area).

What I'm suggesting is instead of rewriting the ast tree, maybe instead implement the explicit logic for dealing with this operator

again: no expert, so maybe a complete bogus suggestion 🤪

[nabacg]

Following your advice, I've added 3 new bytecodes to handle in the Interpreter / Compiler :

• GETPROP_OPTIONAL
• CALL_OPTIONAL
• REF_SPECIAL_OPTIONAL

To handle the optional property get ( target?.property ), optional method call ( targetMethod()?.property ) and also the ref property access ( target?.__proto__ ). That last one is I believe a rhino specific, so I'm not sure how big the target audience is for that last one.. but it seemed like good idea to support it too, for consistency sake if anything else.

[gbrail]

This one requires another rebase, and sadly it isn't easy either -- seemed straightforward but now tests are passing. Could you please take a crack at a "git rebase" here?

Even if you rebase, and then later on we have more conflicts -- since a bunch of people are adding language features at once -- we reduce the burden on future rebases every time we do it.

Thanks!

[nabacg]

I've merged latest master into it @gbrail, hope that will do it.

[gbrail]

Thanks -- one of the things that we have done in GitHub is to disable "merge commits," which keeps the history clear if we ever need to go back. That means that someone (often GitHub) needs to "rebase" the changes.

Locally, you can do that on your branch using "git rebase master," after which you can "git push -f" to update this branch with the new commit history. Git is basically taking all your commits and replaying them on the top of the tree.

The problem is that this branch is intertwined with a bunch of other changes, so if I try to do that myself, I have to go back through the history and understand what changed. Could you please give it a try? If it doesn't work then we'll take a look again later.

[p-bakker]

@gbrail maybe we ought to make that clear in the contributing section in the readme, that we don't accept merge commits?

[nabacg]

Thank you for a detailed info on how to rebase, I frankly just didn't realise there is a rule against merge commits in PRs. That's a perfectly fair ask though, no problem at all. Having said that it probably wouldn't hurt to make it explicit in the README.md.

Back to the topic of this PR - I've rebased everything on top of latest master, hopefully it's ready for a review now?

[p-bakker]

Wrt merge commits: could even fail the pipeline if these are detected

[nabacg]

Wrt merge commits: could even fail the pipeline if these are detected

100% ! Why make a human check it, if a computer can do it, right?

[gbrail]

Thanks for doing this -- I merged this manually (yet another conflict) and it's all merged now. Thanks!