Rfc: inner class #2
Conversation
this adds all the initial opcode handling and special parts -- or the basics anyway.
WalkthroughThis pull request introduces comprehensive support for nested (inner) classes in PHP. The changes add a new AST node, structure members for managing class scopes, updated compiler routines and opcode handling, and new token definitions for inner class references. Enhancements to visibility checks and reflection capabilities are implemented, with updates to error messages for inner class declarations. A broad suite of tests has also been added to verify syntax, access modifiers, autoloading, inheritance, return types, and other behaviors of nested classes. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Compiler
participant InnerHandler as "Inner Class Handler"
participant Resolver as "Scope Resolver"
Parser->>Compiler: Parse inner class declaration (T_INNER_REF)
Compiler->>InnerHandler: Invoke zend_compile_class_decl
InnerHandler->>Resolver: Call zend_resolve_nested_class_name
Resolver-->>InnerHandler: Return fully qualified name
InnerHandler-->>Compiler: Return compiled inner class reference
Compiler->>Runtime: Pass inner class for execution
sequenceDiagram
participant OuterClass
participant Autoloader
participant FileSystem
OuterClass->>Autoloader: Request inner class (e.g., Outer:>Point)
Autoloader->>FileSystem: Load corresponding file (.inc)
FileSystem-->>Autoloader: Return file contents
Autoloader-->>OuterClass: Deliver loaded inner class
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
tests/classes/inner_classes/inheritance.phpt (1)
1-15: 🛠️ Refactor suggestionMissing assertions in inheritance test
While this test correctly demonstrates the syntax for inner class inheritance patterns, it lacks any verification logic. The
--EXPECT--section is empty, suggesting the test only verifies that the code compiles without errors, but doesn't validate the actual inheritance behavior.Consider adding assertions to verify:
- That instances of these classes can be created
- That type checking works as expected with the inheritance relationships
- That method overriding and inheritance behavior functions correctly
🧹 Nitpick comments (14)
tests/classes/inner_classes/simple_declaration_002.phpt (1)
1-14: Test correctly verifies that class declarations are not allowed inside functions.This test case properly validates that PHP should not allow class declarations inside functions. The test structure follows the standard phpt format with clear TEST, FILE, and EXPECTF sections.
One minor grammatical suggestion for the error message:
-Fatal error: Class declarations may not be declared inside functions in %s on line %d +Fatal error: Classes may not be declared inside functions in %s on line %dThe current error message has a redundancy with "Class declarations" and "may not be declared".
tests/classes/inner_classes/access_modifiers_005.phpt (1)
12-14: Grammar issue in error message.There's a minor grammatical error in the expected error message.
-Fatal error: Cannot use the protected(set) modifier on a inner class in %s on line %d +Fatal error: Cannot use the protected(set) modifier on an inner class in %s on line %dThe article "a" should be "an" before "inner" since "inner" starts with a vowel sound.
tests/classes/inner_classes/access_modifiers_006.phpt (1)
12-14: Grammar issue in error message.There's a minor grammatical error in the expected error message.
-Fatal error: Cannot use the private(set) modifier on a inner class in %s on line %d +Fatal error: Cannot use the private(set) modifier on an inner class in %s on line %dThe article "a" should be "an" before "inner" since "inner" starts with a vowel sound.
Zend/zend_compile.c (2)
1797-1829: Confirm Naming Delimiter for Nested Classes.The
zend_resolve_nested_class_name()function combines the outer and inner names using the custom delimiter":>". This approach is fine if consistently supported throughout reflection and autoloading. However, please confirm that no external tooling or naming expectations conflict with the chosen":>"delimiter.
9181-9220: Renaming Classes Declared Inside Other Classes.This logic constructs the nested class’s final name, preserving certain modifiers (final, abstract, readonly) and stripping out public/protected/private. Make sure the rename does not collide with any existing qualification (e.g., namespaces) and that the deduced
required_scopeis tested thoroughly.tests/classes/inner_classes/visibility_003.phpt (1)
2-2: The test title might be misleading.
You're testing a private property rather than a protected one. Consider adjusting the test name or switching the property to protected to match the description.Zend/zend_object_handlers.c (2)
1840-1843: Scope re-evaluation logic.
Reassigningscope = scope->lexical_scopeand jumping back tocheck_lexical_scopecan help handle deeper nesting, but confirm it won’t cause infinite loops ifscope->lexical_scopeis cyclical or unset.
1903-1903: Another lexical scope checkpoint.
This label is repeated further down, unifying the flow across two distinct code regions. Keep it well-documented given the complexity of scope transitions.ext/reflection/php_reflection.c (1)
4079-4093: Implementation for detecting inner classes could be more robustThe implementation uses a simple string search for ":>" to determine if a class is an inner class. This seems brittle and the todo comment suggests this is a known issue. Consider implementing a dedicated flag in the zend_class_entry structure instead of relying on string pattern matching.
- // If the class is an inner class, it will have a T_INNER_REF in its name - // todo: make this better? - - RETURN_BOOL(strstr(ZSTR_VAL(ce->name), ":>") != NULL); + // Check if the class is marked as an inner class using a dedicated flag + RETURN_BOOL(ce->ce_flags & SOME_INNER_CLASS_FLAG);Zend/zend_language_parser.y (1)
280-281: Extended grammar rules for nested classes & types.
The new rules (inner_class_modifiers,inner_type_without_static, etc.) and updates totype_without_staticare coherent. Verify edge cases for nested classes declared within different scopes to avoid ambiguities.Consider adding explicit error messages if
inner_class_modifiersare misused, to improve developer feedback.Also applies to: 630-637, 881-885
Zend/zend_execute.c (1)
1048-1079: Address the typographical errors and clarify bitwise checks.
- In the error messages, "visibile" should be corrected to "visible" for clarity and correctness.
- To avoid confusion with bitwise operations on flags, consider wrapping
current_visibility & ZEND_ACC_PRIVATEin parentheses and ensuring only the PPP bits are checked (e.g.,(info->flags & ZEND_ACC_PPP_MASK)). This makes the condition more explicit and robust.Here is a diff to fix the spelling issues in the error messages:
- zend_type_error("Cannot assign protected %s to higher visibile property %s::%s", + zend_type_error("Cannot assign protected %s to higher visible property %s::%s", - zend_type_error("Cannot assign private %s to higher visibile property %s::%s", + zend_type_error("Cannot assign private %s to higher visible property %s::%s",Zend/zend_vm_execute.h (3)
6614-6710: Consider reducing code duplication.
This handler’s structure and logic (outer class resolution, scope checks, error handling) closely mirror those in the otherZEND_FETCH_INNER_CLASShandlers. Extracting shared logic into a helper function or macro could improve maintainability.
16246-16342: Consider reducing code duplication.
This handler largely repeats the pattern in theCONST_CONSTversion. A shared helper to retrieve and validate the outer and inner class would avoid scattering the same logic.
33996-34092: Consider reducing code duplication.
As with the other inner-class fetch handlers, you may want to unify common code paths for obtaining and validatingouter_ce/inner_ce.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
Zend/tests/errmsg/errmsg_027.phpt(1 hunks)Zend/zend.h(1 hunks)Zend/zend_ast.h(1 hunks)Zend/zend_compile.c(10 hunks)Zend/zend_compile.h(1 hunks)Zend/zend_execute.c(2 hunks)Zend/zend_language_parser.y(8 hunks)Zend/zend_language_scanner.l(1 hunks)Zend/zend_object_handlers.c(4 hunks)Zend/zend_opcode.c(1 hunks)Zend/zend_vm_def.h(3 hunks)Zend/zend_vm_execute.h(24 hunks)Zend/zend_vm_handlers.h(1 hunks)Zend/zend_vm_opcodes.c(3 hunks)Zend/zend_vm_opcodes.h(1 hunks)ext/opcache/zend_persist.c(2 hunks)ext/reflection/php_reflection.c(1 hunks)ext/reflection/php_reflection.stub.php(1 hunks)ext/reflection/php_reflection_arginfo.h(4 hunks)ext/reflection/tests/ReflectionClass_toString_001.phpt(2 hunks)ext/reflection/tests/bug74454.phpt(1 hunks)ext/tokenizer/tokenizer_data.c(1 hunks)ext/tokenizer/tokenizer_data.stub.php(1 hunks)ext/tokenizer/tokenizer_data_arginfo.h(2 hunks)tests/classes/inner_classes/access_modifiers_001.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_002.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_003.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_004.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_005.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_006.phpt(1 hunks)tests/classes/inner_classes/access_modifiers_007.phpt(1 hunks)tests/classes/inner_classes/autoload_001.phpt(1 hunks)tests/classes/inner_classes/autoload_002.phpt(1 hunks)tests/classes/inner_classes/errors_001.phpt(1 hunks)tests/classes/inner_classes/errors_002.phpt(1 hunks)tests/classes/inner_classes/inheritance.phpt(1 hunks)tests/classes/inner_classes/inner_classes.inc(1 hunks)tests/classes/inner_classes/readonly_001.phpt(1 hunks)tests/classes/inner_classes/reflection_001.phpt(1 hunks)tests/classes/inner_classes/return_types_001.phpt(1 hunks)tests/classes/inner_classes/return_types_002.phpt(1 hunks)tests/classes/inner_classes/return_types_003.phpt(1 hunks)tests/classes/inner_classes/return_types_004.phpt(1 hunks)tests/classes/inner_classes/return_types_005.phpt(1 hunks)tests/classes/inner_classes/return_types_006.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_001.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_002.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_003.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_004.phpt(1 hunks)tests/classes/inner_classes/static_variables.phpt(1 hunks)tests/classes/inner_classes/visibility_001.phpt(1 hunks)tests/classes/inner_classes/visibility_002.phpt(1 hunks)tests/classes/inner_classes/visibility_003.phpt(1 hunks)tests/classes/inner_classes/visibility_004.phpt(1 hunks)tests/classes/inner_classes/visibility_005.phpt(1 hunks)tests/classes/inner_classes/visibility_006.phpt(1 hunks)tests/classes/inner_classes/visibility_007.phpt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (138)
ext/tokenizer/tokenizer_data.stub.php (1)
715-719: New token constant added for inner class referencesThe addition of the
T_INNER_REFconstant is well-structured and consistent with other token definitions in the file. This token will be essential for lexing inner class references in PHP code.Zend/tests/errmsg/errmsg_027.phpt (1)
16-16: Error message clarification for nested class declarationsThe error message has been updated to be more specific, indicating that class declarations are not allowed inside functions rather than using the more general "may not be nested" wording. This aligns well with the inner classes feature being implemented.
tests/classes/inner_classes/access_modifiers_003.phpt (1)
1-14: Well-structured test for static modifier on inner classesThis test appropriately verifies that the static modifier cannot be applied to inner classes. The test follows proper PHP test format with clear sections for test description, code, and expected output.
ext/tokenizer/tokenizer_data.c (1)
170-170: Added token mapping for T_INNER_REFThe case statement for
T_INNER_REFhas been correctly added to theget_token_type_namefunction, ensuring that the new token type can be properly represented as a string during tokenization.tests/classes/inner_classes/simple_declaration_001.phpt (3)
6-10: Core inner class declaration looks good.The nested class structure is clear and demonstrates the feature's basic syntax correctly. The test successfully defines a three-level nesting with Outer, Middle, and Inner classes.
12-15: New inner class reference syntax is being tested correctly.The test verifies both string-based references ('Outer:>Middle') and constant-based references (Outer:>Middle::class) for inner classes. This ensures that both ways of referring to inner classes work as expected.
Note: The
:>operator appears to be the new syntax for referencing inner classes, which is a key part of this feature.
17-22: Expected output accurately reflects the desired behavior.The test expects all class_exists() calls to return true, confirming that the inner classes are properly registered and accessible through both string and constant references.
tests/classes/inner_classes/access_modifiers_005.phpt (1)
6-9: Test correctly verifies prohibited protected(set) modifier on inner classes.The test appropriately checks that the protected(set) modifier cannot be used on inner classes.
tests/classes/inner_classes/access_modifiers_006.phpt (1)
6-9: Test correctly verifies prohibited private(set) modifier on inner classes.The test appropriately checks that the private(set) modifier cannot be used on inner classes.
Zend/zend_language_scanner.l (1)
1604-1606: LGTM: Token definition for inner class references.The addition of the
T_INNER_REFtoken for recognizing the:>syntax is well-implemented and follows the same pattern as other token definitions. This new token enables the parser to identify references to inner classes using theOuterClass:>InnerClasssyntax.tests/classes/inner_classes/autoload_002.phpt (3)
5-8: LGTM: Autoloader function implementation.This autoloader correctly loads class files based on the class name and outputs a notification message, following standard autoloading practices.
14-16: LGTM: Properly verifying access error for private inner class.The test correctly expects that accessing a private inner class from outside its containing class should result in a fatal error, which is consistent with PHP's access control rules.
10-11:✅ Verification successful
Verify that the referenced include file exists.
The test attempts to instantiate an inner class
inner_classes:>Line, but there's no verification that the required include file exists. If the file is missing, the test will fail with a different error than expected.Run the following script to check if the required include file exists:
🏁 Script executed:
#!/bin/bash # Check if the inner_classes.inc file exists in the test directory if [ -f "tests/classes/inner_classes/inner_classes.inc" ]; then echo "File exists: inner_classes.inc" else echo "File missing: inner_classes.inc" echo "This test may fail if the file doesn't exist" fiLength of output: 129
Include file verification confirmed
The verification script confirms that the filetests/classes/inner_classes/inner_classes.incexists. No changes are required regarding the include file reference in the test, as the necessary dependency is present.ext/reflection/tests/bug74454.phpt (1)
17-17:✅ Verification successful
Updated parser error message for consistency with inner class changes.
The error message has been updated from "expecting 'function'" to "expecting 'class'", which aligns with the changes to the parser to support inner class declarations. This change maintains test consistency with the updated parser behavior.
Run the following script to examine the content of the include file to better understand the context of this error:
🏁 Script executed:
#!/bin/bash # Examine the content of bug74454.inc to understand the parsing context if [ -f "ext/reflection/tests/bug74454.inc" ]; then echo "Content of bug74454.inc:" cat "ext/reflection/tests/bug74454.inc" else echo "File missing: bug74454.inc" fiLength of output: 184
Parser error message update verified
The file
ext/reflection/tests/bug74454.incnow intentionally contains code with an erroneous inner class syntax (e.g. anifstatement within a class declaration), which correctly triggers the updated error message expecting a"class". This change is consistent with the inner class declaration support changes in the parser.
- The error message is updated from
"expecting 'function'"to"expecting 'class'"as verified in the test file output.- The test file
bug74454.incconfirms that the parser now correctly reports the syntax error in context.tests/classes/inner_classes/access_modifiers_001.phpt (2)
6-9: LGTM: Testing multiple access modifiers restriction.This test correctly verifies that PHP enforces the rule that a class (including inner classes) cannot have multiple access modifiers. Using both
publicandprivateshould result in a fatal error.
13-13: LGTM: Properly verifying error message for multiple access modifiers.The expected error message correctly identifies the issue of multiple access modifiers not being allowed, which is consistent with PHP's language rules.
ext/tokenizer/tokenizer_data_arginfo.h (2)
2-2: Correct stub hash updateThe stub hash has been updated to reflect the changes in the generated file's content. This is expected when introducing new tokens.
148-148: Good addition of T_INNER_REF tokenThis new constant registration adds the T_INNER_REF token to the tokenizer's data symbols, which is necessary for supporting inner class references with the ':>' syntax. The placement is appropriate in the alphabetical sequence of tokens.
tests/classes/inner_classes/errors_001.phpt (1)
1-10: Good test case for non-existent outer class errorThis test correctly validates error handling when attempting to instantiate an inner class from a non-existent outer class. The expected error message is clear and provides appropriate information to developers.
tests/classes/inner_classes/errors_002.phpt (1)
1-13: Good test case for non-existent inner class errorThis test complements errors_001.phpt by validating the error condition when the outer class exists but the specified inner class doesn't. Together, these tests ensure robust error handling for both missing outer class and missing inner class scenarios.
tests/classes/inner_classes/readonly_001.phpt (2)
1-24: Well-designed test for readonly properties in inner classesThis test correctly validates that readonly properties work as expected in inner classes. It demonstrates:
- The syntax for declaring a readonly inner class
- Proper initialization of properties through the constructor
- Appropriate error handling when attempting to modify readonly properties
- Correct formatting of inner class names in error messages (Outer:>Inner::$t)
The test ensures that inner classes maintain consistent language semantics with regular classes regarding readonly properties.
7-7: Clean compact syntax for inner class declarationThe inner class is declared using a concise one-line syntax that includes visibility modifiers, the readonly keyword, and a constructor with a property promotion. This demonstrates that inner classes fully support modern PHP syntax features.
tests/classes/inner_classes/access_modifiers_002.phpt (1)
1-14: LGTM - Test case correctly verifies invalid inner class syntaxThis test properly validates that the PHP parser correctly identifies and reports syntax errors when using an invalid construct like
int class Inner. The expected parse error is appropriately defined.tests/classes/inner_classes/access_modifiers_004.phpt (1)
1-14: LGTM - Test case correctly verifies invalid access modifierThis test properly validates that the PHP interpreter correctly issues a fatal error when the
public(set)access modifier is applied to an inner class. The expected error message is appropriately defined.tests/classes/inner_classes/simple_declaration_003.phpt (2)
6-17: LGTM - Nested class implementation looks goodThe implementation of multi-level nested classes with proper namespace support works correctly. The test demonstrates three-level nesting (Outer > Middle > Inner) and shows that class names are properly qualified.
17-17: Note the new:>operator for inner class accessThe syntax for instantiating an inner class introduces a new scope resolution operator (
:>) which differs from the standard::operator used for static access. This clearly differentiates inner class access from static method/property access.tests/classes/inner_classes/reflection_001.phpt (2)
6-12: LGTM - Nested class declaration looks goodThe implementation of nested classes within a namespace is correctly structured.
14-22: LGTM - Reflection API properly supports inner classesThe test correctly validates that the Reflection API has been extended to support inner classes with new methods like
isInnerClass()and proper handling of visibility checks and class names. The test demonstrates that:
getName()returns the fully qualified name including namespace and inner class pathgetShortName()returns the inner class path without namespaceisInnerClass()correctly identifies inner vs. outer classes- Visibility checks (
isPrivate(),isProtected(),isPublic()) work properly for inner classestests/classes/inner_classes/autoload_001.phpt (2)
1-16: Well-structured test for inner class autoloadingThis test effectively verifies that PHP's autoloading mechanism works correctly with inner classes. The autoloader properly loads the parent class file, and the inner class can be instantiated using the new
:>syntax.
10-10:❓ Verification inconclusive
Confirm that this syntax is documented
The
:>syntax for referencing inner classes is used consistently throughout the tests, but it's worth verifying that this syntax is properly documented for users.
🏁 Script executed:
#!/bin/bash # Check if the syntax is documented in any README or documentation files grep -r ":>" --include="*.md" --include="README*" .Length of output: 53
Action Required: Verify Documentation for
:>SyntaxOur initial search for the
:>syntax in common documentation files (e.g., README and Markdown files) did not return any results. Please confirm whether the inner class referencing syntax is documented in another location or if it was intentionally omitted. If it's not documented, consider adding an entry to the documentation so that users can understand its usage.
- Verify additional documentation sources (e.g., .rst files or a dedicated docs folder) for references to
:>.- If no documentation is found, update the docs to include information on the inner class syntax.
tests/classes/inner_classes/return_types_001.phpt (2)
1-19: Effective test for inner class return typesThis test properly verifies that inner classes can be used as return types and that the compiler correctly handles these types.
8-10: Return type syntax is consistent with the class reference syntaxThe return type declaration and instantiation syntax both use the
:>delimiter for referencing inner classes, which maintains consistency.tests/classes/inner_classes/inner_classes.inc (2)
3-9: Good implementation of inner classes with access modifiersThe implementation shows both public and private inner classes, along with modern PHP features like constructor property promotion.
4-6: Constructor properly initializes public propertiesThe
Pointclass uses constructor property promotion, which is a nice touch showing that inner classes fully support modern PHP features.tests/classes/inner_classes/visibility_006.phpt (3)
6-20: Comprehensive test for visibility scope boundariesThis test correctly verifies that nested inner classes don't inherit access to private methods from their outer classes, which is the expected behavior.
15-17: Properly tests method visibility boundariesThe
testit()method appropriately attempts to call a method that should not be visible from its scope, verifying that enclosing class private methods aren't accessible.
21-21: Consistent syntax for instantiating deeply nested classesThe chained
:>syntax for instantiating a deeply nested inner class is used consistently with other inner class references.tests/classes/inner_classes/visibility_001.phpt (1)
1-26: Well-structured test for inner class visibilityThis test effectively verifies a critical constraint of the inner class implementation: a private inner class cannot be assigned to a property with higher visibility (public). The test correctly expects a TypeError to be thrown when attempting to assign a private
Outer:>Innerinstance to the public$illegalproperty.The syntax
Outer:>Innerappears to be the new notation for referencing inner classes, which is consistent with the PR's objective of implementing inner class support.Zend/zend_ast.h (1)
136-136: Appropriate AST node addition for inner classesAdding
ZEND_AST_INNER_CLASSto the enum is necessary for the compiler to recognize and process inner class declarations. The placement is appropriate, as it's added among other two-child AST node types (afterZEND_AST_ASSIGN_COALESCE).This clean addition enables the parser to build the correct AST representation for inner classes, which is essential for the feature to work.
Zend/zend_vm_def.h (2)
291-292: Addition of ZEND_FETCH_INNER_CLASS to opcode listThe opcode is properly registered in the opcode list with the number 210, maintaining the sequential ordering of opcodes.
8290-8291: Integration with existing opcode handlingThe inner class fetch opcode is properly integrated with the existing opcode handling infrastructure, including proper handling of class fetching operations.
Zend/zend.h (1)
167-170: New members added to class_entry for inner class support.These three new members have been added to the
_zend_class_entrystructure to support inner classes:
required_scope: Points to the class scope required to access this classlexical_scope: Tracks the lexical context where the class was definedrequired_scope_absolute: Indicates if the required scope is absoluteThis implementation enables proper handling of nested class visibility and scope resolution.
Zend/zend_compile.h (1)
897-898: Added enum value for inner class targets.The new
ZEND_MODIFIER_TARGET_INNER_CLASSvalue extends thezend_modifier_targetenum to support inner classes, allowing the compiler to differentiate access modifiers applied to inner classes from those applied to methods, properties, and constants.tests/classes/inner_classes/return_types_005.phpt (2)
1-27: Test case validates protected return type visibility constraints.This test ensures that a public method cannot return a protected inner class type, appropriately triggering a type error. The test explicitly verifies that PHP enforces visibility constraints on return types for inner classes.
9-10:❓ Verification inconclusive
Syntax error in inner class reference.
The syntax for inner class reference uses
:>instead of the standard PHP scope resolution operator::. This appears to be intentional for testing an alternative syntax for inner class access, but should be checked for consistency with the implementation.
🏁 Script executed:
#!/bin/bash # Check if there are other test files using this syntax grep -r ":>" tests/classes/inner_classes/ --include="*.phpt" | head -n 10Length of output: 1036
Action Required: Confirm That the Alternative Inner Class Syntax Is Intentional
After verifying the codebase, we see that the alternative inner class operator
:>is used consistently across multiple test files (e.g., in access_modifiers_007.phpt, autoload_001.phpt, etc.). This indicates that the usage in return_types_005.phpt is likely intentional rather than an error. However, please confirm that the implementation is aligned with using:>for inner class references. If the design calls for supporting this experimental syntax, no change is needed; otherwise, consider updating the tests to use the standard::operator.
- Location Affected:
tests/classes/inner_classes/return_types_005.phpt(Lines 9-10)Zend/zend_vm_opcodes.h (1)
294-296: Added new opcode for inner class functionality.A new opcode
ZEND_FETCH_INNER_CLASShas been defined to support fetching inner classes. TheZEND_VM_LAST_OPCODEvalue has been updated accordingly to reflect this addition. This is essential for the VM to handle inner class references during execution.tests/classes/inner_classes/visibility_004.phpt (3)
6-13: Good test case for visibility constraints on inner classes.The test correctly verifies that a protected inner class cannot be assigned to a public property, enforcing proper visibility rules.
15-18: The test properly triggers the expected error.This sequence correctly instantiates the class and triggers the visibility constraint violation.
21-25: Expected output correctly defines the error condition.The error message accurately reflects that a protected inner class cannot be assigned to a higher visibility property.
tests/classes/inner_classes/return_types_004.phpt (3)
6-12: Good test case for visibility constraints on return types.The test correctly sets up a private inner class and a public method that attempts to return it, which should trigger a visibility constraint error.
14-19: Test sequence properly triggers the visibility constraint.The code correctly tests both the method call and attempting to use the return value, which should fail due to visibility constraints.
22-26: Expected output correctly defines the error condition.The error message accurately reflects that a public method cannot return a private class type.
ext/opcache/zend_persist.c (2)
1124-1143: Good implementation of scope handling for inner classes in OpCache.The
zend_update_required_scopefunction properly updates bothrequired_scopeandlexical_scopereferences when persisting class entries. This ensures that inner class relationships are maintained correctly when classes are cached.
1318-1318: Proper integration of scope updating in the class persistence flow.The call to
zend_update_required_scopeis correctly placed after parent class updates, ensuring that all class relationships are consistently maintained.tests/classes/inner_classes/return_types_003.phpt (4)
6-12: Good test case for inheritance and visibility of inner classes.The test properly defines an outer class with a protected inner class and a protected method that returns the inner class type.
14-19: The subclass correctly tests visibility constraints.Foo extends Outer and overrides getInner with public visibility, creating a visibility mismatch with the protected return type inherited from the parent.
21-23: Test triggers the visibility constraint appropriately.This sequence correctly shows that accessing the inner class is possible from a subclass method, but the visibility mismatch in the return type declaration will trigger an error.
26-33: Expected output correctly defines the test behavior.The output shows that parent::getInner() works correctly within the subclass method, but the method itself fails due to the visibility mismatch between the public method and protected return type.
tests/classes/inner_classes/access_modifiers_007.phpt (3)
6-8: Looks good - the abstract inner class declaration is correctly implemented.This test properly demonstrates the declaration of an abstract inner class within the Outer class. The syntax is clean and in line with PHP's typical class declaration style.
10-10: Correct implementation of extending an inner class.The syntax for referencing and extending an inner class uses the
:>operator, which is a clear and intuitive way to denote the relationship between outer and inner classes.
12-17: Good test coverage of abstract inner class behavior.This test correctly verifies multiple aspects of abstract inner classes:
- Creating a concrete implementation of the abstract inner class
- Using reflection to confirm the class is abstract
- Verifying the expected error when attempting to instantiate the abstract class directly
This ensures the abstract modifier works properly with inner classes.
tests/classes/inner_classes/visibility_002.phpt (3)
6-12: Correctly implements private inner class with proper scoping rules.The test demonstrates that inner classes follow expected visibility patterns, with the private inner class being accessible only within the outer class scope. The inner class can access private members of the outer class, which is consistent with typical OOP encapsulation principles.
14-16: Good test for inner class instantiation within the outer class context.The test shows how inner classes are instantiated from within the outer class context, using the
Outer:>Innersyntax. This is a clean approach that clearly indicates the relationship between the classes.
12-12: Appropriate type declaration for inner class property.The use of a qualified inner class type for the property declaration (
private Outer:>Inner $illegal;) demonstrates the proper way to reference inner class types, which is consistent with the overall syntax for inner classes.ext/reflection/php_reflection_arginfo.h (3)
369-370: Good addition of isInnerClass reflection method.Adding the
isInnerClassmethod to the ReflectionClass API provides a necessary way to identify inner classes programmatically. This maintains consistency with other similar reflection methods and follows the established pattern for boolean reflection methods.
371-376: Appropriate visibility check methods for reflection API.The addition of
isPrivate,isProtected, andisPublicmethods to ReflectionClass is consistent with the reflection API design, matching similar methods already present on ReflectionMethod and ReflectionProperty. This completes the reflection API to fully support visibility of inner classes.
1154-1157: Proper registration of new reflection methods.The implementation correctly registers the new methods in the ReflectionClass function table, following the existing pattern for method registration in the extension. This ensures the methods are accessible through the reflection API.
ext/reflection/php_reflection.stub.php (1)
436-442: Well-defined reflection API for inner class capabilities.The addition of these four methods (
isInnerClass,isPrivate,isProtected, andisPublic) to the ReflectionClass stub provides a complete API for working with inner classes in PHP. The method signatures are consistent with the rest of the reflection API, returning boolean values for class characteristic checks.Zend/zend_opcode.c (1)
433-436: Good addition for proper reference counting and memory management.The added code properly decrements the reference count for the
required_scopemember when a class is destroyed, preventing potential memory leaks in the context of inner classes. This change is consistent with PHP's memory management patterns.tests/classes/inner_classes/return_types_006.phpt (5)
6-14: Good test case for inner class access patterns.This test correctly verifies that a private inner class method can return instances of its own class, and that the outer class can access and return these instances. The use of both
Outer:>PrivateInnerandself:>PrivateInnersyntax demonstrates different ways to reference the inner class.
13-13: The method correctly usesmixedas the return type.Using the
mixedreturn type is appropriate here since it allows for returning an instance of a private inner class that can't be directly referenced in the method signature. This demonstrates proper type hinting with inner classes.
16-17: Fluent chaining and test verification.The test correctly chains the instantiation of
Outerwith thetest()method call and usesvar_dump()to verify the object type and structure. This is a good pattern for testing the inner class functionality.
20-21: Expected output validates inner class instantiation.The expected output correctly shows that an object of type
Outer:>PrivateInneris returned, confirming that inner class instantiation and method execution works as expected even when the inner class is private.
9-9:✅ Verification successful
Verify the inner class reference syntax.
The syntax
Outer:>PrivateInnerfor referencing an inner class is used consistently throughout this test. Make sure this aligns with the RFC's proposed syntax and other tests for inner classes.
🏁 Script executed:
#!/bin/bash # Check for consistent inner class reference syntax across tests echo "Checking for inner class reference syntax patterns across test files..." grep -r ':>' tests/classes/inner_classes/ --include="*.phpt" | head -n 10Length of output: 1189
Inner class reference syntax verified as consistent
Thereturn new Outer:>PrivateInner();statement intests/classes/inner_classes/return_types_006.phptemploys the same:>syntax observed in other tests (e.g.,new Outer:>Inner()andinner_classes:>Point()). This usage appears to be in line with the RFC's proposed syntax for inner classes. No changes are required.Zend/zend_compile.c (8)
896-910: Ensure Consistency for Modifier Target Handling.These lines add support for
T_READONLY,T_ABSTRACT, andT_FINALon inner classes viaZEND_MODIFIER_TARGET_INNER_CLASS, which seems consistent with the new nested class design. However, please double-check that no unintended modifiers (e.g.,static) slip through to inner classes elsewhere in the parser or AST, especially if an upstream grammar rule might allow them.Would you like to run any tests or static analysis to confirm no other modifiers can apply to these inner classes?
947-949: No Issues with New “inner class” Identifier String.Defining “member = ‘inner class’” aligns with the new modifier target. This helps produce clearer error messages for inner-class-specific constraints.
1056-1086: Verify Enforcement of Disallowed Modifiers.Here, multiple access modifiers and static usage for inner classes are forbidden. The compile-time errors appear well-defined. However, in later code chunks, some references to
ZEND_ACC_PUBLIC|ZEND_ACC_PROTECTED|ZEND_ACC_PRIVATEflags remain in the rename process. Ensure that “_SET” flags differ appropriately from the “public/protected/private” flags to avoid contradictory or confusing handling.
1866-1873: Fluid Integration of Inner Classes into Constant Expressions.Using
zend_resolve_nested_class_nameto set the string value for constant expression resolution in inner classes looks consistent and should properly evaluate references to inner classes at compile time.
2923-2958: Compilation Logic for Inner Class References.Introducing
zend_compile_inner_class_ref()and hooking it intozend_compile_class_ref()properly extends support for nested classes. The opcode emission (ZEND_FETCH_INNER_CLASS) is straightforward and aligns well with the fetch logic. Just be sure to maintain consistent error conditions if the outer or inner references are invalid.
7427-7432: Type Declarations for Nested Classes.Allowing inner classes to appear as typed hints (via
ZEND_TYPE_INIT_CLASS) is a solid extension. Please ensure tests are included to confirm correct type resolution under various nested class hierarchies.
9242-9246: Adjusting Anonymous Class Logic.Setting
ce->required_scopeandce->lexical_scopetoNULLmakes sense for anonymous classes, even nested ones, ensuring that scoping rules don’t conflict.
11853-11858: Extension ofzend_compile_exprfor Inner Classes.Recognizing
ZEND_AST_INNER_CLASSwithinzend_compile_expr()ensures nested classes can be referenced seamlessly. Implementation looks consistent with the rest of the changes for nested class support.tests/classes/inner_classes/visibility_005.phpt (1)
6-22: Implementation correctly demonstrates visibility scoping in nested classes.The test demonstrates that inner classes can access private members of their outer classes. This is an important test case for the nested class implementation, as it verifies that visibility boundaries work correctly across the class hierarchy.
ext/reflection/tests/ReflectionClass_toString_001.phpt (2)
33-33: Check if the method count is correct after adding new methods.The number of methods has been updated from 64 to 68, which aligns with the addition of 4 new methods:
isInnerClass,isPrivate,isProtected, andisPublic.
518-544: Reflection API properly extended to support inner classes.The addition of these four methods enhances reflection capabilities to work with inner classes:
isInnerClass()- Determines if a class is an inner classisPrivate(),isProtected(),isPublic()- Checks the visibility modifiers of a classThese methods follow the established pattern of other reflection methods with consistent parameter signatures and return types.
tests/classes/inner_classes/return_types_002.phpt (3)
7-7: Syntax for inner class declaration looks good.The declaration of a private inner class is clear and follows natural class syntax extensions for visibility modifiers.
1-33: Test case effectively validates private inner class visibility.This test correctly verifies that private inner classes cannot be accessed from outside their declaring class, even by child classes. The expected fatal error is appropriate for the access violation being tested.
28-32:✅ Verification successful
Expected output has inconsistent class naming format.
The output shows
Outer:>Innerbut this should match the actual format used when dumping objects with inner classes. Verify that this is consistent with how PHP outputs inner class names.
🏁 Script executed:
#!/bin/bash # Check how inner class names are represented in var_dump output grep -r "object(" --include="*.phpt" tests/classes/inner_classes/ | grep ">"Length of output: 1065
Verified: Inner class naming format is correct
After running the grep search, all test cases—including the one in
tests/classes/inner_classes/return_types_002.phpt—consistently output inner class names in the formatOuter:>Inner, which aligns with PHP's standard dumping of inner classes. No changes are required.tests/classes/inner_classes/static_variables.phpt (1)
6-15: Inner class implementation with constants and static variables looks good.The nested class structure with constants and static variables is implemented correctly. This effectively tests accessing static members from multiple levels of nesting.
tests/classes/inner_classes/visibility_003.phpt (5)
7-11: Inner class declaration looks good.
Declaring an inner class as private is a novel syntax. The assigned operation$i->illegal = $this;cleanly demonstrates that the inner class can manipulate the outer class’s property.
12-12: Verify the custom type syntax.
private Outer:>Inner $illegal;is a new nested class type syntax. Ensure the engine correctly supports this, especially regarding autoload, reflection, and inheritance scenarios.
14-16: InstantiatingInnerwithinOuter::test().
The approach of usingnew Outer:>Inner()->test($this);is consistent with verifying scoping behavior, especially for private inner classes.
19-21: High-level test sequence.
Creating anOuterinstance and calling itstest()method ensures the crucial property assignment is validated at runtime. This is a good, straightforward approach to confirm the behavior.
25-30: Expected output block is correct.
The dumped object structure matches the scenario of storing anOuter:>Innerinstance in theillegalproperty, confirming that the private property was successfully accessed and assigned.Zend/zend_object_handlers.c (3)
405-407: New allowance for nested classes' private properties.
These lines let code “jump” to the success path when thescope->lexical_scopematches the current class (ce). Verify there are no edge cases granting unauthorized access, such as incorrectly matching lexical scopes in other complex scenarios.
1822-1822: Label for lexical scope checks introduced.
Usingcheck_lexical_scope:is consistent with the engine’s GOTO style, but ensure this label is only used when truly necessary, to maintain clarity in future debugging.
1904-1912: Fallback logic in static methods with nested scopes.
If a method is private or protected, the code falls back toget_static_method_fallbackor, if needed, checks an additionallexical_scopeagain. Please confirm tests cover multiple nested scopes to prevent unexpected skipping or multiple fallback attempts.ext/reflection/php_reflection.c (3)
4096-4105: LGTM: Class privacy check implementationThe method correctly checks if a class is private by verifying both required_scope and required_scope_absolute are set, indicating the class is only accessible from its parent scope.
4108-4116: LGTM: Class protection check implementationThe method correctly identifies a protected class by checking if required_scope is set (limited access) while required_scope_absolute is not set (not strictly limited to parent).
4119-4126: LGTM: Class public access check implementationThe method correctly identifies a public class by checking if there's no required_scope, indicating unrestricted access.
tests/classes/inner_classes/visibility_007.phpt (4)
6-9: Ensure correctness of protected method access in nested classes.
The protected instance methodtest()in theOuterclass is correctly invoked from the inner classes via their parent references. This appears to be aligned with the new nested class rules.
10-13: Confirm static context usage.
Invokingprotected static function test()fromInnerlooks consistent with the new syntax (Outer:>Middle::test()). Just verify that protected static methods remain inaccessible outside the nested chain.
14-20: Validate nested class referencing syntax.
The syntaxclass Inner { public function test() { ... } }withinMiddleis presumably new. Check that references likeOuter:>Middle::test();indeed function as intended in PHP.
23-28: Check final output alignment with--EXPECT--.
The test expects two lines of output for each invocation:Outer:>Middle::test Outer::testThis seems correct given the calls. Double-check that the test will pass under the new engine changes.
Zend/zend_vm_opcodes.c (3)
25-25: Increase array sizes carefully.
The expansion from 210 to 211 entries for bothzend_vm_opcodes_namesandzend_vm_opcodes_flagsappears correct. Be sure all references to these arrays handle the new upper bound properly.Also applies to: 239-239
236-236: New opcode name “ZEND_FETCH_INNER_CLASS”.
No issues spotted. This is consistent with the broader nested class functionality being introduced.
450-450: Review new opcode flag value.
0x00040307matches the structure of other flags. Confirm that you have tested all relevant runtime conditions to ensure no unintended side effects.tests/classes/inner_classes/simple_declaration_004.phpt (3)
9-14: Check scope resolution consistency in static methods.
BothtestStatic()andtestSelf()return instances ofstatic:>Middle()andself:>Middle(), respectively. Confirm that these references are resolved according to the correct class context in nested classes.
18-23: Validateparent:>Middlein a derived class.
The code extendsOuterinsideOuter2and referencesparent:>Middle. This usage is a good coverage test for inheritance in nested classes.
25-31: Check runtime outputs.
Thevar_dumpcalls ensure correct instantiation of different nested classes. This segment looks fine.Zend/zend_language_parser.y (3)
234-234: New tokenT_INNER_REF.
Adding "':>'" asT_INNER_REFhelps parse nested class references. Ensure downstream scanning logic is fully updated.
961-964:inner_class_statementintegration.
The new production rules for declaring an inner class align with the broader nested class concept. Be sure to test how these rules interact with final, abstract, or interface constraints.Also applies to: 983-983
1434-1436: Extendedclass_namerule to supportT_INNER_REF.
This expansion ensures references likeclass_name T_INNER_REF nameparse correctly. Looks good.Zend/zend_execute.c (1)
1237-1252: Confirm public inner class use case.The logic correctly blocks private/protected inner classes in certain scopes, but there appears to be no explicit handling or message for public inner classes in the global or other scopes. If you want to ensure public inner classes are also enforced or have consistent error messaging, please verify whether this scenario needs handling or an explicit fallback.
Zend/zend_vm_execute.h (21)
3313-3314: No issues with the new opcode case.
This case neatly integrates the newZEND_FETCH_INNER_CLASSopcode handling alongside existing class-fetch logic.
10900-10915: Checks for returning private or protected classes look good.
This error-handling code correctly enforces that public methods cannot return private/protected inner classes.
21753-21768: Properly enforces returned class scope checks.
The public/protected logic appears consistent with the engine’s requirements.
30247-30262: Scope checks on returned object are correct.
Throws a type error when a public/protected method returns a class outside its allowed scope.
38152-38167: Well-implemented return scope validation.
No issues spotted with how the engine detects and rejects returning private/protected classes from public methods.
50971-50986: Return scope checks remain consistent.
Properly guards against returning private/protected classes from public/protected methods.
58028-58031: New specialized opcode labels are integrated cleanly.
These labels align with the newZEND_FETCH_INNER_CLASShandlers, keeping the VM jump table consistent.
59904-59908: Hybrid VM case for CONST_CONST looks good.
Extends the hybrid VM logic to handle the newly introduced fetch mode.
61179-61183: Hybrid VM case for TMPVAR_CONST added cleanly.
No concerns regarding the jump table changes for this opcode.
62772-62776: Hybrid VM case for UNUSED_CONST is consistent.
Properly wires the new inner-class fetch variant into the VM’s hybrid dispatcher.
67235-67239: Additional opcode handler references are valid.
These lines correctly declare the new inner-class fetch handlers in the handler array.
68197-68197: Spec table entry added for new opcode.
No issues noted.
68231-68231: Spec rule entry updated.
Maintains consistency in the VM’s specialized rule definitions.
68362-68408: No concerns with the repeated usage of spec value 3491.
Seems to fit the standard approach for multi-line specialized opcode expansions.
68580-68580: Spec rule assignment is correctly added.
No concerns identified with these updates to the specialized handling table.
68607-68607: Consistent spec rule for numerical operations.
No issues.
68628-68628: Commutes multiplication spec.
Looks properly integrated into the rules table for multiplication.
68646-68659: Extends identical check with new spec logic.
Proper usage of the SMART_BRANCH/COMMUTATIVE spec flags.
68664-68677: Correct extension for not-identical check.
No concerns.
68682-68693: IS_EQUAL spec update aligns with existing structure.
Nothing problematic noticed.
68698-68825: IS_NOT_EQUAL and other branch specs are introduced cleanly.
No issues found in these expansions.Zend/zend_vm_handlers.h (3)
1375-1378: Addition of ZEND_FETCH_INNER_CLASS operation specificationsThese new operation specifications are correctly implemented to support inner class functionality. The different variants (CONST_CONST, TMPVAR_CONST, UNUSED_CONST) properly handle different operand types for fetching inner classes.
1379-1384: Shifting of existing operation IDsOperation specifications for INIT_FCALL_OFFSET, RECV_NOTYPE, COUNT_ARRAY, and JMP_FORWARD have been shifted to accommodate the new inner class operations. The numbering sequence is maintained properly to ensure internal consistency in the opcode handler table.
1389-1873: Extensive arithmetic and comparison operation specifications addedThe addition of numerous arithmetic (ADD, SUB, MUL), comparison (IS_EQUAL, IS_NOT_EQUAL, IS_SMALLER, IS_SMALLER_OR_EQUAL), and other specialized operations for both long integers and doubles provides comprehensive support for inner class implementation. These operations maintain the pattern of existing operations with proper type handling.
| if(Z_TYPE_P(property) == IS_OBJECT && !zend_check_class_visibility(Z_OBJCE_P(property), info, info->flags)) { | ||
| zend_verify_property_type_error(info, property); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restrict the property flags to PPP bits.
When calling zend_check_class_visibility(), the current implementation passes info->flags directly. Some of these bits (e.g., ZEND_ACC_STATIC, ZEND_ACC_READONLY) are unrelated to the property’s accessibility. Consider masking out non-PPP bits before passing the visibility to avoid unintended behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Zend/zend_object_handlers.c(4 hunks)Zend/zend_vm_def.h(3 hunks)Zend/zend_vm_execute.h(24 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Zend/zend_object_handlers.c
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (18)
Zend/zend_vm_def.h (2)
8296-8297: Update to ZEND_FETCH_CLASS_NAME handler to handle inner class references.The code has been updated to handle inner class references in the FETCH_CLASS_NAME opcode. This is consistent with the new inner class support.
10522-10523: Added ZEND_OP_DATA opcode definition.This is a necessary addition to support inner class functionality by providing an additional operand for operations that require it.
Zend/zend_vm_execute.h (16)
3313-3313: Good addition to include ZEND_FETCH_INNER_CLASS in the special case handling.This integration of the new inner class opcode in the switch statement is correct, as it ensures the value returned (zend_class_entry pointer) is properly handled like other class-related opcodes.
6614-6714: Thorough implementation of the CONST_CONST inner class handler.The handler correctly retrieves inner classes from outer classes with proper error handling. The visibility checks for private and protected inner classes are well-implemented, ensuring proper encapsulation.
Some implementation notes:
- Error messages are clear and descriptive
- Memory is properly managed with
zend_string_release()- Length overflow is checked before string concatenation
- Proper scope checks are performed based on required_scope and required_scope_absolute
10906-10921: Well-implemented visibility checks for method return types.This code correctly enforces that public methods cannot return private or protected classes, and protected methods cannot return private classes from a different scope. This is essential for maintaining proper encapsulation in PHP's type system.
16251-16351: Consistent implementation of the TMPVAR_CONST inner class handler.This handler follows the same pattern as the CONST_CONST handler but is adapted for TMPVAR operands. The implementation maintains consistency with the other handlers, including proper error handling, memory management, and scope checks.
21763-21778: Consistent visibility checks for method return types.This is another instance of the same visibility check logic that was added in previous hunks. It's a necessary duplication due to the structure of the Zend VM code generation system.
30257-30272: Consistent visibility checks for method return types.This is another instance of the same visibility check logic, properly integrated at this point in the execution flow.
34006-34106: Thorough implementation of the UNUSED_CONST inner class handler.This handler follows the same pattern as the other inner class handlers but is adapted for the UNUSED operand type. The implementation properly handles special cases of scope resolution like
self,parent, andstaticreferences.
38167-38182: Consistent visibility checks for method return types.Another instance of the visibility check logic, identical to the previous implementations.
50986-51001: Consistent visibility checks for method return types.Another instance of the visibility check logic at a different point in the execution flow.
58043-58047: Proper registration of handler function pointers.These new entries correctly register the inner class handlers in the opcode handler function table.
59919-59923: Good addition of inner class handler to the VM execution path.This case statement ensures that the CONST_CONST handler is correctly invoked during VM execution.
61194-61198: Good addition of inner class handler to the VM execution path.This case statement ensures that the TMPVAR_CONST handler is correctly invoked during VM execution.
62787-62791: Good addition of inner class handler to the VM execution path.This case statement ensures that the UNUSED_CONST handler is correctly invoked during VM execution.
67250-67254: Proper registration of handler functions in the handler table.These entries ensure that the inner class handlers are correctly registered in the function handler table.
68212-68212: Properly adjusted opcode specifications for VM optimization.These updates to the VM specification tables are necessary to accommodate the new inner class opcodes within the Zend Engine's optimization framework.
Also applies to: 68246-68246, 68377-68423
68595-68595: Carefully adjusted VM specialization rules.These specification updates maintain the integrity of the VM optimization framework while accommodating the new inner class functionality. The adjustments to rule values ensure proper handler selection during opcode execution.
Also applies to: 68603-68603, 68611-68611, 68622-68622, 68627-68627, 68632-68632, 68643-68643, 68648-68648, 68653-68653, 68664-68664, 68669-68669, 68671-68671, 68682-68682, 68687-68687, 68689-68689, 68700-68700, 68705-68705, 68716-68716, 68721-68721, 68729-68729, 68734-68734, 68742-68742, 68747-68747, 68752-68754, 68756-68756, 68761-68763, 68768-68770, 68775-68777, 68782-68784, 68789-68789, 68794-68794, 68799-68799, 68804-68804, 68809-68809, 68814-68814, 68822-68822, 68827-68827, 68832-68832, 68837-68837
| { | ||
| USE_OPLINE | ||
| SAVE_OPLINE(); | ||
|
|
||
| zend_string *inner_class_name, *full_class_name; | ||
| zend_class_entry *outer_ce = NULL, *inner_ce = NULL, *scope = NULL; | ||
|
|
||
| scope = EX(func)->op_array.scope; | ||
|
|
||
| if (OP1_TYPE == IS_CONST) { | ||
| zval *outer_class_zv = RT_CONSTANT(opline, opline->op1); | ||
| outer_ce = zend_lookup_class(Z_STR_P(outer_class_zv)); | ||
| if (!outer_ce) { | ||
| zend_error(E_ERROR, "Outer class '%s' not found for inner class %s:>%s", Z_STRVAL_P(outer_class_zv), Z_STRVAL_P(outer_class_zv), Z_STRVAL_P(RT_CONSTANT(opline, opline->op2))); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| } else if (OP1_TYPE == IS_UNUSED) { | ||
| uint32_t fetch_type; | ||
| zend_class_entry *called_scope; | ||
|
|
||
| fetch_type = opline->op1.num & ZEND_FETCH_CLASS_MASK; | ||
| if (UNEXPECTED(scope == NULL)) { | ||
| SAVE_OPLINE(); | ||
| zend_throw_error(NULL, "Cannot use \"%s\" in the global scope", | ||
| fetch_type == ZEND_FETCH_CLASS_SELF ? "self" : | ||
| fetch_type == ZEND_FETCH_CLASS_PARENT ? "parent" : "static"); | ||
| ZVAL_UNDEF(EX_VAR(opline->result.var)); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| if (fetch_type == ZEND_FETCH_CLASS_SELF) { | ||
| outer_ce = scope; | ||
| } else if (fetch_type == ZEND_FETCH_CLASS_PARENT) { | ||
| if (UNEXPECTED(scope->parent == NULL)) { | ||
| SAVE_OPLINE(); | ||
| zend_throw_error(NULL, | ||
| "Cannot use \"parent\" when current class scope has no parent"); | ||
| ZVAL_UNDEF(EX_VAR(opline->result.var)); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| outer_ce = scope->parent; | ||
| } else if (fetch_type == ZEND_FETCH_CLASS_STATIC) { | ||
| if (Z_TYPE(EX(This)) == IS_OBJECT) { | ||
| called_scope = Z_OBJCE(EX(This)); | ||
| } else { | ||
| called_scope = Z_CE(EX(This)); | ||
| } | ||
| outer_ce = called_scope; | ||
| } else { | ||
| zend_throw_error(NULL, "Unknown scope resolution"); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| } else { | ||
| outer_ce = Z_CE_P(EX_VAR(opline->op1.var)); | ||
| } | ||
|
|
||
| inner_class_name = Z_STR_P(RT_CONSTANT(opline, opline->op2)); | ||
|
|
||
| if (UNEXPECTED(ZSTR_LEN(outer_ce->name) + ZSTR_LEN(inner_class_name) + 2 > ZSTR_MAX_LEN)) { | ||
| zend_error(E_ERROR, "Class name is too long"); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
|
|
||
| full_class_name = zend_string_concat3( | ||
| ZSTR_VAL(outer_ce->name), ZSTR_LEN(outer_ce->name), | ||
| ":>", 2, | ||
| ZSTR_VAL(inner_class_name), ZSTR_LEN(inner_class_name) | ||
| ); | ||
|
|
||
| inner_ce = zend_lookup_class(full_class_name); | ||
| if (!inner_ce) { | ||
| zend_error(E_ERROR, "Inner class '%s' not found in outer class %s", ZSTR_VAL(full_class_name), ZSTR_VAL(outer_ce->name)); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
|
|
||
| if (inner_ce->required_scope) { | ||
| if (inner_ce->required_scope_absolute) { | ||
| // for private classes, we check if the scope we are currently in has access | ||
| if (scope != NULL && (inner_ce->required_scope == scope || scope->lexical_scope == inner_ce->required_scope)) { | ||
| // we are in the correct scope | ||
| } else { | ||
| zend_error(E_ERROR, "Cannot access private inner class '%s'", ZSTR_VAL(full_class_name)); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| } else { | ||
| // for protected classes, we check if the scope is an instance of the required scope | ||
| if (scope != NULL && (instanceof_function(scope, inner_ce->required_scope) || instanceof_function(scope->lexical_scope, inner_ce->required_scope))) { | ||
| // we are in the correct scope | ||
| } else { | ||
| zend_error(E_ERROR, "Cannot access protected inner class '%s'", ZSTR_VAL(full_class_name)); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Z_CE_P(EX_VAR(opline->result.var)) = inner_ce; | ||
|
|
||
| zend_string_release(full_class_name); | ||
|
|
||
| ZEND_VM_NEXT_OPCODE(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New opcode handler ZEND_FETCH_INNER_CLASS for handling inner class fetching.
This new opcode handler implements the logic for fetching inner classes in PHP. Here's a detailed analysis:
- Scope Resolution:
- Gets the current scope from
EX(func)->op_array.scope - Handles different types of outer class references (constant, unused/special keywords, variable)
- Outer Class Resolution:
- For constant references: Looks up class using
zend_lookup_class() - For special keywords (self/parent/static): Resolves based on current scope
- For variables: Uses the class entry from the variable
- Inner Class Resolution:
- Concatenates outer class name and inner class name with
:>separator - Performs lookup using the full class name
- Throws appropriate errors if class not found
- Access Control:
- Checks visibility restrictions for private and protected inner classes
- For private classes: Verifies absolute scope match
- For protected classes: Verifies inheritance relationship
- Error Handling:
- Throws errors for:
- Non-existent outer/inner classes
- Access violations
- Invalid scope usage
The implementation follows good security practices by:
- Validating class name lengths
- Performing proper access control checks
- Using safe string concatenation
- Handling error cases appropriately
However, there is a potential issue with string length validation:
if (UNEXPECTED(ZSTR_LEN(outer_ce->name) + ZSTR_LEN(inner_class_name) + 2 > ZSTR_MAX_LEN)) {
zend_error(E_ERROR, "Class name is too long");
HANDLE_EXCEPTION();
}The length check should be done before any memory allocation to prevent potential buffer overflow.
Consider adding additional validation:
+ if (UNEXPECTED(!outer_ce || !inner_class_name)) {
+ zend_error(E_ERROR, "Invalid class name");
+ HANDLE_EXCEPTION();
+ }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/classes/inner_classes/static_resolution.phpt (1)
16-16: Consider adding more specific error information to the expectation.The expected output contains a generic fatal error message. Consider making it more specific to clearly indicate why static modifiers can't be used on inner classes (e.g., inheritance limitations, scope constraints, etc.).
tests/classes/inner_classes/interface_usage.phpt (1)
12-12: Consider adding a test case for implementing the inner class.The test shows that the inner class doesn't get inherited when implementing the interface, but it might be valuable to add a test case where a class implements both the interface and tries to override or extend the inner class behavior.
tests/classes/inner_classes/trait_usage.phpt (1)
1-25: Consider adding additional test for trait method accessing inner class.While the test verifies the existence of inner classes, it would be valuable to also test a scenario where a method within the trait attempts to instantiate or use its own inner class, to ensure proper scoping behavior.
tests/classes/inner_classes/simple_declaration_004.phpt (1)
27-30: Consider removing the duplicate test case.Lines 29 and 30 appear to be testing the same thing twice with
Outer2::testSelf(). Consider removing one of these duplicate calls for clarity.var_dump(Outer::testSelf()); var_dump(Outer2::testParent()); var_dump(Outer2::testSelf()); -var_dump(Outer2::testSelf());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Zend/zend_compile.c(11 hunks)Zend/zend_compile.h(2 hunks)Zend/zend_object_handlers.c(7 hunks)tests/classes/inner_classes/enum_usage.phpt(1 hunks)tests/classes/inner_classes/interface_usage.phpt(1 hunks)tests/classes/inner_classes/simple_declaration_004.phpt(1 hunks)tests/classes/inner_classes/static_resolution.phpt(1 hunks)tests/classes/inner_classes/trait_usage.phpt(1 hunks)tests/classes/inner_classes/visibility_010.phpt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Zend/zend_compile.h
- Zend/zend_object_handlers.c
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (27)
Zend/zend_compile.c (9)
896-910: Looks consistent for allowing these modifiers on inner classes.The logic for returning
ZEND_ACC_READONLY,ZEND_ACC_ABSTRACT, andZEND_ACC_FINALwhen the target is an inner class appears consistent with the approach of supporting these three modifiers. No immediate issues are observed here.
947-949: No concerns with identifying the member type.Marking the member as an "inner class" looks correct and aligns with the new modifier target.
1797-1834: Nested class name resolution appears solid.The implementation of
zend_resolve_nested_class_name()and associated usage correctly concatenates and releases strings, preventing leaks. No issues found here.
1866-1870: Good logic for detecting inner class references.Using
zend_resolve_nested_class_name()to set the value forzvis sound, ensuring new strings are properly allocated. Looks fine.
2923-2958: Reference compilation for nested classes is well structured.The new
zend_compile_inner_class_ref()function neatly handles the outer and inner class AST nodes. This cleanly extends existing compilation flows to accommodate nested classes.
3002-3008: Correct static usage check.Emitting a compile error if
staticis used on an inner class is consistent with the earlier rules. No issues noted.
7429-7434: Type inference for nested classes.Returning a
zend_typefor an inner class is a straightforward extension that ensures type checks on nested classes. Appears correct.
9244-9245: No issues with resetting scope.Clearing
required_scopeandlexical_scopein this fallback block is straightforward and appears consistent with top-level classes. No concerns here.
11855-11857: Inner class reference compilation in switch-case.Adding the
ZEND_AST_INNER_CLASScase to handle nested class references is coherent with the rest of the changes. Implementation looks good.tests/classes/inner_classes/static_resolution.phpt (2)
9-11: Found potential syntax inconsistency.The test uses
static:>Inner()syntax on line 10 to reference an inner class from a static context. However, in the other test files, the syntax for accessing inner classes appears to be:>(e.g.,Outer:>Inner()). Verify this is the intended syntax for static resolution of inner classes.
13-13: Test structure appears correct.The test correctly attempts to invoke the method that uses static resolution with an inner class, which according to the expected output should result in a fatal error.
tests/classes/inner_classes/enum_usage.phpt (3)
6-8: Implementation looks correct.The test properly demonstrates that an enum can contain an inner class definition, which is a new feature being introduced.
10-11: Consistent inner class access syntax.The code uses the
:>operator to access the inner class, which is consistent with the syntax pattern seen in other test files for this feature.
14-16: Expected output matches test case.The expected output correctly shows that an instance of the inner class can be created and that the class exists, validating the inner class functionality within enums.
tests/classes/inner_classes/interface_usage.phpt (2)
6-8: Interface with inner class implementation looks correct.The test properly demonstrates the declaration of an inner class within an interface, which is a key part of the inner class feature being introduced.
14-15: Test assertions validate expected behavior.The test correctly verifies that inner classes in interfaces exist independently but aren't inherited by implementing classes, which is an important behavior to document.
tests/classes/inner_classes/trait_usage.phpt (3)
6-8: Trait with inner class implementation is correct.The test properly demonstrates the declaration of an inner class within a trait, which is part of the inner class feature being introduced.
12-14: Using trait with inner class is properly tested.The test correctly shows how a class can use a trait that contains an inner class definition.
16-17: Test assertions validate expected behavior for traits.The test correctly verifies that inner classes in traits exist independently but aren't incorporated into classes using the trait, which is consistent with the behavior seen for interfaces.
tests/classes/inner_classes/simple_declaration_004.phpt (3)
6-12: Good implementation of inner class with scope resolution support.The
Outerclass defines a public inner classMiddleand properly demonstrates the use of theself:>scope resolution operator to instantiate the inner class.
14-25: LGTM - Effective demonstration of parent scope resolution.The inheritance pattern is well-implemented, showing how an inner class can extend another inner class from the parent through the
parent:>syntax. The overriddentestSelfmethod correctly returns the child class's version of the inner class.
34-41: Test expectations look correct.The expected output correctly shows the object types as qualified with their parent class (e.g.,
Outer:>Middle), matching the implementation.tests/classes/inner_classes/visibility_010.phpt (5)
6-9: Good use of private(set) visibility.The properties are correctly defined with
public private(set)visibility, allowing them to be read publicly but only set from within the class.
15-16: Good use of readonly and final modifiers for Builder.The Builder class is appropriately marked as
readonly finalwhich aligns with best practices for immutable value objects. The constructor parameters use property promotion with appropriate nullable types.
18-24: Well-implemented fluent interface pattern.The
withEmailandwithNamemethods correctly implement the fluent interface pattern by returning a new instance with updated values, preserving immutability.
26-28: Builder pattern implementation looks good.The
buildmethod correctly instantiates the outer class with the builder instance, completing the builder pattern implementation.
32-33: Great demonstration of the builder pattern with inner classes.The usage example clearly demonstrates how to use the Builder pattern with inner classes using the new
:>syntax.
| if (target == ZEND_MODIFIER_TARGET_INNER_CLASS) { | ||
| if ((flags & ZEND_ACC_PPP_MASK) && (new_flag & ZEND_ACC_PPP_MASK)) { | ||
| zend_throw_exception(zend_ce_compile_error, | ||
| "Multiple access type modifiers are not allowed", 0); | ||
| return 0; | ||
| } | ||
|
|
||
| if ((flags & ZEND_ACC_STATIC) || (new_flag & ZEND_ACC_STATIC)) { | ||
| zend_throw_exception(zend_ce_compile_error, | ||
| "Static inner classes are not allowed", 0); | ||
| return 0; | ||
| } | ||
|
|
||
| if ((flags & ZEND_ACC_PUBLIC_SET) || (new_flag & ZEND_ACC_PUBLIC_SET)) { | ||
| zend_throw_exception(zend_ce_compile_error, | ||
| "Public(set) inner classes are not allowed", 0); | ||
| return 0; | ||
| } | ||
|
|
||
| if ((flags & ZEND_ACC_PROTECTED_SET) || (new_flag & ZEND_ACC_PROTECTED_SET)) { | ||
| zend_throw_exception(zend_ce_compile_error, | ||
| "Protected(set) inner classes are not allowed", 0); | ||
| return 0; | ||
| } | ||
|
|
||
| if ((flags & ZEND_ACC_PRIVATE_SET) || (new_flag & ZEND_ACC_PRIVATE_SET)) { | ||
| zend_throw_exception(zend_ce_compile_error, | ||
| "Private(set) inner classes are not allowed", 0); | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Conflicting rule about inner class visibility.
This block disallows public, protected, and private for inner classes, yet lines [9187-9222] appear to partially support these modifiers. The two segments directly conflict, risking runtime or compile-time errors. Please confirm which behavior is intended and reconcile these differences.
| if (CG(active_class_entry)) { | ||
| // rename the inner class so we may reference it by name | ||
| name = zend_string_concat3( | ||
| ZSTR_VAL(CG(active_class_entry)->name), ZSTR_LEN(CG(active_class_entry)->name), | ||
| ":>", 2, | ||
| ZSTR_VAL(unqualified_name), ZSTR_LEN(unqualified_name) | ||
| ); | ||
|
|
||
| // configure the current ce->flags for a nested class. This should only include: | ||
| // - final | ||
| // - readonly | ||
| // - abstract | ||
| decl->flags |= decl->attr & ZEND_ACC_FINAL; | ||
| if (decl->attr & ZEND_ACC_ABSTRACT) { | ||
| decl->flags |= ZEND_ACC_EXPLICIT_ABSTRACT_CLASS; | ||
| } | ||
| if (decl->attr & ZEND_ACC_READONLY) { | ||
| decl->flags |= ZEND_ACC_READONLY_CLASS | ZEND_ACC_NO_DYNAMIC_PROPERTIES; | ||
| } | ||
|
|
||
| // configure for a nested class. This should only include: | ||
| // - public | ||
| // - private | ||
| // - protected | ||
| int propFlags = decl->attr & (ZEND_ACC_PUBLIC|ZEND_ACC_PROTECTED|ZEND_ACC_PRIVATE); | ||
| // remove the flags from attrs | ||
| decl->attr &= ~(ZEND_ACC_PUBLIC|ZEND_ACC_PROTECTED|ZEND_ACC_PRIVATE|ZEND_ACC_FINAL|ZEND_ACC_ABSTRACT|ZEND_ACC_READONLY); | ||
|
|
||
| // if a class is private or protected, we need to require the correct scope | ||
| ce->required_scope = propFlags & (ZEND_ACC_PRIVATE|ZEND_ACC_PROTECTED) ? CG(active_class_entry) : NULL; | ||
| ce->required_scope_absolute = propFlags & ZEND_ACC_PRIVATE ? true : false; | ||
| ce->lexical_scope = CG(active_class_entry); | ||
|
|
||
| // ensure the class is treated as a top-level class and not an anon class | ||
| toplevel = true; | ||
| } else { |
There was a problem hiding this comment.
Contradictory logic about public/protected/private.
This code suggests partial support for PPP-type modifiers on inner classes, but lines [1056-1086] disallow it. This direct contradiction can lead to unresolvable compilation errors. Please decide whether inner classes can use PPP modifiers and unify these rules.
There was a problem hiding this comment.
dumb question, but what are PPP modifiers?
There was a problem hiding this comment.
@withinboredom PPP stands for Public/Protected/Private - these are the visibility modifiers in PHP that control access to class members:
public: Accessible from anywhereprotected: Only accessible within the class and its child classesprivate: Only accessible within the declaring class
Not a dumb question at all - these are fundamental concepts in PHP's object-oriented programming model!
| private function __construct(self:>Builder $builder) { | ||
| $this->name = $builder->name; | ||
| $this->email = $builder->email; | ||
| } |
There was a problem hiding this comment.
Address potential null values in constructor.
The constructor doesn't handle the case where the builder's properties might be null, but the class properties are defined as non-nullable strings. This could lead to a type error at runtime.
private function __construct(self:>Builder $builder) {
- $this->name = $builder->name;
- $this->email = $builder->email;
+ if ($builder->name === null || $builder->email === null) {
+ throw new \InvalidArgumentException("Name and email are required");
+ }
+ $this->name = $builder->name;
+ $this->email = $builder->email;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function __construct(self:>Builder $builder) { | |
| $this->name = $builder->name; | |
| $this->email = $builder->email; | |
| } | |
| private function __construct(self:>Builder $builder) { | |
| if ($builder->name === null || $builder->email === null) { | |
| throw new \InvalidArgumentException("Name and email are required"); | |
| } | |
| $this->name = $builder->name; | |
| $this->email = $builder->email; | |
| } |
```
ext/gd/libgd/gd.c:2275:14: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
#0 0x5d6a2103e1db in php_gd_gdImageCopy /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd.c:2275
#1 0x5d6a210a2b63 in gdImageCrop /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd_crop.c:57
#2 0x5d6a21018ca4 in zif_imagecrop /home/dcarlier/Contribs/php-src/ext/gd/gd.c:3575
#3 0x5d6a21e46e7a in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1337
#4 0x5d6a221188da in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:57246
#5 0x5d6a221366bd in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:61634
#6 0x5d6a21d107a6 in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1895
#7 0x5d6a21a63409 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2529
#8 0x5d6a22516d5e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:966
php#9 0x5d6a2251981d in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1341
php#10 0x7f10d002a3b7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
php#11 0x7f10d002a47a in __libc_start_main_impl ../csu/libc-start.c:360
php#12 0x5d6a20a06da4 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2806da4) (BuildId: d9a79c7e0e4872311439d7313cb3a81fe04190a2)
```
close phpGH-18006
Summary by CodeRabbit
New Features
Bug Fixes
Tests