[PFA 1/n] Support PFA syntax #20717
[PFA 1/n] Support PFA syntax #20717
Conversation
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
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.
The "Cannot create a Closure" part is copied from "Cannot create Closure for new expression" above
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Didn't see any major issues as far as I'm qualified to tell.
| if (ast->attr == _IS_PLACEHOLDER_ARG) { | ||
| APPEND_STR("?"); | ||
| } else if (ast->attr == _IS_PLACEHOLDER_VARIADIC) { | ||
| APPEND_STR("..."); | ||
| } |
There was a problem hiding this comment.
| if (ast->attr == _IS_PLACEHOLDER_ARG) { | |
| APPEND_STR("?"); | |
| } else if (ast->attr == _IS_PLACEHOLDER_VARIADIC) { | |
| APPEND_STR("..."); | |
| } | |
| if (ast->attr == _IS_PLACEHOLDER_ARG) { | |
| APPEND_STR("?"); | |
| } else if (ast->attr == _IS_PLACEHOLDER_VARIADIC) { | |
| APPEND_STR("..."); | |
| } else { | |
| ZEND_UNREACHABLE(); | |
| } |
|
|
There was a problem hiding this comment.
Whoops. Sorry for introducing these. Can you push the a commit to trim the trailing spaces straight to master to denoise the diff?
| /* used for PFAs/FCCs */ | ||
| #define _IS_PLACEHOLDER_ARG 20 | ||
| #define _IS_PLACEHOLDER_VARIADIC 21 |
There was a problem hiding this comment.
Are these required to be a zval type for the following implementation? Other AST attributes use a “distinct” set of values.
| zend_ast_fcc *fcc_ast = (zend_ast_fcc*) ast; | ||
|
|
||
| zend_ast_destroy(fcc_ast->args); |
There was a problem hiding this comment.
| zend_ast_fcc *fcc_ast = (zend_ast_fcc*) ast; | |
| zend_ast_destroy(fcc_ast->args); | |
| zend_ast_fcc *fcc_ast = (zend_ast_fcc*) ast; | |
| ast = fcc_ast->args; | |
| goto tail_call; |
?
| /* TODO: PFAs */ | ||
| return FAILURE; |
There was a problem hiding this comment.
I believe the expectation for return FAILURE; is that there also is an Exception active:
| /* TODO: PFAs */ | |
| return FAILURE; | |
| zend_throw_error(NULL, "Cannot create a Closure for call expression with more than one argument, or non-variadic placeholders"); | |
| return FAILURE; |
| zend_ast_list *args = zend_ast_get_list(fcc_ast->args); | ||
| ZEND_ASSERT(args->children > 0); | ||
| if (args->children != 1 || args->child[0]->attr != _IS_PLACEHOLDER_VARIADIC) { | ||
| /* TODO: PFAs */ |
There was a problem hiding this comment.
| /* TODO: PFAs */ | |
| zend_throw_error(NULL, "Cannot create a Closure for call expression with more than one argument, or non-variadic placeholders"); |
| 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.
Not sure if this is clearer (the fcc_ast doesn't actually change):
| return (zend_ast*)fcc_ast; | |
| return list; |
2 participants
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.