Store parameter default values in arginfo #4832

kocsismate · 2019-10-18T10:38:49Z

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:

When a method of a built-in class is overridden in user-land and the signatures are incompatible, then the error message displays null for all the default parameter values of the parent method, even though it's not the case (see DateTime::setTime(), for instance)
Invoking ReflectionParameter::getDefaultValue() and ReflectionParameter::getDefaultValueConstantName() for built-in function parameters result in a ReflectionException

As discussed with @nikic, my fix intends to solve these issues the following way:

the arginfo structure is extended with a const char *default_value field in order to store default param values (unparsed)
the gen_stub.php generates a ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE macro invocation for parameters that have a default value
parsing is needed when the default values are retrieved (either by reflection or for displaying an error message)

My patch is missing the following:

The last step because I could not yet find out how to parse an expression
Adding support for retrieving the default value in reflection
Generating the updated arginfos for other extensions, not just for Date
More tests

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.

nikic

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

php-src/Zend/zend_compile.c

Line 8276 in ecc9588

void zend_const_expr_to_zval(zval *result, zend_ast *ast) /* {{{ */

. The constexpr AST can then (if necessary) be evaluated using

php-src/Zend/zend_ast.c

Line 476 in ecc9588

    
           ZEND_API int ZEND_FASTCALL zend_ast_evaluate(zval *result, zend_ast *ast, zend_class_entry *scope)

.

Zend/zend_compile.h

kocsismate · 2019-10-21T21:22:28Z

@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.

ext/reflection/php_reflection.c

Zend/zend_inheritance.c

ext/reflection/php_reflection.c

Zend/zend_inheritance.c

nikic · 2020-02-05T14:41:37Z

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.

kocsismate · 2020-02-05T14:47:43Z

@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.

kocsismate · 2020-02-08T22:16:11Z

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 :)

nikic · 2020-02-10T11:05:44Z

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 :)

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. 24 * 60 * 60 instead of 86400 and b) it is more easily extended to cases where const evaluation is not possible at all, e.g. foo(Foo $param = new Foo()) {}.

Zend/zend_API.h

Zend/zend_compile.c

ext/reflection/php_reflection.c

nikic · 2020-02-11T09:01:54Z

Zend/zend_compile.c

-			/* 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, ""));


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?

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.

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. :)

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.

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/parent may 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?)

kocsismate · 2020-04-01T08:24:44Z

@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.

kocsismate · 2020-04-08T16:39:42Z

Closed in favour of #5353

nikic reviewed Oct 18, 2019

View reviewed changes

Zend/zend_compile.h Show resolved Hide resolved

nikic mentioned this pull request Oct 18, 2019

Add stubs for PDO #4803

Closed

kocsismate force-pushed the default-values branch from ffb730a to 971ca94 Compare October 20, 2019 18:48

carusogabriel added the Feature label Oct 21, 2019

kocsismate force-pushed the default-values branch from 971ca94 to 9a620a8 Compare October 21, 2019 20:55

kocsismate changed the title ~~[WIP] Add support for retrieving default values of parameters of internal functions~~ Add support for retrieving default values of parameters of internal functions Oct 21, 2019

carusogabriel suggested changes Oct 21, 2019

View reviewed changes

ext/reflection/php_reflection.c Show resolved Hide resolved

Zend/zend_inheritance.c Outdated Show resolved Hide resolved

kocsismate commented Oct 21, 2019

View reviewed changes

Zend/zend_inheritance.c Outdated Show resolved Hide resolved

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

nikic reviewed Oct 22, 2019

View reviewed changes

Zend/zend_inheritance.c Outdated Show resolved Hide resolved

kocsismate force-pushed the default-values branch 2 times, most recently from d3d83c1 to e6c9900 Compare October 24, 2019 16:21

kocsismate force-pushed the default-values branch 3 times, most recently from e2536ad to 89641d3 Compare February 8, 2020 21:39

kocsismate force-pushed the default-values branch from 89641d3 to 4aae4e8 Compare February 9, 2020 21:14

nikic reviewed Feb 10, 2020

View reviewed changes

kocsismate force-pushed the default-values branch 2 times, most recently from e058348 to 31b34a4 Compare February 11, 2020 08:10

nikic reviewed Feb 11, 2020

View reviewed changes

kocsismate force-pushed the default-values branch from 31b34a4 to fcedf45 Compare February 17, 2020 21:34

kocsismate force-pushed the default-values branch 2 times, most recently from d953540 to 4f47537 Compare March 7, 2020 20:11

kocsismate force-pushed the default-values branch from 4f47537 to 4238b93 Compare March 25, 2020 21:26

kocsismate changed the title ~~Add support for retrieving default values of parameters of internal functions~~ Store parameter default values in arginfo Mar 25, 2020

kocsismate force-pushed the default-values branch from 4238b93 to ab077ef Compare April 1, 2020 08:12

kocsismate force-pushed the default-values branch from ab077ef to df2053e Compare April 1, 2020 08:17

kocsismate force-pushed the default-values branch from df2053e to ee053c5 Compare April 2, 2020 09:48

kocsismate added 3 commits April 2, 2020 12:25

Store parameter default values in arginfo

9b76b8a

Update arg info headers

c0ea025

Update tests

462c47e

kocsismate force-pushed the default-values branch from ee053c5 to 462c47e Compare April 2, 2020 10:26

kocsismate mentioned this pull request Apr 6, 2020

Store default values of parameters of internal functions in arginfo #5353

Closed

kocsismate closed this Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store parameter default values in arginfo #4832

Store parameter default values in arginfo #4832

kocsismate commented Oct 18, 2019 •

edited

Loading

nikic left a comment

kocsismate commented Oct 21, 2019 •

edited

Loading

nikic commented Feb 5, 2020

kocsismate commented Feb 5, 2020

kocsismate commented Feb 8, 2020 •

edited

Loading

nikic commented Feb 10, 2020

nikic Feb 11, 2020

kocsismate Feb 11, 2020

kocsismate Feb 21, 2020

nikic Apr 2, 2020

kocsismate commented Apr 1, 2020

kocsismate commented Apr 8, 2020

Store parameter default values in arginfo #4832

Store parameter default values in arginfo #4832

Conversation

kocsismate commented Oct 18, 2019 • edited Loading

nikic left a comment

Choose a reason for hiding this comment

kocsismate commented Oct 21, 2019 • edited Loading

nikic commented Feb 5, 2020

kocsismate commented Feb 5, 2020

kocsismate commented Feb 8, 2020 • edited Loading

nikic commented Feb 10, 2020

nikic Feb 11, 2020

Choose a reason for hiding this comment

kocsismate Feb 11, 2020

Choose a reason for hiding this comment

kocsismate Feb 21, 2020

Choose a reason for hiding this comment

nikic Apr 2, 2020

Choose a reason for hiding this comment

kocsismate commented Apr 1, 2020

kocsismate commented Apr 8, 2020

kocsismate commented Oct 18, 2019 •

edited

Loading

kocsismate commented Oct 21, 2019 •

edited

Loading

kocsismate commented Feb 8, 2020 •

edited

Loading