Store parameter default values in arginfo #4832
Conversation
There was a problem hiding this comment.
Looks like a reasonable start. For parsing, you may want to take a look at https://github.com/nikic/php-ast/blob/bc4ccdc6279b0b862eb820ae1ac0f3265f6876d7/ast.c#L297-L331. You'll have to parse something like <?php $default; and pick out the expression AST. Next, you'll want to compile this down to either a zval directly (possibly, this will even always be the case?) or an executable AST using
Line 8276 in ecc9588
Line 476 in ecc9588
ffb730a to
971ca94
Compare
971ca94 to
9a620a8
Compare
|
@nikic Thank you for the suggestions! I think I finished the main task (although compilation fails in CI 🤔 while it works for me on my mac). I'll want to regenerate the stubs (and fix some failing tests due to the name changes of arginfo structures) after the implementation is satisfactory so that it won't make a noise during code review. |
d3d83c1 to
e6c9900
Compare
|
As mentioned in a review comment, I think the general way forward here is to treat internal and userland functions the same: Store the string representation of the default and use that in reflection. This will ensure that they behave the same, and that we don't have to worry about not pre-evaluating values just because they might be needed by reflection. |
|
@nikic Thanks for the hint (again)! It was a top item on my list to continue. ^^ That said, I'll jump at it again in the following days. |
e2536ad to
89641d3
Compare
|
I think I managed to update the PR according to what we discussed. Unfortunately there is a leak under OPCache which most probably happens because I couldn't yet figure out how to use (interned) strings properly when storing the default param values. So I am on it... I also haven't resolved the build issue with the additional headers that are needed for php_reflection.c. Even though first I didn't really get what the benefit of your suggestion was, during implementation I realized that I had already come across a performance issue that had been caused by not being able to resolve constants during compilation: symfony/symfony#25474 It's really cool to see this problem get solved properly :) |
89641d3 to
4aae4e8
Compare
Actually, this problem is already mostly solved by an inline cache in RECV_INIT. Evaluating during compile-time is more efficient, but only slightly. The more important motivation is that a) it could be possible to get the full original expression, i.e. |
e058348 to
31b34a4
Compare
| /* we cannot substitute constants here or it will break ReflectionParameter::getDefaultValueConstantName() and ReflectionParameter::isDefaultValueConstant() */ | ||
| uint32_t cops = CG(compiler_options); | ||
| CG(compiler_options) |= ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION | ZEND_COMPILE_NO_PERSISTENT_CONSTANT_SUBSTITUTION; | ||
| zend_string *default_value_string = zend_new_interned_string(zend_ast_export("", default_ast, "")); |
There was a problem hiding this comment.
Hm... I think a general problem here is that this is going to export non-resolved names. If you're inside namespace Foo and have Bar::BAZ, this is going to give you Bar::BAZ, not Foo\Bar::BAZ ... right?
There was a problem hiding this comment.
Oh, you must be right because there is a test failure with the undefined ReflectionParameter::bar constant (in ReflectionParameter_DefaultValueConstant_basic1.phpt) - which of course doesn't exist.
There was a problem hiding this comment.
I've been trying to fix the issue - with more or less success...
I suppose we should store the constants in the arginfo fully resolved, right? So the namespace should be added as well as self and 'parent' changed to the actual one.
Actually, I managed to perform the resolution by reusing some code from here and there. However, I still have 2 problems:
zend_eval_string(ZSTR_VAL(arg_info->default_value), default_value_zval, "");in ext/reflection still throws an exception when it faces a namespaced constant:Undefined constant 'Foo\CONST_TEST_1'. What can be the problem now?- I suppose that the constant resolution should be done in case of an expression too, like:
FOO + Bar::BAZ + 1, right? I think I can traverse the AST, but I am not sure if I really have to write my own code or is there something out that which exactly does what I need?
I hope it makes sense at all what I am trying to do. :)
There was a problem hiding this comment.
I suppose we should store the constants in the arginfo fully resolved, right? So the namespace should be added as well as
selfand 'parent' changed to the actual one.
I think the main thing we should endeavor to do is make the default value independent of alias information. We can't perform a full resolution, because it's just not possible in PHP:
- Unqualified constant references will still depend on the namespace, as this can't be statically resolved.
self/parentmay depend on the scope, which may not be available (traits, closures).
So I think the only thing we should do here is resolve namespaced names to the degree that is possible (to fully qualified names).
Actually, I managed to perform the resolution by reusing some code from here and there. However, I still have 2 problems:
* `zend_eval_string(ZSTR_VAL(arg_info->default_value), default_value_zval, "");` in ext/reflection still throws an exception when it faces a namespaced constant: `Undefined constant 'Foo\CONST_TEST_1'`. What can be the problem now? * I suppose that the constant resolution should be done in case of an expression too, like: `FOO + Bar::BAZ + 1`, right? I think I can traverse the AST, but I am not sure if I really have to write my own code or is there something out that which exactly does what I need?
It should be possible to use a combination of zend_ast_apply and functions like zend_resolve_class_name_ast for that. A bit tricky because we don't have a dedicated name type, but I guess for current constant expressions the number of expressions that can contain names is actually very limited (constants and class constants, nothing else, right?)
31b34a4 to
fcedf45
Compare
d953540 to
4f47537
Compare
4f47537 to
4238b93
Compare
4238b93 to
ab077ef
Compare
ab077ef to
df2053e
Compare
|
@nikic I separated the changes into different commits, hopefully it makes code reviewing easier. Unfortunately, I'm stuck with resolving constants (see last review comments), so I'd appreciate if you could give me some guidance. |
df2053e to
ee053c5
Compare
ee053c5 to
462c47e
Compare
|
Closed in favour of #5353 |
Currently, there is no way to know the default values of parameters of internal functions as pointed out by @cmb69 in a discussion about stubs (#4803 (comment)). As far as I know, this situation is problematic in 2 cases:
nullfor all the default parameter values of the parent method, even though it's not the case (seeDateTime::setTime(), for instance)ReflectionParameter::getDefaultValue()andReflectionParameter::getDefaultValueConstantName()for built-in function parameters result in aReflectionExceptionAs discussed with @nikic, my fix intends to solve these issues the following way:
const char *default_valuefield in order to store default param values (unparsed)gen_stub.phpgenerates aZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUEmacro invocation for parameters that have a default valueMy patch is missing the following:
I'd like to continue working on the PR, but first I wanted to make sure if I am going in the right direction, and unfortunately, I'd need a little help about the parsing part.