Skip to content

RFC: Static class #14861

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

RFC: Static class #14861

wants to merge 5 commits into from

Conversation

@Bilge
Copy link

@Bilge Bilge commented Jul 7, 2024 •
edited
Loading

This implements static classes as described in the RFC and supersedes #14583. Static classes are implemented with the following semantics:

  • Cannot be instantiated by any mechanism.
    • Cannot be instantiated with the new keyword.
    • Cannot be instantiated with unserialize().
    • Cannot be instantiated via reflection (e.g. new ReflectionClass()->newInstance()).
  • Mutually exclusive with the readonly modifier.
  • Cannot be applied to anonymous classes.
  • Supports inheritance, but only if the super/subclass is also marked static.
  • Supports abstract static classes.
  • Supports traits, but only if all their properties and methods are static.
  • Adds ReflectionClass::isStatic().

Thanks

  • Thanks to @sj-i and @iluuu1994 for implementation assistance.
  • Thanks to @oplanre for the initial implementation prototype.

@Bilge Bilge force-pushed the feature/static-class branch from e7a74bc to 15539e0 Compare July 7, 2024 15:11
@Bilge Bilge marked this pull request as ready for review July 7, 2024 15:27
@Bilge Bilge changed the title Added static classes RFC: Static class Jul 7, 2024
iluuu1994
iluuu1994 reviewed Jul 8, 2024
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

I'm not particularly in favor of this change, but the code mostly LGTM!

Zend/zend_API.c
static zend_always_inline zend_result _object_and_properties_init(zval *arg, zend_class_entry *class_type, HashTable *properties) /* {{{ */
{
if (UNEXPECTED(class_type->ce_flags & (ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT|ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS|ZEND_ACC_ENUM))) {
if (UNEXPECTED(class_type->ce_flags & (ZEND_ACC_STATIC|ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT|ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS|ZEND_ACC_ENUM))) {
Copy link
Member

@iluuu1994 iluuu1994 Jul 8, 2024

Choose a reason for hiding this comment

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

This is repeated often enough to where it probably makes sense to extract it into a new macro. E.g. ZEND_ACC_UNINSTANTIABLE_FLAGS.

Copy link
Author

@Bilge Bilge Jul 8, 2024

Choose a reason for hiding this comment

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

Would you perhaps agree this is best left for a separate refactoring PR?

@Bilge
Copy link
Author

Bilge commented Jul 8, 2024

@iluuu1994 Thanks so much for the (thorough) review!

About half the comments pertain to @oplanre's changes, so I am unable to comment on why they were made, but nevertheless I take ownership of them now, so I will make the requested changes.

Is it acceptable to rebase so changes not wanted can be as if they were never made, or should I add additional commits to revert where required?

@iluuu1994
Copy link
Member

iluuu1994 commented Jul 8, 2024

@Bilge Whichever you prefer. Commits can help for large PRs, but rebasing in this case seems fine.

@Bilge Bilge force-pushed the feature/static-class branch 2 times, most recently from caa6595 to ed7852b Compare July 8, 2024 21:02
oplanre and others added 4 commits July 8, 2024 22:06
@oplanre @Bilge
Implement static classes
8b13efc
@Bilge
Added tests for static class feature.
3ed2d03 
Added compile-time check to enforce all methods must be declared static in a static class.
Added compile-time mutual-exclusivity check for static classes marked with abstract or readonly modifiers.
Added compile-time check to preclude static class inheriting from non-static class and vice-versa.
Added compile-time check to preclude static class using a trait with non-static properties or methods.
Fixed ZEND_ACC_IMPLICIT_ABSTRACT_CLASS collision with ZEND_ACC_STATIC.
@Bilge
Added ReflectionClass::isStatic and accompanying test.
b6d0a30
@Bilge
Fix "readonly" tests relying on specific Bison output.
f6916fe 
Fix reflection test.
@Bilge Bilge force-pushed the feature/static-class branch 2 times, most recently from aaa7c9a to 4c0d0d6 Compare July 9, 2024 21:25
@Bilge
Removed mutual modifier restriction for abstract static class.
23d0272 
Improved a couple of static class tests per feedback.
@Bilge Bilge force-pushed the feature/static-class branch from 4c0d0d6 to 23d0272 Compare July 9, 2024 21:28
@Bilge Bilge requested a review from iluuu1994 July 9, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

@dstogov dstogov Awaiting requested review from dstogov dstogov is a code owner

@bukka bukka Awaiting requested review from bukka bukka is a code owner

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees

No one assigned

Labels

Category: Engine Category: Optimizer Extension: mysqli Extension: reflection Extension: tokenizer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Bilge @iluuu1994 @oplanre