|
| 1 | +--- |
| 2 | +extensions: |
| 3 | + footnotes: true |
| 4 | + pygments: true |
| 5 | +title: Rewriting Reflection with Rust |
| 6 | +--- |
| 7 | + |
| 8 | +# Rewriting Reflection with Rust |
| 9 | + |
| 10 | +Over the last few weeks, I have been working on rewriting the PHP Reflection |
| 11 | +extension in Rust as an exercise in building extensions in Rust. In addition |
| 12 | +to understanding how to implement extensions in Rust, the process also |
| 13 | +highlighted a number of ways that the Reflection extension could be improved. |
| 14 | + |
| 15 | +Discovering these improvements was a byproduct of my process. Since |
| 16 | +[it was introduced in 2003][commit-ref-intro], the Reflection extension has |
| 17 | +amassed over 1,200 commits[^1] by roughly 100 different contributors.[^2] After |
| 18 | +20+ years, there were typos, missed optimizations, and occasional bugs that |
| 19 | +accumulated. By reviewing the existing code with an eye towards translating it |
| 20 | +to Rust, I found a few places where the existing code was misdocumented, |
| 21 | +unoptimized, or buggy. This blog post is not about the Rust code that I wrote, |
| 22 | +but rather the improvements to the C code that I discovered in the process. |
| 23 | + |
| 24 | +I should note that I was intentionally *not* using the [ext-php-rs][ext-php-rs] |
| 25 | +library to abstract away the interactions with the Zend API. While I may opt to |
| 26 | +use that library in the future, I wanted to ensure a solid understanding of how |
| 27 | +writing an extension in Rust differs from one written in C. |
| 28 | + |
| 29 | +## Dynamic Property Shadowing |
| 30 | + |
| 31 | +To be clear, I am *not* suggesting that the extension was previously believed |
| 32 | +to be bug-free. There are a number of existing issues reported on GitHub. |
| 33 | +However, in the process of rewriting the implementation of |
| 34 | +[`ReflectionClass::getProperty()`][docs-get-prop], I noticed an unreported bug |
| 35 | +just from reading the existing C code. |
| 36 | + |
| 37 | +When I filed the bug report ([php/php-src#22441][gh-22441]) the implementation |
| 38 | +appeared [as follows][ref-pre-bug] (with the inapplicable handling of |
| 39 | +fully-qualified property names elided for readability): |
| 40 | + |
| 41 | +```c |
| 42 | +/* {{{ Returns the class' property specified by its name */ |
| 43 | +ZEND_METHOD(ReflectionClass, getProperty) |
| 44 | +{ |
| 45 | + reflection_object *intern; |
| 46 | + zend_class_entry *ce, *ce2; |
| 47 | + zend_property_info *property_info; |
| 48 | + zend_string *name, *classname; |
| 49 | + char *tmp, *str_name; |
| 50 | + size_t classname_len, str_name_len; |
| 51 | + |
| 52 | + if (zend_parse_parameters(ZEND_NUM_ARGS(), "S", &name) == FAILURE) { |
| 53 | + RETURN_THROWS(); |
| 54 | + } |
| 55 | + |
| 56 | + GET_REFLECTION_OBJECT_PTR(ce); |
| 57 | + if ((property_info = zend_hash_find_ptr(&ce->properties_info, name)) != NULL) { |
| 58 | + if (!(property_info->flags & ZEND_ACC_PRIVATE) || property_info->ce == ce) { |
| 59 | + reflection_property_factory(ce, name, property_info, return_value); |
| 60 | + return; |
| 61 | + } |
| 62 | + } else if (Z_TYPE(intern->obj) != IS_UNDEF) { |
| 63 | + /* Check for dynamic properties */ |
| 64 | + if (zend_hash_exists(Z_OBJ_HT(intern->obj)->get_properties(Z_OBJ(intern->obj)), name)) { |
| 65 | + reflection_property_factory(ce, name, NULL, return_value); |
| 66 | + return; |
| 67 | + } |
| 68 | + } |
| 69 | + str_name = ZSTR_VAL(name); |
| 70 | + if ((tmp = strstr(ZSTR_VAL(name), "::")) != NULL) { |
| 71 | + // Handle fully qualified property names, elided for readability |
| 72 | + } |
| 73 | + zend_throw_exception_ex(reflection_exception_ptr, 0, "Property %s::$%s does not exist", ZSTR_VAL(ce->name), str_name); |
| 74 | +} |
| 75 | +``` |
| 76 | + |
| 77 | +The logic is basically: |
| 78 | + |
| 79 | +1. retrieve the reflection object (basically the equivalent of `$this`) |
| 80 | +2. parse the function parameters to identify the property name |
| 81 | +3. check the class' property table; if property is found there, and is not a |
| 82 | +private property in a parent class, return it |
| 83 | +4. if the property is not found in the property table, and an object is |
| 84 | +available, check the object's dynamic properties |
| 85 | +5. if the property still has not been found, check for a fully qualified property |
| 86 | +name |
| 87 | +6. if nothing works, throw an exception |
| 88 | + |
| 89 | +The subtle bug arises from the transition from step 3 to step 4. If you don't |
| 90 | +spot the issue, don't worry, apparently no one else has since it was introduced |
| 91 | +in PHP 5.2.7. Before PHP 5.2.7, dynamic properties were never checked. That |
| 92 | +behavior was reported in [bug #46064][bug-46064], and fixed in |
| 93 | +[commit b9c03aa][commit-b9c03aa][^3]. [Since PHP 5.2.7][3v4l-527], dynamic |
| 94 | +properties have generally been handled correctly. The operative word here is |
| 95 | +"generally". |
| 96 | + |
| 97 | +When classes inherit private properties that are *not* overridden in the child |
| 98 | +class, the *parent* property is stored in the child class property table. This |
| 99 | +is why step 3 above checks the details of a property found in the property |
| 100 | +table; inherited private properties should be ignored by |
| 101 | +`ReflectionClass::getProperty()`. *However*, when a private parent property was |
| 102 | +found and ignored, the code neglected to check for a dynamic property of the |
| 103 | +same name! This was the bug that I reported. |
| 104 | + |
| 105 | +Given that using dynamic properties is generally discouraged (and since PHP 8.2 |
| 106 | +has been deprecated unless the `#[\AllowDynamicProperties]` attribute is |
| 107 | +applied) such an edge case was extremely unlikely to be encountered, which is |
| 108 | +why it went undiscovered for almost 18 years. |
| 109 | + |
| 110 | +## Type checking |
| 111 | + |
| 112 | +I also came across something that appears to be a bug, but where "fixing" it |
| 113 | +would be a backwards-incompatible change.[^4] Specifically, the |
| 114 | +[`ReflectionProperty` class][docs-ref-prop], which represents a specific |
| 115 | +property on a class, does not always validate the arguments provided to its |
| 116 | +methods! Consider the following code: |
| 117 | + |
| 118 | +```php startinline=True |
| 119 | +class Foo { |
| 120 | + public $prop; |
| 121 | +} |
| 122 | +class Bar { |
| 123 | + public $prop; |
| 124 | +} |
| 125 | + |
| 126 | +$b = new Bar(); |
| 127 | +$ref = new ReflectionProperty(Foo::class, 'prop'); |
| 128 | +$ref->setValue($b, true); |
| 129 | +var_dump($b); |
| 130 | +``` |
| 131 | + |
| 132 | +The `ReflectionProperty` object conceptually represents the `$prop` property |
| 133 | +that is a part of the `Foo` class. However, the `ReflectionProperty::setValue()` |
| 134 | +call is given an instance of the `Bar` class, which is entirely unrelated to |
| 135 | +`Foo`. Rather than complaining (e.g. by throwing an exception), the |
| 136 | +`ReflectionProperty` happily writes to the `$prop` property |
| 137 | +*on the `Bar` instance*. |
| 138 | + |
| 139 | +I had actually stumbled across a bug report about this a year earlier |
| 140 | +([php/php-src#17730][gh-17730]) but did not look into it at the time. Now, in |
| 141 | +the process of rewriting the Reflection extension in Rust, when I got to the |
| 142 | +point of implementing `ReflectionProperty::setValue()` I realized that I didn't |
| 143 | +need to implement validation logic, and independently rediscovered the bug. |
| 144 | +Before filing a report, I checked the existing bugs reported for the extension, |
| 145 | +and found the open GitHub issue. |
| 146 | + |
| 147 | +"Fixing" this mistake is not as simple as adding the missing validation, because |
| 148 | +doing so would break existing code that has been working. While code should not |
| 149 | +rely on the *presence* of exceptions, it almost always relies on the *absence* |
| 150 | +of exceptions, i.e. that the function calls succeed. Accordingly, adding |
| 151 | +validation needs to go through PHP's RFC process. I added entries to the mass |
| 152 | +["Deprecations for PHP 8.6" RFC][rfc-86-depr] related to this missing |
| 153 | +validation, we'll see in a few weeks if the community agrees that providing |
| 154 | +unrelated objects to `ReflectionProperty` methods should be deprecated and |
| 155 | +eventually trigger exceptions. |
| 156 | + |
| 157 | +## Typos and Cleanup |
| 158 | + |
| 159 | +My process for the extension rewrite was to proceed one method at a time. I |
| 160 | +would *remove* the implementation from my copy of |
| 161 | +`ext/reflection/php_reflection.c`, paste it into the relevant Rust file, and |
| 162 | +then convert the C code to Rust in-place. In the process, I read through every |
| 163 | +line of the original C code multiple times, and inevitably spotted some typos |
| 164 | +or documentation mistakes. |
| 165 | + |
| 166 | +Rust does not allow treating non-boolean values as booleans, e.g. in conditions |
| 167 | +or arguments. Thus, when I encountered integers being used as booleans, the |
| 168 | +rewrite in Rust occasionally forced me to change the type of a variable from |
| 169 | +some flavor of integer to an actual boolean. Similarly, when the C code used |
| 170 | +`0` and `1` in place of `false` and `true`, Rust's type checking complained, and |
| 171 | +I updated function parameters. As I went, I collected a list of things to fix |
| 172 | +in the C implementation. |
| 173 | + |
| 174 | +Separate from the places that Rust complained about (from a type checking |
| 175 | +perspective), or where the documentation was wrong, by reading each line so many |
| 176 | +times I came across a number of places where the C implementation could be |
| 177 | +simplified or optimized, e.g. by removing unneeded lookups or reusing existing |
| 178 | +helpers. The Rust version that I am building is intended as an exercise; to |
| 179 | +improve the extension for others I needed to submit those improvements back to |
| 180 | +the C version. |
| 181 | + |
| 182 | +The initial result was [php/php-src#22564][gh-22564], a collection of 32 cleanup |
| 183 | +commits (33 after adjusting in response to reviewer feedback) that made those |
| 184 | +improvements in small, self-contained commits. Since I maintain the Reflection |
| 185 | +extension, there isn't really anyone else responsible for reviewing changes to |
| 186 | +that code. While I could always merge things without review, my preference is |
| 187 | +always to get at least one additional set of eyes on my changes, and by |
| 188 | +performing the cleanup in small commits it was easier for someone unfamiliar |
| 189 | +with the extension to review my tweaks. |
| 190 | + |
| 191 | +## Looking Ahead |
| 192 | + |
| 193 | +I have no plans to try and push a Rust reimplementation of the Reflection |
| 194 | +extension. The point of this rewrite was to use logic that I was already |
| 195 | +familiar with (the Reflection extension) for my initial work on writing an |
| 196 | +extension in Rust. In the same way that |
| 197 | +[I started with Project Euler problems][blog-rust-euler] when learning Rust, so |
| 198 | +I could focus on the language rather than the logic, here I started with |
| 199 | +Reflection so I could focus on the Rust-C integration. |
| 200 | + |
| 201 | +I have a few other thoughts from this process that I will write up in later |
| 202 | +blog posts, and of course I haven't fully reimplemented the Reflection extension |
| 203 | +yet, but I can now say that I understand how to build PHP extensions in Rust. |
| 204 | + |
| 205 | +[^1]: `git rev-list --count HEAD -- ext/reflection/php_reflection.c` reports |
| 206 | +1,215 commits as of my most recent changes in [commit 2e2b03e][commit-2e2b03e]. |
| 207 | + |
| 208 | +[^2]: Based on `git log --format="%aE" ext/reflection/php_reflection.c`, there |
| 209 | +have been contributions to the main implementation file from 105 different |
| 210 | +emails, and with the `"%aN"` format there have been contributions from 95 names. |
| 211 | +Combining them to report the unique name-email pairs of contributors and |
| 212 | +manually filtering out contributors with multiple names or emails leaves 89 |
| 213 | +contributors by my count. For the entire `ext/reflection/` directory, there |
| 214 | +have been contributions from 135 emails, 125 names, and 118 unique contributors. |
| 215 | + |
| 216 | +[^3]: There appear to be two different commits with this fix. |
| 217 | +[Commit b9c03aa][commit-b9c03aa] is what shows up in the history of the |
| 218 | +reflection extension, but GitHub does not report that it was part of the |
| 219 | +`php-5.2.7` tag (or indeed, any tag until PHP 5.3). On the other hand, |
| 220 | +[commit a04ec69][commit-a04ec69] has seven fewer lines in a test file but the |
| 221 | +same actual fix, and *was* a part of the `php-5.2.7` tag. Not sure what is |
| 222 | +going on there. |
| 223 | + |
| 224 | +[^4]: Strictly speaking, *not* throwing an exception in |
| 225 | +`ReflectionClass::getProperty()` could break code that assumes an exception |
| 226 | +will be thrown. However, as I explained in my talk about semantic versioning at |
| 227 | +PHPTek, the presence of exceptions, especially where they are caused by a logic |
| 228 | +error in the implementation, should not be relied on. |
| 229 | + |
| 230 | +[3v4l-527]: https://3v4l.org/Gf30Fg |
| 231 | +[blog-rust-euler]: ./20250919-rust-euler |
| 232 | +[bug-46064]: https://bugs.php.net/bug.php?id=46064 |
| 233 | +[commit-2e2b03e]: https://github.com/php/php-src/blob/2e2b03e1ceeef4cf8ba2a5e863ccc2622311dac6/ext/reflection/php_reflection.c |
| 234 | +[commit-a04ec69]: https://github.com/php/php-src/commit/a04ec69406aed8d695dc8fb1edac38d7bc8a30c7 |
| 235 | +[commit-b9c03aa]: https://github.com/php/php-src/commit/b9c03aa4a58820ac0c917ae247553d58e90672d9 |
| 236 | +[commit-ref-intro]: https://github.com/php/php-src/commit/aa96d170228ac1088c9008e78137d31e93c57f55 |
| 237 | +[docs-get-prop]: https://www.php.net/manual/en/reflectionclass.getproperty.php |
| 238 | +[docs-ref-prop]: https://www.php.net/manual/en/class.reflectionproperty.php |
| 239 | +[ext-php-rs]: https://ext-php.rs/ |
| 240 | +[gh-17730]: https://github.com/php/php-src/issues/17730 |
| 241 | +[gh-22441]: https://github.com/php/php-src/issues/22441 |
| 242 | +[gh-22564]: https://github.com/php/php-src/pull/22564 |
| 243 | +[ref-pre-bug]: https://github.com/php/php-src/blob/eeb02a0f14033ace9ac766028b14c29cda5e1cca/ext/reflection/php_reflection.c#L4619-L4678 |
| 244 | +[rfc-86-depr]: https://wiki.php.net/rfc/deprecations_php_8_6 |
0 commit comments