Zend type tree #6
Conversation
The idea here is to create something that can be used for other things
essentially making this quite flexible.
required scope: when defined, the class can only be used in the
required scope, or its descendents when absolute is false
lexical scope: the scope the class is defined in -- either a namespace
or another class.
Now we modify the grammar to allow specifying visibility on a class.
This adds a new EG to support the addition of using namespaces as a class's lexical scope
Now we can define nested classes inside other classes
WalkthroughThis pull request implements multiple enhancements to the Zend Engine. It revises type-checking and error handling logic in object initialization and return type validation, and extends support for nested class declarations. New members in class entry structures allow tracking of required and lexical scopes. The update also introduces namespace management routines and augments parser rules for nested classes. In addition, several new tests have been added to validate inner class behavior, access modifiers, inheritance, traits, and autoloading. Finally, related extension components and build configurations have been adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NSResolver as zend_resolve_namespace
participant NTable as GlobalNamespaceTable
participant CreateNS as create_namespace
participant InsertNS as insert_namespace
User->>NSResolver: Request to resolve namespace (name)
NSResolver->>NTable: Look up namespace by name (lowercase)
alt Namespace found
NTable-->>NSResolver: Return existing namespace
else Namespace not found
NSResolver->>CreateNS: Allocate new namespace object
CreateNS-->>NSResolver: New namespace object
NSResolver->>InsertNS: Insert new namespace into table
InsertNS-->>NSResolver: Confirmation
end
NSResolver-->>User: Return namespace object
Note over NSResolver: Cleanup via zend_destroy_namespaces on shutdown
sequenceDiagram
participant Compiler
participant Parser as zend_language_parser.y
participant Scan as zend_scan_nested_class_decl
participant Defer as zend_defer_class_decl
participant Compile as zend_compile_class_decl
Compiler->>Parser: Parse class declaration with nested class
Parser-->>Scan: Identify nested class modifiers and statement
Scan-->>Compiler: Register inner class name
alt Nested class declaration detected
Compiler->>Defer: Defer compilation of inner class
Defer-->>Compiler: Queue inner class for later compilation
else
Compiler->>Compile: Directly compile nested class
end
Compiler->>Compiler: After outer class, compile all deferred inner classes
Note over Compiler: Ensures proper symbol resolution and scope handling
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Zend/zend_API.c (1)
2922-3162:⚠️ Potential issueInvestigate collision handling in interned_type_tree usage
In
intern_type_node(), a simple index-based lookup is performed usinghash(FNV-1a) as the array index. If different type nodes produce the same hash,zend_hash_index_find_ptr()will always retrieve the first entry, and the new node is still added under the same numeric index. Future lookups would continue to match the old node, making the newly added node unreachable. A more robust collision-handling approach or a keyed hash might be necessary to properly intern these type nodes.
🧹 Nitpick comments (24)
Zend/zend_language_parser.y (2)
288-289: Add documentation for the new grammar types.The addition of
nested_class_statementandnested_class_modifiersis straightforward, but please consider adding comments explaining their purpose to aid future maintainers.
631-638: Validate error handling for invalid nested class modifiers.When converting member modifiers to nested class flags, ensure that invalid or conflicting modifiers (e.g., multiple access levels) trigger parser errors. Currently, the check calls
YYERRORifzend_modifier_list_to_flags()returns 0, which is good, but adding more descriptive error messages upstream (if practical) can improve clarity.tests/classes/inner_classes/resolution_002.phpt (1)
16-17: Verifies inheritance relationships through instanceof.These lines correctly test that:
Innerextends the innerOutsideclassMiddleextends the namespace-levelOutsideclassConsider using
var_dump()instead of echo with instanceof for more explicit boolean output in future tests, though the current approach works as intended.-echo Outer\Inner::class . ': ' . (new Outer\Inner()) instanceof Outer\Outside . "\n"; -echo Outer\Middle::class . ': ' . (new Outer\Middle()) instanceof Outside . "\n"; +echo Outer\Inner::class . ': ' . ((new Outer\Inner()) instanceof Outer\Outside ? '1' : '0') . "\n"; +echo Outer\Middle::class . ': ' . ((new Outer\Middle()) instanceof Outside ? '1' : '0') . "\n";tests/classes/inner_classes/simple_declaration_003.phpt (1)
17-17: Verify nested class instantiation syntaxThe instantiation syntax with chained namespace resolution (
Outer\Middle\Inner) looks correct. Consider adding additional tests that verify different ways of instantiating nested classes, such as from within parent/sibling classes or with use statements.tests/classes/inner_classes/inheritance.phpt (2)
4-8: Consider explaining why opcache is requiredThe test requires opcache, but it's not clear why. Please add a comment explaining why this extension is necessary for the test to run correctly.
20-21: Consider adding more specific expectationsWhile a blank
--EXPECT--section implies the test should execute without errors, consider adding more specific expectations. For example, you could add a simple echo statement after class definitions to verify execution reaches that point, or dump class information to verify inheritance relationships were established correctly.Zend/zend_vm_def.h (1)
4431-4439: Consider extracting repeated scope checks into a helper function.
The code block handling “public method cannot return private/protected class” has a similar structure repeated. This can be refactored into a dedicated helper function (or macro) that receives the method's current scope and the returned object's required scope, thereby reducing code duplication and easing future maintenance.tests/classes/inner_classes/visibility_005.phpt (2)
6-22: Consider potential naming collision or clarity.
All three classes (Outer,Middle,Inner) each declare their owntest()method. While this is valid, you might consider differentiating the names (e.g.,testOuter(),testMiddle(),testInner()) to avoid confusion in larger codebases.
25-27: Expected output lines are correct.
Everything aligns with the invocation of nested class methods. It might be beneficial to add an extra negative test to confirm that attempting to call these private methods outside their nesting fails as expected.Zend/zend_object_handlers.c (4)
1845-1848: Repeated fallback check.
This block duplicates the pattern of reassigningscope = scope->lexical_scope. Consider centralizing or encapsulating this logic to avoid scatteredgotousage.
1915-1919: Lexical scope re-check
Redundantifblock. It might be beneficial to unify these repetitive checks into a single inline function that attempts lexical scope fallback for method lookups.
2003-2006: Scope fallback duplication
We see another iteration of the same block for fallback toscope->lexical_scope. Centralizing this in a helper function would reduce duplication and maintenance overhead.
2093-2095: Visibility check fallback
Reiterates the earlier logic. For readability, consider a small utility function that handles scope checks and returns the final resolved scope, rather than multiple scatteredgotojumps.tests/classes/inner_classes/visibility_008.phpt (1)
1-31: Well-structured test for sibling method access in nested classesThis test properly validates that inner classes within a child class can access both protected methods and protected inner classes of the parent class. The code pattern follows good testing practices by clearly demonstrating the functionality and providing expected output.
Consider adding a comment at the beginning of the test explaining what specific aspect of visibility rules this test is validating, which would make the purpose clearer for future developers:
<?php +// This test verifies that inner classes can access protected members (both methods and inner classes) +// from parent classes, following PHP's inheritance and visibility rules. class Outer {tests/classes/inner_classes/simple_declaration_004.phpt (1)
1-39: Good test for scope resolution with nested classesThis test effectively validates how class names resolve in the context of nested classes and inheritance. The test verifies both explicit resolution via fully qualified names (
Outer\Middle) and implicit resolution through inheritance contexts.For improved code clarity, the PHP return type declarations could include the full namespace path in some cases:
- public static function testSelf(): Middle { + public static function testSelf(): self\Middle {This makes it more explicit which
Middleclass is being referred to, although the current code is also correct and demonstrates how PHP resolves these types automatically.tests/classes/inner_classes/simple_declaration_005.phpt (1)
1-35: Good test for enforcing type compatibility with nested classesThis test correctly validates that PHP enforces return type compatibility when overriding methods that return inner class types. The error message confirms that
Outer2\Middlemust be compatible withOuter\Middlewhen overriding thetestSelf()method.There's a redundant duplicate call on line 30:
var_dump(Outer::testSelf()); var_dump(Outer2::testParent()); var_dump(Outer2::testSelf()); -var_dump(Outer2::testSelf());The duplicate call is unnecessary, especially since the code expects a fatal error that would prevent execution of this line anyway.
Zend/zend_vm_execute.h (2)
10802-10820: Consider extracting common scope-checking logic into a helper.This block repeats across multiple opcodes. Extracting it into a helper or macro would reduce duplication and improve long-term maintainability.
37860-37877: Check performance overhead of repetitive scope checks.Although correctness is priority, repeated object scope checks may introduce overhead. Consider if any optimization or centralizing could help.
Zend/zend_namespaces.c (3)
1-19: Check license consistency.The file references version 2.00 of the Zend license, whereas other files in the engine commonly reference version 3.01 or 3.00. Verify that this license header is correct and consistent with the rest of the project licensing requirements.
65-88: Validate concurrent initialization of the global namespace.
zend_resolve_namespacesets upEG(global_namespace)andEG(namespaces)if uninitialized. In multi-threaded or parallel request scenarios, ensure that this function cannot cause race conditions if multiple code paths call it simultaneously. If necessary, introduce locking or confirm that existing VM-level synchronization is sufficient.
90-96: Optional: Provide a fast path for lookups that fail.
zend_lookup_namespaceconverts the name to lowercase, checks the hash, and frees the temporary string. If it’s frequently called for non-existing namespaces, consider an early exit or caching negative lookups to reduce overhead. This optimization may not be critical but could improve performance in some workloads.Zend/zend_execute.c (3)
1048-1079: Consider validatinginfo->ceprior to usage.While this function's checks for class scope are logically consistent, consider adding defensive checks (e.g., ensuring
info->ceis not NULL). This helps avoid undefined behavior in unusual deployment scenarios.
1082-1086: Clarify the success vs. failure check.This condition relies on
zend_check_type_visibility()returning a non-zero value for failure. For clarity, you could rename or invert the return logic (e.g., returning a boolean for “visibility is valid” vs. “visibility is invalid”) to reduce confusion around 0/1 return semantics.
1205-1262: Augment test coverage for recursive union & intersection types.The new logic correctly short-circuits on union (returning true if any node matches) and intersection (returning false on the first mismatch). To guard against regressions, consider expanding tests to cover multiple nested unions/intersections and callable/static checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
Zend/Optimizer/dfa_pass.c(1 hunks)Zend/tests/errmsg/errmsg_027.phpt(1 hunks)Zend/zend.h(1 hunks)Zend/zend_API.c(5 hunks)Zend/zend_API.h(6 hunks)Zend/zend_compile.c(20 hunks)Zend/zend_compile.h(5 hunks)Zend/zend_execute.c(8 hunks)Zend/zend_execute.h(1 hunks)Zend/zend_execute_API.c(3 hunks)Zend/zend_globals.h(2 hunks)Zend/zend_language_parser.y(4 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_namespaces.h(1 hunks)Zend/zend_object_handlers.c(7 hunks)Zend/zend_opcode.c(2 hunks)Zend/zend_types.h(1 hunks)Zend/zend_vm_def.h(1 hunks)Zend/zend_vm_execute.h(5 hunks)configure.ac(1 hunks)ext/opcache/jit/zend_jit_helpers.c(2 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)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/aliases.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/enum_usage.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/instantiation_001.phpt(1 hunks)tests/classes/inner_classes/interface_usage.phpt(1 hunks)tests/classes/inner_classes/readonly.phpt(1 hunks)tests/classes/inner_classes/reflection_001.phpt(1 hunks)tests/classes/inner_classes/resolution_001.phpt(1 hunks)tests/classes/inner_classes/resolution_002.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/simple_declaration_005.phpt(1 hunks)tests/classes/inner_classes/static_variables.phpt(1 hunks)tests/classes/inner_classes/trait_usage_001.phpt(1 hunks)tests/classes/inner_classes/visibility_001.phpt(1 hunks)tests/classes/inner_classes/visibility_002.phpt(1 hunks)tests/classes/inner_classes/visibility_003.phpt(1 hunks)tests/classes/inner_classes/visibility_004.phpt(1 hunks)tests/classes/inner_classes/visibility_005.phpt(1 hunks)tests/classes/inner_classes/visibility_006.phpt(1 hunks)tests/classes/inner_classes/visibility_007.phpt(1 hunks)tests/classes/inner_classes/visibility_008.phpt(1 hunks)tests/classes/inner_classes/visibility_009.phpt(1 hunks)tests/classes/inner_classes/visibility_010.phpt(1 hunks)win32/build/config.w32(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
Zend/zend_execute_API.c (2)
Zend/zend_namespaces.c (1)
zend_destroy_namespaces(98-105)Zend/zend_namespaces.h (1)
zend_destroy_namespaces(27-27)
Zend/zend_namespaces.h (1)
Zend/zend_compile.h (1)
name(992-992)
Zend/zend_namespaces.c (1)
Zend/zend_namespaces.h (3)
name(25-25)name(26-26)zend_destroy_namespaces(27-27)
ext/reflection/php_reflection.c (1)
ext/reflection/php_reflection.stub.php (14)
ReflectionClass(233-443)isPrivate(191-191)isPrivate(438-438)isPrivate(516-516)isPrivate(612-612)isProtected(194-194)isProtected(442-442)isProtected(519-519)isProtected(615-615)isPublic(188-188)isPublic(440-440)isPublic(513-513)isPublic(609-609)isInnerClass(436-436)
Zend/zend_execute.c (1)
Zend/zend_execute.h (3)
zend_verify_property_type_error(528-528)zend_verify_scalar_type_hint(99-99)zend_check_user_type_slow(107-108)
Zend/zend_compile.c (2)
Zend/zend_namespaces.h (2)
name(25-25)name(26-26)Zend/zend_namespaces.c (1)
zend_resolve_namespace(65-88)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: WINDOWS_X64_ZTS
🔇 Additional comments (151)
configure.ac (1)
1760-1760: New source file added for namespace managementThe addition of
zend_namespaces.cto the Zend module's source files is appropriate and consistent with the PR's objective of implementing the Zend type tree. This new file will likely handle the namespace management functionality required for supporting nested class declarations.Zend/zend.h (1)
167-170: Enhanced class entry structure to support nested classesThe addition of these three new fields to the
_zend_class_entrystructure will enable proper scope management for nested class declarations:
required_scope: Points to the class in which instances of this class must be createdlexical_scope: Tracks the enclosing/containing class where this class is definedrequired_scope_absolute: Controls whether subclasses of the required scope can instantiate this classThis change is well-designed and appropriately addresses the requirements for implementing nested class declarations with proper scope controls.
Zend/zend_language_parser.y (2)
955-957:✅ Verification successful
Confirm behavior for nested class declarations.
The new rule correctly creates a class node for nested classes. Be sure to verify the usage of
$<num>$ = CG(zend_lineno);to ensure correct line information in error messages, especially for complex nested declarations.
🏁 Script executed:
#!/bin/bash # Verify that nested classes generate accurate line numbers in logs # by searching for compile-time error triggers and ensuring line usage is correct. rg -A 10 "YYERROR"Length of output: 11031
Nested Class Declaration Line Number Verification
The parser rule now assigns the current line number via
$<num>$ = CG(zend_lineno);right after theT_CLASS T_STRINGtoken sequence. Comparison with similar rules (e.g. in trait declarations) indicates this usage is consistent and appropriate. While our search for error triggers (YYERROR) returned multiple instances across the parser, no anomalies were detected that would indicate incorrect linenumber reporting for nested classes.Please double-check that all error messages triggered inside nested declarations use this line information to pinpoint the source correctly.
- Location to verify:
Zend/zend_language_parser.ylines 955-957 (class declaration rule).Your usage appears consistent with the surrounding patterns; however, remain alert for any irregularities in error logs.
977-977:❓ Verification inconclusive
Set the nested class’s attributes after creation.
Assigning the modifiers to
$$->attrensures the nested class respects the proper visibility. Verify that no conflicting flags exist, such asZEND_ACC_TRAIT, which may cause unexpected behaviors in nested classes.
🏁 Script executed:
#!/bin/bash # Description: Search for references to nested class flags to ensure no conflicts with trait or interface flags. ast-grep --pattern $'enum ZEND_ACC_TRAIT { $$$ }'Length of output: 872
Action Required: Manually Verify Flag Conflicts in Nested Class Attribute Setting
The code at line 977 in
Zend/zend_language_parser.yassigns the parsed modifiers to the newly created nested class instance. Please confirm manually that none of these modifiers include conflicting flags, likeZEND_ACC_TRAIT, which could interfere with nested class behaviors. Although our automated check usingast-grepdid not yield clear output—likely due to non-UTF-8 file issues—the intent is to ensure that no unintended trait-related flags are propagated.Next Steps:
- Manually inspect the definition and usage of
ZEND_ACC_TRAITwithin your repository (especially in header or source files related to class flags).- Verify that the assignment at
$$->attr = $1;does not inadvertently set this flag for nested classes.tests/classes/inner_classes/resolution_001.phpt (1)
1-16: Validate test coverage.This test confirms that
Outer\Innerproperly extendsOutsideand thatinstanceofchecks pass. It looks complete and effectively verifies inheritance resolution. Great to see simple but crucial coverage.tests/classes/inner_classes/access_modifiers_006.phpt (1)
6-9: Validate error message for invalid nested class modifier.This test ensures that using
private(set)in a nested class produces the correct fatal error. The coverage for this scenario is essential for verifying strict access control rules in nested classes.tests/classes/inner_classes/access_modifiers_003.phpt (1)
1-14: Test looks good - properly validates static modifier restriction on inner classes.The test correctly verifies that using the
staticmodifier on an inner class should result in a fatal error. This is consistent with the expected behavior for nested class declarations.tests/classes/inner_classes/access_modifiers_005.phpt (1)
1-14: Test correctly validates that protected(set) modifier is not allowed on nested classes.This test properly checks that using the
protected(set)modifier on an inner class results in a fatal error, which aligns with the expected behavior for access modifiers on nested classes.tests/classes/inner_classes/errors_002.phpt (1)
1-16: Test correctly verifies error handling for non-existent inner classes.This test properly validates the error handling when attempting to instantiate an inner class that doesn't exist within a defined outer class. The expected fatal error and stack trace are appropriate for this scenario.
tests/classes/inner_classes/errors_001.phpt (1)
1-13: Test properly verifies error handling when outer class doesn't exist.This test correctly validates the error handling when attempting to instantiate an inner class from a non-existent outer class. The expected fatal error and stack trace are appropriate for this scenario.
tests/classes/inner_classes/enum_usage.phpt (1)
1-17: LGTM: Good test case for inner classes within enums.This test correctly validates that inner classes can be declared within PHP enums and properly accessed using the namespace operator. It verifies both object instantiation and class existence checking.
tests/classes/inner_classes/access_modifiers_004.phpt (1)
1-14: LGTM: Test correctly validates public(set) restriction on inner classes.This test properly verifies that the
public(set)access modifier cannot be applied to nested classes. The test follows the standard pattern for PHP error tests with appropriate error message verification.tests/classes/inner_classes/simple_declaration_002.phpt (1)
1-14: LGTM: Good boundary test for inner class restrictions.This test correctly validates that class declarations are not permitted inside functions, even within class methods. It establishes an important boundary for the inner class feature implementation.
tests/classes/inner_classes/access_modifiers_002.phpt (1)
1-14: LGTM: Good negative test for parser validation.This test correctly verifies that the PHP parser properly handles and reports syntax errors when an invalid inner class declaration is encountered. The error message validation is accurate and specific about the syntax problem.
Zend/tests/errmsg/errmsg_027.phpt (1)
16-16: Improved error message clarityThe updated error message is more specific and accurately describes the restriction - class declarations cannot be inside functions, rather than using the more ambiguous "may not be nested" wording. This change aligns with the PR's broader work on supporting actual nested classes while maintaining proper restrictions on function-local class declarations.
tests/classes/inner_classes/access_modifiers_001.phpt (3)
1-3: Good test for multiple access modifiersThis test properly verifies that using multiple access modifiers on a class declaration results in a fatal error, which is important to validate as part of the inner classes implementation.
6-9: Clear test case structureThe test case clearly demonstrates the issue by attempting to declare an inner class with conflicting access modifiers (both public and private), which should trigger a syntax error.
13-13: Appropriate error message checkThe expected fatal error message is specific and accurately describes the issue with multiple access modifiers.
ext/reflection/tests/bug74454.phpt (1)
17-17: Updated parser error messageThe expected error message has been updated to reflect changes in the parser's token expectations. Now it indicates that a "class" was expected rather than a "function", which aligns with the changes in how class declarations are parsed and validated in this PR.
tests/classes/inner_classes/return_types_001.phpt (4)
1-3: Good test for inner class return typesThis test properly verifies the functionality of return type declarations with inner classes, which is an important aspect of type safety in the new nested class implementation.
6-12: Well-structured nested class definitionThe test case properly defines an Outer class containing an Inner class, and correctly implements a static method with a return type that references the inner class using its fully qualified name. This demonstrates proper namespace handling for inner classes.
14-15: Proper instantiation and method invocationThe code correctly demonstrates how to access and invoke a static method on an inner class using the proper namespace syntax, which is an important aspect to test.
17-18: Correct output verificationThe expected output correctly verifies that the returned object is indeed of type Outer\Inner, confirming that both the method return type and instantiation work properly.
tests/classes/inner_classes/inner_classes.inc (1)
3-9: Well-structured inner class implementationThis file correctly demonstrates the syntax for inner classes with different access modifiers. The
Pointclass uses constructor property promotion (a PHP 8.0+ feature) to efficiently define properties, while theLineclass shows a minimal inner class declaration.tests/classes/inner_classes/simple_declaration_001.phpt (2)
6-10: Good test case for nested class declarationsThe test properly demonstrates multi-level nesting with three levels of class declarations (Outer → Middle → Inner). This is a good test for verifying the basic functionality of the new feature.
12-15: Comprehensive class existence verificationThe test checks class existence using both string notation and the
::classconstant, which provides good coverage of how developers might reference these classes in real code.tests/classes/inner_classes/autoload_001.phpt (2)
5-8: Well-implemented autoloader functionThe autoloader function is correctly implemented to handle class name resolution based on the namespace hierarchy. It extracts the first segment of the namespace path to locate the appropriate include file.
10-11: Appropriate test for inner class instantiationThis creates a good test case showing that an inner class can be instantiated using its namespaced name and that its properties are accessible. The test validates both autoloading and proper object instantiation.
win32/build/config.w32 (1)
243-243: Addition of zend_namespaces.c to build systemThe addition of
zend_namespaces.cto the build configuration is necessary to include the new namespace management functionality that supports inner classes. This change ensures the new source file is properly compiled as part of the Zend engine.tests/classes/inner_classes/readonly.phpt (1)
1-24: Test thoroughly validates readonly inner class behaviorThis test properly verifies that readonly properties in inner classes behave as expected - allowing initialization but throwing a fatal error when modification is attempted. The test structure with appropriate sections (--TEST--, --FILE--, --EXPECTF--) is correct.
tests/classes/inner_classes/visibility_004.phpt (1)
1-26: Effective test for inner class visibility restrictionsThis test correctly validates that a protected inner class cannot be used as a type for a public property. The test appropriately triggers and verifies the expected TypeError exception with the correct error message and stack trace.
tests/classes/inner_classes/return_types_006.phpt (1)
1-22: Return type test correctly validates private inner class visibilityThis test effectively demonstrates that a private inner class instance can be returned from an inner method. Using
mixedas the return type forOuter::test()is appropriate here since the inner class is private and not directly accessible from outside.Zend/Optimizer/dfa_pass.c (1)
285-285: Important scope check added for type optimizationThe additional condition
!ce->required_scopeensures that type optimization only happens for classes that don't require a specific scope. This is essential for properly handling inner classes, which may have required scopes.tests/classes/inner_classes/autoload_002.phpt (1)
1-20: Correct test configuration and implementation.This test verifies that PHP correctly applies visibility restrictions when autoloading classes. The test checks that a private class can't be instantiated from the global scope even when autoloading is involved.
Zend/zend_namespaces.h (1)
1-29: Clear and well-structured API for namespace management.The header properly defines three key API functions for handling namespaces in the Zend Engine:
zend_resolve_namespace: Resolves a namespace name to a class entryzend_lookup_namespace: Looks up a namespace by namezend_destroy_namespaces: Cleans up namespace resourcesThe header follows PHP coding conventions with proper licensing and documentation.
tests/classes/inner_classes/aliases.phpt (1)
1-19: Effective test for visibility restrictions with class aliases.This test correctly verifies that using
class_alias()doesn't allow bypassing private class visibility restrictions. The test creates a private inner class and attempts to instantiate it via an alias, which should result in a fatal error.ext/opcache/zend_persist.c (2)
1124-1143: Proper implementation of class scope persistence.The new
zend_update_ce_scopesfunction correctly updates class scope references when persisting class entries. It handles bothrequired_scopeandlexical_scopeproperties by retrieving their corresponding entries from shared memory using the translation table.
1318-1318: Good integration of scope update into the persistence flow.The call to
zend_update_ce_scopesis appropriately placed in thezend_accel_persist_class_tablefunction, right after updating parent class entries, ensuring that all class references are properly maintained during persistence.tests/classes/inner_classes/visibility_001.phpt (3)
6-13: Effectively tests visibility constraints for inner classes.This test correctly verifies that a private inner class cannot be exposed through a public property, which is an important encapsulation principle. The class structure creates a clear violation case that should trigger the expected fatal error.
15-18: Test flow is appropriate for verifying fatal error.The sequence of creating an instance, calling test(), and attempting to dump the object is well-structured to trigger and capture the expected fatal error. The execution will never reach
var_dump($x)due to the fatal error, which is intended behavior.
20-25: Expected output correctly captures the fatal error pattern.The test properly uses
%sand%dplaceholders to match the dynamic parts of the error message (file path and line numbers), while keeping the actual error content fixed for consistent verification.tests/classes/inner_classes/trait_usage_001.phpt (5)
6-8: Tests inner class definition within a trait.This correctly defines a trait containing an inner class, which is a key scenario to test with the new inner class implementation.
10-10: Verifies direct instantiation of an inner class from a trait.This line correctly tests that an inner class defined in a trait can be instantiated using the trait name as a namespace qualifier.
12-14: Tests trait usage in a regular class.This correctly tests that a trait containing an inner class can be used by a regular class, which is an important use case to verify.
16-17: Verifies class resolution within trait and class namespaces.This effectively tests an important distinction: inner classes defined in traits remain in the trait's namespace and don't get imported into the namespace of classes that use the trait.
20-24: Expected output is consistent with PHP's namespace and trait behavior.The expected output correctly anticipates that
Outer\Innerexists butFoo\Innerdoes not, which aligns with how traits should behave with inner classes.tests/classes/inner_classes/resolution_002.phpt (2)
6-14: Effectively tests class name shadowing and resolution.This test creates a comprehensive scenario to verify how inner classes handle name resolution when the same class name exists in both the outer namespace and as an inner class. The inheritance relationships test both local (inner) and global (namespace) class resolution.
19-21: Expected output correctly matches the instanceof results.The expected output correctly anticipates both instanceof checks returning true (displayed as "1"), which verifies that class inheritance works properly across both inner class boundaries and namespace boundaries.
Zend/zend_compile.c (20)
41-41: Include statement looks fine.
The addition of#include "zend_namespaces.h"is appropriate for accessing newly introduced namespace resolution functions.
428-428: Initialization ofCG(nested_class_queue)
SettingCG(nested_class_queue)to NULL is consistent with how Zend Compiler Globals are generally handled, preventing stale data across compilation units. No immediate concurrency concerns are evident, assuming compilation is a single-threaded process.
898-898: Extending modifiers to nested classes
AddingZEND_MODIFIER_TARGET_NESTED_CLASSto the relevant checks forreadonly,abstract, andfinalensures that these modifiers are recognized for nested classes. This approach aligns with the new nested class feature.Also applies to: 903-903, 911-912
949-950: Clarifying error messages
Definingmember = "nested class"helps produce clearer error messages when modifiers are incorrectly applied to nested classes.
1058-1088: Restricting certain modifiers on nested classes
The compile-time checks correctly throw errors for multiple access modifiers, or disallowed usage ofstatic,public(set),protected(set), orprivate(set)on nested classes. This robustly enforces the intended design of nested class restrictions.
1183-1207: Introducingzend_resolve_class_in_scope
The new function properly attempts to resolve a class name in the lexical scope chain. String handling and memory management (allocating/releasingtry_name) appear correct.
1267-1272: Nested name resolution
The early return ifzend_resolve_class_in_scopesucceeds is consistent with the logic for intercepting nested classes. This integration looks solid.
7420-7420: Simple return statement
Returning thetypevariable at the end of this flow is concise, and there are no apparent correctness issues in this snippet.
7657-7657: Assigningtype_treefor argument info
Settingarg_infos->type_tree = zend_type_to_interned_tree(arg_infos->type);aligns with the approach of interning type information for optimization. No concerns found.
7905-7905: Interning property type information
Similar to the previous assignment,arg_info->type_tree = zend_type_to_interned_tree(arg_info->type);is a coherent way to unify property metadata.
9056-9056: Refiningzend_compile_implements
Exposingzend_class_entry *ceas a parameter is logical for supporting nested classes’ interface declarations. Implementation looks consistent with the rest of the engine.
9116-9151: Deferred and recursive scanning for nested classes
Thezend_defer_class_decl()function queues class declarations encountered within an active class.zend_scan_nested_class_decl()recursively scans and registers nested classes by name. Both functions properly handle memory and guard against declaring classes inside functions. Overall, this is a sound approach to deferring nested class compilation.
9178-9204: Handling nested classes’ scope requirements
Storingrequired_scopeandlexical_scopefor nested classes is crucial to respect the parent class's visibility constraints. The fallback to namespace resolution outside an active class is also logical.
9222-9223: Anonymous class fallback scope
For anonymous classes, settingrequired_scope = NULLand falling back to either the active class or the current namespace is a consistent strategy.
9265-9267: Implementing interfaces for nested classes
Invokingzend_compile_implements(implements_ast, ce);ensures interface declarations are handled similarly for nested classes. This remains consistent with the new approach.
9283-9285: Scanning nested classes in namespace scopes
Invokingzend_scan_nested_class_decl(stmt_ast, name);forZEND_NAMESPACE_CLASSeffectively handles further nested classes inside a namespaced class.
9315-9315: Goto to compile nested classes (1)
Thegoto compile_nested_classes;usage follows established Zend Engine patterns for branching to the final compilation phase of nested classes.
9324-9324: Goto to compile nested classes (2)
Again usinggoto compile_nested_classes;is consistent with the engine’s style and overall approach to finalizing nested classes outside the normal fall-through logic.
9394-9396: Compiling deferred nested classes
After finishing the main class, queued nested classes are compiled by iterating overCG(nested_class_queue). This finalization step prevents partial compilation states and ensures fully nested definitions. The hashtable is released properly, avoiding memory leaks.Also applies to: 9401-9411
11702-11705: Deferring class declarations inside another class
Deferring the compilation of an innerZEND_AST_CLASSnode whenCG(active_class_entry)is set ensures correct nesting logic. Breaking out of the switch to handle it later is a solid approach.tests/classes/inner_classes/return_types_003.phpt (3)
1-34: Test confirms visibility constraints for inner class return typesThis test file correctly verifies that a public method cannot return a protected inner class, which is a crucial visibility constraint for inner classes. The test demonstrates both a valid case (protected method returning a protected class) and an invalid case (public method returning a protected class).
14-19: Excellent test case for method overriding with visibility constraintsThis is a good test case showing the interaction between inheritance and inner class visibility. The override in the child class correctly triggers a type error because public methods cannot expose protected inner classes as return types.
29-33: Verify error message formatting is consistent with PHP standardsThe error message correctly identifies the visibility conflict between a public method and protected inner class. Make sure this error wording has been reviewed by the PHP language team for consistency with other type errors.
tests/classes/inner_classes/simple_declaration_003.phpt (2)
1-21: Test verifies basic nested class declaration and resolutionGood test case that confirms deeply nested class declarations work properly with the correct namespace resolution. This validates that the fully qualified name is properly constructed for classes nested three levels deep.
8-16: Nested class structure follows expected hierarchyThe class structure clearly demonstrates nesting at multiple levels. This is a fundamental test for the inner class implementation.
tests/classes/inner_classes/inheritance.phpt (2)
1-21: Test verifies circular inheritance detection works with inner classesThis test checks that PHP correctly handles potential circular inheritance involving inner classes. Since there's no expected output, the test is validating that the code compiles without errors, which is the expected behavior.
12-18: Complex inheritance relationships between inner classesThe test creates a complex inheritance structure where:
MiddleextendsOuter\OtherInner1extendsInner2Inner2extendsOtherThis tests that inheritance relationships can be properly resolved across different nesting levels.
Zend/zend_vm_def.h (2)
4440-4445: Double-check coverage for private methods.
Currently, the snippet handles public and protected methods returning objects with a required scope. Ensure either that private methods returning objects with conflicting scopes are caught in another code path, or that it’s not needed by design. If it’s needed, consider explicitly including a private method check to avoid confusion.
4448-4451: Validate the new type checks for all return paths.
The call tozend_check_type_slownow includes a type tree reference. Please verify thatret_info->type_treeis properly initialized in all code paths reaching this check, preventing potential null pointer dereferences or incorrect type verification.tests/classes/inner_classes/interface_usage.phpt (4)
6-8: LGTM! Inner class implementation within an interface looks correct.The test demonstrates that inner classes can be defined within interfaces, which is a logical extension of the inner class feature.
10-10: LGTM! Proper namespace syntax for accessing inner class.The namespace resolution syntax with
Outer\Innercorrectly accesses the inner class defined within the interface.
12-15: LGTM! Appropriate class existence checks.These checks validate expected behavior: the inner class is accessible through the interface namespace (
Outer\Inner) but not through the implementing class namespace (Foo\Inner). This confirms that inner classes are scoped to their containing type, not to implementing types.
17-21: LGTM! Expected output matches behavior intent.The expected output correctly validates that:
- The inner class can be instantiated
- It exists when accessed via the interface name
- It doesn't exist when accessed via an implementing class
tests/classes/inner_classes/return_types_005.phpt (4)
6-8: LGTM! Protected inner class declaration looks correct.The test properly sets up a protected inner class within an outer class, which is necessary for testing visibility constraints.
9-11: LGTM! Method with return type referring to protected inner class.The method correctly specifies a return type using the short-hand syntax
Innerwhich refers to the protected inner class. This setup is good for testing visibility constraints with return types.
14-19: LGTM! Test correctly verifies visibility constraint violation.The test tries to use a protected inner class type outside its accessibility scope, which should trigger the appropriate type error. The function declaration using fully qualified
Outer\Inneras parameter type is a good test case.
21-26: LGTM! Expected error message is appropriate.The expected error message correctly identifies that a public method cannot return a protected class, which upholds proper visibility constraints. This ensures that return types follow the same visibility rules as other class members.
tests/classes/inner_classes/return_types_004.phpt (4)
6-8: LGTM! Private inner class declaration looks correct.The test properly sets up a private inner class within an outer class, which is necessary for testing visibility constraints.
9-11: LGTM! Method with return type referring to private inner class.The method correctly specifies a return type using the short-hand syntax
Innerwhich refers to the private inner class. This setup is good for testing visibility constraints with return types.
14-19: LGTM! Test correctly verifies visibility constraint violation.The test tries to use a private inner class type outside its accessibility scope, which should trigger the appropriate type error. The function declaration using fully qualified
Outer\Inneras parameter type is a good test case.
21-26: LGTM! Expected error message is appropriate.The expected error message correctly identifies that a public method cannot return a private class, which upholds proper visibility constraints. This ensures that return types follow the same visibility rules as other class members.
Zend/zend_globals.h (2)
88-88: LGTM! Added nested_class_queue to compiler globals.The addition of
nested_class_queueto the compiler globals structure will support tracking nested class declarations during compilation, which is essential for implementing inner classes.
195-196: LGTM! Added namespace tracking to executor globals.The additions of
namespacesandglobal_namespaceto the executor globals structure will support namespace management during execution, which is necessary for implementing the nested class feature properly.ext/reflection/tests/ReflectionClass_toString_001.phpt (2)
33-33: Methods count updated to reflect new additions.The total method count is correctly updated from 64 to 68 to reflect the addition of four new methods to the ReflectionClass.
518-544: New reflection methods for class visibility and inner class detection.The addition of these four new methods (
isInnerClass(),isPrivate(),isPublic(),isProtected()) enhances the reflection capabilities to support the new inner class features. This correctly enables introspection of class visibility modifiers and inner class status.tests/classes/inner_classes/access_modifiers_007.phpt (1)
1-28: Test for abstract inner classes.This test properly validates that:
- Abstract inner classes can be extended
- Concrete subclasses of abstract inner classes can be instantiated
- Abstract inner classes cannot be directly instantiated
The test structure follows PHP's phpt format with appropriate test description, code, and expected output sections.
ext/reflection/php_reflection.stub.php (1)
436-442: New reflection methods for class visibility and inner class detection.These four new methods extend the ReflectionClass API to support introspection of:
- Whether a class is an inner class (
isInnerClass())- Class visibility modifiers (
isPrivate(),isPublic(),isProtected())The method signatures follow the established pattern in the Reflection API, returning boolean values consistently with other similar methods.
tests/classes/inner_classes/return_types_002.phpt (3)
6-16: Proper encapsulation of private inner class.The outer class correctly defines a private inner class that is only accessible within its scope. The private method
getInner()properly returns the inner class type, while the public methodgetInner2()uses a mixed return type to avoid exposing the private class type in the public API.
18-23: Validation of private inner class access restrictions.This code correctly tests that:
- A subclass can access a private inner class indirectly through parent methods
- A subclass cannot directly instantiate a private inner class
The method
getInner()in classFoodoesn't actually override the private method inOutersince they have different visibilities, but this appears to be intentional for testing purposes.
25-36: Expected error for private class instantiation from invalid scope.The test correctly verifies that attempting to instantiate a private inner class from an invalid scope (subclass) results in a TypeError. The expected output section properly includes both the successful output from accessing the inner class through
getInner2()and the fatal error from trying to directly instantiate it.tests/classes/inner_classes/visibility_003.phpt (1)
1-31: LGTM: Properly tests outer class property access from inner classesThis test properly verifies that inner classes can access private properties of their enclosing outer class - an important feature of inner class implementation. The test demonstrates that the
Innerclass can directly assign itself to the$illegalprivate property of theOuterclass, confirming proper lexical scope inheritance.Zend/zend_types.h (1)
145-164: Well-structured type node implementation for supporting type treesThe addition of the
zend_type_nodestructure introduces proper support for complex type representations in the PHP type system. The structure is well-designed with:
- An enum to distinguish between simple, union, and intersection types
- A union to efficiently store either simple types or compound type collections
- Support for multiple types in compound structures
This implementation will enable proper handling of union and intersection types, enhancing the type system's expressiveness.
Zend/zend_opcode.c (1)
435-535: Proper memory management for namespace classesThe addition of the
ZEND_NAMESPACE_CLASScase in thedestroy_zend_classfunction ensures proper memory management for namespace classes. Usingpefree(ce, 0)instead offree(ce)for namespace classes is appropriate as it handles the memory allocated with persistence correctly.tests/classes/inner_classes/visibility_009.phpt (1)
1-38: Comprehensive test for deeply nested class property accessThis test effectively validates property visibility rules with deeply nested classes. It confirms that:
- Inner classes can access private static properties of their outer classes
- Inner classes can access private instance properties of their outer classes
- Proper scoping is maintained even with multiple levels of nesting
- Class names are correctly qualified in the output
The test provides good coverage of nested class visibility behaviors.
tests/classes/inner_classes/reflection_001.phpt (1)
1-65: Test looks good - comprehensive reflection testing for inner classes.This test file properly checks reflection capabilities for inner classes with different visibility modifiers. It verifies that
ReflectionClasscorrectly reports inner class status, visibility modifiers, and name information. The test structure is clean and the expected output matches what the code should produce.Zend/zend_execute_API.c (3)
42-42: Nice addition of namespace management header.The inclusion of the zend_namespaces.h header prepares this file to handle namespace-related operations.
152-153: Good initialization of namespace globals.Properly initializing the namespace-related globals to NULL during executor initialization is a good practice to ensure they're in a known state before use.
502-502: Proper cleanup of namespace resources.Adding the call to
zend_destroy_namespaces()during shutdown ensures that all namespace-related resources are properly freed, preventing memory leaks.tests/classes/inner_classes/instantiation_001.phpt (5)
6-13: Good test setup for inner classes with different visibility modifiers.The Outer class is well-structured with private, protected, and public inner classes to test all visibility scenarios.
15-31: Comprehensive testing of inner class instantiation from global scope.The test correctly verifies that private and protected inner classes cannot be instantiated from the global scope, while public inner classes can be. The error handling with try-catch blocks is a good approach.
33-52: Well-structured tests for non-inheritance context.This section properly tests that an unrelated class cannot instantiate private or protected inner classes, enforcing proper visibility boundaries.
54-73: Good testing of inheritance visibility rules.This section correctly verifies that a child class can access protected inner classes of its parent but still cannot access private inner classes, which aligns with standard PHP visibility rules.
76-89: Expected output matches visibility rules for inner classes.The expected output properly reflects PHP's visibility rules as applied to inner classes - showing correct error messages for illegal instantiations and successful object creation for allowed cases.
ext/reflection/php_reflection_arginfo.h (3)
2-2: Hash update in generated fileThe stub hash has been updated in this generated file, which is expected when modifications are made to the corresponding .stub.php file.
369-375: New reflection methods for class visibility and structureThese additions enhance the Reflection API with new methods to determine if a class is an inner class and to check its visibility modifiers. This is consistent with support for nested classes and proper visibility controls being added in this PR.
1154-1157: Implementation registration for new reflection methodsThe new methods are properly registered in the
class_ReflectionClass_methodsarray, following the pattern of other methods and ensuring they'll be available at runtime.tests/classes/inner_classes/visibility_007.phpt (3)
1-4: Well-structured test descriptionThe test clearly specifies its purpose: validating that inner classes can access protected methods from outer classes.
6-22: Properly implemented nested class structureThe test creates a nested class hierarchy (Outer > Middle > Inner) with protected methods, and verifies that inner classes can correctly access protected methods from their parent contexts. This is an essential test for validating the visibility rules of nested classes.
23-27: Correct test validationThe test instantiates the inner class and invokes its method, with appropriate expected output that confirms both Middle and Outer protected methods are accessible from the inner class.
tests/classes/inner_classes/static_variables.phpt (4)
1-3: Clear test purpose descriptionThe test description clearly states it's checking class reference syntax, static variables, and inner classes functionality.
6-15: Well-designed nested class structure with constants and static variablesThe test creates a nested class structure with constants and static variables at different levels of nesting, which is important for verifying namespace resolution and access to static elements in nested classes.
17-22: Comprehensive validation of class access syntaxThe test verifies all aspects of accessing nested classes: class names, constants, and static variables - ensuring the namespace resolution works correctly through the fully qualified names.
24-30: Expected output confirms correct namespace resolutionThe expected output correctly shows the fully qualified class names and confirms that constants and static variables are properly accessible through the nested class structure.
tests/classes/inner_classes/visibility_006.phpt (3)
1-3: Clear test description for visibility boundariesThe test title accurately describes that it's testing scope boundaries don't bypass visibility restrictions.
6-20: Proper test of private method visibility in nested classesThe nested class structure correctly sets up a test case with private methods and attempts to access them from an inner class, which should fail due to visibility restrictions.
21-28: Appropriate error verificationThe test correctly expects a fatal error when trying to access the private method, confirming that inner classes cannot access private methods of outer classes despite being nested within them. This is important for maintaining proper encapsulation.
tests/classes/inner_classes/visibility_005.phpt (1)
1-3: Test name is comprehensive and clear.
The description effectively articulates that we are verifying external access to private methods across multiple nested classes.Zend/zend_execute.h (1)
108-108: Ensure all calls handle the newly introduced parameter.
The function signature now includeszend_type_node *type_tree. Please confirm that all internal invocations ofzend_check_user_type_slow()pass in a propertype_treevalue or null as needed.Would you like a script to search for all calls of
zend_check_user_type_slowto confirm usage?Zend/zend_object_handlers.c (2)
384-409: Potential risk of repeated scope fallback.
The labelcheck_lexical_scopeand thegotostructure repeatedly check forscope->lexical_scope. This could risk looping ifscope->lexical_scopeeventually refers back to the same scope. Consider adding a safeguard (e.g., checking against an already visited set of scopes).
1826-1827: Storing the original scope is a good practice.
Capturingoriginal_scopeprotects the prior context for error reporting. Be cautious about inadvertently omitting it if future refactors change how nested scopes are chained.tests/classes/inner_classes/visibility_002.phpt (1)
1-31: Well-designed test for private member access from inner classesThis test effectively validates that inner classes can access and modify private members of their outer class, which is an important aspect of encapsulation with nested classes. The design demonstrates both instantiation of a private inner class from within the outer class and the ability of the inner class to modify private properties of the outer class.
ext/reflection/php_reflection.c (4)
4079-4087: Well-implemented method for checking private class visibilityThis method correctly determines if a class is private by checking if it has a required scope and if the required_scope_absolute flag is set. The implementation follows the standard pattern for reflection methods and properly handles the visibility logic.
4090-4099: Good implementation of protected class visibility checkThe isProtected method correctly determines a class's protected status by checking if there's a required scope but the required_scope_absolute flag is not set. This complements the isPrivate method nicely and maintains consistent logic.
4102-4111: Clean implementation of public class visibility checkThis method provides a simple and effective way to determine if a class is public by checking if the required_scope property is not set. The code is well-formatted and follows the established pattern for ReflectionClass methods.
4114-4125: Correctly implemented inner class detection methodThe isInnerClass method properly determines if a class is an inner class by checking if it has a lexical scope and ensuring that scope is not of type ZEND_NAMESPACE_CLASS. This will help distinguish between classes defined within other classes versus those defined in namespaces.
Zend/zend_vm_execute.h (3)
21558-21575: Validate error messages align with future scope-handling features.The code logic here matches the previous hunk, but confirm that these type errors and their language remain accurate for planned scope or visibility expansions.
30052-30069: Maintain consistent handling in repeated checks.The same scope-check logic is duplicated here. Ensure any future updates to public/protected return restrictions are consistently applied across all opcode handlers.
50679-50696: Ensure robust runtime tests.Given the complexity of these return type scope checks, confirm test coverage includes edge cases (e.g., returning objects from private methods, final methods, trait scenarios).
Zend/zend_compile.h (4)
483-483: Type tree field consistently added to all type-related structures.The
type_treefield has been added tozend_internal_arg_info,zend_arg_info, andzend_internal_function_infostructures, maintaining consistency across all type-related structures in the Zend Engine. This unified approach will help manage complex type information consistently across different parts of the engine.Also applies to: 491-491, 503-503
901-901:✅ Verification successful
Support for nested class modifiers added.
The addition of
ZEND_MODIFIER_TARGET_NESTED_CLASSto thezend_modifier_targetenum enables the application of modifiers (like public, private, protected) to nested class declarations. This is a key component for implementing nested class support.
🏁 Script executed:
#!/bin/bash # Look for usage of the new modifier target grep -r "ZEND_MODIFIER_TARGET_NESTED_CLASS" --include="*.c" ./Length of output: 610
Verified Change: The addition of
ZEND_MODIFIER_TARGET_NESTED_CLASSto the enum inZend/zend_compile.his being correctly utilized. Multiple conditionals inZend/zend_compile.cconfirm its integration for supporting nested class modifiers. No further changes are required.
455-455:✅ Verification successful
New type_tree field added to zend_property_info struct.
The addition of
zend_type_node *type_treeto thezend_property_infostruct enhances type handling capabilities, presumably for supporting more complex type hierarchies needed for nested classes.
🏁 Script executed:
#!/bin/bash # Verify zend_type_node structure definition grep -r "typedef struct.*zend_type_node" ./Length of output: 112
Verification Successful: Confirmed Type Definition
The newly added fieldzend_type_node *type_treeis valid since thezend_type_nodetype is properly defined inZend/zend_types.h(i.e.,typedef struct _zend_type_node zend_type_node;). This addition appropriately enhances the type handling capabilities forzend_property_info, supporting more complex type hierarchies.
1068-1068:✅ Verification successful
New class type constant for namespace classes.
The
ZEND_NAMESPACE_CLASSconstant with value3introduces a new class type alongside the existingZEND_INTERNAL_CLASS (1)andZEND_USER_CLASS (2). This constant will be used to identify and manage classes within namespaces differently from other class types.
🏁 Script executed:
#!/bin/bash # Find where ZEND_NAMESPACE_CLASS is used grep -r "ZEND_NAMESPACE_CLASS" --include="*.c" ./ # Check for code that handles different class types grep -r "switch.*ZEND_.*_CLASS" --include="*.c" ./Length of output: 1436
Verified: The new constant is correctly integrated.
TheZEND_NAMESPACE_CLASSconstant (with value3) is consistently being used throughout the codebase to handle namespace-specific class logic. Verification confirms its usage in multiple modules (e.g.,Zend/zend_compile.c,Zend/zend_namespaces.c,Zend/zend_object_handlers.c, andZend/zend_opcode.c), ensuring namespace classes are treated distinctly fromZEND_INTERNAL_CLASS (1)andZEND_USER_CLASS (2).
- Usage Verification:
- In
Zend/zend_compile.c, the constant is used for scope checks.- In
Zend/zend_namespaces.c, it is assigned to namespace types.- In
Zend/zend_object_handlers.candZend/zend_opcode.c, conditionals correctly incorporate the new constant.No modifications are required based on the review.
Zend/zend_API.h (2)
131-131: Consistent update to argument information macros to add a new NULL parameter.All argument information macros have been updated to add a fourth NULL parameter. This is a consistent change across 19 different macros that define argument information structures. This change is part of enhancing the type system within the Zend Engine to support more complex type representations.
Also applies to: 133-133, 135-135, 139-139, 141-141, 143-143, 147-147, 149-149, 151-151, 155-155, 157-157, 159-159, 163-163, 165-165, 170-170, 183-183, 193-193, 203-203, 216-216
451-452: New function added for type tree conversion.A new function
zend_type_to_interned_treehas been added, which converts azend_typeto an interned tree representation. This function is part of the implementation of the enhanced type system that supports complex type representations, including unions and intersections.tests/classes/inner_classes/visibility_010.phpt (4)
6-14: User class implements properties with private setters and a private constructor.The
Userclass demonstrates the newprivate(set)visibility modifier which makes properties publicly readable but only privately writable. This promotes immutability while allowing access to property values. The private constructor ensures that instances can only be created via the Builder pattern.
15-29: Inner Builder class implementation with fluent interface.The nested
Builderclass is marked aspublic readonly final, demonstrating multiple modifiers for an inner class. It implements the Builder pattern with a fluent interface, using immutable instances that return new objects on each modification. The constructor uses property promotion with nullable types and default values, along with theprivate(set)visibility modifier.
32-33: Builder pattern usage with namespace-like syntax.This code demonstrates how to use the inner class through a namespace-like syntax (
User\Builder). It creates a new builder, sets properties using the fluent interface, and builds the final immutableUserobject. This pattern is common in other languages and now available in PHP.
1-42: Well-structured test for inner classes and property visibility modifiers.This test demonstrates several new PHP features:
- Inner/nested classes
- Advanced property visibility modifiers (
private(set))- Multiple class modifiers (
public readonly final)- Builder pattern implementation using inner classes
- Accessing inner classes using namespace-like syntax
The test has clear expectations and demonstrates an elegant implementation of the builder pattern using new language features.
ext/opcache/jit/zend_jit_helpers.c (2)
1905-1906: Validate thatarg_info->type_treeis consistently initialized.This new parameter is passed to
zend_check_user_type_slow, but there's no explicit check for null references here. Confirm thatarg_info->type_treeis always valid (or gracefully handled) in all calling contexts to avoid unexpected behavior.
1922-1923: Verify the consistency for return type checks witharg_info->type_tree.Similar to argument checks, ensure that
arg_info->type_treewon't be null or incorrectly assigned for return type verification. If there's any code path that omits this field, it could trigger undefined behavior in type checks.Zend/zend_namespaces.c (3)
20-33: Consider usingemallocif the namespace entry need not persist.Currently,
pemalloc(sizeof(zend_class_entry), 0)may be acceptable if we’re not caching the allocated memory beyond a request boundary; otherwise, usingpemallocwith the second parameter set to 1 oremallocmight be more appropriate. Double-check the intended lifecycle of these namespace entries to prevent memory mismanagement.
34-63: Confirm parsing logic for nested namespaces.The function builds a chain of namespaces by repeatedly creating or fetching a namespace for the substring from the start up to each delimiter. This results in a nested structure but includes the entire path (e.g.,
Foo\\Bar→Foo, thenFoo\\Bar). If the intention is to create separate incremental namespaces (e.g.,Foo,Foo\Bar), this is fine. Otherwise, verify whether slicing the string from the last backslash is necessary.
98-105: Ensure all references toEG(namespaces)are nullified on shutdown.This destruction code sets both
EG(namespaces)andEG(global_namespace)to NULL, which is excellent for preventing use-after-free. Confirm no other global references remain elsewhere in the engine to these namespace structures after shutdown.
| zend_class_entry *original_scope = scope; | ||
|
|
||
| check_lexical_scope: |
There was a problem hiding this comment.
Same fallback label approach used here.
Again, ensure that lexical scope chaining cannot form a cycle. If cyclical references arise, it may cause infinite recursion or a jump loop.
Consider a detection mechanism that breaks if scope->lexical_scope repeats a previously visited pointer.
| if (class_type->required_scope) { | ||
| const zend_class_entry *scope = zend_get_executed_scope(); | ||
| if (UNEXPECTED(scope == NULL)) { | ||
| zend_type_error("Cannot instantiate class %s from the global scope", ZSTR_VAL(class_type->name)); | ||
| ZVAL_NULL(arg); | ||
| Z_OBJ_P(arg) = NULL; | ||
| return FAILURE; | ||
| } | ||
|
|
||
| if (class_type->required_scope_absolute) { | ||
| if (scope != class_type->required_scope && scope->lexical_scope != class_type->required_scope) { | ||
| zend_type_error("Cannot instantiate private class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); | ||
| ZVAL_NULL(arg); | ||
| Z_OBJ_P(arg) = NULL; | ||
| return FAILURE; | ||
| } | ||
| } else if (!instanceof_function(scope, class_type->required_scope) && !instanceof_function(scope->lexical_scope, class_type->required_scope)) { | ||
| zend_type_error("Cannot instantiate protected class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Confirm consistent error handling for protected classes
In the “protected class” branch (line 1836), a zend_type_error() is thrown, but the function does not explicitly return or set the arg to NULL. While the thrown exception will likely terminate the current process of instantiation, consider adding a return statement (and clearing arg similarly to the “private class” branch) to keep logic consistent and avoid partial initialization if an exception is overlooked.
} else if (!instanceof_function(scope, class_type->required_scope) && !instanceof_function(scope->lexical_scope, class_type->required_scope)) {
zend_type_error("Cannot instantiate protected class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name));
+ ZVAL_NULL(arg);
+ Z_OBJ_P(arg) = NULL;
+ return FAILURE;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (class_type->required_scope) { | |
| const zend_class_entry *scope = zend_get_executed_scope(); | |
| if (UNEXPECTED(scope == NULL)) { | |
| zend_type_error("Cannot instantiate class %s from the global scope", ZSTR_VAL(class_type->name)); | |
| ZVAL_NULL(arg); | |
| Z_OBJ_P(arg) = NULL; | |
| return FAILURE; | |
| } | |
| if (class_type->required_scope_absolute) { | |
| if (scope != class_type->required_scope && scope->lexical_scope != class_type->required_scope) { | |
| zend_type_error("Cannot instantiate private class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); | |
| ZVAL_NULL(arg); | |
| Z_OBJ_P(arg) = NULL; | |
| return FAILURE; | |
| } | |
| } else if (!instanceof_function(scope, class_type->required_scope) && !instanceof_function(scope->lexical_scope, class_type->required_scope)) { | |
| zend_type_error("Cannot instantiate protected class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); | |
| } | |
| } | |
| if (class_type->required_scope) { | |
| const zend_class_entry *scope = zend_get_executed_scope(); | |
| if (UNEXPECTED(scope == NULL)) { | |
| zend_type_error("Cannot instantiate class %s from the global scope", ZSTR_VAL(class_type->name)); | |
| ZVAL_NULL(arg); | |
| Z_OBJ_P(arg) = NULL; | |
| return FAILURE; | |
| } | |
| if (class_type->required_scope_absolute) { | |
| if (scope != class_type->required_scope && scope->lexical_scope != class_type->required_scope) { | |
| zend_type_error("Cannot instantiate private class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); | |
| ZVAL_NULL(arg); | |
| Z_OBJ_P(arg) = NULL; | |
| return FAILURE; | |
| } | |
| } else if (!instanceof_function(scope, class_type->required_scope) && !instanceof_function(scope->lexical_scope, class_type->required_scope)) { | |
| zend_type_error("Cannot instantiate protected class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); | |
| ZVAL_NULL(arg); | |
| Z_OBJ_P(arg) = NULL; | |
| return FAILURE; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_execute.c(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_execute.c (2)
Zend/zend_compile.h (5)
zend_get_unmangled_property_name(971-971)zend_visibility_string(436-436)bool(1137-1147)type(934-934)type(1012-1012)Zend/zend_execute.h (2)
zend_verify_property_type_error(528-528)zend_verify_scalar_type_hint(99-99)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
🔇 Additional comments (7)
Zend/zend_execute.c (7)
1048-1078: New visibility checker for class types with clear error messages.This function checks if a class type is visible in the current context, handling public, protected, and private classes appropriately. The implementation correctly enforces visibility rules:
- Public classes are always visible
- Protected classes are visible in subclasses with protected/private visibility
- Private classes are only visible in the same class with private visibility
The error messages are descriptive and clearly indicate why a visibility constraint was violated.
1081-1086: Added type visibility check for property assignment.This is a critical addition that enforces class type visibility rules when assigning objects to properties. The code now properly verifies that the object's class respects the required visibility constraints before accepting the assignment.
1283-1300: Updated main type checking function with type tree support.This code correctly updates the
zend_check_typefunction to include the new type tree parameter, preserving all the existing type checking functionality while adding support for the enhanced type system.
1317-1320: Updated argument type verification with type tree support.The function correctly passes the new type tree parameter to
zend_check_typewhen verifying argument types.
1329-1330: Type tree support in variadic argument validation.The code now properly passes the type tree when checking variadic arguments, ensuring consistent type checking throughout the engine.
1354-1354: Added type tree support to internal argument validation.The internal argument validation now includes the type tree parameter for complete type checking.
1560-1560: Type tree in internal return type verification.This update ensures consistent type checking for internal function return values.
Zend/zend_execute.c
Outdated
| static int zend_type_node_matches(const zend_type_node *node, zval *zv) | ||
| { | ||
| switch (node->kind) { | ||
| case ZEND_TYPE_SIMPLE: { | ||
| return 2; | ||
| } | ||
|
|
||
| case ZEND_TYPE_UNION: { | ||
| for (uint32_t i = 0; i < node->compound.num_types; i++) { | ||
| if (zend_type_node_matches(node->compound.types[i], zv)) { | ||
| return 1; | ||
| } | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| case ZEND_TYPE_INTERSECTION: { | ||
| for (uint32_t i = 0; i < node->compound.num_types; i++) { | ||
| if (!zend_type_node_matches(node->compound.types[i], zv)) { | ||
| return 0; | ||
| } | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| default: | ||
| return 0; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
New type node matching function for enhanced type checking.
This implementation supports checking type nodes against values, handling simple types, unions, and intersections. However, the ZEND_TYPE_SIMPLE case seems incomplete as it returns 2 without actually validating the type match.
For simple types, we should verify the value actually matches the type:
case ZEND_TYPE_SIMPLE: {
- return 2;
+ // Verify the value actually matches the simple type
+ // Return 2 if it's a direct match, 1 if it can be coerced, 0 if not compatible
+ // (Implementation details will depend on node->simple_type and Z_TYPE_P(zv))
+ // ...
}Committable suggestion skipped: line range outside the PR's diff.
| static zend_always_inline bool zend_check_type_slow( | ||
| zend_type *type, zval *arg, zend_reference *ref, void **cache_slot, | ||
| zend_type *type, zend_type_node *type_tree, zval *arg, zend_reference *ref, void **cache_slot, | ||
| bool is_return_type, bool is_internal) | ||
| { | ||
| uint32_t type_mask; | ||
| if (EXPECTED(type_tree != NULL) && type_tree->kind != ZEND_TYPE_SIMPLE) { | ||
| const int result = zend_type_node_matches(type_tree, arg); | ||
| if (result < 2) { | ||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Updated type checking to support the new type tree structure.
The modified implementation now handles the type tree parameter, but there's a potential issue with the interpretation of the return value from zend_type_node_matches. When the result is 2 (indicating a simple type match), the function proceeds with additional checks rather than returning early.
Consider handling the return value more explicitly:
if (EXPECTED(type_tree != NULL) && type_tree->kind != ZEND_TYPE_SIMPLE) {
const int result = zend_type_node_matches(type_tree, arg);
if (result < 2) {
return result;
}
+ // If result is exactly 2, it's a direct match with a simple type
+ if (result == 2) {
+ return 1; // Indicate success
+ }
// Otherwise continue with additional checks
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static zend_always_inline bool zend_check_type_slow( | |
| zend_type *type, zval *arg, zend_reference *ref, void **cache_slot, | |
| zend_type *type, zend_type_node *type_tree, zval *arg, zend_reference *ref, void **cache_slot, | |
| bool is_return_type, bool is_internal) | |
| { | |
| uint32_t type_mask; | |
| if (EXPECTED(type_tree != NULL) && type_tree->kind != ZEND_TYPE_SIMPLE) { | |
| const int result = zend_type_node_matches(type_tree, arg); | |
| if (result < 2) { | |
| return result; | |
| } | |
| } | |
| static zend_always_inline bool zend_check_type_slow( | |
| zend_type *type, zend_type_node *type_tree, zval *arg, zend_reference *ref, void **cache_slot, | |
| bool is_return_type, bool is_internal) | |
| { | |
| if (EXPECTED(type_tree != NULL) && type_tree->kind != ZEND_TYPE_SIMPLE) { | |
| const int result = zend_type_node_matches(type_tree, arg); | |
| if (result < 2) { | |
| return result; | |
| } | |
| if (result == 2) { | |
| return 1; // Indicate success | |
| } | |
| // Otherwise continue with additional checks | |
| } | |
| // ... rest of the function implementation ... | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Zend/zend_API.c (1)
1819-1838:⚠️ Potential issueMissing return statement for protected class error handling
In the protected class scope check (line 1836), after throwing the type error, there's no return statement or clearing of the
argvalue. This is inconsistent with the private class handling in lines 1830-1834, which setsargto NULL and returns FAILURE after the error.Add proper cleanup and return for the protected class case:
} else if (!instanceof_function(scope, class_type->required_scope) && !instanceof_function(scope->lexical_scope, class_type->required_scope)) { zend_type_error("Cannot instantiate protected class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); + ZVAL_NULL(arg); + Z_OBJ_P(arg) = NULL; + return FAILURE; }
🧹 Nitpick comments (1)
Zend/zend_API.c (1)
2965-3021: Potential performance optimization for hash computationThe hash function implementation for type nodes is thorough, but in the simple type case, it might be more efficient to combine the type mask and nullable flag into a single hash operation to reduce the number of hash mixing steps.
Consider optimizing the hash function in the simple type case:
case ZEND_TYPE_SIMPLE: { const zend_type type = node->simple_type; - hash ^= (zend_ulong)ZEND_TYPE_FULL_MASK(type); - hash *= 16777619; + // Combine type mask and nullable flag in a single step + hash ^= ((zend_ulong)ZEND_TYPE_FULL_MASK(type) << 1) | (ZEND_TYPE_ALLOW_NULL(type) ? 1 : 0); + hash *= 16777619; if (ZEND_TYPE_HAS_NAME(type)) { zend_string *name = ZEND_TYPE_NAME(type); hash ^= zend_string_hash_val(name); hash *= 16777619; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Zend/zend_API.c(5 hunks)ext/zend_test/test.c(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (6)
ext/zend_test/test.c (1)
1204-1205: Structure field addition aligned with type definition changes.The update adds a fourth
NULLfield to thezend_internal_function_infostructure in the arg_info array, consistent with structural changes to the Zend Engine. This adaptation ensures compatibility with the updated type system implementation.Zend/zend_API.c (5)
2922-2964: LGTM: Type tree implementation looks well-structuredThe implementation of the interned type tree with hash table and comparison functions provides a robust foundation for type management. The type node comparison logic handles different type cases appropriately.
3022-3064: LGTM: Type node comparison implementationThe equality check for type nodes is correctly implemented, with appropriate handling of different node kinds and proper comparison of all relevant fields.
3136-3162: LGTM: Updated zend_normalize_internal_type functionThe function has been correctly modified to return a type node instead of void, returning the result of
zend_type_to_interned_tree(). This properly integrates with the new type tree system.
4906-4909: LGTM: Property info type tree integrationThe property info structure correctly uses the new type tree functionality based on whether it's for an internal or user class.
3431-3431: LGTM: Function argument type tree integrationThe function registration process correctly uses the type tree by adding a call to
zend_normalize_internal_type()to generate and store the type tree for function arguments.
|
|
||
| static zend_type_node *intern_type_node(zend_type_node *node) { | ||
| const zend_ulong hash = zend_type_node_hash(node); | ||
| zend_type_node *existing; | ||
|
|
||
| if (interned_type_tree == NULL) { | ||
| interned_type_tree = pemalloc(sizeof(HashTable), 1); | ||
| zend_hash_init(interned_type_tree, 64, NULL, NULL, 1); | ||
| } | ||
|
|
||
| if ((existing = zend_hash_index_find_ptr(interned_type_tree, hash))) { | ||
| if (zend_type_node_equals(existing, node)) { | ||
| pefree(node, 1); | ||
| return existing; // reuse interned node | ||
| } | ||
| } | ||
|
|
||
| zend_hash_index_add_new_ptr(interned_type_tree, hash, node); | ||
| return node; | ||
| } | ||
|
|
||
| ZEND_API zend_type_node *zend_type_to_interned_tree(const zend_type type) { | ||
| if (type.type_mask == 0) { | ||
| return NULL; | ||
| } | ||
|
|
||
| if (!ZEND_TYPE_HAS_LIST(type)) { | ||
| zend_type_node *node = pemalloc(sizeof(zend_type_node), 1); | ||
| node->kind = ZEND_TYPE_SIMPLE; | ||
| node->simple_type = type; | ||
| return intern_type_node(node); | ||
| } | ||
|
|
||
| zend_type_list *list = ZEND_TYPE_LIST(type); | ||
| const zend_type_node_kind kind = ZEND_TYPE_IS_INTERSECTION(type) ? | ||
| ZEND_TYPE_INTERSECTION : ZEND_TYPE_UNION; | ||
|
|
||
| zend_type_node **children = NULL; | ||
| uint32_t num_children = 0; | ||
|
|
||
| zend_type *subtype; | ||
|
|
||
| ZEND_TYPE_LIST_FOREACH(list, subtype) { | ||
| zend_type_node *child = zend_type_to_interned_tree(*subtype); | ||
|
|
||
| if (child->kind == kind) { | ||
| for (uint32_t i = 0; child->compound.num_types; i++) { | ||
| ADD_TO_TREE(children, num_children, child->compound.types[i]); | ||
| } | ||
| } else { | ||
| ADD_TO_TREE(children, num_children, child); | ||
| } | ||
| } ZEND_TYPE_LIST_FOREACH_END(); | ||
|
|
||
| qsort(children, num_children, sizeof(zend_type_node*), compare_type_nodes); | ||
|
|
||
| size_t deduped_count = 0; | ||
| for (size_t i = 0; i < num_children; i++) { | ||
| if (i == 0 || compare_type_nodes(&children[i], &children[i - 1]) != 0) { | ||
| children[deduped_count++] = children[i]; | ||
| } | ||
| } | ||
|
|
||
| zend_type_node *node = pemalloc(sizeof(zend_type_node), 1); | ||
| node->kind = kind; | ||
| node->compound.num_types = deduped_count; | ||
| node->compound.types = pemalloc(sizeof(zend_type_node *) * deduped_count, 1); | ||
| memcpy(node->compound.types, children, sizeof(zend_type_node *) * deduped_count); | ||
|
|
||
| return intern_type_node(node); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for potential memory allocation failures
The code for interning type nodes allocates memory but doesn't include checks for allocation failures, which could lead to crashes in low-memory situations.
Add error checking for memory allocations:
zend_type_node *node = pemalloc(sizeof(zend_type_node), 1);
+ if (!node) {
+ return NULL; // Handle memory allocation failure
+ }
node->kind = kind;
node->compound.num_types = deduped_count;
node->compound.types = pemalloc(sizeof(zend_type_node *) * deduped_count, 1);
+ if (!node->compound.types) {
+ pefree(node, 1);
+ return NULL; // Handle memory allocation failure
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static zend_type_node *intern_type_node(zend_type_node *node) { | |
| const zend_ulong hash = zend_type_node_hash(node); | |
| zend_type_node *existing; | |
| if (interned_type_tree == NULL) { | |
| interned_type_tree = pemalloc(sizeof(HashTable), 1); | |
| zend_hash_init(interned_type_tree, 64, NULL, NULL, 1); | |
| } | |
| if ((existing = zend_hash_index_find_ptr(interned_type_tree, hash))) { | |
| if (zend_type_node_equals(existing, node)) { | |
| pefree(node, 1); | |
| return existing; // reuse interned node | |
| } | |
| } | |
| zend_hash_index_add_new_ptr(interned_type_tree, hash, node); | |
| return node; | |
| } | |
| ZEND_API zend_type_node *zend_type_to_interned_tree(const zend_type type) { | |
| if (type.type_mask == 0) { | |
| return NULL; | |
| } | |
| if (!ZEND_TYPE_HAS_LIST(type)) { | |
| zend_type_node *node = pemalloc(sizeof(zend_type_node), 1); | |
| node->kind = ZEND_TYPE_SIMPLE; | |
| node->simple_type = type; | |
| return intern_type_node(node); | |
| } | |
| zend_type_list *list = ZEND_TYPE_LIST(type); | |
| const zend_type_node_kind kind = ZEND_TYPE_IS_INTERSECTION(type) ? | |
| ZEND_TYPE_INTERSECTION : ZEND_TYPE_UNION; | |
| zend_type_node **children = NULL; | |
| uint32_t num_children = 0; | |
| zend_type *subtype; | |
| ZEND_TYPE_LIST_FOREACH(list, subtype) { | |
| zend_type_node *child = zend_type_to_interned_tree(*subtype); | |
| if (child->kind == kind) { | |
| for (uint32_t i = 0; child->compound.num_types; i++) { | |
| ADD_TO_TREE(children, num_children, child->compound.types[i]); | |
| } | |
| } else { | |
| ADD_TO_TREE(children, num_children, child); | |
| } | |
| } ZEND_TYPE_LIST_FOREACH_END(); | |
| qsort(children, num_children, sizeof(zend_type_node*), compare_type_nodes); | |
| size_t deduped_count = 0; | |
| for (size_t i = 0; i < num_children; i++) { | |
| if (i == 0 || compare_type_nodes(&children[i], &children[i - 1]) != 0) { | |
| children[deduped_count++] = children[i]; | |
| } | |
| } | |
| zend_type_node *node = pemalloc(sizeof(zend_type_node), 1); | |
| node->kind = kind; | |
| node->compound.num_types = deduped_count; | |
| node->compound.types = pemalloc(sizeof(zend_type_node *) * deduped_count, 1); | |
| memcpy(node->compound.types, children, sizeof(zend_type_node *) * deduped_count); | |
| return intern_type_node(node); | |
| } | |
| ZEND_API zend_type_node *zend_type_to_interned_tree(const zend_type type) { | |
| if (type.type_mask == 0) { | |
| return NULL; | |
| } | |
| if (!ZEND_TYPE_HAS_LIST(type)) { | |
| zend_type_node *node = pemalloc(sizeof(zend_type_node), 1); | |
| node->kind = ZEND_TYPE_SIMPLE; | |
| node->simple_type = type; | |
| return intern_type_node(node); | |
| } | |
| zend_type_list *list = ZEND_TYPE_LIST(type); | |
| const zend_type_node_kind kind = ZEND_TYPE_IS_INTERSECTION(type) ? | |
| ZEND_TYPE_INTERSECTION : ZEND_TYPE_UNION; | |
| zend_type_node **children = NULL; | |
| uint32_t num_children = 0; | |
| zend_type *subtype; | |
| ZEND_TYPE_LIST_FOREACH(list, subtype) { | |
| zend_type_node *child = zend_type_to_interned_tree(*subtype); | |
| if (child->kind == kind) { | |
| for (uint32_t i = 0; child->compound.num_types; i++) { | |
| ADD_TO_TREE(children, num_children, child->compound.types[i]); | |
| } | |
| } else { | |
| ADD_TO_TREE(children, num_children, child); | |
| } | |
| } ZEND_TYPE_LIST_FOREACH_END(); | |
| qsort(children, num_children, sizeof(zend_type_node*), compare_type_nodes); | |
| size_t deduped_count = 0; | |
| for (size_t i = 0; i < num_children; i++) { | |
| if (i == 0 || compare_type_nodes(&children[i], &children[i - 1]) != 0) { | |
| children[deduped_count++] = children[i]; | |
| } | |
| } | |
| zend_type_node *node = pemalloc(sizeof(zend_type_node), 1); | |
| + if (!node) { | |
| + return NULL; // Handle memory allocation failure | |
| + } | |
| node->kind = kind; | |
| node->compound.num_types = deduped_count; | |
| node->compound.types = pemalloc(sizeof(zend_type_node *) * deduped_count, 1); | |
| + if (!node->compound.types) { | |
| + pefree(node, 1); | |
| + return NULL; // Handle memory allocation failure | |
| + } | |
| memcpy(node->compound.types, children, sizeof(zend_type_node *) * deduped_count); | |
| return intern_type_node(node); | |
| } |
```
ext/gd/libgd/gd.c:2275:14: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
#0 0x5d6a2103e1db in php_gd_gdImageCopy /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd.c:2275
#1 0x5d6a210a2b63 in gdImageCrop /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd_crop.c:57
#2 0x5d6a21018ca4 in zif_imagecrop /home/dcarlier/Contribs/php-src/ext/gd/gd.c:3575
#3 0x5d6a21e46e7a in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1337
#4 0x5d6a221188da in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:57246
#5 0x5d6a221366bd in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:61634
#6 0x5d6a21d107a6 in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1895
#7 0x5d6a21a63409 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2529
#8 0x5d6a22516d5e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:966
php#9 0x5d6a2251981d in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1341
php#10 0x7f10d002a3b7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
php#11 0x7f10d002a47a in __libc_start_main_impl ../csu/libc-start.c:360
php#12 0x5d6a20a06da4 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2806da4) (BuildId: d9a79c7e0e4872311439d7313cb3a81fe04190a2)
```
close phpGH-18006
Summary by CodeRabbit
New Features
ReflectionClassfor checking class visibility (isPrivate, isProtected, isPublic, isInnerClass).Bug Fixes
Tests