-
Notifications
You must be signed in to change notification settings - Fork 7.9k
adds support for DNF types in internal functions and properties #11969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the approach makes sense!
But I do think having a zend_string*
name be mutually exclusive with a char*
name is better and less error prone in the long run even if it means possibly some more intrusive work atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only one mistake I can spot currently, maybe add a union type to the ZendTestClass stub to pick that one up?
ext/zend_test/test.c
Outdated
// we have to perform type-checking to avoid arginfo/zpp mismatch error, | ||
bool type_matches = ( | ||
instanceof_function(obj->ce, zend_ce_iterator) | ||
|| ( | ||
instanceof_function(obj->ce, zend_ce_traversable) | ||
&& instanceof_function(obj->ce, zend_ce_countable) | ||
) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it makes sense to provide an API to parse a param from a zend_type, but that's unrelated to this. :)
This comment was marked as resolved.
This comment was marked as resolved.
Right, so we need to add a method which takes a union of two classes then :) |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now
Ilija found where properties types were not being properly copied, and I have amended #11961 So it might make more sense for me to clean-up that PR for PHP 8.2, and then rebase on top of that fix to get rid of the weird hacks? |
Sure, I'll happily rebase and remove these memory cleanup workarounds! |
Fixes are merged |
Not that this does not add support for items generated by gen_stubs, only for items registered dynamically via the zend API. Closes php#10120
This allows tests to pass ASAN
63d2b67
to
d948c74
Compare
Doesn't matter much in practice but we are good citizens.
Thanks @Girgias ! 🎉 |
DISCLAIMER: this PR does not add support for DNF types codegen in
gen_stubs.php
.Currently, the routines for registering functions/methods and typed properties make assertions that cause valid
zend_type
s to be incorrectly rejected (see #10120).These assertions are related to the current limitations of the
gen_stubs.php
script.This results in the zend API having a de-facto hard-dependency on the the
gen_stubs.php
codegen tool, thus rendering it unusable when using said tool is not possible or desirable (for example when writing extensions in languages like Rust or Zig that have native C-FFI support but cannot make use of complex preprocessor macros).This pull-requests lifts those limitations while keeping
gen_stub.php
-specific behaviour.Closes #10120.
Note that tests fail under ASAN because we're hitting #11883 in the added test cases.