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

kocsismate · 2020-04-06T08:32:47Z

This PR is the same as #4832, just with a cut scope so that it only deals with internal functions.

I made the split because the original one has some complications (storing default parameter values of userland functions) which might be beyond my skills to solve, but this part seems to be useful on its own. Later on, we could try to make the other parts work, but until then, I wouldn't have to keep the whole PR rebased.

Girgias · 2020-04-06T12:20:30Z

Appveyor failures are related.

nikic · 2020-04-07T13:59:47Z

Zend/zend_compile.h

 } zend_internal_arg_info;

 /* arg_info for user functions */
 typedef struct _zend_arg_info {
 	zend_string *name;
 	zend_type type;
+	zend_string *default_value;


Maybe void *_unused; for now.

ext/reflection/php_reflection.c

ext/reflection/tests/ReflectionParameter_toString_basic.phpt

This reverts commit 18cfbfe.

This avoids the dependency on language scanner headers. The function is exactly the same as the one used by ext/ast, so this functionality is also more broadly useful.

nikic · 2020-04-08T10:49:34Z

As a followup, we might want to a) use null instead of NULL when printing userland defaults and b) make sure null is always used in stubs (there are a few NULL occurrences right now).

We will also start working on eliminating UNKNOWN defaults, to increase coverage.

On my build, the fast-path now handle everything apart from 234 default values, most of them are constant fetches. We could add fast-path handling for constants as well, though it would be a tad more involved.

kocsismate · 2020-04-08T11:19:16Z

@nikic Thank you very much for all the help! 🎉I'll ~~rebase to master and~~ commit (UPDATE: I've just noticed that you already did it).

I fully agree with the first point! Actually, I've wanted to ask you if it's something that's ok to change. Additionally, I thought about lowercasing NULL in other places, too (e.g. var_dump). Can I do it?

Regarding UNKNOWN: It's on my to-do list, however I prioritized this task lower than the resource to object migrations (hopefully, I can do a couple of them), and the 2 RFCs I have. I'm not exactly sure, but my assumption is that contrarily to the other tasks, fixing default values could be worked on in the feature freeze period, too. Could you confirm this? And what about warning promotions? Should we stop before freeze starts?

nikic · 2020-04-08T12:19:17Z

I fully agree with the first point! Actually, I've wanted to ask you if it's something that's ok to change. Additionally, I thought about lowercasing NULL in other places, too (e.g. var_dump). Can I do it?

I would not change var_dump(). That's going to be a PITA for anyone using phpt tests.

Regarding UNKNOWN: It's on my to-do list, however I prioritized this task lower than the resource to object migrations (hopefully, I can do a couple of them), and the 2 RFCs I have. I'm not exactly sure, but my assumption is that contrarily to the other tasks, fixing default values could be worked on in the feature freeze period, too. Could you confirm this? And what about warning promotions? Should we stop before freeze starts?

Yeah, I think that removal of UNKNOWN default values can also happen after feature freeze. Not sure about warning promotions...

@cmb69 It looks like there are segfaults on one of the Windows builders, but I don't really have a clue what could be the cause. Could you please take a look?

cmb69 · 2020-04-08T13:27:02Z

I cannot reproduce the failing test locally. I'll try to figure out what's wrong via AppVeyor, what might take quite a while.

nikic · 2020-04-08T13:41:40Z

Looks like this is a ZTS specific issue, reproducing on Linux under valgrind:

==19291== Conditional jump or move depends on uninitialised value(s)
==19291==    at 0x52411B: zend_resolve_class_name (zend_compile.c:974)
==19291==    by 0x52429D: zend_resolve_class_name_ast (zend_compile.c:1008)
==19291==    by 0x53DF87: zend_eval_const_expr (zend_compile.c:9274)
==19291==    by 0x53C3FA: zend_const_expr_to_zval (zend_compile.c:8678)
==19291==    by 0x34BE22: get_default_via_ast (php_reflection.c:1320)
==19291==    by 0x34C173: get_default_from_arg_info (php_reflection.c:1386)
==19291==    by 0x34C1BF: get_parameter_default (php_reflection.c:1391)
==19291==    by 0x353203: zim_reflection_parameter_getDefaultValue (php_reflection.c:2725)
==19291==    by 0x5D2FE1: ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER (zend_vm_execute.h:1683)
==19291==    by 0x637AC2: execute_ex (zend_vm_execute.h:52059)
==19291==    by 0x63BBA1: zend_execute (zend_vm_execute.h:56143)
==19291==    by 0x55BED1: zend_execute_scripts (zend.c:1662)

FC(imports) is uninitialized.

