Skip to content

Partials v2 #12

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

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from
Draft

Partials v2 #12

wants to merge 29 commits into from

Conversation

arnaud-lb
Copy link
Owner

@arnaud-lb arnaud-lb commented Jun 20, 2025

Based on php#6898.

With the following changes:

  • trampoline is now a user function. This avoids re-entering the VM, and allows to re-use the trampoline stack frame when calling the real function. This is similar to __call trampolines.
  • trampoline and prototype are merged into a single zend_function. This simplifies a few things and speeds up PFA creation a bit.
  • Reduced the number of new ZEND_ACC_ and ZEND_CALL_ flags since there are no free slots anymore
  • Behavior changes:
    • Partial new is not allowed
    • Updated optional/required parameter rules
    • Named placeholders are supported.

Still a WIP. There are a few things to check and cleanup.

TODO:

  • More tests
  • Address TODO comments
  • Fix reflection
  • Update callable name / ReflectionFunction::getName()
  • JIT support
  • Pipe optimization
  • Attributes (including SensitiveParameter)
  • Clone with
  • PFAs in constant expressions?
  • bind() validation (similarly to fake closures)

krakjoe and others added 7 commits June 20, 2025 19:31
Like __call trampolines, we can use an opcode-based trampoline to reduce
recursion/reentering, and reuse the call frame of the trampoline.

Also: Rebase fixes, update tests, add tests.
Comment on lines +208 to +209
/* Internal functions use an incompatible arg_info struct
* TODO: unify zend_arg_info / zend_internal_arg_info? */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be nice to do, but that would require allocating zend_strings rather than using C strings which are inside the binary. Would make a lot of other stuff simpler tho.

Comment on lines 669 to 671
/* used for place holders */
#define _IS_PLACEHOLDER_ARG 20
#define _IS_PLACEHOLDER_VARIADIC 21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be bellow _IS_NUMBER

@Crell
Copy link

Crell commented Jun 30, 2025

It looks like we still need a test for partially applying an invokable object that isn't a Closure.

$f = f(?, ?);

$f(1, 2);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a closing PHP tag in every test.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a linter for this kind of things :)

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.

5 participants