-
Notifications
You must be signed in to change notification settings - Fork 83
Add Asymmetric Visibility flags #250
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
nikic
merged 8 commits into
nikic:master
from
SergiiDolgushev:feat_asymmetric_visibility_flags
Jul 12, 2025
Merged
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
68e2620
Add Asymmetric Visibility flags
SergiiDolgushev 18b8637
Fix tests
SergiiDolgushev 381b82c
Review feedback
SergiiDolgushev d04a1d6
Added test
SergiiDolgushev 40ffe7b
Undefine new flags for <8.4
SergiiDolgushev 2f18242
Introduce ASYMMETRIC_VISIBILITY only for properties
SergiiDolgushev da56130
Introduce ASYMMETRIC_VISIBILITY only for properties #2
SergiiDolgushev 18ea5b5
Use non-zero and non-conflicting Asymmetric Visibility flag values fo…
SergiiDolgushev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| --TEST-- | ||
| Asymmetric Visibility in php 8.4 | ||
| --SKIPIF-- | ||
| <?php if (PHP_VERSION_ID < 80400) die('skip PHP >=8.4 only'); ?> | ||
| --FILE-- | ||
| <?php | ||
| require __DIR__ . '/../util.php'; | ||
| $code = <<<'PHP' | ||
| <?php | ||
| class PublicPropsWithAV | ||
| { | ||
| public public(set) int $p1 = 0; | ||
| public protected(set) int $p2 = 0; | ||
| public private(set) int $p3 = 0; | ||
| protected public(set) int $p4 = 0; | ||
| protected protected(set) int $p5 = 0; | ||
| protected private(set) int $p6 = 0; | ||
| private public(set) int $p7 = 0; | ||
| private protected(set) int $pp8 = 0; | ||
| private private(set) int $p9 = 0; | ||
| } | ||
| PHP; | ||
| $node = ast\parse_code($code, $version=110); | ||
| echo ast_dump($node), "\n"; | ||
| --EXPECTF-- | ||
| AST_STMT_LIST | ||
| 0: AST_CLASS | ||
| name: "PublicPropsWithAV" | ||
| docComment: null | ||
| extends: null | ||
| implements: null | ||
| stmts: AST_STMT_LIST | ||
| 0: AST_PROP_GROUP | ||
| flags: MODIFIER_PUBLIC | MODIFIER_PUBLIC_SET (1025) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "p1" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| 1: AST_PROP_GROUP | ||
| flags: MODIFIER_PUBLIC | MODIFIER_PROTECTED_SET (2049) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "p2" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| 2: AST_PROP_GROUP | ||
| flags: MODIFIER_PUBLIC | MODIFIER_PRIVATE_SET (4097) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "p3" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| 3: AST_PROP_GROUP | ||
| flags: MODIFIER_PROTECTED | MODIFIER_PUBLIC_SET (1026) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "p4" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| 4: AST_PROP_GROUP | ||
| flags: MODIFIER_PROTECTED | MODIFIER_PROTECTED_SET (2050) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "p5" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| 5: AST_PROP_GROUP | ||
| flags: MODIFIER_PROTECTED | MODIFIER_PRIVATE_SET (4098) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "p6" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| 6: AST_PROP_GROUP | ||
| flags: MODIFIER_PRIVATE | MODIFIER_PUBLIC_SET (1028) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "p7" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| 7: AST_PROP_GROUP | ||
| flags: MODIFIER_PRIVATE | MODIFIER_PROTECTED_SET (2052) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "pp8" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| 8: AST_PROP_GROUP | ||
| flags: MODIFIER_PRIVATE | MODIFIER_PRIVATE_SET (4100) | ||
| type: AST_TYPE | ||
| flags: TYPE_LONG (4) | ||
| props: AST_PROP_DECL | ||
| 0: AST_PROP_ELEM | ||
| name: "p9" | ||
| default: 0 | ||
| docComment: null | ||
| hooks: null | ||
| attributes: null | ||
| attributes: null | ||
| type: null | ||
| __declId: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Seems like in PHP 7.3
ZEND_ACC_PRIVATEwas1024: https://github.com/php/php-src/blob/89dc78e0f0c1a4f27b889f232b109a3919ecf478/Zend/zend_compile.h#L214. But for all later versions it was updated to4: https://github.com/php/php-src/blob/master/Zend/zend_compile.h#L220C9-L220C25.Because of that introducing
MODIFIER_PUBLIC_SETfor 7.2./7.3 leads to failed tests. Is there a simple way to introduce modifiers from this PR only for PHP8.4+?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.
@nikic seems like setting
0for the new flags (<8.4) fixes this. Can you please give it a look? And run tests one more time? I`ve run 7.3 and 8.4 tests locally, and it seemed fine.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 don't think it's a good idea to set these to zero. People can reasonably expect that the flags are all disjoint and non-zero. We should find values here that work (but don't have to match any specific PHP version).
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.
Sorry, I probably should be more clear in my initial comment. Please let me try doing better job at explaining my vision of the problem here:
1024value was used forZEND_ACC_PRIVATE, which is registered asMODIFIER_PRIVATEZEND_ACC_PRIVATEwas changed from1024to4, and after 7.4 there were no any flags with1024value.ZEND_ACC_PUBLIC_SETflag with1024value.Because of that, we can't use
1024value forMODIFIER_PUBLIC_SETfor <8.4 versions. As it conflicts withZEND_ACC_PRIVATEflag on <7.4.Setting
0value forMODIFIER_PUBLIC_SETflag for <8.4 seemed as simplest approach. As this flag (and other set modifiers) is not available in those PHP versions at all.Seems like you are suggesting to use non-zero but not-conflicting (non-existing) flag values for set modifiers for versions where they were unavailable?
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`ve just pushed 18ea5b5 and all local test runs were green for that change. @nikic can you please give it a look?