Skip to content

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

Merged
merged 14 commits into from
Aug 18, 2023

Conversation

ju1ius
Copy link
Contributor

@ju1ius ju1ius commented Aug 14, 2023

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

Copy link
Member

@Girgias Girgias left a 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

@Girgias Girgias requested a review from kocsismate August 14, 2023 14:44
Copy link
Member

@Girgias Girgias left a 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?

Comment on lines 929 to 958
// 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)
)
);
Copy link
Member

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

@ju1ius

This comment was marked as resolved.

@Girgias
Copy link
Member

Girgias commented Aug 14, 2023

EDIT: ah no, to hit the code path in zend_register_functions we need a function or method with a union type argument w/ two or more class names, and that is not covered AFAICT.

Right, so we need to add a method which takes a union of two classes then :)

@ju1ius

This comment was marked as resolved.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now

@Girgias
Copy link
Member

Girgias commented Aug 14, 2023

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?

@ju1ius
Copy link
Contributor Author

ju1ius commented Aug 14, 2023

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!

@Girgias
Copy link
Member

Girgias commented Aug 15, 2023

Fixes are merged

@ju1ius ju1ius force-pushed the ju1ius/internal-dnf-types branch from 63d2b67 to d948c74 Compare August 15, 2023 17:06
@Girgias Girgias merged commit 7f1c3bf into php:master Aug 18, 2023
@ju1ius
Copy link
Contributor Author

ju1ius commented Aug 19, 2023

Thanks @Girgias ! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DNF types support for internal functions
3 participants