Skip to content

Fix arginfo generation from stubs for namespaced functions #9406

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

Closed
wants to merge 5 commits into from

Conversation

alcaeus
Copy link
Contributor

@alcaeus alcaeus commented Aug 23, 2022

This PR fixes two issues:
The first commit ensures that the @deprecated and @alias annotations are respected for namespaced functions as well as for those without a namespace.
The second commit fixes an issue where having functions with the same name in multiple namespaces would generate two identical ZEND_FUNCTION entries. Note that this also significantly refactors the code used to generate function arginfo to reduce duplication.

Changes don't affect any existing arginfo files in core, as none of the default extensions use namespaced functions. However, the changes could cause issues in downstream code, namely if an extension uses stub files to generate arginfo for namespaces functions. I'm not sure how to best deal with this and would like some feedback as to what kind of BC breaks we can make in the build scripts.

Last but not least, there is one remaining edge case which I deliberately did not solve. Consider the following stub file:

namespace Foo {
    function Bar_baz(): void {}
}

namespace Foo\Bar {
    function baz(): void {}
}

In this case, we would once again create two identical ZEND_FUNCTION entries in the arginfo file. This could also happen with classes as long as they are defined in the same stub file, as the script always uses a single underscore to replace a backslash in the fully qualified name without treating the namespace and member name differently. However, due to the extensive refactoring involved to fix this as well as the low probability of that happening, I decided to blatantly ignore that issue for the time being.

Previously, if there were two funtions with the same name in different namespaces, the resulting arginfo file would be invalid.
@kocsismate
Copy link
Member

Regarding the issue with Foo\Bar\Baz and Foo\Bar_Baz: I also found this edge case, and I also decided not to fix it: c345b6e#r71259703

@alcaeus
Copy link
Contributor Author

alcaeus commented Aug 23, 2022

Yes - barring using characters not allowed in function names there will always be the chance of conflict. Not worth it IMO 🤷‍♂️

Copy link
Member

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

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

I think this a very nice and useful fix. The BC break is unfortunate, but it's for the good, and probably it's not difficult to address in other 3rd party code.

@jmikola
Copy link
Member

jmikola commented Aug 24, 2022

I think this a very nice and useful fix. The BC break is unfortunate, but it's for the good, and probably it's not difficult to address in other 3rd party code.

Does this PR warrant an update to UPGRADING.INTERNALS, or is that typically done later on and closer to release?

@kocsismate
Copy link
Member

Does this PR warrant an update to UPGRADING.INTERNALS, or is that typically done later on and closer to release?

Yes, it does, and the UPGRADING.INTERNALS file is updated continuously.

@kocsismate kocsismate requested review from cmb69 and Girgias August 25, 2022 07:19
@kocsismate
Copy link
Member

kocsismate commented Aug 25, 2022

I'll wait with the merge for the opinion of other reviewers until next Tuesday the latest (before RC1).

@kocsismate kocsismate requested a review from bwoebi August 25, 2022 07:21
@alcaeus
Copy link
Contributor Author

alcaeus commented Aug 25, 2022

Yes, it does, and the UPGRADING.INTERNALS file is updated continuously.

Added the following upgrade note:

Identifier names for namespaced functions generated from stub files through gen_stub.php have been changed. This will require changes in extensions using namespaced funtions.

Any wording suggestions are highly appreciated.

@kocsismate
Copy link
Member

Added the following upgrade note:

FYI: generally, any upgrading note is added by the one who applies the commit in order to prevent conflicts due to simultaneous updates. I think though that any other change in the upgrading file is highly unlikely at this point, so I'm fine with having it in the PR :)

@alcaeus
Copy link
Contributor Author

alcaeus commented Aug 25, 2022

FYI: generally, any upgrading note is added by the one who applies the commit in order to prevent conflicts due to simultaneous updates.

TIL - I'll consider it target practice should I find myself in that situation at some point in time ;)

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The implementation looks good, and apparently this is a necessary evil; still might cause quite some PITA for extension maintainers.

@remicollet may want to review (although he's AFK, AFAIK).

Co-authored-by: Christoph M. Becker <[email protected]>
@kocsismate
Copy link
Member

I think though that any other change in the upgrading file is highly unlikely at this point, so I'm fine with having it in the PR

Reality has just kicked in :D so please fix the conflict.

@@ -62,6 +62,10 @@ PHP 8.2 INTERNALS UPGRADE NOTES
2. Build system changes
========================

* Identifier names for namespaced functions generated from stub files through
Copy link
Member

Choose a reason for hiding this comment

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

I fixed the conflict and slightly adjusted the wording. Feel free to change it should you find any issues with it.

@@ -109,6 +109,15 @@ function zend_test_compile_string(string $source_string, string $filename, int $
/** @deprecated */
function zend_test_deprecated(mixed $arg = null): void {}

/** @alias zend_test_alias */
Copy link
Member

Choose a reason for hiding this comment

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

There was a problem with the aliases. If you run ./build/gen_stub.php --verify --force-regeneration then gen_stub.php complains about the inexistent alias functions (zend_test_alias, zend_test_deprecated_alias etc.). So I fixed them in ef21bbe

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.

5 participants