cmb69 · 2020-04-08T14:04:06Z

FC(imports) is uninitialized.

Oh, indeed. Is that var ever explicitly initialized?

nikic · 2020-04-08T15:13:25Z

AppVeyor is green now.

kocsismate · 2020-04-08T15:15:07Z

@nikic Should I squash your commits or can they go as is?

nikic · 2020-04-08T15:16:40Z

@kocsismate Please squash them together (probably into your first commit?)

cmb69 · 2020-04-08T15:17:03Z

AppVeyor is green now.

Yes. :) (something like cmb69@99fc913 may be fine as well; AppVeyor CI is still running)

PS: nope, apparently insufficient

kocsismate · 2020-04-08T15:20:54Z

@nikic OK, there will be only one commit then with you and me as authors.

kocsismate · 2020-04-08T16:48:43Z

@nikic I've just noticed that somehow I didn't commit the void *_unused; change when I first tried to address your review comments... Should I do it in master now?

Patch contributed by Sjon Hortensius. git-svn-id: https://svn.php.net/repository/phpdoc/en/trunk@350519 c90b9560-bf6c-de11-be94-00142212c4b1

kocsismate force-pushed the internal-default-values branch from 2fbc742 to 809daa8 Compare April 6, 2020 09:04

nikic reviewed Apr 7, 2020

View reviewed changes

kocsismate force-pushed the internal-default-values branch 3 times, most recently from d5f0eaa to 18cfbfe Compare April 7, 2020 22:00

kocsismate and others added 13 commits April 8, 2020 12:03

Store default parameter values of internal functions in arg info

ea43d8c

Update stubs

fe52d36

Update tests

d45d179

Address code review comments

4a8a22d

Try to reorganize language scanner dependencies

4266358

Revert "Try to reorganize language scanner dependencies"

62f170e

This reverts commit 18cfbfe.

Fix typo

7cf5852

Extract as zend_compile_string_to_ast() function

a3d1850

This avoids the dependency on language scanner headers. The function is exactly the same as the one used by ext/ast, so this functionality is also more broadly useful.

Remove reflection part of genfiles

157f390

Handle simple cases more efficiently

59b902f

Forward declare _zend_arena struct

1942986

Handle a few more cases in the fast-path

18ddd11

Code cleanup

1cdbe90

nikic force-pushed the internal-default-values branch from 4616b58 to 1cdbe90 Compare April 8, 2020 10:41

nikic approved these changes Apr 8, 2020

View reviewed changes

Initialize file context

dd62bcd

php-pulls closed this in 3709e74 Apr 8, 2020

kocsismate mentioned this pull request Apr 8, 2020

Store parameter default values in arginfo #4832

Closed

4 tasks

kocsismate deleted the internal-default-values branch April 8, 2020 16:39

kocsismate mentioned this pull request Apr 10, 2020

Improve the default value format in incompatible signature error messages #5361

Closed

SjonHortensius added a commit to SjonHortensius/php-doc-en that referenced this pull request Sep 2, 2020

Reflection can now get defaults for internals, see php/php-src#5353

03c142d

SjonHortensius mentioned this pull request Sep 2, 2020

Reflection can now get defaults for internals php/doc-en#142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kocsismate commented Apr 6, 2020

Girgias commented Apr 6, 2020

nikic Apr 7, 2020

nikic commented Apr 8, 2020

kocsismate commented Apr 8, 2020 •

edited

Loading

nikic commented Apr 8, 2020

cmb69 commented Apr 8, 2020

nikic commented Apr 8, 2020

cmb69 commented Apr 8, 2020

nikic commented Apr 8, 2020

kocsismate commented Apr 8, 2020

nikic commented Apr 8, 2020

cmb69 commented Apr 8, 2020 •

edited

Loading

kocsismate commented Apr 8, 2020

kocsismate commented Apr 8, 2020

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

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

Conversation

kocsismate commented Apr 6, 2020

Girgias commented Apr 6, 2020

nikic Apr 7, 2020

Choose a reason for hiding this comment

nikic commented Apr 8, 2020

kocsismate commented Apr 8, 2020 • edited Loading

nikic commented Apr 8, 2020

cmb69 commented Apr 8, 2020

nikic commented Apr 8, 2020

cmb69 commented Apr 8, 2020

nikic commented Apr 8, 2020

kocsismate commented Apr 8, 2020

nikic commented Apr 8, 2020

cmb69 commented Apr 8, 2020 • edited Loading

kocsismate commented Apr 8, 2020

kocsismate commented Apr 8, 2020

kocsismate commented Apr 8, 2020 •

edited

Loading

cmb69 commented Apr 8, 2020 •

edited

Loading