ext/reflection: various cleanups and simplifications#22564
Open
DanielEScherzer wants to merge 33 commits into
Open
ext/reflection: various cleanups and simplifications#22564DanielEScherzer wants to merge 33 commits into
DanielEScherzer wants to merge 33 commits into
Conversation
Girgias
reviewed
Jul 2, 2026
Girgias
left a comment
Girgias
left a comment
Member
There was a problem hiding this comment.
Looks good to me overall, minor nits.
As a follow-up, it would make sense to review all calls to array_init() and see when and if array_init_size() could be used instead.
| if (!(opline->extended_value & (ZEND_BIND_IMPLICIT|ZEND_BIND_EXPLICIT))) { | ||
| continue; | ||
| } | ||
| array_init(return_value); |
Member
There was a problem hiding this comment.
Is it possible to know the size of the array ahead of time and preallocate the correct size?
Member
Author
There was a problem hiding this comment.
I think only if we do the loop twice, which might help memory but at the cost of speed
| zend_class_init_statics(ce); | ||
| } | ||
|
|
||
| array_init(return_value); |
Member
There was a problem hiding this comment.
Might make sense to use the variant that allows setting the count as ce->default_static_members_count
Member
Author
There was a problem hiding this comment.
Given that it isn't always going to be all of the ce->default_static_members_count members, I'm not sure that that would help, I can try and experiment in a later commit
DanielEScherzer
force-pushed
the
reflection-cleanup-202607
branch
from
f2708e6 to
a0d3051
Compare
Use the `_class_constant_check_flag()` helper function to simplify things
…lper Use the `_class_constant_check_flag()` helper function to simplify things
Since PHP 8.1 ReflectionProperty has always allowed access to non-public properties and the method is a no-op.
Affected methods: * `ReflectionMethod::getModifiers()` * `ReflectionClassConstant::getModifiers()` * `ReflectionClass::getModifiers()` * `ReflectionProperty::getModifiers()` They were all documented bitfields of "access modifiers", however they all included modifiers unrelated to visibility/access.
…ing()` When a type corresponds to an iterator that may be null, the string representation is known ahead of time to be "?iterable". Use `ZSTR_INIT_LITERAL()` rather than building up a string with `zend_string_concat2()` from a "?" and the "iterable" known string.
Update local variables and arguments to * `zend_read_static_property_ex()` * `zend_string_alloc()` * `zend_string_init()` * `zend_string_release_ex()` * `zend_verify_property_type()` * `zend_verify_ref_assignable_zval()` * Object handlers' get_closure() function * `RETURN_ZVAL` (macro) * `ZEND_TYPE_INIT` (macro) * `ZEND_TYPE_INIT_CODE` (macro)
The previous commit addressed places where `0` or `1` where used in places where a boolean was already expected, such a function arguments declared to be booleans. Now, address some inner implementation details of the reflection extension that use integers for conditions: * `dynam_prop` local variable in `ReflectionProperty::__construct()` * `first` local variable in `_extension_string()` * `is_object` parameter to `reflection_class_object_ctor()` * `variadic` parameter to `reflection_method_invoke()`
…ng()` The only caller (`ReflectionExtension::__toString()`) always provided an empty string as the indentation; removing the indentation entirely simplifies the building of the string representation, including replacing some uses of `smart_str_append_printf()` with `smart_str_appends()`, which is more performant. The indentation levels passed to `_extension_ini_string()` and `_extension_class_string()` from within `_extension_string()` are kept for now and will be addressed in follow-up commits.
…string()` The only caller (`_extension_string()`) always provided `_extension_ini_string()` with the same indentation that it was called with; after the prior commit removed indentation support from `_extension_string()` it became clear that the INI string function also always received an empty string as the indentation. Removing the indentation entirely simplifies the building of the string representation, including replacing a use of `smart_str_append_printf()` with `smart_str_appends()`, which is more performant.
…_string()` The only caller (`ReflectionZendExtension::__toString()`) always provided an empty string as the indentation; removing the indentation slightly simplifies the building of the string representation.
The `zend_function` union has both an `op_array` member for userland code, and an `internal_function` member for internal code. The elements at the start of each match, and are also made available under the `common` member which only has the fields present in both `op_array` and `internal_function`. For function pointers that have not been checked to determine if they are userland functions or internal functions, access common fields through `common`. While technically there shouldn't be a difference due to the common layout (up through the relevant fields), semantically accessing information about an internal function through `op_array` or about a userland function through `internal_function` does not make sense.
Instead of a single-use two-line helper function `zend_type_to_string_without_null()`, move the relevant logic into `ReflectionNamedType::getName()` directly. To maintain the behavior of removing the `MAY_BE_NULL` flag from a *copy* of the stored type information, make an explicit copy of the type before the flag is removed.
For global functions there is no class entry set, the constructor a few lines above explicitly sets `intern->ce = NULL;`, as does `reflection_function_factory()`. Replace the access with `NULL` to avoid suggesting that there might be a class entry to use.
Avoid `array_init(return_value);` in cases where the function does not correspond to a closure or has no static variables, to avoid an unneeded allocation for an array that will be empty.
For classes with no static properties, avoid allocating an array that will be empty. Includes a regression test for the function since it was previously untested, to confirm that even when a class declares no static properties of its own, inherited static properties are still returned.
Both `ReflectionFunctionAbstract::hasTentativeReturnType()` and `ReflectionFunctionAbstract::getTentativeReturnType()` were documented as checking or retrieving the "return type" of a function rather than the "tentative return type". The two are different; update the documentation accordingly.
… cast `called_scope` was declared to be a pointer to a `zend_class_entry`, no cast is needed.
`reflection_extension_factory()` was used to initialize a `ReflectionExtension` instance based on the name of the extension. It would look for an extension with the given name in the global `module_registry` and, if found, delegate to `reflection_extension_factory_ex()` to do the actual initialization using the located `zend_module_entry` pointer. However, both callers of `reflection_extension_factory()` already had a `zend_module_entry` pointer! Replace both uses of `reflection_extension_factory()` with `reflection_extension_factory_ex()` using the available pointer, and remove the `reflection_extension_factory()` method. In a follow-up commit the _ex variant will be renamed to remove the suffix.
After the original `reflection_extension_factory()` was removed in the prior commit, the _ex suffix is no longer needed. Remove it.
…r()` Use `zend_string_release_ex()` rather than the generic `zend_string_release()` since we know that the string is not marked as persistent (based on the `false` parameter to `zend_string_init()` two lines earlier).
Once the `property_reference` has been dereferenced to identify the `zend_property_info`, no need to do so again since the result was already stored in a variable.
This is testing `ReflectionProperty::isDefault()`, `ReflectionParameter::isDefault()` does not exist.
The file name was correct, but the test name mentioned `Reflection::getClosureThis()` (which does not exist). Update the test name to say `ReflectionMethod::getClosureThis()`.
"we we looking at" was presumably meant to be "we are looking at".
Consistently indent with tabs, have a space after the `while` keyword, and adjust the lines of a few braces.
For attributes without arguments, avoid allocating an array that will be empty.
…ization In both `ReflectionClass::getConstants()` and `ReflectionClass::getReflectionConstants()`, for classes without constants, avoid allocating an array that will be empty.
DanielEScherzer
force-pushed
the
reflection-cleanup-202607
branch
from
a0d3051 to
e3a7f4b
Compare
Girgias
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Over the last few weeks I worked on recreating the extension in Rust (as an exercise in building extensions in Rust, not because I think we should replace the current implementation) and noticed a few things in the process.