Inner class RFC #1
Conversation
WalkthroughThe pull request introduces extensive support for inner (nested) classes in PHP. Major modifications include extending the internal class structure with new members to track scope, adding a new AST node for inner classes, and enhancing functions in the compiler, executor, and VM to handle nested class declarations and references. New tokens, grammar rules, and opcode handlers have been added to recognize and process the inner class syntax. Additionally, reflection, tokenizer, and persistence mechanisms are updated, and a comprehensive suite of tests has been introduced to validate access modifiers, inheritance, autoloading, visibility, and return types for inner classes. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as PHP Source Code
participant Parser as PHP Parser
participant Compiler as Zend Compiler
participant ClassEntry as _zend_class_entry
participant Emitter as Opcode Emitter
Source->>Parser: Provide class with nested inner class (":>" syntax)
Parser->>Compiler: Parse inner class declaration (using T_INNER_REF)
Compiler->>Compiler: Rename inner class & update scope (set required_scope, lexical_scope)
Compiler->>ClassEntry: Update class entry structure with new scoping info
Compiler->>Emitter: Emit opcode ZEND_FETCH_INNER_CLASS for inner class
Emitter-->>Compiler: Proceed with compilation
sequenceDiagram
participant VM as Virtual Machine
participant OpHandler as Opcode Handler (ZEND_FETCH_INNER_CLASS)
participant Lookup as Class Lookup System
participant Checker as Visibility Checker
VM->>OpHandler: Dispatch ZEND_FETCH_INNER_CLASS opcode
OpHandler->>Lookup: Retrieve outer and inner class entries
Lookup-->>OpHandler: Return class information
OpHandler->>Checker: Verify class visibility using zend_check_class_visibility
Checker-->>OpHandler: Return visibility status (valid/error)
OpHandler->>VM: Return inner class instance or trigger fatal error
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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: 10
🔭 Outside diff range comments (1)
tests/classes/inner_classes/inheritance.phpt (1)
1-15:⚠️ Potential issueTest is incomplete - missing assertions or expected output
The test demonstrates the inheritance syntax for inner classes, but has an empty
--EXPECT--section. Without any actual verification, this test merely confirms that the code compiles without errors but doesn't validate the runtime behavior.Consider enhancing the test to actually verify inner class inheritance works:
class Outer { abstract class Other {} class Middle extends Outer:>Other { class Inner1 extends Outer:>Other {} class Inner2 extends Outer:>Middle {} + + public function testInheritance() { + // Verify inner classes inherit correctly + var_dump(new Inner1 instanceof Outer::Other); + var_dump(new Inner2 instanceof Outer::Middle); + } } } + +$middle = new Outer::Middle(); +$middle->testInheritance(); ?> --EXPECT-- +bool(true) +bool(true)
🧹 Nitpick comments (9)
tests/classes/inner_classes/access_modifiers_004.phpt (1)
1-14: Appropriate test for access modifier restrictionsThis test correctly verifies that the
public(set)modifier cannot be applied to inner classes. The test follows standard PHPT format and includes the proper error message expectation.For consistency with error message standards, consider adding an article "a" before "inner class" in the error message to match PHP's usual error phrasing style.
-Fatal error: Cannot use the public(set) modifier on a inner class in %s on line %d +Fatal error: Cannot use the public(set) modifier on an inner class in %s on line %dtests/classes/inner_classes/reflection_001.phpt (1)
1-32: Test looks good but is missing validation for parent-child relationshipsThe test covers essential reflection functionality for inner classes including name handling and visibility checks. It correctly tests the fully qualified name format with the new
:>separator and validates that inner classes are properly identified.Consider enhancing this test to also verify parent-child relationships between classes, such as checking if
getDeclaringClass()returns the correct parent class for an inner class.You could add these checks:
// Check declaring class relationship $declaringClass = $ref->getDeclaringClass(); var_dump($declaringClass->getName() === 'n\s\Outer:>Middle');tests/classes/inner_classes/static_variables.phpt (1)
1-3: Good test description but consider adding negative/edge cases.These lines define the test scenario for verifying
::class, statics, and inner classes. While this is a good start, consider adding negative tests (e.g., referencing non-existent inner classes) to ensure that error handling is thoroughly tested.Zend/zend_language_parser.y (2)
630-637: New production rule forinner_class_modifiers.This rule defaults to
ZEND_ACC_PUBLICif no modifier is specified. Confirm whether the RFC intends all inner classes to be public by default, or if restricted access (private/protected) is also supported or planned.
960-963: Ruleinner_class_statementfor nested class declarations.Defining inner classes with a simple syntax
{ ... }approach is clear. Validate whether inherited usage of traits, interfaces, or abstract methods is allowed in inner classes and covered by tests.ext/reflection/php_reflection.c (2)
4078-4093: Adds method to check if a class is an inner classThe implementation uses a string-based approach, checking for the presence of ":>" in the class name, which works but might be brittle if naming conventions change. The TODO comment acknowledges this is a temporary solution.
Consider using a more robust approach with a dedicated flag or property indicating inner class status instead of 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 an inner class using dedicated property or flag + RETURN_BOOL(ce->lexical_scope != NULL);
4078-4127: Visibility methods need PHPDoc commentsThe new methods for checking class visibility (
isInnerClass,isPrivate,isProtected, andisPublic) lack proper documentation comments explaining their purpose and return values.Add PHPDoc comments to clearly document the purpose of each new method:
+/* {{{ Returns whether the current class is an inner class */ ZEND_METHOD(ReflectionClass, isInnerClass) { reflection_object *intern; zend_class_entry *ce; ZEND_PARSE_PARAMETERS_NONE(); GET_REFLECTION_OBJECT_PTR(ce); // 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); } +/* }}} */ +/* {{{ Returns true if the class is private */Similarly for the other methods.
Zend/zend_vm_execute.h (1)
10880-10895: Extract repeated returned-class scope checks into a helper.
These checks for “public method returning a private/protected class” are repeated in multiple places. Factor them into a dedicated helper function or macro to reduce duplication and ensure consistency.Zend/zend_vm_handlers.h (1)
1389-1877: Extensive addition of specialized arithmetic and comparison opcodes.A large set of specialized instructions has been introduced or re-introduced (e.g.,
ZEND_ADD_LONG_NO_OVERFLOW_SPEC_*,ZEND_IS_NOT_EQUAL_DOUBLE_SPEC_*). While these can optimize performance by handling specific operand types, ensure they are fully integrated in the engine (handlers, JIT, etc.). Also confirm there are no redundant or conflicting definitions.Would you like a script to cross-check each opcode definition with corresponding handlers and usage sites to confirm consistency?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
Zend/zend.h(1 hunks)Zend/zend_ast.h(1 hunks)Zend/zend_compile.c(9 hunks)Zend/zend_compile.h(1 hunks)Zend/zend_execute.c(1 hunks)Zend/zend_language_parser.y(8 hunks)Zend/zend_language_scanner.l(1 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/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/inheritance.phpt(1 hunks)tests/classes/inner_classes/inner_classes.inc(1 hunks)tests/classes/inner_classes/properties_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)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (109)
ext/tokenizer/tokenizer_data.stub.php (1)
715-719: Addition of T_INNER_REF token looks goodThe addition of the
T_INNER_REFconstant follows the established pattern in the file. This new token is necessary for the tokenizer to recognize the inner class reference syntax (:>).tests/classes/inner_classes/access_modifiers_006.phpt (1)
1-13: Test case for private(set) modifier restriction is well structuredThis test case correctly verifies that the
private(set)modifier cannot be used on inner classes. The test structure follows the standard PHPT format with proper sections for test description, code, and expected output.ext/tokenizer/tokenizer_data.c (1)
170-170: T_INNER_REF token handler properly addedThe new case for
T_INNER_REFtoken is correctly implemented in theget_token_type_namefunction, ensuring that the string representation of this token is available when needed.tests/classes/inner_classes/access_modifiers_003.phpt (1)
1-13: Test case for static modifier restriction is well structuredThis test case correctly verifies that the
staticmodifier cannot be used on inner classes. The expected fatal error is consistent with the language's design constraints for inner classes.tests/classes/inner_classes/simple_declaration_002.phpt (1)
1-14: Valid test for ensuring inner classes cannot be declared inside functionsThis test correctly verifies that inner classes cannot be declared inside function bodies, which is a critical restriction to document and enforce. The test structure follows the standard PHPT format with clear sections for test name, code, and expected output.
The error message is properly formatted to use %s and %d placeholders for dynamic file paths and line numbers.
tests/classes/inner_classes/autoload_002.phpt (1)
1-17:✅ Verification successful
Well-structured test for private inner class autoloading
This test properly verifies that the autoloader is triggered for inner classes and that visibility modifiers (private) are respected even through autoloading. The test expects the correct behavior pattern of first showing the autoload event and then failing with an appropriate error.
Note that this test depends on an external file
inner_classes.incin the same directory. Please ensure this dependency file is included in the PR or exists in the target environment.
🏁 Script executed:
#!/bin/bash # Check if the required include file exists if [ -f "tests/classes/inner_classes/inner_classes.inc" ]; then echo "Dependency file exists" cat "tests/classes/inner_classes/inner_classes.inc" else echo "Missing dependency: tests/classes/inner_classes/inner_classes.inc" fd -p "inner_classes.inc" -t f fiLength of output: 323
Autoload Test and Dependency Confirmed: No Further Action Required
- The test in
tests/classes/inner_classes/autoload_002.phptcorrectly triggers the autoloader for inner classes and verifies that private visibility is enforced.- The dependency file
tests/classes/inner_classes/inner_classes.incwas confirmed to exist and contains the expected class definitions, including the privateLineinner class.tests/classes/inner_classes/simple_declaration_001.phpt (1)
1-22: Effective test for nested class declarationsThis test thoroughly verifies the support for multi-level inner class declarations and provides validation for both string-based and ::class constant references to nested classes. The test ensures that the inner class syntax with
:>works as intended.The test is well-structured and correctly validates all the expected outcomes with appropriate assertions.
Zend/zend_vm_def.h (3)
1801-1886: New opcodeZEND_FETCH_INNER_CLASSadded to handle inner class resolution.This opcode handles fetching inner class declarations. Here's how it works:
- Takes the outer class name and inner class name as operands
- Resolves the outer class:
- From constant (op1 is CONST)
- From scope (op1 is UNUSED)
- From variable (op1 is VAR)
- Constructs the full inner class name by concatenating:
- Outer class name
- ":>" separator
- Inner class name
- Looks up the inner class in the class table
- Validates access:
- Checks if inner class is private/protected
- Verifies calling scope has access
- Caches the resolved inner class for future lookups
- Returns the inner class entry
The opcode properly handles:
- Different ways of specifying the outer class
- Access control for inner classes
- Caching for performance
- Error cases like class not found
This is a key part of implementing inner class support in PHP.
4519-4534: Added inner class access validation inZEND_VERIFY_RETURN_TYPE.This code adds validation for returning inner class instances:
- Checks if the returned class has scope restrictions (
required_scope)- For public methods:
- Prevents returning private inner classes
- Prevents returning protected inner classes
- For protected methods:
- Prevents returning private inner classes from a different scope
This ensures proper access control when returning inner class instances.
6282-6283: Added inner class handling toZEND_FETCH_CLASS.The comment indicates that inner class references are caught at compile-time for the
ZEND_FETCH_CLASSopcode.tests/classes/inner_classes/access_modifiers_005.phpt (1)
1-14: Test correctly verifies that protected(set) cannot be used on inner classes.This test ensures that the protected(set) access modifier is properly rejected when applied to an inner class, with an appropriate error message. The test follows the standard PHP test structure with clear test description, code sample, and expected output.
tests/classes/inner_classes/access_modifiers_001.phpt (1)
1-14: Test correctly validates multiple access modifiers restriction.This test ensures that using multiple access modifiers (public and private together) on an inner class is properly rejected with an appropriate error message. The test follows the standard PHP test structure and helps verify that inner classes maintain consistency with PHP's existing access modifier rules.
tests/classes/inner_classes/access_modifiers_002.phpt (1)
1-14: Test correctly verifies that invalid syntax is rejected.This test ensures that using an invalid syntax for inner class declarations (putting "int" before "class") is properly rejected with a clear parse error. This helps maintain the consistency of the PHP language grammar for inner classes.
Zend/zend_language_scanner.l (1)
1604-1606: New token T_INNER_REF for inner class references.You've added the ":>" token to the lexical scanner, which will be recognized and returned as T_INNER_REF. This token is essential for supporting the inner class reference syntax as described in the RFC. This addition is properly placed in the ST_IN_SCRIPTING state, consistent with other language tokens.
ext/tokenizer/tokenizer_data_arginfo.h (1)
148-148: New token registration for inner class references looks goodThe registration of the
T_INNER_REFtoken is properly added to support the new inner class reference syntax. This will allow the tokenizer to recognize the new syntax element.tests/classes/inner_classes/autoload_001.phpt (1)
5-8: The autoload implementation looks goodThe autoload function is properly registered to handle loading the class files. The implementation correctly loads files based on the class name and outputs a log message.
Zend/zend_compile.h (2)
897-897: Appropriate addition of inner class to modifier targetsThe addition of
ZEND_MODIFIER_TARGET_INNER_CLASSto thezend_modifier_targetenum is appropriate and allows for proper handling of modifiers for inner classes.
1-1398:❓ Verification inconclusive
Check for additional places needing updates for inner class support
While adding the modifier target is a good start, inner class implementation likely requires additional changes throughout the codebase for complete support. Consider reviewing whether other parts of this file need updates for inner classes (such as visibility handling, inheritance rules, etc.).
🏁 Script executed:
#!/bin/bash # Search for references to enum type or related constants echo "Searching for references to zend_modifier_target..." rg "zend_modifier_target" --type c --type cpp --type h echo "\nSearching for functions that might need updates for inner classes..." rg "zend_modifier_list_to_flags|zend_add_member_modifier" --type c --type cpp --type h echo "\nSearching for class compilation related functions..." rg "zend_compile_class|class_declaration" --type c --type cpp --type hLength of output: 3998
Action: Verify Additional Inner Class Handling Enhancements
The current changes correctly incorporate the new
zend_modifier_targetin modifier functions (e.g.zend_modifier_list_to_flagsandzend_add_member_modifier). Our search confirms that these functions—as well as the related class compilation routines (zend_compile_class_ref,zend_compile_class_decl, etc.)—already reference the modifier target. However, inner class support may still require a broader review of visibility handling, inheritance, and member resolution throughout the compilation process.
- Review Modifier Functions: Confirm that usage of
zend_modifier_targetinzend_modifier_list_to_flagsandzend_add_member_modifieradequately covers all cases for inner classes.- Examine Class Compilation Routines: Verify that functions like
zend_compile_class_refandzend_compile_class_declcorrectly handle inner class scenarios, particularly for inheritance, visibility, and namespace-related behavior.- Check Peripheral Updates: Determine if additional updates are needed in areas such as error handling, attribute processing, or AST adjustments when inner classes are involved.
Please review these aspects manually to ensure complete and consistent support for inner classes.
Zend/zend_ast.h (1)
136-136: LGTM - AST node for inner classes added correctlyThe
ZEND_AST_INNER_CLASSconstant is appropriately placed in the "2 child nodes" section of the enum, maintaining the organizational structure of the AST system.tests/classes/inner_classes/simple_declaration_003.phpt (1)
1-21: Test validates basic inner class functionality effectivelyThis test effectively verifies the basic syntax and functionality of nested inner classes, including:
- Proper declaration of multi-level inner classes
- The ability to instantiate an inner class using the
:>syntax- Correct handling of fully qualified class names that include the namespace and parent classes
The test's expectations match the implementation.
tests/classes/inner_classes/inner_classes.inc (1)
1-9: Good implementation of inner classes structure.The code correctly defines a class with both public and private inner classes. This demonstrates the basic functionality of the new inner class feature, with appropriate visibility modifiers.
Zend/zend.h (1)
167-169: Proper extension of class_entry structure to support inner classes.The three new fields added to the
_zend_class_entrystructure are essential for implementing inner class functionality:
required_scope: Stores the parent class that defines visibility boundarieslexical_scope: Keeps track of the lexical context where the inner class is definedrequired_scope_absolute: Likely indicates if the scope requirements are absoluteThese additions are minimal and focused on supporting the necessary scoping rules for inner classes.
tests/classes/inner_classes/return_types_004.phpt (1)
1-26: Comprehensive test for private inner class visibility enforcement.This test correctly verifies that PHP enforces visibility rules when a private inner class is used as a return type. The test ensures that a public method cannot return a private inner class, which is an important visibility constraint for maintaining proper encapsulation.
The error message is clear and descriptive, helping developers understand why their code fails when they violate this visibility rule.
Zend/zend_vm_opcodes.h (1)
294-296:✅ Verification successful
Proper VM opcode addition for inner class support.
The new
ZEND_FETCH_INNER_CLASSopcode (210) has been added to support the inner class functionality at the VM level. TheZEND_VM_LAST_OPCODEconstant has been appropriately updated to reflect this addition.Make sure the opcode implementation in zend_vm_def.h and related handlers are complete to properly support this new feature.
🏁 Script executed:
#!/bin/bash # Check if the ZEND_FETCH_INNER_CLASS opcode is properly implemented in VM definition files # Look for ZEND_FETCH_INNER_CLASS in zend_vm_def.h echo "Checking for ZEND_FETCH_INNER_CLASS implementation in zend_vm_def.h:" rg "ZEND_FETCH_INNER_CLASS" Zend/zend_vm_def.h # Look for zend_compile_inner_class_ref function which should emit this opcode echo -e "\nChecking for inner class compilation functions:" rg "zend_compile_inner_class_ref" Zend/zend_compile.cLength of output: 786
Opcode addition for inner class support is fully integrated.
- The new
ZEND_FETCH_INNER_CLASSopcode (ID: 210) is correctly defined inzend_vm_def.hwith the appropriate handler.- The function
zend_compile_inner_class_refinzend_compile.cconfirms that inner class references are being compiled to emit this opcode.- The update to
ZEND_VM_LAST_OPCODEaccurately reflects this addition.No further changes are required based on the current verification.
Zend/zend_vm_opcodes.c (4)
25-25: Increase in opcode array size for new FETCH_INNER_CLASS opcode.The array size has been increased from 210 to 211 to accommodate the new ZEND_FETCH_INNER_CLASS opcode. This is part of implementing inner class support in PHP.
236-236: New opcode added for inner class support.A new opcode "ZEND_FETCH_INNER_CLASS" has been added to support the fetching of inner class definitions at runtime. This is consistent with the naming convention of other FETCH opcodes in the engine.
239-239: Corresponding flags array size increase.The flags array size has been increased to match the size of the opcodes array, maintaining consistency between the two related arrays.
450-450: Flag value defined for the new inner class opcode.The flag value 0x00040307 has been added for the ZEND_FETCH_INNER_CLASS opcode. This defines the opcode's behavior characteristics in the VM.
Zend/zend_opcode.c (1)
433-436: Properly release references to required_scope for inner classes.This code ensures that when a class is destroyed, if it has a reference to its required scope (outer class), that reference is properly decremented and cleared. This prevents memory leaks when dealing with inner classes, as the parent-child class relationship needs proper cleanup.
tests/classes/inner_classes/return_types_005.phpt (4)
1-3: Good test case for protected return type visibility.This test validates an important aspect of inner class visibility - a public method cannot return a protected inner class type. The test name and description clearly indicate what's being tested.
6-12: Proper test class structure with protected inner class.The test creates an appropriate class hierarchy with a protected inner class and a public method that attempts to return an instance of that inner class. The
:>syntax is used correctly to reference the inner class.
14-20: Complete test scenario covering multiple edge cases.This test covers:
- Calling the method from outside the class scope
- Trying to use the returned value in a function with a type hint
- Attempting to dump the returned value
This provides comprehensive coverage of the protected return type behavior.
21-26: Appropriate error expectations for visibility violation.The expected output correctly specifies that a TypeError should be thrown when a public method tries to return a protected inner class. The error message and stack trace expectations are properly formatted.
Zend/zend_compile.c (9)
896-909: Confirm valid modifiers for inner classes.
These lines addZEND_MODIFIER_TARGET_INNER_CLASSto the checks forT_READONLY,T_ABSTRACT, andT_FINAL. While the changes appear consistent with the introduction of inner classes, please ensure that no additional or conflicting modifiers are required or disallowed per the RFC or language specifications.Would you like to run a quick test or code scan to verify that these modifications fully align with the inner class RFC?
947-948: Descriptive label for inner class modifier handling.
Adding"inner class"underZEND_MODIFIER_TARGET_INNER_CLASSclarifies error messages and debugging. No issues found here.
1056-1086: Verify locked-down constraints on inner classes.
This block disallows multiple access modifiers, forbids static inner classes, and disallows explicit public/protected/private sets. While it aligns with a restricted approach to inner classes, please confirm this matches the intended design from the RFC and doesn’t inadvertently limit valid usage scenarios.Would you like a script to scan the tests verifying these constraints or to confirm references in the RFC?
1799-1827: FQCN assembly for nested classes.
Thezend_resolve_nested_class_name()function neatly composes a fully qualified name by chaining “outer:>inner”. Implementation looks correct for preventing leaks and properly releasingouter_name.
1866-1870: Support nested class AST resolution.
If the AST node is an inner class, the function resolves its full name for constant expressions. This logic is straightforward and seems sound for your new feature.
2923-2960: New compilation routine for reference to inner classes.
zend_compile_inner_class_refbuilds references by recursively compiling the outer class, then the inner class, finally emittingZEND_FETCH_INNER_CLASS. This is a solid approach.Would you consider expanding test coverage for multi-level nesting (e.g.,
ClassA::ClassB::ClassC) to ensure correct resolution?
7429-7431: Inner class type initialization.
The code returns a correctly initialized class type forZEND_AST_INNER_CLASS. This aligns well with other additions.
9182-9224: Renaming and scope-handling for inner classes.
Here you rename the inner class and set various flags (e.g.,required_scope,lexical_scope) to treat it as top-level. Make sure the re-labelling (:>) and these flags correctly reflect the intended scope and do not conflict with any namespace or inheritance constraints.Let me know if you’d like an automated search or an additional test to confirm no collisions occur in multi-level nested scenarios.
11852-11854: Add support for fetching inner classes in expressions.
The switch case forZEND_AST_INNER_CLASSsimply callszend_compile_inner_class_ref, which is consistent with the rest of these changes.tests/classes/inner_classes/properties_001.phpt (1)
1-24: Test case validates inner class property typing and visibility rulesThis test effectively validates a key aspect of the inner class RFC: accessing private inner class instances through public methods is not allowed, correctly resulting in a TypeError.
The syntax
self:>Innerfor property type declaration is a good demonstration of how inner classes are referenced.ext/opcache/zend_persist.c (2)
1124-1134: Implementation for persisting inner class scope informationGood addition of the
zend_update_required_scopefunction which ensures that inner class scope relationships are properly maintained when classes are cached by opcache. This is crucial for preserving the lexical scoping rules of inner classes.
1309-1309: Appropriate placement of scope update in class table persistenceThe call to
zend_update_required_scopeis correctly placed after the parent class update, ensuring that class relationships are fully established before scoping relationships are finalized.ext/reflection/php_reflection_arginfo.h (5)
2-2: Updated stub hash reflects the changes to ReflectionClass.The hash has been updated to match the newly added methods in the ReflectionClass, which is the correct approach when modifying method signatures.
369-370: LGTM: New isInnerClass method with correct return type.The isInnerClass method has been correctly defined with the same argument info as other boolean-returning reflection methods.
371-376: LGTM: Visibility check methods for ReflectionClass.These methods (isPrivate, isProtected, isPublic) follow the same pattern as existing visibility methods for other reflective entities like ReflectionMethod and ReflectionProperty.
858-861: LGTM: Method declarations for new ReflectionClass methods.The ZEND_METHOD declarations for the new reflection methods have been properly added.
1154-1157: LGTM: Method registrations in ReflectionClass's function table.All four new methods are correctly registered with the appropriate access modifiers and argument info.
tests/classes/inner_classes/access_modifiers_007.phpt (5)
1-3: LGTM: Properly structured test for abstract inner classes.The test name and description clearly indicate the purpose of testing abstract inner classes.
6-8: LGTM: Outer class with abstract inner class definition.The syntax for declaring an abstract inner class is clear and follows expected patterns for class modifiers.
10-10: LGTM: Inner class extension syntax.The
:>syntax is used correctly to extend the inner class, demonstrating the proper way to reference inner classes.
12-17: LGTM: Comprehensive test cases for abstract inner classes.The test covers three important aspects:
- Instantiating a concrete class that extends an abstract inner class
- Using reflection to verify the inner class is abstract
- Confirming that direct instantiation of the abstract inner class fails
This ensures proper abstract class behavior is maintained for inner classes.
19-27: LGTM: Expected output verifies all test cases.The expected output correctly includes:
- The successful instantiation of the extended class
- Verification that isAbstract() returns true
- The appropriate fatal error when attempting to instantiate an abstract class
This confirms that abstract inner classes behave like regular abstract classes.
tests/classes/inner_classes/return_types_003.phpt (5)
1-2: LGTM: Clear test purpose for protected inner classes.The test name clearly indicates it's testing protected inner class functionality.
6-12: LGTM: Properly structured outer class with protected inner class.The test correctly defines:
- A protected inner class
- A protected method that returns an instance of that inner class
- Proper use of the
self:>syntax to reference the inner class in the return typeThis tests type declaration support for inner classes.
14-19: LGTM: Testing method visibility and return type compatibility.The derived class Foo changes the visibility of getInner() from protected to public, which should create a conflict when returning a protected inner class type.
21-23: LGTM: Comprehensive test of both successful and error cases.The test attempts both:
- A valid execution path to demonstrate the inner class can be used normally
- A path that should trigger a type error due to visibility mismatch
This covers important edge cases for visibility restrictions.
25-33: LGTM: Expected output verifies type enforcement.The expected output correctly shows:
- Successful use of the inner class within allowed contexts
- A TypeError when attempting to return a protected class from a public method
This confirms PHP properly enforces visibility restrictions for inner class types.
ext/reflection/php_reflection.stub.php (2)
436-437: LGTM: isInnerClass method with correct signature.The isInnerClass() method is defined with the proper return type, consistent with other boolean-returning reflection methods.
438-442: LGTM: Visibility check methods match existing reflection patterns.These three methods (isPrivate, isProtected, isPublic) follow the same pattern as other reflection visibility methods, providing a complete set of visibility checks for classes.
tests/classes/inner_classes/return_types_002.phpt (4)
6-16: Clear implementation of inner class return type handlingThe Outer class demonstrates proper inner class structure with the new inner class syntax. The
self:>Innernotation is used correctly to reference the inner class within its containing class. The code effectively tests the encapsulation of private inner classes.
18-23: Good test for inheritance constraints with inner classesThis child class implementation correctly tests the boundary between inheritance and visibility. The
parent:>Innerreturn type syntax is being used appropriately to test access limitations across class boundaries.
25-26: Effective test case executionThis test correctly triggers the visibility constraint that should be enforced when attempting to access a private inner class through inheritance.
28-32: Expected output reflects correct inner class visibility behaviorThe expected output appropriately demonstrates that:
- The inner instance can be obtained through
parent::getInner2()(via internal access)- Direct instantiation of a private inner class from outside its containing class fails with a fatal error
This matches expected OOP encapsulation principles for the new inner class feature.
tests/classes/inner_classes/return_types_006.phpt (3)
6-14: Good test for inner class method chainingThis test case effectively demonstrates how a private inner class can create instances of itself and return them through method chaining. The one-liner implementation of the
test()method in the outer class is clean and concise.
16-17: Test properly verifies inner class instantiation chainThis test properly verifies that an inner class can be returned through a chain of method calls while maintaining proper visibility access.
19-21: Expected output matches the test's purposeThe expected output correctly shows that we should get an instance of the private inner class after the method chain completes execution.
tests/classes/inner_classes/visibility_001.phpt (3)
6-20: Good visibility test between inner classesThis test effectively demonstrates that private inner classes within the same outer class can reference each other. The parameter type declaration for the constructor properly uses the fully qualified name
Outer:>Inner.
21-21: Simple and effective test executionThe test calls the static method that exercises the inner class relationship without unnecessary complexity.
23-24: Missing expected outputThe test has an empty expected output section. Either the test is meant to simply verify that no errors occur (which would be better expressed with an explicit comment), or the expected output from the
var_dumpin thereset()method is missing.Consider adding the expected output from the
var_dump($prev === $this->inner)call, which should bebool(false)since the two instances should be different objects.tests/classes/inner_classes/static_variables.phpt (3)
24-30: Expected output looks consistent with feature goals.The checks for class names, constant values, and static property values appear correct. No immediate issues detected.
17-22:✅ Verification successful
Confirm custom inner class dereferencing syntax.
The
:>operator is unique to this RFC. Verify that the engine changes support this syntax comprehensively (e.g., with constants, static properties, and reflection).
🏁 Script executed:
#!/bin/bash # Search for :> usage in test files for broader coverage rg ":>" tests/classes/inner_classesLength of output: 7072
Custom inner class dereferencing syntax verified
The tests in
tests/classes/inner_classescover the use of the:>operator across multiple scenarios—including constants, static properties, instantiation, and reflection—which confirms comprehensive support for this custom syntax. For instance:
- Usage in static variables and constants is demonstrated in
static_variables.phpt.- Instantiation and inner class access are exercised in tests like
visibility_001.phptandsimple_declaration_004.phpt.- Reflection and other return type scenarios are well covered in tests under
reflection_001.phptand variousreturn_types_*.phptfiles.No further changes are needed.
6-15:✅ Verification successful
Validate property types and class nesting RFC usage.
The nested class approach with
Outer -> Middle -> Inneris logical and aligns with the proposed RFC. Ensure that typed properties (public static int $bar) are supported by the target PHP version.
🏁 Script executed:
#!/bin/bash # Ensure the PHP version supports typed properties php --versionLength of output: 216
PHP version and nested class RFC usage verified—no further changes required.
The PHP version (8.2.26) supports typed properties, and the nested class structure in the test file aligns with the intended RFC usage. All aspects of the snippet have been confirmed as valid.
Zend/zend_language_parser.y (3)
279-279:❓ Verification inconclusive
Added
inner_type_without_staticin type declarations.This new grammar element helps parse nested type references. Verify that the rest of the type-checking logic and error messages are updated to handle this rule properly.
🏁 Script executed:
#!/bin/bash # Look for usage of 'inner_type_without_static' to confirm integration rg "inner_type_without_static" Zend/Length of output: 362
Verify updates for nested type grammar rule
inner_type_without_staticThe grep output confirms that the new grammar element is correctly added in
Zend/zend_language_parser.y(both in the%typedeclaration and the production rules). However, please verify that all related type-checking logic and error messages have been updated appropriately to handle nested type references introduced by this change.
- Confirm that downstream modules responsible for type-checking properly integrate this new rule.
- Ensure error messaging reflects any issues arising from nested type references.
984-985:❓ Verification inconclusive
Attaching modifiers to inner class statements.
Allowing modifiers on inner classes (e.g.,
final,abstract) broadens the feature’s scope. Validate that usage (likeprotected inner class) either works as intended or explicitly fails if not supported by the RFC.
🏁 Script executed:
#!/bin/bash #!/bin/bash # Look for tests or code references to confirm enforcement of inner class modifiers rg "inner_class_modifiers"Length of output: 292
Verify Correct Handling of Inner Class Modifiers
The production in
Zend/zend_language_parser.y(lines 984–985) now attaches the inner class modifiers to the AST node by executing{ $$ = $2; $$->attr = $1; } ;Our grep search confirms the presence of the
inner_class_modifiersproduction. However, please ensure that:
- Modifiers such as
final,abstract, and especially cases likeprotected inner classbehave as intended.- If the RFC does not support certain modifier combinations, the parser should flag them with an explicit error.
Please validate this behavior by either reviewing additional tests or adding test cases as needed.
234-234:✅ Verification successful
Token definition for inner class references.
Introducing
T_INNER_REF "':>'"extends the parser to handle the new symbol for inner classes. Ensure you’ve considered potential conflicts with object operators (->) and static references (::).
🏁 Script executed:
#!/bin/bash # Check if there's an overlap with existing operators in the codebase rg --context 4 "->|::" Zend/Length of output: 435
Operator Conflict Verification: Please Re-run the Search Command with Corrected Regex
The original search command failed due to misinterpretation of the operators. To verify that there’s no unintended overlap with existing operators (
->and::), please use the revised command below:This command uses multiple
-eflags to avoid argument parsing errors. Once you run this script, please verify that there are no unexpected conflicts with the new token definition for inner class references.
🏁 Script executed:
#!/bin/bash # Correctly search for object operators '->' and static references '::' within the Zend directory rg --context 4 -e "->" -e "::" Zend/Length of output: 71470
Token Extension Verified: No Conflicts Detected
After re-running the search with the corrected regex, we see that while the object operator (
->) and static reference (::) are widely used throughout the codebase, there is no indication that they conflict with the newly defined token for inner class references (T_INNER_REF "':>'"). The literal string':>'is sufficiently distinct from both->and::, so no ambiguity or overlapping behavior should occur.
- The corrected
rgcommand confirmed that occurrences of->and::are present in standard contexts and do not mix with the new token.- The new inner class token remains syntactically and semantically distinct from the existing operators.
ext/reflection/php_reflection.c (3)
4095-4104: Class visibility check for private classesThis implementation correctly determines if a class is private by checking both
required_scopeandrequired_scope_absoluteare set. This aligns with the expected behavior for inner class visibility controls.
4106-4116: Class visibility check for protected classesThis implementation determines protected visibility by checking if
required_scopeis set butrequired_scope_absoluteis not. The logic is sound and follows the expected visibility semantics.
4118-4127: Class visibility check for public classesThis implementation correctly determines if a class is public by checking if
required_scopeis not set, which indicates the class has no visibility restrictions.tests/classes/inner_classes/simple_declaration_004.phpt (6)
1-3: LGTM - Test header correctly describes the purpose.The test header clearly indicates this test is for validating scope resolution access with inner classes.
6-15: Interesting syntax for inner classes and scope resolution.The code demonstrates two key aspects of the new inner class feature:
- Declaration syntax:
public class Middle {}within another class- Return type and instantiation syntax using
:>operator:self:>Middleandnew self:>Middle()The test correctly differentiates between
selfandstaticscope resolution operators, which have different behaviors when inherited.
17-24: Good demonstration of inheritance with inner classes.This code properly tests inner class inheritance using
extends parent:>Middle. This validates that inner classes can extend inner classes from parent classes, which is a fundamental feature for inner class functionality.
26-31: Comprehensive test cases for different scope resolution scenarios.These test cases cover all important variations:
Outer::testStatic()andOuter::testSelf()test direct class accessOuter2::testParent()tests parent class inner class accessOuter2::testStatic()andOuter2::testSelf()test inherited method behaviorThis ensures the scope resolution operators work correctly in all contexts.
33-43: Expected output correctly matches scope resolution rules.The expected output correctly shows:
self:>Middleconsistently resolves toOuter:>Middlestatic:>Middledynamically resolves to the calling class's inner class (Outer2:>Middlewhen called fromOuter2)parent:>Middlecorrectly resolves to the parent class's inner classThese expectations align with PHP's traditional scope resolution behavior extended to inner classes.
7-7:✅ Verification successful
Verify inner class visibility modifier behavior.
The test uses
publicvisibility for inner classes. It would be valuable to verify that all visibility modifiers (private, protected) work as expected with inner classes.
🏁 Script executed:
#!/bin/bash # Check if there are tests for different visibility modifiers with inner classes fd -t f -e phpt . tests/classes/inner_classes/ | xargs grep -l "private class\|protected class" | head -5Length of output: 359
Verification: Inner Class Visibility Modifier Tests Present
The repository already contains tests validating inner class behavior with
privateandprotectedmodifiers. For example, tests such as:
tests/classes/inner_classes/access_modifiers_001.phpttests/classes/inner_classes/properties_001.phpttests/classes/inner_classes/return_types_002.phpttests/classes/inner_classes/return_types_003.phpttests/classes/inner_classes/return_types_004.phptconfirm coverage for non-public inner classes. No further action is needed for the simple declaration test.
Zend/zend_vm_execute.h (20)
3313-3314: Ensure completeness of this switch case.
Defining a newcase ZEND_FETCH_INNER_CLASSwithout additional processing is valid if all logic is handled in the extended opcode handlers. Confirm that no extra handling is needed here (e.g., logging, error checks, or specialized logic).Would you like to verify consistency with the rest of the opcode handling via a search script or manual check?
21719-21734: Duplicate scope check logic.
This block repeats the “public method returning a private/protected class” checks. Refer to earlier comments for a proposed refactor approach.
30201-30216: Duplicate scope check logic.
Same comment applies here as in previous sites—consider centralizing this logic.
38081-38096: Duplicate scope check logic.
Same repeated pattern when returning a private/protected inner class from a public/protected method.
50896-50911: Duplicate scope check logic.
Same repeated pattern again.
57953-57957: New jump table labels look consistent.
Defining new handlers here looks correct. No immediate issues.
59829-59833: Hybrid VM case for inner class fetching.
These new HYBRID_CASE definitions properly link the opcode to its handler. Looks good.
61104-61108: Correct addition of hybrid opcode case.
No concerns—consistent with other cases above.
62697-62701: Unused-const hybrid VM case.
Again, consistent with the pattern for new fetch handlers.
67160-67164: Added to opcode handler funcs array.
The references match the newly introduced handlers. Looks correct.
68122-68122: Verify this magic number.
Confirm that “3491” references the correct specialized rule/label.
68156-68156: Repeated magic number usage.
As above, ensure these numeric codes match the intended specialized handlers.
68287-68333: Large block of repeated “3491” lines.
Double-check that all references to “3491” are correct for each specialized handler scenario.
68505-68532: New specification flags.
Defining specialized opcode rules for commutative or typed operations is standard. Implementation here appears consistent with existing patterns.
68532-68545: Further specialized-ops definitions.
Nothing problematic stands out. Good to see consistent patterns for typed ops.
68553-68566: Commutative rules for multiplication.
Implementation follows the typical approach.
68574-68584: IS_IDENTICAL specialized logic.
Matches existing style.
68592-68602: IS_NOT_IDENTICAL specialized logic.
Again, consistent with the engine’s patterns.
68610-68623: IS_EQUAL specialized logic.
No apparent issues.
68626-68750: Additional specialized instructions.
Extensive assignment, increment/decrement, and jump logic expansions look fine.Zend/zend_vm_handlers.h (2)
1375-1378: Introduction of new opcodes for fetching inner classes.It's great to see these newly introduced
ZEND_FETCH_INNER_CLASS_SPEC_*opcodes aligning with the inner class feature. Make sure the opcode handlers are properly implemented elsewhere in the engine (e.g., inzend_vm_execute.h) and thoroughly tested to ensure that nested class references behave correctly during compilation and execution.Would you like a script to grep for these opcodes in the codebase to confirm that all necessary handler implementations exist, or to review relevant test coverage?
1379-1384: Inconsistency with summary regarding reintroduced opcodes.The summary mentions removing certain opcodes like
ZEND_INIT_FCALL_OFFSET_SPEC_CONSTandZEND_COUNT_ARRAY_SPEC_*, yet here they appear to be added. This discrepancy suggests either the summary is outdated or the code changes reintroduce these opcodes for a valid reason.Please verify whether reintroducing these opcodes is intentional or if there was a merge conflict or stale reference. You can also confirm their usage with:
#!/bin/bash # Search for all references to these opcodes across the codebase rg "ZEND_INIT_FCALL_OFFSET_SPEC_CONST|ZEND_RECV_NOTYPE_SPEC|ZEND_COUNT_ARRAY_SPEC" -A 5Likely an incorrect or invalid review comment.
This is a bit of a theoretical issue, but the maximum string length is actually ZSTR_MAX_LEN instead of SIZE_MAX. The resulting check is a bit slower but should still be relatively cheap. Closes phpGH-18049.
A frameless icall with 3 arguments is a special case because it uses OP_DATA, but this was not added to the list, so the opline pointed to the wrong address resulting in UBSAN report or crash. Closes phpGH-18048.
* PHP-8.4: Fix phpGH-18037: SEGV Zend/zend_execute.c
* PHP-8.3: Correct check for maximum string length in JIT helpers
* PHP-8.4: Correct check for maximum string length in JIT helpers
Co-authored-by: Niels Dossche <[email protected]>
* PHP-8.4: Fixed pointer subtraction for scale (php#17986)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Zend/zend_vm_def.h (1)
8292-8293: Added inner class case toZEND_FETCH_CLASSopcode.The
ZEND_FETCH_CLASSopcode has been updated to handle inner class fetching by adding a case forZEND_FETCH_INNER_CLASSin the list of class fetch types.Consider adding a comment to document this new case:
case ZEND_FETCH_CLASS: case ZEND_DECLARE_ANON_CLASS: + /* Handle inner class fetching */ case ZEND_FETCH_INNER_CLASS:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Zend/zend_execute.c(2 hunks)Zend/zend_object_handlers.c(1 hunks)Zend/zend_vm_def.h(3 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)
⏰ 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: FREEBSD
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (16)
Zend/zend_object_handlers.c (1)
404-407: Critical enhancement for inner class property access.This code modification allows an inner class to access private properties of its outer class, which is essential for nested class functionality. The condition checks if the current scope has a lexical scope (outer class) that matches the class containing the property.
tests/classes/inner_classes/visibility_001.phpt (3)
6-13: Proper test for outer class visibility restrictions.The test correctly validates that a private inner class (
Inner) cannot be assigned to a public property ($illegal), enforcing proper encapsulation rules for inner classes. TheOuter:>Innersyntax is used to reference the inner class type.
15-18: Effective test case setup.This section correctly tests the scenario by creating an instance of
Outer, calling thetestmethod, and attempting to dump the object, which will fail with the expected fatal error.
20-25: Appropriate error expectation.The expected output correctly anticipates a TypeError about assigning a private inner class to a property with higher visibility. The error message and stack trace format are properly defined.
tests/classes/inner_classes/visibility_002.phpt (3)
6-17: Correctly tests inner class access to outer class privates.This test effectively demonstrates that an inner class can access private members of its outer class. The
Innerclass correctly accesses and modifies the private$illegalproperty of theOuterclass, which is allowed by the code changes inzend_object_handlers.c.
19-23: Proper test execution.This section correctly sets up and executes the test scenario by creating an
Outerinstance, calling itstestmethod, and dumping the object to verify that the private property has been successfully modified.
25-30: Expected output confirms successful access.The expected output correctly verifies that the inner class has successfully accessed and modified the private property of the outer class, showing the object structure with the
Innerinstance assigned to the private property.tests/classes/inner_classes/visibility_004.phpt (1)
6-13: Verifies visibility rules for protected inner classes.Similar to visibility_001.phpt, this test correctly validates that a protected inner class cannot be assigned to a public property, maintaining proper encapsulation for inner classes with different visibility levels.
tests/classes/inner_classes/visibility_003.phpt (5)
6-17: Interesting approach to inner class property access.This test confirms that inner classes can access outer class private properties, which creates a logical encapsulation model where inner classes are treated as part of the outer class's implementation. The
private Outer:>Inner $illegalproperty syntax uses the new:>notation to properly type a property as an inner class instance.
12-12: New syntax for inner class type declaration.The property declaration uses the new
:>syntax to denote an inner class type. This is a key syntax addition for this feature and appears consistent with other parts of the implementation.
14-16: Inner class instantiation syntax is clear.The test method demonstrates the correct way to instantiate an inner class from within an outer class method using
new Outer:>Inner(). The chained method call totest($this)passes the outer instance to the inner class, completing the visibility test.
25-30: Expected result validates the implementation.The expected output confirms that the inner class implementation works as designed - the inner class instance was successfully assigned to the outer class's private property, demonstrating proper scoping rules for inner classes.
7-11:✅ Verification successful
Confirm inner class access scope design.
The inner class is correctly implemented as private, making it only accessible from within the outer class. This test shows that inner classes can modify the private state of their containing class, which is consistent with how inner classes work in other languages like Java.
🌐 Web query:
How do inner classes access outer class private members in Java?💡 Result:
In Java, inner classes (non-static nested classes) can access all members of their outer class, including private fields and methods, due to their inherent association with an instance of the enclosing class. Here's how this mechanism works:
Inner Class Access to Outer Class Members
- Implicit Reference: Every inner class instance holds an implicit reference to the enclosing outer class object. This allows direct access to the outer class's members without explicit qualification.
- Scope Privileges: The inner class operates within the scope of the outer class, granting it full access privileges, including private members. For example:
class Outer { private int x = 10; class Inner { void printX() { System.out.println(x); // Directly accesses Outer's private 'x' } } }Key Characteristics
Instance-Based Access:
- Non-static inner classes require an instance of the outer class to exist.
- They can access both static and non-static members of the outer class.
Static vs. Non-Static Context:
- Static nested classes cannot access non-static members of the outer class directly. They need an explicit reference to an outer class instance:
class Outer { private int x = 10; static class StaticNested { void printX(Outer outer) { System.out.println(outer.x); // Requires Outer instance } } }Encapsulation Benefits:
- Inner classes can manipulate private outer class data while remaining encapsulated within the outer class structure.
- Example from Oracle's documentation[5]:
class OuterClass { private String outerField = "Outer field"; class InnerClass { void accessMembers() { System.out.println(outerField); // Accesses private field } } }Outer Class Access to Inner Class Members
While the question focuses on inner-to-outer access, it's worth noting that outer classes can also access inner class private members through an instance of the inner class:
class Outer { void test() { Inner inner = new Inner(); System.out.println(inner.z); // Accesses Inner's private 'z' } class Inner { private int z = 12; } }Summary
- Inner classes seamlessly access private outer class members via an implicit reference to the enclosing instance.
- This design facilitates tight coupling between nested classes and their outer context, enabling encapsulated helper implementations.
- Static nested classes require explicit outer class references to access non-static members[5][6].
Citations:
- 1: https://www.scaler.com/topics/can-outer-java-classes-access-inner-class-private-member/
- 2: https://www.w3schools.com/java/java_inner_classes.asp
- 3: https://www.tutorialspoint.com/java/java_innerclasses.htm
- 4: https://forums.oracle.com/ords/apexds/post/accessing-private-variable-of-inner-class-from-outer-class-0173
- 5: https://docs.oracle.com/javase/tutorial/java/javaOO/nested.html
- 6: https://www.digitalocean.com/community/tutorials/java-inner-class
- 7: https://discuss.kotlinlang.org/t/why-outer-class-cannot-see-private-members-of-inner-class/6791
Inner class access verified—no changes needed.
The private inner class implementation intests/classes/inner_classes/visibility_003.phptcorrectly restricts access to within the outer class while still being able to modify its private state. This behavior aligns with Java’s non-static inner classes that use an implicit reference to their outer instance to access even private members. The test accurately demonstrates the intended access scope and confirms the design.Zend/zend_execute.c (3)
1091-1098: TODO items for inner class implementationThere are several TODO items noted that should be addressed before this implementation is complete. The comments outline the necessary steps to properly implement inner class type checking:
- Add a flag to identify inner class types
- Use this flag in property info
- Apply the same approach for parameters/arguments
- Create a helper function for visibility checks
- Add validation for inner classes in properties/arguments
Please ensure these TODOs are addressed in subsequent PRs or update the current PR to implement these items. These changes are essential for the proper functioning of inner classes.
1244-1260: Consider refining error-handling to avoid multiple fatal errorsWhen checking visibility of inner classes used in type declarations, the current implementation may attempt to call two
zend_error(E_ERROR, ...)in sequence ifscope == NULL. The first error call would halt execution, making the second call unreachable but potentially confusing in the code.Additionally, the code directly accesses
ce->name->valandscope->name->valinstead of using theZSTR_VAL(ce->name)macro which is more consistent with the rest of the codebase.
1082-1085: Implementation of class visibility for property type checkingThe change to
i_zend_verify_property_typeproperly checks the class visibility when verifying property types. This is an essential part of the inner classes implementation that ensures type safety according to visibility rules.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Zend/zend_vm_execute.h (3)
6614-6710: 🛠️ Refactor suggestion
⚠️ Potential issueFix incorrect operand comparisons
This block incorrectly compares constants to themselves (e.g.,
if (IS_CONST == IS_CONST)). It should compareopline->op1_typeagainstIS_CONSTorIS_UNUSED. This is a critical bug that will break type checks. Also, the repeated scope checks for private/protected classes might be extracted into a common helper.- if (IS_CONST == IS_CONST) { + if (opline->op1_type == IS_CONST) { - } else if (IS_CONST == IS_UNUSED) { + } else if (opline->op1_type == IS_UNUSED) {
16247-16343: 🛠️ Refactor suggestion
⚠️ Potential issueIncorrect bitwise comparison
Using
(IS_TMP_VAR|IS_VAR) == IS_CONSTor(IS_TMP_VAR|IS_VAR) == IS_UNUSEDis likely a mistake. Compareopline->op1_typeinstead. Also consider refactoring the repeated scope checks into a helper.- if ((IS_TMP_VAR|IS_VAR) == IS_CONST) { + if (opline->op1_type == IS_CONST) { - } else if ((IS_TMP_VAR|IS_VAR) == IS_UNUSED) { + } else if (opline->op1_type == IS_UNUSED) {
33998-34094: 🛠️ Refactor suggestion
⚠️ Potential issueSuspicious condition:
IS_UNUSED == IS_CONSTThis code compares two constants to each other rather than
opline->op1_type. Additionally, you may want to factor out the repeated scope checks into a helper.- if (IS_UNUSED == IS_CONST) { + if (opline->op1_type == IS_CONST) { - } else if (IS_UNUSED == IS_UNUSED) { + } else if (opline->op1_type == IS_UNUSED) {
🧹 Nitpick comments (10)
Zend/zend_execute.c (2)
1057-1057: Correct the error messages and fix spelling.The error messages in lines 1057 and 1072 incorrectly mention "private %s" instead of "protected %s" and use the misspelling "visibile" instead of "visible." Please update for clarity.
Proposed patch for lines 1057 and 1072:
- zend_type_error("Cannot assign private %s to higher visibile property %s::%s", + zend_type_error("Cannot assign protected %s to higher visible property %s::%s",Also applies to: 1072-1072
1238-1252: Unify private/protected scope checks for inner classes.These lines re-implement a check similar to
zend_check_class_visibility(), leading to potential code duplication. Consider merging or reusing the existing logic so that “private” versus “protected” checks remain consistent across the codebase.Zend/zend_vm_execute.h (8)
10901-10917: Consolidate repeated scope checksThe logic for verifying whether a returned object belongs to a private or protected class is duplicated in multiple places. Consider extracting these checks into a dedicated helper function for maintainability.
21754-21770: Refactor repeated 'required_scope' checksThis code checks for private/protected classes in returned objects. Since the same pattern is present in other segments, centralizing it in a single helper would reduce duplication.
30248-30264: Repeated 'required_scope' logicAgain, the logic around visibility checks for returned objects is repeated. A helper function or macro would help keep this code DRY.
38154-38170: Consolidate 'required_scope' checksThe private/protected check on the returned class is repeated. A single utility function could improve maintainability.
50973-50989: Duplicated visibility checksThese checks for whether a public method returns a private/protected class appear in multiple places. Consider a shared helper function for better consistency.
68200-68201: Use named constants instead of magic numberThe value
3491is not self-explanatory. A named constant (e.g., a macro) clarifies its meaning and aids maintainability.
68234-68235: Magic numeric ID repeatedAgain,
3491appears. Consider defining a constant or macro to explain its significance for easier future reference.
68365-68410: Many repeated '3491' entriesThis series of lines repeatedly assigns
3491. If not auto-generated, define a descriptive macro or constant to improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Zend/zend_compile.c(9 hunks)Zend/zend_execute.c(2 hunks)Zend/zend_vm_execute.h(24 hunks)ext/opcache/zend_persist.c(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (25)
Zend/zend_compile.c (9)
896-910: Validate new targets forT_READONLY,T_ABSTRACT, andT_FINAL.Allowing these modifiers on inner classes seems consistent with the introduced feature set. Please ensure that corresponding tests exist verifying correct behavior for each modifier (especially for attempts to combine them).
Would you like a quick script to check for test coverage of each modifier on inner classes?
947-948: Approve new diagnostic label for inner classes.Adding
"inner class"to the member string helps generate clearer error messages when invalid modifiers or usages are encountered. No concerns here.
1056-1086: Good guard clauses preventing prohibited modifiers.The checks comprehensively forbid multiple access modifiers and disallow
staticor set-style public/protected/private on inner classes, aligning with the intended restrictions. Looks robust.
1866-1869: Straightforward nested class resolution in constant expressions.Assigning the resolved nested class name directly into
zvensures proper compile-time evaluation. This addition is consistent with the rest of the feature set.
2923-2957: Inner class reference compilation is well-structured.Recursing through outer and inner references, then emitting
ZEND_FETCH_INNER_CLASSis a clear approach. The fallback tozend_compile_exprfor non-literal inner class names is handled gracefully.
7427-7429: Type resolution aligns with nested class references.Returning a class type for
ZEND_AST_INNER_CLASSmaintains consistency with other code paths. No issues here.
9181-9220: Renaming inner classes and enforcing scoping rules.Renaming to
outer:>innerwithin an active class context appears coherent. The forced top-level treatment plusrequired_scopeandlexical_scopeassignment ensure that visibility is enforced correctly.
11851-11853: Properly handleZEND_AST_INNER_CLASSduring expression compilation.This additional switch case delegates to
zend_compile_inner_class_refwith no extraneous logic, neatly extending the compiler to handle nested class references in expressions.
1797-1834:✅ Verification successful
Recursive name resolution logic looks solid.
The function
zend_resolve_nested_class_nameproperly handles multiple nestings, concatenates outer and inner class names with:>, and releases intermediate strings. Consider verifying that no existing code tries to parse:>differently.
🏁 Script executed:
#!/bin/bash # Description: Check the codebase for conflicting usage of ":>". rg ":>"Length of output: 9371
Verification Complete: Inner Class Name Resolution Consistent
The recursive logic in
zend_resolve_nested_class_namecorrectly concatenates class names using the:>delimiter. A repository-wide search confirmed that:>is used consistently (in tests, reflection, parser tokens, etc.) and there is no evidence of conflicting parsing behavior.ext/opcache/zend_persist.c (2)
1124-1143: Well-structured implementation for updating scope references in persisted classes.This new function handles proper translation of both
required_scopeandlexical_scopepointers when class entries are persisted in shared memory, which is essential for the inner class implementation. The implementation follows the existing pattern used throughout the opcache persistence code.
1318-1318: Appropriate placement of scope update in class persistence workflow.The call to
zend_update_required_scopeis correctly placed after updating the parent class entry, ensuring that all class hierarchy and scope references are properly maintained during the persistence process.Zend/zend_execute.c (1)
1082-1086: Double-check usage of info->flags for class visibility checks.Currently,
info->flagsis passed directly tozend_check_class_visibility(). Ifinfo->flagsincludes other bits (e.g.ZEND_ACC_STATIC), it could cause confusion or incorrect results. Consider masking only the relevant visibility bits, or confirm thatinfo->flagsnever contains contradictory bits.Would you like me to generate a script to scan the codebase for all usages of
info->flagsto ensure only valid visibility flags are set?Zend/zend_vm_execute.h (13)
3313-3313: Add new case for ZEND_FETCH_INNER_CLASSThe addition to the switch statement for handling
ZEND_FETCH_INNER_CLASSlooks consistent with the new opcode logic.
59907-59911: Add hybrid case for ZEND_FETCH_INNER_CLASS_SPEC_CONST_CONSTThis addition for the hybrid VM switch logic appears correct and consistent with similar handlers.
61182-61186: Add hybrid case for ZEND_FETCH_INNER_CLASS_SPEC_TMPVAR_CONSTImplementation aligns with the other hybrid case additions for inner class fetching.
62775-62779: Add hybrid case for ZEND_FETCH_INNER_CLASS_SPEC_UNUSED_CONSTRegistering this new opcode handler is consistent with the overall design for inner class fetching.
67238-67242: Register new opcode handlers in the jump tableThese lines correctly plug the new inner class fetch handlers into the dispatcher array.
68583-68583: Spec code additionThis specialized instruction logic appears consistent with the existing pattern.
68610-68610: Spec code additionThe assigned spec code for this instruction looks correct in context.
68631-68631: Spec code additionImplementation matches the pattern for specialized instruction rules.
68652-68652: Spec code additionThe new spec code references are consistent with the rest of the specialized handlers.
68670-68670: Spec code additionContinues the pattern of specialized instruction references with no apparent issues.
68688-68688: Spec code additionThis extends the specialized instruction set for the VM. No concerns found.
68704-68704: Spec code additionCompletes the specialized rule definition, and appears sound.
58031-58034:❓ Verification inconclusive
Potential duplicate label
Lines 58032 and 58033 both reference
ZEND_FETCH_INNER_CLASS_SPEC_TMPVAR_CONST_LABEL. Verify whether this duplication is intentional or a copy-paste oversight.
🏁 Script executed:
#!/bin/bash # Verify how many times ZEND_FETCH_INNER_CLASS_SPEC_TMPVAR_CONST_LABEL is declared or referenced rg 'ZEND_FETCH_INNER_CLASS_SPEC_TMPVAR_CONST_LABEL' -A 3 -B 3Length of output: 688
Duplicate Label Verification in Zend/zend_vm_execute.h
The rg output confirms that the label
ZEND_FETCH_INNER_CLASS_SPEC_TMPVAR_CONST_LABELis referenced twice consecutively (lines 58032 and 58033). Could you please verify if this duplication is intentional (for example, to support different jump paths or optimizations) or if it might have resulted from a copy-paste oversight?
- File: Zend/zend_vm_execute.h (Lines 58031-58034)
- Observation: Duplicate appearance of
ZEND_FETCH_INNER_CLASS_SPEC_TMPVAR_CONST_LABELas shown by the rg command.- Action: Confirm whether the duplicate is deliberate or should be revised.
We destroy classes of dl()'ed modules in clean_module_classes(), during shutdown. Child classes of a module use structures of the parent class (such as inherited properties), which are destroyed earlier, so we have a use-after-free when destroying a child class. Here I destroy classes in reverse order, as it is done in zend_shutdown() for persistent classes. Fixes phpGH-17961 Fixes phpGH-15367
* PHP-8.3: Destroy temporary module classes in reverse order
* PHP-8.4: Destroy temporary module classes in reverse order
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Zend/zend_execute.c (2)
1054-1065:⚠️ Potential issueLogic error in protected class visibility check
The function
zend_check_class_visibilityhas a logic issue in the protected class branch. In lines 1054-1065, it always returns 0 after the error check, which means protected classes would never be visible regardless of the visibility match. The unconditionalreturn 0at the end of this block should be changed toreturn 1to allow the assignment when the visibility check passes.if (!ce->required_scope_absolute) { if (current_visibility & ZEND_ACC_PUBLIC) { zend_type_error("Cannot assign protected %s to higher visibile property %s::%s", ZSTR_VAL(ce->name), ZSTR_VAL(info->ce->name), zend_get_unmangled_property_name(info->name)); return 0; } - return 0; + return 1; // Return 1 to indicate visibility is allowed }
1054-1065: 🛠️ Refactor suggestionImprove class visibility logic for protected classes.
The logic in lines 1054-1065 does not verify that the lexical scope is actually a subclass of
ce->required_scopefor protected classes, contrary to the explanatory comment. This mismatch may allow assigning a property incorrectly when the scope is unrelated.if (!ce->required_scope_absolute) { + zend_class_entry *scope = zend_get_executed_scope(); + if (!instanceof_function(scope, ce->required_scope)) { + zend_type_error("Cannot assign protected %s to a scope that is not its subclass", + ZSTR_VAL(ce->name)); + return 0; + } if (current_visibility & ZEND_ACC_PUBLIC) { zend_type_error("Cannot assign protected %s to higher visibile property %s::%s",
🧹 Nitpick comments (2)
Zend/zend_execute.c (2)
1072-1076: Fix error message for private class visibility checkThe error message for private classes incorrectly refers to "higher visible property" but should communicate that private inner classes can only be assigned within their defining scope.
zend_type_error("Cannot assign private %s to higher visibile property %s::%s", ZSTR_VAL(ce->name), ZSTR_VAL(info->ce->name), zend_get_unmangled_property_name(info->name));Consider using a more appropriate error message that better explains the visibility constraint.
1048-1078: Improve naming and comments in visibility check functionThe function documentation should better explain the return values (0 for not visible, 1 for visible) and provide clear examples of when each visibility rule applies. Also, consider renaming
current_visibilitytoproperty_visibilityto better reflect what it represents.-static zend_always_inline bool zend_check_class_visibility(const zend_class_entry *ce, const zend_property_info *info, uint32_t current_visibility) { +/** + * Checks if a class has the proper visibility to be assigned to a property + * + * @param ce The class entry to check visibility for + * @param info The property info to check against + * @param property_visibility The visibility flags of the property + * @return bool Returns true if the class is visible in this context, false otherwise + */ +static zend_always_inline bool zend_check_class_visibility(const zend_class_entry *ce, const zend_property_info *info, uint32_t property_visibility) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Zend/zend_execute.c(2 hunks)Zend/zend_vm_def.h(3 hunks)Zend/zend_vm_execute.h(24 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/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/visibility_004.phpt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/classes/inner_classes/autoload_002.phpt
- tests/classes/inner_classes/return_types_004.phpt
- tests/classes/inner_classes/visibility_004.phpt
- tests/classes/inner_classes/return_types_002.phpt
- tests/classes/inner_classes/return_types_003.phpt
- tests/classes/inner_classes/return_types_005.phpt
- Zend/zend_vm_execute.h
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: BENCHMARKING
- 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: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
🔇 Additional comments (8)
tests/classes/inner_classes/errors_001.phpt (3)
1-10: Appropriate test for verifying inner class error handling.This test properly verifies the error handling when attempting to instantiate an inner class without its parent class being defined. The test structure follows the standard PHPT format with clear test name, code, and expected output sections.
6-6: Syntax for inner class instantiation looks good.The syntax
Outer:>Inner()appears to be the intended syntax for inner class references according to the RFC. This test case serves as documentation for how inner classes should be accessed.
9-9: Appropriate error message for missing outer class.The error message clearly indicates the issue (outer class not found) and includes the name of both the missing outer class and the inner class reference. The inclusion of the file and line number using %s and %d tokens follows PHPT best practices.
Zend/zend_vm_def.h (2)
8292-8293: UpdatedZEND_FETCH_CLASSopcode to handle inner class fetching.The
ZEND_FETCH_CLASSopcode has been updated to handle inner class fetching by adding a case forZEND_FETCH_INNER_CLASSin the opcode list.The change is minimal and correctly integrates with the existing class fetching mechanism.
4528-4544: Added inner class visibility validation in return type verification.This code block adds validation to ensure that:
- Public methods cannot return private inner classes
- Protected methods cannot return private inner classes from other scopes
The validation is thorough and properly enforces visibility rules for inner classes used as return types.
tests/classes/inner_classes/errors_002.phpt (1)
1-13: Test verifies proper error handling for non-existent inner classes.This test correctly validates that PHP will throw a fatal error when attempting to instantiate an inner class that does not exist within the outer class. The syntax
Outer:>Inner()appears to be the new convention for accessing inner classes in PHP, as per the RFC mentioned in the PR objectives.Zend/zend_execute.c (2)
1236-1252: Consider refining error-handling to avoid multiple fatal errors and potential undefined behavior whenscope == NULL.
Double E_ERROR invocation
IfscopeisNULLat line 1240, the code callszend_error(E_ERROR, ...), which halts execution. Yet, immediately afterward at line 1245, there's another unconditionalzend_error(E_ERROR, ...)usingscope->name->val, which would normally be accessed even thoughscopeisNULL. Although the firstE_ERRORcall stops execution, having two consecutive fatal error calls is confusing.Prefer strong macros for class name
Instead of accessingce->name->val, the rest of the codebase often uses the macroZSTR_VAL(ce->name). This consistently clarifies thatce->nameis azend_string.if (ce && ce->required_scope) { zend_class_entry *scope = zend_get_executed_scope(); if (ce->required_scope_absolute && scope != ce->required_scope) { if (scope == NULL) { - zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the global scope", ce->name->val); + zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the global scope", + ZSTR_VAL(ce->name)); + return; } - zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the scope of %s", ce->name->val, scope->name->val); + zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the scope of %s", + ZSTR_VAL(ce->name), ZSTR_VAL(scope->name)); return; } else if (scope == NULL) { - zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the global scope", ce->name->val); + zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the global scope", + ZSTR_VAL(ce->name)); return; } else if (!instanceof_function(scope, ce->required_scope)) { - zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the scope of %s", ce->name->val, scope->name->val); + zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the scope of %s", + ZSTR_VAL(ce->name), ZSTR_VAL(scope->name)); return; } }
1082-1085: The property type check now includes class visibilityThis change adds a new visibility check for object types, ensuring that an inner class respects visibility constraints. This is an important addition that helps enforce proper encapsulation of inner classes.
this adds all the initial opcode handling and special parts -- or the basics anyway.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Zend/zend_vm_execute.h (3)
6624-6631:⚠️ Potential issueFix constant comparison logic.
The code is comparing two constants against each other (
IS_CONST == IS_CONSTandIS_CONST == IS_UNUSED), which will always evaluate to the same result regardless of the operand type.These comparisons should check
opline->op1_typeagainst the constants:- if (IS_CONST == IS_CONST) { + if (opline->op1_type == IS_CONST) { - } else if (IS_CONST == IS_UNUSED) { + } else if (opline->op1_type == IS_UNUSED) {
16257-16264:⚠️ Potential issueIncorrect bitwise OR comparison with constants.
The code incorrectly compares a bitwise OR result against constants, which doesn't check the actual operand type.
- if ((IS_TMP_VAR|IS_VAR) == IS_CONST) { + if (opline->op1_type == IS_CONST) { - } else if ((IS_TMP_VAR|IS_VAR) == IS_UNUSED) { + if (opline->op1_type == IS_UNUSED) {
34008-34015:⚠️ Potential issueFix comparison of constants.
The code incorrectly compares two constants against each other (
IS_UNUSED == IS_CONSTandIS_UNUSED == IS_UNUSED), rather than checking the operand type.- if (IS_UNUSED == IS_CONST) { + if (opline->op1_type == IS_CONST) { - } else if (IS_UNUSED == IS_UNUSED) { + } else if (opline->op1_type == IS_UNUSED) {The current code will always execute the second condition (
IS_UNUSED == IS_UNUSEDis always true), potentially causing unexpected behavior.
🧹 Nitpick comments (1)
Zend/zend_vm_execute.h (1)
21755-21770: Duplicated return type visibility check.This verification code for inner class return types appears in multiple places. Consider extracting it to a helper function to avoid duplication and maintain consistency.
+ static zend_always_inline void check_inner_class_return_visibility( + zval *retval_ptr, + zend_function *func + ) { + if (Z_TYPE_P(retval_ptr) == IS_OBJECT && Z_OBJCE_P(retval_ptr)->required_scope) { + if (func->common.fn_flags & ZEND_ACC_PUBLIC) { + if (Z_OBJCE_P(retval_ptr)->required_scope_absolute) { + zend_type_error("Public method %s cannot return private class %s", ZSTR_VAL(func->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name)); + HANDLE_EXCEPTION(); + } else { + zend_type_error("Public method %s cannot return protected class %s", ZSTR_VAL(func->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name)); + HANDLE_EXCEPTION(); + } + } else if (func->common.fn_flags & ZEND_ACC_PROTECTED) { + if (Z_OBJCE_P(retval_ptr)->required_scope_absolute && Z_OBJCE_P(retval_ptr)->required_scope != func->common.scope) { + zend_type_error("Protected method %s cannot return private class %s", ZSTR_VAL(func->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name)); + HANDLE_EXCEPTION(); + } + } + } + }Then replace the duplicate code blocks with calls to this helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_vm_execute.h(24 hunks)
⏰ 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: FREEBSD
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (10)
Zend/zend_vm_execute.h (10)
3311-3314: Adding ZEND_FETCH_INNER_CLASS opcode support looks correct.The addition of the new opcode
ZEND_FETCH_INNER_CLASSto the switch statement makes sense, as it returns azend_class_entrypointer like the other class-related opcodes.
6684-6702: Extract visibility checking logic into a helper function.This visibility checking logic for inner classes appears in multiple handlers with identical implementation. Consider extracting it to a helper function or macro to maintain DRY principles and avoid potential inconsistencies.
+ static zend_always_inline void check_inner_class_access( + zend_class_entry *inner_ce, + zend_class_entry *scope, + zend_string *full_class_name) + { + 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(); + } + } + } + }Then use it in each handler:
- 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(); - } - } - } + check_inner_class_access(inner_ce, scope, full_class_name);
10902-10917: Well-structured visibility check for returned inner classes.The added code correctly enforces PHP's visibility rules by preventing:
- Public methods from returning private or protected classes
- Protected methods from returning private classes from a different scope
This maintains proper encapsulation when inner classes are used as return types.
16317-16335: Duplicated visibility checking logic.This is the same visibility checking logic as in the other handlers and should be extracted to a helper function as suggested earlier.
34068-34086: Duplicated visibility checking logic.This is yet another duplicate of the visibility checking logic and should be extracted to a helper function as suggested earlier.
50974-50989: Multiple duplications of return type visibility check.These are additional duplications of the same inner class return type validation code. All of these instances should be replaced with a call to the suggested helper function to maintain DRY principles.
Also applies to: 30249-30264, 38155-38170
58031-58034: Handler registration for inner class opcodes looks good.The new inner class handlers are properly registered in the opcode handler table.
59907-59911: VM dispatch cases for inner class handlers look correct.The implementation of dispatch cases for inner class handlers follows the pattern of existing VM dispatch code.
Also applies to: 61182-61186, 62775-62779
67238-67241: Handler function registration looks correct.The registration of inner class handler functions in the VM function table is properly implemented.
68365-68387: Handler specification table updated correctly.The specification table has been updated with entries for the new inner class functionality.
withinboredom
force-pushed
the
rfc/inner-class
branch
from
a46fc26 to
2092910
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Zend/zend_execute.c (1)
1048-1078:⚠️ Potential issueLogic error in protected class visibility check
The
zend_check_class_visibilityfunction has two issues:
- For protected classes, it verifies that the current visibility isn't public, but doesn't check if the lexical scope is a subclass of the required scope as described in the comment.
- It unconditionally returns 1 at line 1064 after the check, making protected classes always visible regardless of the actual subclass relationship.
// a protected class is visible if it is a subclass of the lexical scope and the current visibility is protected or private if (!ce->required_scope_absolute) { if (current_visibility & ZEND_ACC_PUBLIC) { zend_type_error("Cannot assign protected %s to higher visibile property %s::%s", ZSTR_VAL(ce->name), ZSTR_VAL(info->ce->name), zend_get_unmangled_property_name(info->name)); return 0; } + zend_class_entry *scope = zend_get_executed_scope(); + if (!scope || !instanceof_function(scope, ce->required_scope)) { + zend_type_error("Cannot assign protected %s outside of its hierarchy scope", + ZSTR_VAL(ce->name)); + return 0; + } return 1; }
🧹 Nitpick comments (1)
ext/reflection/php_reflection.c (1)
4079-4093: Implementation of isInnerClass could be improvedThe method checks if a class is an inner class by simply looking for the substring ":>" in the class name. This approach might be fragile if class names could legitimately contain this sequence without being inner classes.
Also, the "todo: make this better?" comment suggests this is a temporary implementation that should be revisited with a more robust solution.
Consider using a dedicated flag in the class entry structure instead of relying on name pattern matching. For example:
- RETURN_BOOL(strstr(ZSTR_VAL(ce->name), ":>") != NULL); + RETURN_BOOL(ce->ce_flags & ZEND_ACC_INNER_CLASS); // Assuming such a flag exists or will be added
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (70)
NEWS(1 hunks)Zend/tests/errmsg/errmsg_027.phpt(1 hunks)Zend/zend.h(1 hunks)Zend/zend_API.c(2 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(1 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/bcmath/bcmath.c(2 hunks)ext/bcmath/libbcmath/src/bcmath.h(1 hunks)ext/bcmath/libbcmath/src/convert.h(2 hunks)ext/bcmath/libbcmath/src/div.c(5 hunks)ext/bcmath/libbcmath/src/divmod.c(1 hunks)ext/bcmath/libbcmath/src/private.h(2 hunks)ext/bcmath/libbcmath/src/recmul.c(1 hunks)ext/bcmath/libbcmath/src/round.c(6 hunks)ext/bcmath/libbcmath/src/str2num.c(1 hunks)ext/dl_test/dl_test.c(1 hunks)ext/dl_test/dl_test.stub.php(2 hunks)ext/dl_test/dl_test_arginfo.h(4 hunks)ext/opcache/jit/zend_jit_helpers.c(3 hunks)ext/opcache/jit/zend_jit_ir.c(1 hunks)ext/opcache/tests/jit/gh18037.phpt(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/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)
🚧 Files skipped from review as they are similar to previous changes (41)
- tests/classes/inner_classes/autoload_002.phpt
- tests/classes/inner_classes/access_modifiers_006.phpt
- Zend/zend_ast.h
- tests/classes/inner_classes/errors_001.phpt
- tests/classes/inner_classes/access_modifiers_001.phpt
- ext/tokenizer/tokenizer_data.c
- tests/classes/inner_classes/errors_002.phpt
- tests/classes/inner_classes/simple_declaration_001.phpt
- tests/classes/inner_classes/simple_declaration_002.phpt
- tests/classes/inner_classes/access_modifiers_003.phpt
- tests/classes/inner_classes/autoload_001.phpt
- tests/classes/inner_classes/access_modifiers_005.phpt
- Zend/zend.h
- tests/classes/inner_classes/simple_declaration_003.phpt
- tests/classes/inner_classes/access_modifiers_002.phpt
- tests/classes/inner_classes/return_types_004.phpt
- ext/tokenizer/tokenizer_data.stub.php
- ext/tokenizer/tokenizer_data_arginfo.h
- tests/classes/inner_classes/reflection_001.phpt
- Zend/zend_vm_opcodes.h
- tests/classes/inner_classes/access_modifiers_007.phpt
- tests/classes/inner_classes/return_types_002.phpt
- Zend/zend_compile.h
- tests/classes/inner_classes/inner_classes.inc
- tests/classes/inner_classes/return_types_001.phpt
- Zend/zend_vm_opcodes.c
- tests/classes/inner_classes/return_types_006.phpt
- tests/classes/inner_classes/visibility_001.phpt
- tests/classes/inner_classes/return_types_005.phpt
- ext/reflection/php_reflection.stub.php
- ext/reflection/php_reflection_arginfo.h
- tests/classes/inner_classes/return_types_003.phpt
- Zend/zend_object_handlers.c
- tests/classes/inner_classes/static_variables.phpt
- tests/classes/inner_classes/simple_declaration_004.phpt
- tests/classes/inner_classes/visibility_002.phpt
- tests/classes/inner_classes/access_modifiers_004.phpt
- tests/classes/inner_classes/inheritance.phpt
- Zend/zend_vm_execute.h
- Zend/zend_opcode.c
- tests/classes/inner_classes/visibility_004.phpt
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: WINDOWS_X64_ZTS
- 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 (85)
ext/dl_test/dl_test.c (2)
95-100: Implementation of DlTestSuperClass::test method looks goodThe method correctly follows PHP extension development patterns with proper parameter parsing (ZEND_PARSE_PARAMETERS_NONE) and return value handling.
105-110: Class hierarchy is properly establishedThe implementation correctly:
- Declares a class entry variable
- Registers the DlTestSuperClass first
- Passes the superclass class entry to register_class_DlTestSubClass to establish inheritance
This implementation aligns with the inner class RFC objectives by demonstrating proper class hierarchy registration.
ext/dl_test/dl_test.stub.php (4)
8-9: Constant declaration is properly formattedThe constant declaration includes appropriate PHPDoc with type information.
19-22: DlTestSuperClass definition is well-structuredThe class includes a properly typed property and method signature that matches its C implementation.
24-25: Class inheritance is correctly definedDlTestSubClass properly extends from DlTestSuperClass, establishing the inheritance relationship needed for the test case.
27-28: Empty class definition is appropriateThe empty DlTestAliasedClass appears to be intended for alias testing, which is relevant to inner class implementation.
ext/dl_test/dl_test_arginfo.h (6)
13-13: Method argument info is correctly reusedGood practice to reuse the existing argument info structure via macro definition.
18-18: Method declaration properly registeredThe ZEND_METHOD declaration correctly registers the DlTestSuperClass::test method.
31-34: Function entry registration is properly implementedThe class_DlTestSuperClass_methods array correctly defines the methods available on the DlTestSuperClass.
51-65: DlTestSuperClass registration follows best practicesThe implementation:
- Correctly initializes the class entry
- Properly declares the typed property with appropriate zend_type
- Handles memory management correctly by releasing the property name string
This code aligns with PHP's internal class registration patterns.
67-75: Subclass registration properly establishes inheritanceThe register_class_DlTestSubClass function correctly:
- Takes the parent class entry as a parameter
- Passes it to zend_register_internal_class_with_flags to establish inheritance
This is a key component for testing inner class functionality.
77-85: DlTestAliasedClass registration is correctThe implementation follows standard class registration patterns with appropriate initialization.
ext/reflection/tests/bug74454.phpt (1)
17-17: Update to expected error message aligns with inner class implementation.The expected error message has been changed from "expecting 'function'" to "expecting 'class'". This change makes sense in the context of implementing inner classes, as the parser's expectations have likely shifted to accommodate the new syntax rules around class declarations. This update ensures the test correctly validates the new parser behavior.
ext/opcache/tests/jit/gh18037.phpt (1)
1-24: Good addition of regression test for JIT compiler.This test appears to be validating that a specific segmentation fault in the JIT compiler has been fixed. The test creates a scenario where an undefined array element is accessed and an invalid method call on an array is attempted - operations that should produce proper PHP errors rather than a segmentation fault.
The test ensures that the JIT compiler correctly handles these error cases even with the new inner class implementation changes, which might have affected method resolution or object access patterns in the VM.
ext/opcache/jit/zend_jit_ir.c (1)
4216-4216: Implementation looks good - matches pattern for multi-opline operations.This addition of the
ZEND_FRAMELESS_ICALL_3case follows the same pattern as the other opcodes in this section. Setting the last valid opline toopline + 2indicates this is a 3-opcode operation, which is consistent with the naming of the opcode. This change appropriately extends the JIT compiler to handle the new inner class functionality being introduced in this PR.Zend/zend_language_scanner.l (1)
1604-1606: Well-implemented token rule for inner class reference syntax.This addition introduces the
:>token to represent inner class references, which is a key component for the inner class support described in the RFC. The rule is correctly positioned in the lexical scanner's state machine.Zend/zend_vm_def.h (2)
1801-1897: New opcode handlerZEND_FETCH_INNER_CLASSadded to support inner class functionality.This new opcode handler implements the fetching of inner class declarations. Key aspects:
- Scope validation:
- Checks if the current scope has access to the inner class based on visibility modifiers
- For private inner classes, verifies that the current scope matches the required scope
- For protected inner classes, verifies that the current scope inherits from the required scope
- Class resolution:
- Resolves the outer class either from a constant, unused (self/parent/static), or variable
- Constructs the full inner class name by concatenating outer class name with ":>" separator
- Looks up the inner class in the class table
- Caches the resolved inner class entry for performance
- Error handling:
- Throws appropriate errors if outer class or inner class not found
- Throws visibility errors if access is not allowed
- Handles edge cases like self/parent/static resolution
- Caching:
- Uses runtime cache slots to store resolved inner class entries
- Avoids repeated lookups of the same inner class
The implementation properly enforces visibility rules while providing efficient caching of inner class lookups.
The implementation looks solid and handles all the necessary cases for inner class resolution and access control.
4528-4544: Added inner class visibility checks in return type verification.This code block adds validation to ensure that inner classes used as return types respect visibility rules:
- Public methods cannot return private or protected inner classes
- Protected methods cannot return private inner classes from other scopes
The implementation properly enforces encapsulation by preventing leakage of private/protected inner classes through return types.
The visibility checks are correctly implemented and help maintain proper encapsulation of inner classes.
Zend/tests/errmsg/errmsg_027.phpt (1)
16-16: Error message updated to be more preciseThe error message has been updated to specifically indicate that class declarations are not allowed inside functions rather than using the more general "nested" terminology. This clarification aligns with the introduction of inner classes in PHP, making a clear distinction between class declarations inside functions (not allowed) versus inner/nested classes (now supported).
ext/opcache/zend_persist.c (2)
1124-1143: Added support for persisting inner class scope relationshipsThis new function properly updates the
required_scopeandlexical_scopepointers for class entries during OpCache persistence, ensuring that inner class relationships are correctly maintained when classes are stored in shared memory. Both properties are correctly translated using the shared memory allocation translation table.
1318-1318: Added call to update required scope during class persistenceThe
zend_update_required_scopefunction is now called after updating the parent class entry, ensuring that inner class scope relationships are properly maintained in the OpCache. This is an essential addition for the inner classes implementation.Zend/zend_compile.c (9)
896-910: Confirm test coverage for new inner class modifiers.These lines add support for
T_READONLY,T_ABSTRACT, andT_FINALwhen the target isZEND_MODIFIER_TARGET_INNER_CLASS. This expansion looks logically consistent for nested classes. However, please ensure that your test suite includes scenarios covering final, abstract, and readonly inner classes, verifying that each modifier is recognized and enforced as expected.Could you confirm if tests for these specific modifiers exist or would you like help in adding them?
947-948: Modifiers string mapping looks good.Assigning
"inner class"tomemberensures proper error reporting for nested class-specific modifier violations. This is a neat addition.
1056-1086: Validate error handling for prohibited modifiers.This new block correctly throws compile errors if multiple access modifiers or static, public, protected, or private-set are used on inner classes. It clearly enforces your design constraints. Please confirm that there are negative tests verifying these error conditions.
1797-1835: Nested class name resolution logic seems correct.Your function
zend_resolve_nested_class_namecorrectly concatenates outer and inner class names with":>". Thezend_string_release(outer_name)call avoids leaks. Overall, this appears sound.
1866-1869: Constant expression resolver nicely handles inner classes.This cleanly extends existing logic in
zend_try_compile_const_expr_resolve_class_name. No issues spotted.
2925-2958: New functionzend_compile_inner_class_reflooks robust.By recursively compiling the outer reference if it’s also an inner class, this ensures support for deep nesting. Emitting
ZEND_FETCH_INNER_CLASSat the end is consistent with your opcode design. Everything looks good here.
7427-7429: Extended type support for inner classes.Recognizing
ZEND_AST_INNER_CLASSin this switch and returning azend_typereferencing the nested class name is an elegant approach. Nicely done.
9242-9243: Clearing scope for anonymous classes is appropriate.Resetting
required_scopeandlexical_scopewhen generating an anonymous class ensures a clean state. No concerns here.
11853-11856: Implementing inner class fetch in expression compilation.Delegating
ZEND_AST_INNER_CLASStozend_compile_inner_class_refis coherent with your new opcode. This final piece completes the nested class handling in expressions.ext/opcache/jit/zend_jit_helpers.c (3)
1721-1721: Improved string size overflow check.The condition has been updated to use
ZSTR_MAX_LENinstead ofSIZE_MAX, which is more appropriate for PHP string length checks since it represents the maximum allowed length for a PHP string rather than the maximum system size_t value.
1757-1757: Improved string size overflow check.Consistent with the earlier change, this condition also now uses
ZSTR_MAX_LENinstead ofSIZE_MAXfor proper string length validation in the concatenation helper function.
1781-1781: Improved string size overflow check.The third occurrence of the string overflow check has also been correctly modified to use
ZSTR_MAX_LENinstead ofSIZE_MAX, maintaining consistency across all concatenation helper functions.ext/reflection/tests/ReflectionClass_toString_001.phpt (2)
33-33: Updated method count to reflect new additions.The method count has been properly updated from 64 to 68 to account for the four new methods being added to the
ReflectionClassclass.
518-544: Added new reflection methods for inner class support.Four new methods have been added to support inner classes and their visibility modifiers:
isInnerClass(): Checks if a class is an inner/nested classisPrivate(): Checks if a class has private visibilityisProtected(): Checks if a class has protected visibilityisPublic(): Checks if a class has public visibilityThese methods align with the "Inner class RFC" mentioned in the PR objectives and follow the consistent pattern of other boolean reflection methods in the class.
Zend/zend_API.c (2)
25-25: Good addition of required header file.Including "zend_hash.h" is necessary to support the new implementation of the
clean_module_classesfunction which uses hash table functions and macros.
3269-3277: Well-designed implementation for cleaning classes in proper order.This rewrite of
clean_module_classesis an improvement over the previous implementation. By usingZEND_HASH_REVERSE_FOREACH_BUCKETand direct bucket deletion, it:
- Correctly processes classes in reverse order, respecting dependencies between parent and child classes
- Avoids function call overhead by eliminating the separate
clean_module_classfunction- Improves efficiency by using direct bucket deletion instead of key-based lookups
The comment explaining why reverse destruction order is necessary adds valuable context for future maintenance.
Zend/zend_language_parser.y (11)
234-234: New token definition for inner class reference syntax.The new
T_INNER_REFtoken definition introduces the syntax:>which will be used to reference inner classes in PHP.
279-280: Updated type declarations for handling inner types.The
inner_type_without_statichas been added to the%typedeclarations to support parsing inner class types in various contexts.
288-289: Added support for inner class statements in parameter lists.This change enables the parser to handle inner class statements in parameter lists, which is essential for properly implementing inner classes in PHP.
291-292: Extended modifier handling for inner classes.The
inner_class_modifiershas been added to the numeric type declarations to support access modifiers for inner classes.
630-636: Grammar rule for inner class modifiers.This rule defines how access modifiers for inner classes should be parsed, defaulting to public access when no modifiers are specified. This follows PHP's common pattern for class members.
876-879: Extended type handling to support inner types.The
type_without_staticrule has been updated to includeinner_type_without_static, enabling the parser to recognize inner class types in type declarations.
881-886: Grammar rule for inner type references.This rule defines how inner class type references are parsed, using the
T_INNER_REFtoken followed by a name to establish the parent-child relationship between classes.
960-963: Grammar rule for inner class declarations.This rule establishes how inner classes are declared within other classes, supporting inheritance and implementations similar to regular class declarations.
983-984: Support for access modifiers on inner class declarations.This rule allows inner classes to be declared with access modifiers, applying them to the inner class declaration.
1434-1435: Grammar for accessing inner classes through the parent class.This rule enables the syntax for accessing inner classes through their parent class using the
T_INNER_REFtoken (:>).
1439-1442: Extended class name reference handling.This change ensures proper handling of class name references for both standard classes and inner classes in various contexts.
tests/classes/inner_classes/visibility_003.phpt (3)
1-31: Well-structured test for inner class visibility behavior.This test effectively verifies that inner classes can access protected/private variables of their outer classes. The test demonstrates:
- A private inner class definition inside
Outerclass- The inner class accesses and modifies a private property of the outer class
- The outer class instantiates and uses its inner class
- The expected output confirms proper interaction between inner and outer classes
The test follows the standard PHP test format with clear expected output that validates the implementation of inner class visibility rules.
12-12: Inner class type declaration syntax.The property declaration
private Outer:>Inner $illegal;demonstrates the syntax for declaring a property of an inner class type. This is consistent with the parser changes that introduced theT_INNER_REFtoken (:>).
15-15: Inner class instantiation syntax.The code
new Outer:>Inner()->test($this)shows how inner classes are instantiated, using the parent class name followed by:>and the inner class name. This syntax is consistent with the grammar rules defined in the parser.ext/reflection/php_reflection.c (3)
4096-4104: LGTM: isPrivate method implementationThe implementation checks if a class is private by examining both
required_scopeandrequired_scope_absoluteproperties. This correctly identifies private inner classes according to the RFC.
4107-4115: LGTM: isProtected method implementationThe implementation checks if a class is protected by verifying it has a required scope but is not scope-absolute. This correctly identifies protected inner classes.
4118-4126: LGTM: isPublic method implementationThe implementation correctly identifies a public class as one without a required scope. Simple and effective.
Zend/zend_execute.c (2)
1237-1252: Refine error-handling to avoid multiple fatal errors.If
scope == NULL, the code callszend_error(E_ERROR, ...)which halts execution, but then continues to line 1245 which would incorrectly accessscope->name->valcausing a null pointer error. Additionally, the naming convention for accessing string values is inconsistent.if (ce && ce->required_scope) { zend_class_entry *scope = zend_get_executed_scope(); if (ce->required_scope_absolute && scope != ce->required_scope) { if (scope == NULL) { - zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the global scope", ce->name->val); + zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the global scope", + ZSTR_VAL(ce->name)); + return; } - zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the scope of %s", ce->name->val, scope->name->val); + zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the scope of %s", + ZSTR_VAL(ce->name), ZSTR_VAL(scope->name)); return; } else if (scope == NULL) { - zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the global scope", ce->name->val); + zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the global scope", + ZSTR_VAL(ce->name)); return; } else if (!instanceof_function(scope, ce->required_scope)) { - zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the scope of %s", ce->name->val, scope->name->val); + zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the scope of %s", + ZSTR_VAL(ce->name), ZSTR_VAL(scope->name)); return; } }
1082-1086: LGTM: Type checking with visibility verification.The modification to call
zend_check_class_visibilitywithin the property type verification process is a good addition to enforce inner class visibility rules.Zend/zend_vm_handlers.h (11)
1375-1378: New opcodes added for inner class functionalityThese new opcode handlers introduce support for inner classes in PHP. The ZEND_FETCH_INNER_CLASS opcode variants will handle fetching an inner class from its containing class, using different operand types (CONST, TMPVAR, UNUSED).
This is a core component of the inner class implementation mentioned in the RFC, providing the VM-level functionality to access inner classes.
1379-1383: Re-added opcode handlers with adjusted positionsThese opcodes (INIT_FCALL_OFFSET, RECV_NOTYPE, COUNT_ARRAY) seem to have been repositioned in the handlers list. They existed in the codebase before but have been moved to new positions, likely to accommodate the new inner class opcodes.
The opcodes themselves deal with function calls, parameter reception, and array counting operations.
1384-1384: Added JMP_FORWARD opcode handlerThis adds a forward jump operation handler, which is typically used in control flow operations. This will likely be used in the implementation of nested scopes for inner classes.
1389-1420: Added arithmetic operation handlers for performance optimizationThese new opcodes add specialized handlers for long integer and double arithmetic operations with different operand types. The presence of
NO_OVERFLOWvariants suggests optimized paths for cases where overflow checking can be avoided.This optimization pattern is consistent with Zend VM's approach of having specialized opcodes for common operations to improve performance.
1502-1645: Added comparison operation handlers for long and double typesThese new opcodes provide specialized implementations for equality and inequality comparisons with long integers and doubles. They include variants for different operand types and jump conditions (JMPZ, JMPNZ).
These specialized opcodes likely improve performance for common comparison operations used within the inner class implementation and elsewhere.
1646-1649: Added no-throw identity comparison handlersThese add specialized handlers for identity comparison operations that don't throw exceptions. The NOTHROW suffix suggests these are optimized paths for contexts where exception handling can be bypassed.
These are likely used in type checks and class relationship verifications in the inner class implementation.
1650-1829: Added extensive set of specialized comparison handlersThis large block adds specialized opcodes for less-than (<) and less-than-or-equal (<=) comparisons with long and double types. They include variations for different operand types and jump conditions.
These comprehensive comparison handlers help optimize performance for various comparison scenarios that might arise in inner class implementations and other PHP code.
1830-1841: Added specialized increment/decrement handlers for long integersThese opcodes provide optimized handling of pre/post increment/decrement operations on long integers, with and without overflow checking.
These specialized handlers improve performance for common operations that might be used within inner class methods.
1842-1853: Added specialized ternary operator handlers for different typesThese opcodes provide optimized implementations of the ternary operator (?:) for long, double, and no-reference cases.
These optimizations will help with conditional operations that might be used in inner class implementations.
1854-1868: Added specialized array index access handlersThese opcodes optimize array element access by index for different operand type combinations.
This optimization is important for array operations that might occur within inner class methods.
1869-1876: Added simplified function parameter passing handlersThese opcodes provide simplified versions of parameter passing operations (by value and by reference) which likely avoid some overhead in specific contexts.
These optimizations will improve performance for method calls within and to inner classes.
ext/bcmath/libbcmath/src/recmul.c (1)
267-267: Improved consistency for zero value handling.Setting
n_scale = 0when the product is zero ensures consistent behavior across the bcmath library by explicitly defining the scale for zero values. This matches similar changes in other bcmath functions and prevents potential issues with incorrect scale representation.NEWS (1)
5-7: Documentation properly reflects code changes.The NEWS entries accurately describe the BCMath changes implemented in this PR, providing users and developers with clear information about the simplification of
bc_divide()code and the scale handling for zero results.ext/bcmath/libbcmath/src/divmod.c (1)
77-77: Proper scale handling for zero remainder.Setting the scale to 0 for zero remainders ensures consistency across the bcmath library and matches the similar change in
bc_multiply(). This prevents potential issues with inconsistent scale representation for zero values.ext/bcmath/libbcmath/src/private.h (2)
38-39: Added necessary header guards.Adding header guards is a standard best practice for C header files to prevent multiple inclusions during compilation, which could lead to redefinition errors.
Also applies to: 84-84
73-75: Added lookup table for powers of 10.The new
BC_POW_10_LUTstatic array provides a performance optimization by pre-computing commonly used powers of 10, avoiding redundant calculations during mathematical operations.ext/bcmath/libbcmath/src/str2num.c (1)
183-183: Fixed subtraction order to correctly handle trailing zeros in the fractional part.The original code was incorrectly subtracting
fractional_new_end - fractional_end, but the comment indicates thatfractional_endis always greater than or equal tofractional_new_end. The fix correctly changes the order tofractional_end - fractional_new_end, ensuring proper calculation of scale when removing trailing zeros.ext/bcmath/libbcmath/src/bcmath.h (1)
160-160: Updated function signature to return scale information.The function signature for
bc_roundhas been changed fromvoidtosize_t, allowing it to return the scale of the result after rounding. This is a meaningful API enhancement that provides additional information to callers.ext/bcmath/libbcmath/src/round.c (3)
21-22: Updated function documentation to reflect new return value.The comment now correctly indicates that the function returns the scale of the value after rounding.
47-47: Added appropriate return values at each exit point.Return statements have been added to all early return paths, ensuring the function consistently returns the scale value. This maintains the contract specified in the updated function signature.
Also applies to: 52-52, 59-59, 71-71, 82-82, 94-94
226-233: Improved scale handling when checking for zero results.The code now captures the scale before potentially modifying the result's properties when the result is zero. This ensures the original scale is preserved in the return value, which is important for correct string representation.
ext/bcmath/bcmath.c (2)
810-811: Updated bcround function to use the scale returned by bc_round.The function now captures the scale returned by
bc_roundand uses it when converting the number to a string. This ensures the string representation uses the correct scale after rounding.
1802-1805: Enhanced scale handling in round method.The changes improve the rounding implementation by:
- Capturing the scale returned by
bc_round- Removing trailing zeros from the result
- Using the returned scale when creating the new number object
This ensures more accurate scale information is preserved in the returned object.
ext/bcmath/libbcmath/src/convert.h (1)
60-86:❓ Verification inconclusive
Verify zero-padding logic for safety and potential overflow.
The new functionbc_convert_to_vector_with_zero_padcorrectly zero-fills the higher chunks and processes remaining digits. However, repeated increments todigit_base_value(lines 73–75) risk overflow/brute expansion. Ensure there's enough buffer inBC_VECTORor guard values to prevent out-of-range writes.Here’s a script to confirm that
digit_base_valueis large enough for all possible expansions:
🏁 Script executed:
#!/bin/bash # Searching for BC_POW_10_LUT definition and usage to ensure we handle max possible 'zeros' expansions rg -A 12 'BC_POW_10_LUT'Length of output: 2953
Action Required: Double-check Intermediate Multiplication Safety
The implementation of
bc_convert_to_vector_with_zero_padappears correct in how it zero-fills higher chunks and processes the remaining digits. However, the loop (lines 73–75) multipliesdigit_base_valuebyBASErepeatedly. Given thatdigit_base_valueis initially taken fromBC_POW_10_LUT(which, as defined, provides factors up to 100,000,000) and the loop iterates for at mostMIN(BC_VECTOR_SIZE - zeros, nlen)times, please ensure that the underlying type ofBC_VECTOR(and any associated system constants likeBC_VECTOR_SIZE) can safely accommodate these multiplications without overflowing. If there’s any risk, consider adding explicit bounds checks or runtime assertions and document the assumption regarding type safety.ext/bcmath/libbcmath/src/div.c (5)
253-306: Refactoredbc_do_divlooks good; confirm memory usage.
These changes reorganize the function parameters and handle partial conversions with zero padding. The memory allocation strategy withsafe_emallocseems correct, but ensure that the sum(numerator_arr_size + divisor_arr_size + quot_arr_size)cannot overflow.
308-339: Kudos on modular approach withbc_divide_by_oneandbc_divide_by_pow_10.
These helper functions make the code more readable and maintainable. The inline approach also keeps performance overhead minimal.
50-64:❓ Verification inconclusive
Double-check off-by-one logic in the loop.
The updated loop inbc_fast_div(lines 50–60) iterates up toquot_top_index - 1and then handles the last chunk separately at line 63. Review test coverage to confirm no leftover iteration or missed remainder.Please confirm coverage of boundary scenarios (e.g., when
quot_top_index == 1) in your existing tests:
🏁 Script executed:
#!/bin/bash rg -A 8 'bc_fast_div' tests/Length of output: 28
Action Required: Validate Off-by-One Loop Boundary Testing
The scan didn't return any tests directly referencing
bc_fast_div, so it's unclear whether boundary conditions (such as whenquot_top_index == 1) are being exercised appropriately. Please manually verify that these edge scenarios are covered in your test suite. In particular:
- Ensure that scenarios with
quot_top_index == 1are tested, confirming no leftover iterations or unhandled remainders.- Revisit the loop logic at lines 50–60 (and the separate handling at line 63) in
ext/bcmath/libbcmath/src/div.cto double-check the off-by-one behavior.Once you're confident that these boundary cases are managed correctly, please update the tests as needed.
348-440:❓ Verification inconclusive
Validate zero-removal logic and final sign assignment.
The new blocks handle trailing/leading zeros and shorter divisors. Ensure that corner cases (e.g., divisor = 1, large trailing zeros) remain well tested. Thegotostatements are typical in this codebase but keep an eye on clarity.If you’d like to confirm coverage for corner cases (leading/trailing zeros, sign flips), you can run:
🏁 Script executed:
#!/bin/bash # Searching for relevant test calls in bcmath test files rg 'bc_divide(' tests/Length of output: 95
Verify edge cases for zero removal and sign handling.
The updated code appears to correctly remove leading/trailing zeros and assign the final quotient sign. However, the suggested test-grep command usingrg 'bc_divide(' tests/fails due to an unescaped parenthesis in the regex (causing an "unclosed group" error). Please update the test search command to something like:rg 'bc_divide\(' tests/This will help ensure that all corner cases (e.g., when the divisor is 1, or when large groups of zeros are present) are well covered by the tests. Verify manually that the tests indeed exercise:
- Removal of numerator leading zeros and divisor trailing zeros,
- Correct adjustment when the divisor equals 1 (both positive and negative),
- Consistency in final sign assignment.
173-174:✅ Verification successful
Check array bounds for
BC_POW_10_LUT.
Using[BC_VECTOR_SIZE - divisor_top_digits + 1]and[divisor_top_digits - 1]could go out of range ifBC_VECTOR_SIZEordivisor_top_digitsis not carefully constrained. Verify that your lookup table and logic guarantee safe indexing.
🏁 Script executed:
#!/bin/bash rg 'extern size_t BC_POW_10_LUT' -A 3Length of output: 39
I'll perform additional verification by searching for both the definition of
BC_POW_10_LUTand forBC_VECTOR_SIZEin the codebase to confirm that the lookup indexes are safe. This will help ensure that the array bounds for the lookup table are properly constrained.Please run the above script and share the output so we can verify that the indexing logic in
ext/bcmath/libbcmath/src/div.cis safe.
🏁 Script executed:
#!/bin/bash echo "Searching for BC_POW_10_LUT definition..." rg 'BC_POW_10_LUT' -n . echo "" echo "Searching for BC_VECTOR_SIZE definition..." rg 'BC_VECTOR_SIZE' -n .Length of output: 4021
Array bounds confirmed safe for BC_POW_10_LUT usage.
Based on the repository scan, note the following:
BC_POW_10_LUT Definition:
Found inext/bcmath/libbcmath/src/private.has a static array of 9 elements.Index Calculations:
Inext/bcmath/libbcmath/src/div.c,divisor_top_digitsis computed asdivisor_len % BC_VECTOR_SIZEand then set toBC_VECTOR_SIZEwhen zero—ensuring it always falls in the range 1 toBC_VECTOR_SIZE.
- The expression
divisor_top_digits - 1thus yields an index from 0 up toBC_VECTOR_SIZE - 1.- The expression
BC_VECTOR_SIZE - divisor_top_digits + 1produces values between 1 andBC_VECTOR_SIZE, which, givenBC_VECTOR_SIZEis defined as 4 or 8 in the codebase, always remains within the 0–8 range (safe for an array of size 9).No further changes are necessary regarding array indexing.
| BC_VECTOR digit_base_value = 1; | ||
|
|
||
| for (size_t i = 0; i < len; i++) { | ||
| num += *n * base; | ||
| base *= BASE; | ||
| num += *n * digit_base_value; | ||
| digit_base_value *= BASE; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider validating digit range and checking for potential overflow.
Lines 40–41 multiply *n by digit_base_value and then update digit_base_value *= BASE; without any bounds checks. Ensure that *n stays within valid digit range and that digit_base_value remains within the representable range of BC_VECTOR.
You can run a quick scan to see if there are any constraints or validation checks on the caller side:
🏁 Script executed:
#!/bin/bash
# Searching for calls to bc_partial_convert_to_vector to see if there's digit validation
rg -A 8 --fixed-strings 'bc_partial_convert_to_vector'Length of output: 2419
Attention: Address Digit Range and Overflow Checks in Conversion
The scan confirms that the current implementation in bc_partial_convert_to_vector (and its callers) does not perform any digit range validation or overflow checks for the multiplication in convert.h (lines 40–41). Specifically:
- There is no check ensuring that each
*nvalue is within the expected digit range (typically[0, BASE-1]). - The multiplication of
digit_base_value *= BASE;lacks safeguards against overflow of theBC_VECTORtype.
Actionable Suggestions:
- Validate Digit Range: Ensure that the digit read from
nis within the valid range before using it in the multiplication. - Prevent Overflow: Implement checks to ensure that updating
digit_base_valuewithBASEdoes not exceed the representable range ofBC_VECTOR, possibly by checking conditions before multiplication or using safe arithmetic routines.
Please make appropriate changes to address these issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Zend/zend_execute.c (2)
1054-1065: 🛠️ Refactor suggestionIncomplete check for protected classes.
The function comment states that a protected class is visible if it is a subclass of the lexical scope, but the code doesn't actually verify this condition.
Add a subclass check for protected classes:
if (!ce->required_scope_absolute) { + zend_class_entry *scope = zend_get_executed_scope(); + if (!instanceof_function(scope, ce->required_scope)) { + zend_type_error("Cannot assign protected %s to a scope that is not a subclass of its required scope", + ZSTR_VAL(ce->name)); + return 0; + } if (current_visibility & ZEND_ACC_PUBLIC) { zend_type_error("Cannot assign protected %s to higher visibile property %s::%s", ZSTR_VAL(ce->name), ZSTR_VAL(info->ce->name), zend_get_unmangled_property_name(info->name)); return 0; } // ... }
1048-1078:⚠️ Potential issueLogic error in protected class visibility check.
The function
zend_check_class_visibilitycontains a bug in the protected class branch. After checking if the current visibility is public (lines 1056-1063), there's an unconditionalreturn 0;on line 1065 that causes protected classes to always fail the visibility check regardless of context.This should be changed to
return 1;to correctly implement the visibility check for protected classes:// a protected class is visible if it is a subclass of the lexical scope and the current visibility is protected or private if (!ce->required_scope_absolute) { if (current_visibility & ZEND_ACC_PUBLIC) { zend_type_error("Cannot assign protected %s to higher visibile property %s::%s", ZSTR_VAL(ce->name), ZSTR_VAL(info->ce->name), zend_get_unmangled_property_name(info->name)); return 0; } - return 0; + return 1; // Return 1 to indicate visibility is allowed }
🧹 Nitpick comments (3)
Zend/zend_compile.c (1)
1797-1827: Potential naming collisions with nested classes
Using":>"as the delimiter might cause naming collisions if future language constructs adopt a similar separator. Currently, this looks fine if truly unique in the language grammar.Consider adding an extra check or documentation note clarifying that
":>"is reserved for internal use only so that it doesn't inadvertently conflict with any future PHP syntax.Zend/zend_execute.c (2)
1072-1076: Improve error message clarity for private classes.The error message for private classes doesn't clearly indicate the scope relationship issue.
Enhance the error message to be more descriptive:
zend_type_error("Cannot assign private %s to higher visibile property %s::%s", - ZSTR_VAL(ce->name), - ZSTR_VAL(info->ce->name), - zend_get_unmangled_property_name(info->name)); + ZSTR_VAL(ce->name), + ZSTR_VAL(info->ce->name), + zend_get_unmangled_property_name(info->name), + "as private classes can only be used within their declaring class");
1237-1252: Refine error-handling to avoid potential undefined behavior whenscope == NULL.When validating type declarations, the code now checks that inner classes are used in the correct scope, but the error handling could be improved to avoid potential undefined behavior.
Consider adding explicit
returnstatements after error conditions to avoid running subsequent code, and use theZSTR_VALmacro consistently:if (ce && ce->required_scope) { zend_class_entry *scope = zend_get_executed_scope(); if (ce->required_scope_absolute && scope != ce->required_scope) { if (scope == NULL) { - zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the global scope", ce->name); + zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the global scope", + ZSTR_VAL(ce->name)); + return; } - zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the scope of %s", ce->name, scope->name); + zend_error(E_ERROR, "Private inner class %s cannot be used as a type declaration in the scope of %s", + ZSTR_VAL(ce->name), ZSTR_VAL(scope->name)); return; } else if (scope == NULL) { - zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the global scope", ce->name); + zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the global scope", + ZSTR_VAL(ce->name)); return; } else if (!instanceof_function(scope, ce->required_scope)) { - zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the scope of %s", ce->name, scope->name); + zend_error(E_ERROR, "Protected inner class %s cannot be used as a type declaration in the scope of %s", + ZSTR_VAL(ce->name), ZSTR_VAL(scope->name)); return; } }This improves code safety by ensuring that the first error won't attempt to use
scope->namewhenscopeis NULL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Zend/zend_compile.c(10 hunks)Zend/zend_execute.c(2 hunks)tests/classes/inner_classes/readonly_001.phpt(1 hunks)
⏰ 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: FREEBSD
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (13)
tests/classes/inner_classes/readonly_001.phpt (3)
7-7: Good test case for readonly properties in inner classes.The inner class declaration uses modern PHP features like constructor property promotion and the readonly modifier. This is a well-designed test to verify that readonly property behavior works correctly within inner classes.
15-18: Properly structured expected output for object dump.The expected output correctly reflects the object structure of the inner class instance with the initialized readonly property.
20-23: Appropriate error message for readonly property violation.The test correctly expects a fatal error when attempting to modify a readonly property. The error message format follows PHP's standard pattern, with placeholders for dynamic elements like file path and line numbers.
Zend/zend_compile.c (9)
896-910: Confirm correct usage ofreadonly,abstract, andfinalfor inner classes
The addition ofZEND_MODIFIER_TARGET_INNER_CLASShere makes these three modifiers permissible on inner classes. This is consistent with your subsequent checks that disallow other modifiers for inner classes. Please ensure that supporting only these three is indeed the intended design.Could you verify alignment with the RFC or your documentation to confirm that allowing these specific modifiers matches the intended behavior of inner classes?
947-948: Enhance error reporting for inner classes
Defining "inner class" in the error messages clarifies feedback to developers when invalid modifiers are encountered.
1866-1870: Consistent handling of nested class references
Returning a fully resolved string viazend_resolve_nested_class_nameseems logical. Memory handling appears correct, provided these names are ref-counted or interned as expected.
7427-7429: Expanding type resolution for inner classes
The logic to build azend_typefor inner classes extends the existing type system consistently.
9181-9220: Renaming and flag handling for inner classes
This logic ensures that inner classes are renamed to include the parent class name and that only the chosen flags (final,abstract,readonly) are applied. Good job removing the bitwise AND bug forZEND_ACC_READONLY_CLASS.
9242-9243: Clearing scope references for non-nested classes
Settingce->required_scopeandce->lexical_scopetoNULLensures that these classes are not treated as nested. Just verify that it correctly handles classes declared at top-level scope.
11853-11855: Hooking into the compiler for inner class references
Invokingzend_compile_inner_class_refin the expression parser seamlessly integrates inner class accesses. This helps maintain uniformity with other AST kinds.
1056-1086:❓ Verification inconclusive
Restricting excessive or conflicting modifiers for inner classes
This block properly disallows multiple parallel access modifiers and preventsstatic,public(set),protected(set), orprivate(set)for inner classes, staying consistent with the notion that inner classes are not standard top-level declarations.Would you like to run a quick test command to confirm that you have coverage for these constraints?
🏁 Script executed:
#!/bin/bash # Searching for test coverage related to static/public/protected/private usage in inner classes rg -A 10 "test.*inner class.*(static|public|protected|private)"Length of output: 65
Review Verification: Confirm Test Coverage for Inner Class Modifier Restrictions
The constraint logic in
Zend/zend_compile.c(lines 1056–1086) correctly enforces that inner classes do not mix multiple access modifiers or usestatic,public(set),protected(set), orprivate(set). However, our initial test search did not uncover any matching test cases for these conditions. Please manually verify that the test suite includes coverage of these error conditions, or consider adding tests to cover these scenarios:
- Multiple access type modifiers on inner classes.
- The use of
staticwith inner classes.- The use of
public(set),protected(set), orprivate(set)on inner classes.
2923-2958:❓ Verification inconclusive
Recursive approach to compiling inner class references
The recursive call tozend_compile_inner_class_reffor further nested classes is straightforward. Just ensure there’s no possibility for cyclic references or infinite recursion.If you want to confirm no cyclical references can be formed, here’s a proposed script:
🏁 Script executed:
#!/bin/bash # Searching for any references that might re-target an ancestor class within the same scope rg -A 15 "parent::" # Or any pattern that might indicate cyclical resolutionLength of output: 101923
Manual Verification Recommended: Confirm that Recursive AST Traversal is Acyclic
The recursive calls in
zend_compile_inner_class_refappear to follow a standard nested inner-class structure and should be safe if the AST is correctly constructed. However, the automated search using the provided "parent::" pattern returned many unrelated hits from various test files and did not yield conclusive evidence of cyclical references for inner class compilation.
- Verify manually that the AST produced by the parser for inner classes is acyclic and cannot generate self-referential cycles.
- If possible, add tests with multiple levels of inner-class nesting to ensure termination.
- Consider incorporating explicit cycle detection if uncertain about the guarantees of AST construction.
Zend/zend_execute.c (1)
1082-1085: Class visibility check integrated with property type verification.The addition of
zend_check_class_visibilityto the property type verification system properly integrates inner class visibility rules with the type system.
```
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
See https://wiki.php.net/rfc/short-and-inner-classes
Summary by CodeRabbit
New Features
Tests
readonlyproperties in inner classes.