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

Implement optional chaining for function call #1702

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

andreabergia
Copy link
Contributor

@andreabergia andreabergia commented Oct 17, 2024

PR stacked on top of #1694

It implements the optional chaining operator for function call, i.e. f?.() and similar.

There is a lot of complexity in this PR caused by the fact that rhino has many different ways of calling a function:

  • f() is a NAME_AND_THIS followed by a CALL
  • a.b() is a PROP_AND_THIS followed by a CALL
  • a[b]() is an ELEM_AND_THIS followed by a CALL
  • there are even other cases, such as a.__parent__() or eval.

The approach I've used is similar to the one used for optional property access, i.e. for f?.(x) it will do something like:

lookup f
put this
if isNullOrUndefined {
  pop
  put undefined on the stack
} else {
  evaluate x
  call function
}

This is necessary to do the proper short-circuiting required by the spec (there's a lot of unit test cases to verify them).

There is a little bit of duplication in the code, but I think it makes for much easier reading. Also, the bytecode and runtime code generated for the "normal" function call (non-optional) is exactly the same as before; there are no new branches added.

We pass a lot of test262 cases; the ones we do not are for the following reasons:

  • call-expression.js, super-property-optional-call.js: use class
  • 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, punctuator-decimal-lookahead.js: these use things like fn``x``?.a which we do not support because we also do not support the non-optional access, i.e. fn``x.a` already does not work. Tracked by Syntax rejected when accessing property of literal objects #1703
  • eval-optional-call.js: eval.? has a special semantic that I did not implement. Tracked by Indirect eval call semantics #1704
  • optional-chain-prod-arguments.js: there are some existing problems with the spread syntax, tracked by Support ES2015 Spread syntax #1217
  • new-target-optional-call.js: uses new.target which is unsupported, but the test is not marked as requiring it. Probably needs a fix in test262.
  • member-expression.js: uses async, class, and new.target
  • iteration-statement-for-in.js, iteration-statement-for-of-type-error.js: uses for (const key of xxx), which does not work. Tracked by Implement ES2015 const  #939

Made special ref work with optional chaining

Made optional chaining work with expressions, `a?.[b]`

Removed useless `++maxStack`

Add new token in `AstNode`

Fixed bug in interpreter code generation - was jumping a bit too far
`a?.b.c` is `undefined` if `a?.b` is undefined, _not_ a reference error
 because `c` does not exist! This is called "short circuiting".

 The implementation is not particularly efficient - we have more than
 one check "it's undefined? then jump to the end". Ideally we'd generate
 only one such jump; however, that is quite complicated with our code
 structure and hopefully this pattern is not super common to be a real
 problem right now.
All cases are handled:
```js
f?.()
a.b?.()
a[0]?.()
a.__parent__()
```

Handled also `eval` (which has special opcodes)
@andreabergia andreabergia marked this pull request as ready for review October 17, 2024 15:23
@rbri
Copy link
Collaborator

rbri commented Oct 17, 2024

@andreabergia have started some smoke testing with HtmlUnit....

@rbri
Copy link
Collaborator

rbri commented Oct 18, 2024

@andreabergia smoke test was successful

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

Successfully merging this pull request may close these issues.

2 participants