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 PPC 0021 - Optional Chaining #22772

Draft
wants to merge 3 commits into
base: blead
Choose a base branch
from

Conversation

rabbiveesh
Copy link

@rabbiveesh rabbiveesh commented Nov 21, 2024

Hey all! I have an implementation of optional chaining here, covering (so far) array deref ($a?->[0] and $a?->@*; $a?->@[0..10] coming soon) and the same set for hash derefs. also covering subroutine deref both w/ and w/o arguments.

For arrays and hashes, it's convenient that the value we need is the tip of the stack, so the optchain op will pop it, check for definedness and return it to the stack and run the normal AV op, or push a fresh undef + short-circuit. the setup in perly.y is a little clever in that we take advantage of the fact that the op inside of newAVREF or newHVREF just pushes whatever we care about to the stack, so we put the value on the stack + give the newAVREF an OP_NULL.

For subrefs, b/c the order on the stack was invented by the devil, we had to take a different approach, and we make an anonymous pad temporary and thread it in the places we want using op_targ, so the newCVREF there takes an newPADxVOP to take the temporary we made instead of the tricky stack business we did.

I hope that made sense; my mind will never be the same since it laid eyes on toke.c

thanks so much @leonerd for all the help!

  • This set of changes requires a perldelta entry, and I need help writing it.

@iabyn
Copy link
Contributor

iabyn commented Nov 22, 2024 via email

@rabbiveesh rabbiveesh marked this pull request as draft November 22, 2024 11:08
@rabbiveesh
Copy link
Author

rabbiveesh commented Nov 22, 2024

On Thu, Nov 21, 2024 at 11:40:01AM -0800, Veesh Goldman wrote: Hey all! I have an implementation of optional chaining here, covering (so far) array deref ($a?->[0] and $a?->@, @.** coming soon) and the same set for hash derefs. also covering subroutine deref both w/ and w/o arguments.
I haven't examined this PR in depth, but some initial things I spotted as concerns:

Thanks for the feedback - i'll get to each line in turn.

  • pp_optchain() has been written with the old-style dSP/POPs paradigm.
    In order to work under PERL_RC_STACK builds, it needs to use the new rpp_
    interface. See "=head1 Reference-counted argument stack" in perlguts for
    details, and maybe see pp_and() for an example.

re the stack manipulation - i'll update that, didn't realize that something changed (this is my first time even writing XS so i appreciate the guidance)

  • does the OP_OPTCHAIN ever have a targ, and if so what's it for? I don't understand why its targ needs to assigned the value of the invocant, and to be cleared at the end of scope.

Yes, it does get a targ when we're doing a subref or method call, b/c the invocant is not in the right place on the stack for us to get at it directly. See how it's implemented in the rule for $subref->() on line 1184. I'm not happy with this b/c there's too much logic in the parser, but I'm not comfortable enough with C to know how to implement this more cleanly.

  • It would be nice to have a couple of lines of code comments above
    pp_optchain() to explain its purpose: its name alone is very generic, and
    gives no hint that it's actually a highly specialised op for doing the
    undef-arrow thing, rather than being some general purpose LOGOP. In fact,
    unless you see other uses for this op, I'd be tempted to rename it to
    something more specific, like OP_UNDEFARROW or whatever.

as far as things go, calling it OP_DAND might even work, seeing as it's defined-and. The name of the op was kinda pulled out of the PPC calling the operator optional chaining, so it's quite specific if you're coming from that angle. I don't understand ops well enough to evaluate if as written it could serve as a generic defined-and operator. Would love some hand holding on that.

  • There don't seems to be any doc updates.

Apologies, I should've opened this as a draft; i just wanted to get some eyes on this so that perhaps someone with more skill could help move this along. I will get to documentation at some point before this is mergable. Speaking of which, where should the docs for this go?

  • Deparse.pm doesn't appear to have been updated to handle the new op.
  • There don't appear to be any tests to check what the return value is in
    the undef case (for both scalar and list and contexts). Also, are all the
    branches in pp_optchain() exercised by the tests? Also, lvalue context
    needs testing, both for undef and defined.

Thanks for pointing these out - i'll get those updated + covered.

  • Are you aware that most sets of arrow ops, especially chained ones,
    are converted in peep() to a single OP_MULTIDEREF for performance
    purposes? Are there any plans to extend this for "?->" ?

This is certainly above my paygrade. It does seem like this will be important for how we implement the optchaining, b/c the spec dictates that

$array?->[0][1]

should have the following 3 possible behaviors:

  1. if $array is undef, immediately short circuit with the empty list
  2. if $array is defined and $array->[0] is not (or otherwise not amenable for array indexing), die b/c it can't dereference undef
  3. if $array is defined and so is $array->[0], then get the element at [0][1]

The way i began implementing is too local to handle this (as the failing test i just added shows); so perhaps moving the implementation up to MULTIDEREF might be the key. But for that I 100% need an hour or 2 of someone's time to walk me through how that optimization process works.

Thank you so much for your time + assistance, it's my honor to give back to the language.

@rabbiveesh
Copy link
Author

And after looking into it, the lvalue case is actually more complicated (current implemenation gives us a compile time error), and i'll need some hand holding there also. It looks like i need to update Perl_op_lvalue_flags, but i'll need some guidance

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