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

Merged

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

@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

@gbrail
Copy link
Collaborator

gbrail commented Oct 23, 2024

Thanks -- I merged the previous PR so could you please rebase?

@andreabergia andreabergia force-pushed the optional-chain-and-nullish-coalesce-fun branch from 5c8c512 to 79237f9 Compare October 23, 2024 06:29
@andreabergia
Copy link
Contributor Author

Thanks -- I merged the previous PR so could you please rebase?

Done

@gbrail
Copy link
Collaborator

gbrail commented Oct 25, 2024

This is a lot and it moves us forward a lot! I can see that it fixes a lot of tests, I can see that it fits in our existing structure and it's generally good quality, and it works. I'm happy to merge it and thanks!

@gbrail gbrail merged commit e9592d7 into mozilla:master Oct 25, 2024
1 check passed
@andreabergia andreabergia deleted the optional-chain-and-nullish-coalesce-fun branch October 27, 2024 15:54
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.

3 participants