-
Notifications
You must be signed in to change notification settings - Fork 8k
[PFA 1/n] Support PFA syntax #20717
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
[PFA 1/n] Support PFA syntax #20717
Conversation
TimWolla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactoring for the zend_ast_fcc struct to hold parameters make sense to me to ship independently, but the grammar changes visible to userland (e.g. the support for ?) should probably be deferred to a later PR to not expose an incomplete implementation to users.
|
Right. It seemed reasonable to me for master, since I'm going to implement the rest soon. I've pushed a change to emit a compile error when trying to use |
Zend/zend_compile.c
Outdated
|
|
||
| zend_ast_list *args = zend_ast_get_list(((zend_ast_fcc*)args_ast)->args); | ||
| if (args->children != 1 || args->child[0]->attr != _IS_PLACEHOLDER_VARIADIC) { | ||
| zend_error_noreturn(E_COMPILE_ERROR, "Cannot create a Closure for call expression with more than one arguments, or non-variadic placeholders"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Cannot create a Closure" part is copied from "Cannot create Closure for new expression" above
TimWolla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't see any major issues as far as I'm qualified to tell.
Zend/zend_types.h
Outdated
| /* used for PFAs/FCCs */ | ||
| #define _IS_PLACEHOLDER_ARG 20 | ||
| #define _IS_PLACEHOLDER_VARIADIC 21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these required to be a zval type for the following implementation? Other AST attributes use a “distinct” set of values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'm now using a separate constant for AST attributes, and I was able to keep only one of these 2.
| if (list->kind == ZEND_AST_CALLABLE_CONVERT) { | ||
| zend_ast_fcc *fcc_ast = (zend_ast_fcc*)list; | ||
| fcc_ast->args = zend_ast_list_add(fcc_ast->args, arg); | ||
| return (zend_ast*)fcc_ast; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is clearer (the fcc_ast doesn't actually change):
| return (zend_ast*)fcc_ast; | |
| return list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer return (zend_ast*)fcc_ast, but no strong opinion
TimWolla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am no longer seeing an issue, but please also seek another approval from someone more familiar with engine code.
ndossche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only took a brief look but I don't see an obvious problem
|
|
||
| argument_list: | ||
| '(' ')' { $$ = zend_ast_create_list(0, ZEND_AST_ARG_LIST); } | ||
| '(' ')' { $$ = zend_ast_create_arg_list(0, ZEND_AST_ARG_LIST); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zend_ast_create_arg_list is always used with ZEND_AST_ARG_LIST. Looks a bit redundant but perhaps this is for the future proofing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this, but the variadic specialization trick implicitly expects the kind argument before the variadic ones, and I didn't want to create another variant of the specialization macro.
zend_ast_create_arg_list(init_children, ...) is defined as ZEND_AST_SPEC_CALL(zend_ast_create_arg_list, __VA_ARGS__), so that a call to zend_ast_create_arg_list() expands to zend_ast_create_arg_list_N(__VA_ARGS__) with N the number of va_args minus 1. As a result, zend_ast_create_arg_list(0, child1) expands to zend_ast_create_arg_list_0(child1), which is incorrect (should be zend_ast_create_arg_list_1(child1)).
|
Heads up: This broke this build: https://github.com/php/php-src/actions/runs/20767748729/job/59637430230 Might be expected given this is only a partial implementation. |
This is unexpected. This PR was designed to be a self-contained refactoring that should not break anything. See also my initial comment in #20717 (review). |
Spinned off arnaud-lb#22
RFC: https://wiki.php.net/rfc/partial_function_application_v2
For FCCs, the parser generates a normal function call AST node, the but argument list is a
ZEND_AST_CALLABLE_CONVERT/zend_ast_fccnode.We extend this for PFAs so that
zend_ast_fcccan represent arguments.In this PR:
zend_ast_fccso that arguments can be representedzend_ast_fccarguments in SHM / file cachezend_ast_arg_list_add(): Same aszend_ast_list_add(), but wraps the list in aZEND_AST_CALLABLE_CONVERTwhen adding any placeholder argument.Technically the arg list wrapping is not required, but it results in simpler code later as it will be very convenient in the compiler (determines whether a function calls is a PFA/FCC), and for PFA-in-const-expr support. It also allows to unify FCCs and PFAs in the grammar.