Rfc: nested classes #5
Conversation
WalkthroughThis pull request introduces broad changes to support nested and inner classes in the Zend Engine. New members are added to the class entry structures to manage required and lexical scopes. The changes update enumeration and grammar definitions within the parser and compile phases to recognize nested classes and adjust modifiers. Namespace management is enhanced through new source files and build configuration updates. Additionally, extensive new tests cover inner classes, access modifiers, return types, autoloading, instantiation, resolution, enums, and reflection, while several functions and error handling paths are updated for improved type and visibility checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Instantiator as _object_and_properties_init
participant ScopeChecker as zend_check_type_visibility
participant ClassEntry
Caller->>Instantiator: Request instance creation
Instantiator->>ClassEntry: Check required_scope/lexical_scope
alt Scope not satisfied
ClassEntry-->>Instantiator: Return type error message
Instantiator-->>Caller: Error thrown
else Scope valid
Instantiator->>ScopeChecker: Verify property and return type visibility
ScopeChecker-->>Instantiator: Visibility OK
Instantiator-->>Caller: Return instance
end
sequenceDiagram
participant Parser
participant NamespaceManager
participant GlobalTable
Parser->>NamespaceManager: Call zend_resolve_namespace(name)
NamespaceManager->>GlobalTable: Lookup namespace entry
alt Namespace exists
GlobalTable-->>NamespaceManager: Return existing namespace
else Namespace missing
NamespaceManager->>GlobalTable: Create and insert new namespace
end
NamespaceManager-->>Parser: Return namespace entry
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: 0
🧹 Nitpick comments (1)
Zend/zend.h (1)
167-170: Appropriate structure additions to support nested classes.These three new fields add the necessary infrastructure to implement inner classes:
required_scope: Points to the parent/outer class that contains this classlexical_scope: Handles the lexical scope for proper name resolutionrequired_scope_absolute: Determines whether scope references are absoluteConsider adding comments to explain the purpose of these fields, particularly what
required_scope_absolutecontrols, to improve code maintainability for future developers.+ /* Class scope information for nested classes */ zend_class_entry *required_scope; zend_class_entry *lexical_scope; + /* Whether the required scope is an absolute reference */ bool required_scope_absolute;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Zend/zend.h(1 hunks)Zend/zend_compile.h(1 hunks)Zend/zend_language_parser.y(4 hunks)tests/classes/inner_classes/simple_declaration.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 (7)
tests/classes/inner_classes/simple_declaration.phpt (1)
1-22: Well-structured test for nested class declarations.This test correctly validates the basic functionality of nested class declarations by checking both string-based class name lookups and the
::classconstant syntax. It confirms that PHP properly recognizes the nested class hierarchy using namespace-like syntax.A few considerations for future enhancements:
- Adding tests for accessing nested classes from within their parent classes
- Testing visibility modifiers (public, private, protected) on nested classes
- Verifying inheritance and interfaces with nested classes
Zend/zend_compile.h (1)
897-897: Correct extension of modifier targets for inner classes.Adding
ZEND_MODIFIER_TARGET_INNER_CLASSto the enum is a logical extension that enables specifying modifiers for inner classes. The placement at the end of the enumeration maintains backward compatibility with existing code.This change integrates well with the other modifications to support the nested classes feature.
Zend/zend_language_parser.y (5)
288-288: Verify that addinginner_class_statementas a grammar type does not introduce new parser conflicts.
This new type definition looks appropriate, but please re-run the parser generator to confirm that no shift/reduce or reduce/reduce conflicts were introduced and that%expect 0remains valid.Would you like a script to automate checking for bison parser conflicts?
291-291:inner_class_modifiersdeclaration is consistent with existing patterns.
Defininginner_class_modifiersas<num>alongside other modifiers is coherent and follows the established approach in this grammar file.
631-638: Confirm defaulting toZEND_ACC_PUBLICfor inner classes.
AssigningZEND_ACC_PUBLICwhen no explicit modifiers are provided might be intentional, but please verify it aligns with the intended visibility rules for nested classes. Additionally, confirm thatzend_modifier_list_to_flags()handles invalid or conflicting modifiers (e.g.,abstract|final) for inner classes.
954-957: Implementation ofinner_class_statementappears consistent.
The new rule parallels top-level class syntax and attaches the same AST structure (ZEND_AST_CLASS). Ensure that downstream code correctly interprets these AST nodes as nested classes during compilation and runtime.
977-977: Attaching modifiers to an inner class.
This snippet correctly applies the gatheredinner_class_modifiersto the newly createdinner_class_statement. The approach mirrors how other class modifiers are fed into the AST.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Zend/zend_compile.c (1)
947-948: Align naming convention for “nested class.”“ZEND_MODIFIER_TARGET_INNER_CLASS” and “nested class” seem inconsistent. Consider using the same terminology to reduce confusion:
} else if (target == ZEND_MODIFIER_TARGET_INNER_CLASS) { - member = "nested class"; + member = "inner class"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_compile.c(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
🔇 Additional comments (2)
Zend/zend_compile.c (2)
1056-1086:❓ Verification inconclusive
Verify that restricted modifiers for inner classes are covered by tests.
The code disallows multiple access modifiers, static, and explicit public/protected/private sets for inner classes. This is fine if intentional, but verify that these new error paths have corresponding negative tests to ensure consistent enforcement.
🏁 Script executed:
#!/bin/bash # Search for test coverage of new error messages regarding invalid inner class modifiers. rg -A 3 "Multiple access type modifiers are not allowed" tests/ rg -A 3 "Static inner classes are not allowed" tests/ rg -A 3 "inner classes are not allowed" tests/Length of output: 170
Action: Verify Negative Test Coverage for Inner Class Modifiers
The grepped search did not return any tests with the expected error messages ("Multiple access type modifiers are not allowed", "Static inner classes are not allowed", and related messages). This suggests that the negative test cases for these new error paths may be missing or not using these exact strings. Please manually verify that:
- Negative tests exist that trigger these error conditions.
- The tests capture the proper error messages when inner class modifiers are misused.
If such tests are absent, adding them would ensure consistent enforcement of these restrictions.
896-913:❓ Verification inconclusive
Confirm support for these modifiers on inner classes is tested.
Allowing
T_READONLY,T_ABSTRACT, andT_FINALforZEND_MODIFIER_TARGET_INNER_CLASSis a significant language change. Ensure there are corresponding tests verifying how the engine handles these modifiers on inner classes. Also confirm this logic aligns with language specs (e.g. whetherreadonlyfor classes is permitted).Please run a search in your PHP test files (PHPT, etc.) to confirm that all relevant modifier combinations are adequately tested:
🏁 Script executed:
#!/bin/bash # Searching for usage of T_READONLY, T_ABSTRACT, and T_FINAL with inner/nested classes rg -A 5 "class.*readonly" tests/ rg -A 5 "class.*abstract" tests/ rg -A 5 "class.*final" tests/Length of output: 1461
Action: Verify that inner class modifiers are adequately tested
The search output shows several PHPT tests involving abstract classes, but it does not clearly reveal tests for inner classes using
T_READONLYorT_FINAL. Since allowingT_READONLY,T_ABSTRACT, andT_FINALforZEND_MODIFIER_TARGET_INNER_CLASSis a significant language change, please ensure that:
- There are dedicated tests confirming the engine’s behavior when inner classes use the
readonlyandfinalmodifiers.- The existing abstract-related tests are either applicable to inner classes or are supplemented with specific tests for inner classes.
- This logic fully complies with language specifications (e.g. verify whether
readonlyis permitted on inner classes).If the existing test suite does not cover these combinations, please add appropriate tests and confirm their correctness manually.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Zend/zend_namespaces.c (1)
47-54: LGTM, but consider handling allocation failuresThe namespace resolution logic looks correct, but there's no error handling for memory allocation failures. Consider adding checks after memory allocation operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Zend/zend_compile.h(2 hunks)Zend/zend_globals.h(1 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_namespaces.h(1 hunks)configure.ac(1 hunks)win32/build/config.w32(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Zend/zend_compile.h
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.h (1)
Zend/zend_compile.h (1)
name(988-988)
⏰ 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: LINUX_X32_DEBUG_ZTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (7)
Zend/zend_namespaces.h (1)
1-28: Looks good - new header file properly structured.The header file is well organized with standard licensing information, appropriate header guards, and includes the necessary dependency. The two API functions are declared correctly with the
ZEND_APImarker, consistent naming conventions, and clear return types.configure.ac (1)
1760-1760: Correctly added new source file to build system.The
zend_namespaces.cfile has been properly added to the Zend module source files list, ensuring it will be compiled as part of the PHP build process.win32/build/config.w32 (1)
243-243: Correctly added new source file to Windows build configuration.The new
zend_namespaces.cfile has been properly added to the sources list for the Windows build, maintaining build consistency across platforms.Zend/zend_globals.h (1)
194-195: Appropriately added namespace support to executor globals.The two new fields added to the
_zend_executor_globalsstructure are well-placed:
HashTable *namespaces- For storing namespace informationzend_class_entry *global_namespace- For referencing the global namespaceThese additions follow the pattern used for other symbol tables (like function_table, class_table) in the executor globals, which is appropriate for the nested classes feature.
Zend/zend_namespaces.c (3)
77-78: Verify case handling in namespace resolutionThe code converts namespace names to lowercase for lookup, which seems appropriate for PHP's case-insensitive namespace handling. Just confirming this aligns with PHP's namespace case sensitivity rules.
89-95: LGTM: The lookup function is properly separated from resolutionGood separation of concerns by having a separate lookup function that doesn't create namespaces. This follows the principle of least surprise.
65-71:❓ Verification inconclusive
Consider adding cleanup for namespaces hash table
The global namespace and hash table are initialized here, but there's no visible cleanup code. Ensure there's a corresponding function that handles proper destruction of this hash table to prevent memory leaks.
🏁 Script executed:
#!/bin/bash # Look for cleanup code for the namespaces hash table rg -A 5 "EG\(namespaces\)" --type cLength of output: 1624
Action: Verify Global Cleanup Routine for Namespaces
The initialization of the global namespaces hash table in
Zend/zend_namespaces.c(lines 65–71) doesn’t include a local cleanup call. However, it appears that the engine may rely on a centralized shutdown (e.g. within the executor globals cleanup routine) to destroy EG(namespaces). Please verify that the executor shutdown sequence (such as in functions likezend_destroy_executor_globalsor its equivalent) properly callszend_hash_destroyon EG(namespaces) so that all elements are freed viaZEND_CLASS_DTOR. If this centralized cleanup isn’t in place, you’ll need to add an explicit destruction routine to prevent memory leaks.
withinboredom
force-pushed
the
rfc/nested-classes
branch
from
a204a04 to
5f0f77d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Zend/zend_namespaces.c (2)
24-25:⚠️ Potential issueFix memory allocation size in create_namespace
There's a memory allocation issue in the
create_namespacefunction. The current code allocates the size of a pointer to azend_class_entry, not the size of the structure itself.-zend_class_entry *ns = pemalloc(sizeof(zend_class_entry *), 0); +zend_class_entry *ns = pemalloc(sizeof(zend_class_entry), 0);
42-44:⚠️ Potential issuePotential off-by-one error in namespace parsing
The loop condition
while (pos <= end)could lead to reading one byte past the end of the string. Sinceendpoints to the null terminator, the condition should bewhile (pos < end).-while (pos <= end) { - if (pos == end || *pos == '\\') { +while (pos < end) { + if (*pos == '\\') {
🧹 Nitpick comments (4)
Zend/zend_compile.c (2)
896-896: Enable new modifiers for nested classes.Allowing
readonly,abstract, andfinalon nested classes appears consistent with the newZEND_MODIFIER_TARGET_NESTED_CLASStarget, but please confirm that these modifiers align with your final language design for inner/nested classes. Historically,readonlywas primarily intended for properties, so ensure that supporting it at the class level is deliberate and properly tested.You might add explicit checks or comments clarifying the exact behavior and expected usage of
readonlywhen applied to nested classes, in order to maintain clarity and avoid confusion.Also applies to: 901-901, 909-910
947-948: Verify naming consistency for “nested class.”Here, the string is set to "nested class" for display or error messages. However, the PR objectives and codebase references sometimes use the term "inner class." Ensure that the final naming convention is consistently applied across the engine and documentation to reduce confusion among developers.
Zend/zend_namespaces.c (2)
20-22: Improve header inclusion order and add missing headersConsider organizing includes in a consistent order (system headers first, then Zend headers) and include any missing headers that might be needed.
#include "zend_namespaces.h" +#include <stdlib.h> +#include "zend.h" #include "zend_API.h" #include "zend_hash.h"
65-71: Add initialization check for EG(namespaces)Before using
zend_hash_init(), you should check ifEG(namespaces)is already initialized to avoid potential double initialization.if (EG(global_namespace) == NULL) { EG(global_namespace) = create_namespace(zend_empty_string); EG(global_namespace)->lexical_scope = NULL; + if (!EG(namespaces)) { zend_hash_init(EG(namespaces), 8, NULL, ZEND_CLASS_DTOR, 0); + } zend_hash_add_ptr(EG(namespaces), zend_empty_string, EG(global_namespace)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Zend/zend_compile.c(3 hunks)Zend/zend_compile.h(2 hunks)Zend/zend_globals.h(1 hunks)Zend/zend_language_parser.y(4 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_namespaces.h(1 hunks)configure.ac(1 hunks)win32/build/config.w32(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- configure.ac
- Zend/zend_namespaces.h
- Zend/zend_compile.h
- Zend/zend_globals.h
- win32/build/config.w32
- Zend/zend_language_parser.y
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (1)
Zend/zend_compile.c (1)
1056-1086:❓ Verification inconclusive
Validate disallowed modifiers for nested classes.
These new checks prohibit any access (PPP) and
staticmodifiers on nested classes, which could be a significant design choice. Confirm that such restrictions match the intended capabilities of nested/inner classes. Also, consider systematically testing error paths (e.g., “static inner class,” “public(set) inner class,” etc.) to verify that these compilation errors are triggered correctly.
🏁 Script executed:
#!/bin/bash # Description: Search for references to static/public/protected/private usage in nested classes # to ensure coverage in tests and confirm errors are triggered as designed. rg -A 3 'class\s+\w+\s+(static|public|protected|private)' tests/Length of output: 66
I'll generate a new shell script to search for the specific compile error messages thrown by the checks (e.g., "Static inner classes are not allowed", "Multiple access type modifiers are not allowed", etc.) within the tests directory to help verify that tests are in place for these error conditions.
🏁 Script executed:
#!/bin/bash # Description: Search for specific compile error messages related to nested classes in tests rg -n "Static inner classes are not allowed" tests/ rg -n "Multiple access type modifiers are not allowed" tests/ rg -n "Public(set) inner classes are not allowed" tests/ rg -n "Protected(set) inner classes are not allowed" tests/ rg -n "Private(set) inner classes are not allowed" tests/Length of output: 298
Attention: Verify Nested Class Modifier Restrictions & Test Coverage
The updated checks in
Zend/zend_compile.ccorrectly enforce restrictions for nested classes by disallowing multiple access type modifiers,static, and variousaccess_setmodifiers. However, our automated searches in the tests directory did not detect any test cases targeting these specific error messages. Please ensure that you:
- Add or verify unit tests that deliberately trigger these compile error paths (e.g., scenarios for "Static inner classes are not allowed," "Multiple access type modifiers are not allowed," etc.).
- Manually confirm that these error conditions occur as expected when the disallowed modifiers are applied on nested classes.
Zend/zend_namespaces.c
Outdated
| zend_string *needle = zend_string_init(ZSTR_VAL(name), len, 0); | ||
|
|
There was a problem hiding this comment.
Incorrect namespace segment extraction
The string initialization is using ZSTR_VAL(name) (start of the full name) instead of start (beginning of the current segment). This results in creating namespace prefixes instead of individual segments.
-zend_string *needle = zend_string_init(ZSTR_VAL(name), len, 0);
+zend_string *needle = zend_string_init(start, len, 0);📝 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.
| zend_string *needle = zend_string_init(ZSTR_VAL(name), len, 0); | |
| zend_string *needle = zend_string_init(start, len, 0); |
withinboredom
force-pushed
the
rfc/nested-classes
branch
from
5f0f77d to
b4354c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/classes/inner_classes/simple_declaration_004.phpt (1)
33-38: Consider making the expected output more robust.The expected output specifies object IDs (#1) which might be brittle if PHP's internal object ID assignment changes. Consider either:
- Using a more flexible comparison that ignores object IDs
- Adding a comment explaining that object IDs might vary in different PHP versions/implementations
This would make the test more resilient to implementation changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Zend/tests/errmsg/errmsg_027.phpt(1 hunks)Zend/zend_namespaces.c(1 hunks)ext/reflection/tests/bug74454.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)
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.c (4)
Zend/zend_namespaces.h (3)
name(25-25)name(26-26)zend_destroy_namespaces(27-27)Zend/zend_compile.h (2)
name(988-988)zend_initialize_class_data(983-983)Zend/zend_execute_API.c (4)
zend_class_entry(714-726)zend_class_entry(1174-1295)zend_class_entry(1298-1301)zend_class_entry(1304-1319)Zend/zend_compile.c (1)
zend_class_entry(1406-1446)
⏰ 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_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: FREEBSD
🔇 Additional comments (13)
Zend/tests/errmsg/errmsg_027.phpt (1)
16-16: Error message clarifiedThe error message has been updated to be more specific, correctly indicating that class declarations are not allowed inside functions rather than using the vaguer "may not be nested" language. This change provides better clarity to developers.
tests/classes/inner_classes/simple_declaration_001.phpt (1)
1-22: Well-structured test for nested class functionalityThis test appropriately validates the basic functionality of nested classes by checking their existence through both string class names and the
::classconstant syntax. The test structure is clean and the assertions are comprehensive.ext/reflection/tests/bug74454.phpt (1)
17-17: Updated error message for consistencyThe change updates the expected parser error message to match the new syntax expectations, changing from "expecting function" to "expecting class". This keeps the test consistent with the updated error messaging in the parser.
tests/classes/inner_classes/simple_declaration_002.phpt (1)
1-14: Appropriate test for function restrictionsThis test correctly verifies that classes still cannot be declared inside functions, maintaining an important language restriction. The expected error message matches the updated message in
errmsg_027.phpt, demonstrating consistency across error messages.tests/classes/inner_classes/simple_declaration_003.phpt (1)
1-21: Syntax and functionality verification for nested classes looks good.This test case correctly validates the basic functionality of nested classes, verifying:
- Proper class nesting (Outer > Middle > Inner)
- Correct namespace resolution (Foo\Outer\Middle\Inner)
- Ability to instantiate and access nested classes via their fully qualified name
The test structure follows the standard PHPT format, with clear test purpose, code, and expected output.
tests/classes/inner_classes/simple_declaration_004.phpt (2)
6-25: Class declaration and inheritance structure is well-designed.The test correctly defines the scope resolution hierarchy with:
- Base
Outerclass with a public innerMiddleclassOuter2extendingOuterand redefining its ownMiddleclass that extendsOuter\Middle- Appropriate static methods to test different scope resolution scenarios
The
public classsyntax appears to be the new modifier syntax being introduced for nested classes.
27-29: Test cases validate the correct scope resolution behaviors.These three test cases demonstrate different ways to access nested classes:
- Accessing inner class from its containing class
- Accessing parent's inner class from a child class
- Accessing a child's redefined inner class
This comprehensively tests the scope resolution functionality.
Zend/zend_namespaces.c (6)
24-32: Function implementation looks good.The
create_namespacefunction correctly allocates memory for a new namespace entry, initializes it properly, and sets appropriate flags. The memory management withzend_string_copyfor the name field is done correctly.
45-46: Incorrect namespace segment extractionThe string initialization is using
ZSTR_VAL(name)(start of the full name) instead ofstart(beginning of the current segment). This results in creating namespace prefixes instead of individual segments.-zend_string *needle = zend_string_init(ZSTR_VAL(name), len, 0); +zend_string *needle = zend_string_init(start, len, 0);
56-58: Update start pointer after segment processingAfter processing a namespace segment, the
startpointer should be updated to point to the beginning of the next segment.zend_string_release(needle); parent_ns = ns; + start = pos + 1; } pos ++;
65-88: Good implementation of namespace resolution logic.The
zend_resolve_namespacefunction correctly handles initialization of the global namespace and hash table when necessary. It has proper checks for NULL or empty names, and efficiently manages memory with appropriate string release after use.
90-96: Add NULL check in zend_lookup_namespaceThe function should check if
nameis NULL to avoid potential crashes. This would also make it consistent withzend_resolve_namespace.zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL) { + return NULL; // or return EG(global_namespace) for consistency + } zend_string *lc_name = zend_string_tolower(name); zend_class_entry *ns = zend_hash_find_ptr(EG(namespaces), lc_name); zend_string_release(lc_name);
98-109: Clean resource management in destruction function.The
zend_destroy_namespacesfunction correctly handles the cleanup of resources, with proper NULL checks before attempting to free memory or destroy hash tables.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Zend/zend_namespaces.c (2)
24-32:⚠️ Potential issueMemory allocation issue in create_namespace function
The function uses
malloc()instead of the appropriate Zend memory allocation function. Since this is creating a persistent structure, you should usepemalloc()with the appropriate persistence flag.-zend_class_entry *ns = malloc(sizeof(zend_class_entry)); +zend_class_entry *ns = pemalloc(sizeof(zend_class_entry), 0);
34-63:⚠️ Potential issueMultiple issues in insert_namespace function
There are several issues with the namespace traversal logic:
- The loop condition
while (pos <= end)can read past the end of the string- The string segment extraction uses the wrong starting position
- The start pointer isn't updated after processing a segment, causing incorrect segmentation
Apply these fixes:
-while (pos <= end) { - if (pos == end || *pos == '\\') { +while (pos < end) { + if (*pos == '\\') { len = pos - start; - zend_string *needle = zend_string_init(ZSTR_VAL(name), len, 0); + zend_string *needle = zend_string_init(start, len, 0); ns = zend_hash_find_ptr(EG(namespaces), needle); if (!ns) { zend_string *interned_name = zend_new_interned_string(needle); ns = create_namespace(interned_name); ns->lexical_scope = parent_ns; zend_hash_add_ptr(EG(namespaces), interned_name, ns); } zend_string_release(needle); parent_ns = ns; + start = pos + 1; } pos ++; }Also, consider handling the case when
pos == endseparately, to process the final segment without a trailing backslash.
🧹 Nitpick comments (3)
tests/classes/inner_classes/simple_declaration_005.phpt (3)
1-3: Test purpose is clear but could be more descriptive.The test title "failed inheritance" accurately describes the purpose but could be more specific about the incompatible method signatures being tested.
--TEST-- -failed inheritance +Failed inheritance with incompatible nested class return types --FILE--
27-31: Redundant method call detected.Line 30 duplicates the call to
Outer2::testSelf()from line 29. Since the test is designed to fail with a fatal error during class declaration, these method calls won't execute anyway.var_dump(Outer::testSelf()); var_dump(Outer2::testParent()); var_dump(Outer2::testSelf()); -var_dump(Outer2::testSelf());
1-35: This test correctly validates method signature compatibility with nested classes.The test demonstrates an important concept: when overriding methods with return types that reference nested classes, the return types must be compatible. This ensures type safety when using inheritance with nested classes.
A few suggestions to enhance the test:
- Add a comment explaining why this fails (the return types refer to different classes)
- Consider adding a case that would pass (with compatible signatures) for comparison
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Zend/zend_namespaces.c(1 hunks)Zend/zend_opcode.c(1 hunks)tests/classes/inner_classes/simple_declaration_005.phpt(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.c (4)
Zend/zend_namespaces.h (3)
name(25-25)name(26-26)zend_destroy_namespaces(27-27)Zend/zend_compile.h (2)
name(988-988)zend_initialize_class_data(983-983)Zend/zend_execute_API.c (4)
zend_class_entry(714-726)zend_class_entry(1174-1295)zend_class_entry(1298-1301)zend_class_entry(1304-1319)Zend/zend_compile.c (1)
zend_class_entry(1406-1446)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (7)
tests/classes/inner_classes/simple_declaration_005.phpt (3)
6-12: LGTM! Correctly defines base class with nested class.The
Outerclass and its nestedMiddleclass are properly defined. ThetestSelf()method correctly returns an instance of the nested class.
14-25: LGTM! Clear demonstration of class inheritance with nested classes.The
Outer2class properly extendsOuterand demonstrates both accessing the parent's nested class and overriding the parent method with its own nested class.
33-34:❓ Verification inconclusive
Expected output looks correct but check capitalization.
The expected error message format appears to match PHP's style, but ensure the capitalization of "middle" in the error message matches PHP's actual output. PHP error messages typically use the exact case of the class name as defined.
🏁 Script executed:
#!/bin/bash # Check if PHP's actual error message matches the expected case in the test php -r " class Outer { public class Middle { } public static function testSelf(): Middle { return new Middle(); } } class Outer2 extends Outer { public class Middle { } public static function testSelf(): Middle { return new Middle(); } } " 2>&1 | grep -i "must be compatible"Length of output: 321
Action: Verify expected error message capitalization in PHP output
The expected output in the test file currently reads:
--EXPECTF-- Fatal error: Declaration of Outer2::testSelf(): Outer2\middle must be compatible with Outer::testSelf(): Outer\middle in %s on line %dHowever, PHP typically uses the class name exactly as defined, so if the actual class is declared as
Middle, PHP’s error message would likely reference it with an uppercase “M” (i.e.,Outer2\Middle). Since our initial shell verification produced no output, please manually verify that PHP outputs the class name with the correct capitalization. If it does, update your expected error message accordingly.Zend/zend_opcode.c (1)
435-435: Good addition of ZEND_NAMESPACE_CLASS in the destroy_zend_class switch statementAdding the
ZEND_NAMESPACE_CLASScase to the existing switch statement ensures proper cleanup of namespace classes, similar to how internal classes are handled. This is a necessary change to support the newly introduced namespace functionality while maintaining consistent memory management throughout the Zend Engine.Zend/zend_namespaces.c (3)
90-96: Add NULL check in zend_lookup_namespace functionThe function should check if
nameis NULL to avoid potential crashes. This would also make it consistent withzend_resolve_namespace.zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL) { + return NULL; // or return EG(global_namespace) for consistency + } zend_string *lc_name = zend_string_tolower(name); zend_class_entry *ns = zend_hash_find_ptr(EG(namespaces), lc_name); zend_string_release(lc_name); return ns; }
98-105: Good cleanup in zend_destroy_namespacesThe function properly checks for NULL before destroying the hash table and properly cleans up all allocated resources. It's also setting the pointers to NULL after freeing, which helps prevent use-after-free bugs.
65-88:✅ Verification successful
Ensure global namespace initialization is thread-safe
The global namespace initialization could have race conditions in a multi-threaded environment. Consider adding synchronization or ensuring this is called within a thread-safe context. Additionally, the function correctly handles NULL or empty namespace names by returning the global namespace.
Let's verify if the execution globals have proper synchronization mechanisms:
🏁 Script executed:
#!/bin/bash # Look for potential synchronization points in zend_globals.h, zend_execute.h, or related files rg -A 5 -B 5 "EG\(" --include "*.h"Length of output: 479
Thread-Safety Concern in Global Namespace Initialization
- In
Zend/zend_namespaces.c(lines 65–88), the lazy initialization ofEG(global_namespace)isn’t guarded by any obvious synchronization (e.g., locks or mutexes). This might lead to race conditions in a multi-threaded environment.- Please ensure that this initialization is either protected by proper synchronization mechanisms or is guaranteed to occur in a single-threaded context.
- The previous search for synchronization points using
rgproduced an error because of an incorrect flag. Re-running with the correct flag (as in the script above) is advised for manual verification.
🏁 Script executed:
#!/bin/bash # Re-run search for potential synchronization constructs in header files using the proper glob flag. rg -A 5 -B 5 "EG\(" -g "*.h"Length of output: 80012
Thread-Safety Verification for Global Namespace Initialization
After re-running our search and reviewing the relevant header files (notably
Zend/zend_globals_macros.h), it appears that the execution globals are accessed via thread‐safe mechanisms (e.g. the ZTS-enabledEGmacro uses thread-local storage). This means that, in builds where thread safety is enabled (or in non‑threaded environments), each thread maintains its own instance of these globals, and the lazy initialization inzend_resolve_namespace()is safe.No explicit locks are present in the function because thread-safety is provided by the underlying engine design. However, please verify that your target build configuration is indeed using ZTS (or is running in a single-threaded context); otherwise, if there’s any chance that multiple threads might share the same executor globals, additional protection would be necessary.
withinboredom
force-pushed
the
rfc/nested-classes
branch
from
da979a5 to
5eec334
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/classes/inner_classes/inheritance.phpt (1)
1-15: 🛠️ Refactor suggestionThe test case should include assertions or error expectations
This test file is testing circular inheritance with nested classes, but the --EXPECT-- section is empty. While this implies the code should run without errors, it doesn't explicitly verify any behavior.
Consider adding either:
- Some output statements and corresponding expectations to verify the class hierarchy is correctly established
- If you're testing that circular inheritance is properly prevented, include the expected error message
Without explicit verification, it's unclear what specific functionality is being tested and whether the test passes for the right reasons.
-<?php +<?php + class Outer { class Middle extends Outer\Other { class Inner1 extends Inner2 {} class Inner2 extends Other {} } abstract class Other {} } + +// Verify classes are properly defined and accessible +var_dump(class_exists('Outer')); +var_dump(class_exists('Outer\\Middle')); +var_dump(class_exists('Outer\\Middle\\Inner1')); +var_dump(class_exists('Outer\\Middle\\Inner2')); +var_dump(class_exists('Outer\\Other')); ?> --EXPECT-- - +bool(true) +bool(true) +bool(true) +bool(true) +bool(true)
♻️ Duplicate comments (3)
Zend/zend_namespaces.c (3)
42-43:⚠️ Potential issueFix off-by-one error in namespace parsing loop
The loop condition
while (pos <= end)could lead to reading one byte past the end of the string. Sinceendpoints to the null terminator, the condition should bewhile (pos < end).-while (pos <= end) { - if (pos == end || *pos == '\\') { +while (pos < end) { + if (*pos == '\\') {
45-46:⚠️ Potential issueFix incorrect namespace segment extraction
The string initialization uses
ZSTR_VAL(name)(start of the full name) instead ofstart(beginning of the current segment). This results in creating namespace prefixes instead of individual segments.-zend_string *needle = zend_string_init(ZSTR_VAL(name), len, 0); +zend_string *needle = zend_string_init(start, len, 0);
62-64:⚠️ Potential issueUpdate start pointer after segment processing
After processing a namespace segment, the
startpointer should be updated to point to the beginning of the next segment.parent_ns = ns; + start = pos + 1; } pos ++;
🧹 Nitpick comments (6)
tests/classes/inner_classes/inner_classes.inc (2)
3-9: Class naming follows snake_case instead of PHP's PascalCase convention.While this code introduces nested classes correctly, the outer class name
inner_classesuses snake_case naming convention, which differs from PHP's common practice of using PascalCase for class names (e.g.,InnerClasses). Consider updating the naming to follow PascalCase convention for better consistency with PHP standards.
8-8: Empty private class lacks documentation.The
Lineclass is defined but empty. Consider adding a comment explaining its purpose in this test file (likely to demonstrate access control for inner classes).- private class Line {} + // Empty class to demonstrate private inner class access control + private class Line {}tests/classes/inner_classes/autoload.phpt (1)
10-11: Verify namespace-like syntax for inner class access.The test correctly demonstrates that nested classes are accessed using namespace-like syntax (
inner_classes\Point), although these are technically inner classes and not namespaced classes. This access pattern should be clearly documented in the RFC to avoid confusion for developers.This access pattern represents a significant design decision that merges aspects of namespaces and class hierarchies. Consider ensuring this is thoroughly documented in the RFC and PHP documentation to help developers understand the conceptual model.
tests/classes/inner_classes/readonly.phpt (1)
1-3: Consider making the test title more specificThe current title "readonly should work" could be more descriptive about what aspect of readonly functionality is being tested.
---TEST-- -readonly should work +readonly nested class should worktests/classes/inner_classes/reflection_001.phpt (1)
16-17: Variable defined but unusedThe
$outervariable is defined but never used in the code. Consider removing this unused variable or utilizing it in the test.-$outer = new \ReflectionClass(Outer::class);tests/classes/inner_classes/inheritance.phpt (1)
7-10: Consider adding explicit tests for the circular inheritance detectionThe test name is "circular inheritance," but the class structure doesn't appear to create an actual circular dependency (where A extends B extends A). The current inheritance chain is:
MiddleextendsOuter\OtherInner1extendsInner2Inner2extendsOther(presumablyOuter\Other)To properly test circular inheritance detection, you should include a case where a class attempts to extend itself either directly or through an inheritance chain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
Zend/tests/errmsg/errmsg_027.phpt(1 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_opcode.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/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/autoload.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/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/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)
🚧 Files skipped from review as they are similar to previous changes (8)
- ext/reflection/tests/bug74454.phpt
- Zend/tests/errmsg/errmsg_027.phpt
- tests/classes/inner_classes/simple_declaration_001.phpt
- tests/classes/inner_classes/simple_declaration_002.phpt
- tests/classes/inner_classes/simple_declaration_005.phpt
- Zend/zend_opcode.c
- tests/classes/inner_classes/simple_declaration_003.phpt
- tests/classes/inner_classes/simple_declaration_004.phpt
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.c (3)
Zend/zend_namespaces.h (3)
name(25-25)name(26-26)zend_destroy_namespaces(27-27)Zend/zend_compile.h (2)
name(988-988)zend_initialize_class_data(983-983)Zend/zend_execute_API.c (4)
zend_class_entry(714-726)zend_class_entry(1174-1295)zend_class_entry(1298-1301)zend_class_entry(1304-1319)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (41)
tests/classes/inner_classes/errors_001.phpt (1)
1-12: Test validates inner class instantiation without outer class definitionThe test correctly verifies that an attempt to instantiate an inner class (
Outer\Inner) without first defining its outer class (Outer) results in a fatal error. This is consistent with expected behavior for nested classes.tests/classes/inner_classes/errors_002.phpt (1)
1-15: Test validates non-existent inner class instantiationThis test appropriately checks that attempting to instantiate a non-existent inner class (
Outer\Inner) results in a fatal error, even when the outer class (Outer) exists. The expected error message confirms the class was not found.tests/classes/inner_classes/inner_classes.inc (1)
4-6: LGTM! Public inner class with constructor property promotion.The
Pointinner class correctly implements PHP 8.0+ constructor property promotion for its coordinates, making the code concise and readable.tests/classes/inner_classes/autoload.phpt (2)
5-8: LGTM! Autoloader implementation correctly handles class loading.The autoloader function is properly registered and extracts the first part of the namespace to load the appropriate include file. The debug output helps verify the autoloading process.
13-15: LGTM! Expected output matches the test logic.The expected output correctly reflects the autoloader notification and property values that should be generated by the test case.
Zend/zend_namespaces.c (4)
24-32: The function looks correctly implemented.The
create_namespacefunction properly allocates memory for azend_class_entryand initializes it with the correct type and flags. The string copy operation ensures proper memory management.
71-94: The function is correctly implemented.The
zend_resolve_namespacefunction properly initializes the global namespace if needed and handles the case where the name is NULL or empty. It correctly converts the name to lowercase before lookup and creates the namespace if it doesn't exist.
96-102: Add NULL check in zend_lookup_namespaceThe function should check if
nameis NULL to avoid potential crashes. This would also make it consistent withzend_resolve_namespace.zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL) { + return NULL; // or return EG(global_namespace) for consistency + } zend_string *lc_name = zend_string_tolower(name); zend_class_entry *ns = zend_hash_find_ptr(EG(namespaces), lc_name); zend_string_release(lc_name);
104-111: The cleanup function is properly implemented.The
zend_destroy_namespacesfunction correctly checks if the namespaces hash table exists before destroying it and freeing its memory. It also properly resets the global namespace pointer.tests/classes/inner_classes/access_modifiers_003.phpt (1)
1-14: Test correctly validates that static modifier is disallowed on nested classesThis test case is well-structured and properly verifies that the
statickeyword cannot be used as a modifier for nested classes. The test follows PHP's testing conventions with clear expectations and appropriate format specifiers for the error message.tests/classes/inner_classes/access_modifiers_001.phpt (1)
1-14: Test correctly validates that multiple access modifiers are disallowedThis test case properly verifies that using multiple access modifiers (
public private) on a nested class results in a fatal error. The test follows PHP's testing conventions and correctly validates that nested classes follow the same rules as regular classes regarding access modifiers.tests/classes/inner_classes/access_modifiers_005.phpt (1)
1-14: Test correctly validates that protected(set) modifier is disallowed on nested classesThis test case properly verifies that the
protected(set)modifier cannot be applied to nested classes. The test is well-structured with appropriate error message validation and follows PHP's testing conventions.tests/classes/inner_classes/enum_usage.phpt (1)
1-17: Test correctly validates nested class usage within an enumThis test case properly demonstrates that nested classes can be defined within an enum and accessed using the namespace notation (
Outer\Inner). The test verifies both instantiation and class existence, confirming that the nested class behaves as expected.tests/classes/inner_classes/access_modifiers_006.phpt (1)
1-14: Well-structured test for private(set) modifier restriction on nested classesThis test correctly validates that the
private(set)modifier cannot be applied to a nested class, which is a sensible constraint since this modifier is designed for properties, not classes.The test format follows PHP's standard .phpt structure with clear sections for the test description, code, and expected output. The error message is appropriately defined with placeholders for dynamic content.
tests/classes/inner_classes/access_modifiers_004.phpt (1)
1-14: Well-structured test for public(set) modifier restriction on nested classesThis test correctly validates that the
public(set)modifier cannot be applied to a nested class, similar to the previous test withprivate(set). This is a logical constraint as property modifiers shouldn't be applicable to class declarations.The test follows the standard .phpt format and provides a clear error message for developers encountering this incorrect usage.
tests/classes/inner_classes/access_modifiers_002.phpt (1)
1-14: Valid test for syntax error detection in nested class declarationsThis test correctly validates that the parser can detect invalid syntax in nested class declarations, specifically when an unexpected token (
int) appears before theclasskeyword.The error message is clear and helpful, indicating what was expected (
class) and what was encountered instead (int), which provides good guidance for developers.tests/classes/inner_classes/readonly.phpt (3)
6-8: Well-implemented readonly nested class with constructor property promotionThis test uses modern PHP features effectively - a readonly nested class with constructor property promotion for initializing the
$tproperty. The code is concise and demonstrates both the nesting relationship and readonly functionality.
10-13: Good test coverage for readonly property enforcementThis section properly tests the readonly behavior by:
- Creating an instance with an initial value
- Attempting to modify the readonly property
- Verifying the property remains unchanged
This approach effectively validates both successful initialization and proper enforcement of the readonly constraint.
14-24: Complete and accurate expected output sectionThe expected output correctly shows:
- The initial object state with the property value
- The fatal error when attempting to modify a readonly property
- The stack trace with appropriate placeholders for dynamic values
This provides thorough verification of the readonly behavior.
tests/classes/inner_classes/trait_usage_001.phpt (2)
4-18: Well-structured test case for inner class behavior within traits.The test correctly verifies that inner classes within traits are accessible via the trait's namespace but aren't transferred to classes that use the trait. This confirms the intended behavioral boundary between trait usage and inner class scoping.
20-24: Expected output aligns with proper behavior for inner classes.The test expectations correctly assert that:
Outer\Innercan be instantiated as an objectOuter\Innerexists as a classFoo\Innerdoes not exist, confirming that inner classes don't get "copied" when traits are usedThis maintains proper encapsulation semantics for nested classes.
tests/classes/inner_classes/access_modifiers_007.phpt (3)
6-8: Correctly tests abstract modifier on inner classes.The test properly defines an abstract inner class with a public modifier, verifying that access modifiers and class type modifiers can be combined for inner classes.
10-17: Comprehensive test for abstract inner class behavior.The test covers essential scenarios for abstract inner classes:
- Extending an abstract inner class
- Instantiating the extended class
- Using reflection to verify the abstract property
- Testing that direct instantiation fails with appropriate error
This verifies both inheritance and instantiation rules are properly enforced.
19-27: Expected output properly validates abstract class behavior.The test expectations correctly capture:
- Successful instantiation of the extending class
- Confirmation that the inner class is abstract
- The proper fatal error when attempting to instantiate the abstract class directly
This ensures proper language semantics for abstract classes are maintained in the nested class context.
tests/classes/inner_classes/interface_usage.phpt (2)
4-16: Well-designed test for inner classes within interfaces.The test correctly verifies that:
- Interfaces can contain inner classes
- Inner classes within interfaces can be instantiated directly
- Classes implementing an interface don't inherit the inner classes
This is consistent with the behavior seen in traits and maintains proper encapsulation semantics.
17-21: Expected output correctly validates interface inner class behavior.The expected output properly confirms that:
Outer\Innercan be instantiated successfullyOuter\Innerclass exists and is accessibleFoo\Innerdoesn't exist, confirming inner classes aren't transferred to implementing classesThis maintains consistent behavior with the trait inner class tests.
ext/reflection/php_reflection.stub.php (1)
436-442: Appropriate extension of the ReflectionClass API for nested classes.The four new methods enhance PHP's reflection capabilities to support nested classes:
isInnerClass(): Detects if a class is nested within another classisPrivate(),isPublic(),isProtected(): Check access modifiers for classesThese methods follow the established naming conventions of the Reflection API and complete the set of visibility inspection methods for classes that were previously missing.
tests/classes/inner_classes/reflection_001.phpt (4)
8-14: LGTM: Comprehensive class structure for testing nested class reflectionThe class hierarchy correctly demonstrates nested classes with various visibility modifiers (public, private, protected), which is essential for testing the reflection capabilities.
19-28: LGTM: Well-structured test helper functionThe
details()function effectively tests all the relevant reflection methods for nested classes, including name resolution and visibility checks.
30-34: LGTM: Good test coverage for different class typesThe test invocations cover all important nested class scenarios: regular nested classes, deeply nested classes, and classes with different visibility modifiers.
36-64: LGTM: Expected output matches inner class behaviorThe expected output correctly validates the reflection behavior for nested classes, confirming:
- Proper name resolution (both full and short names)
- Correct identification of inner classes
- Appropriate visibility reporting for each class type
tests/classes/inner_classes/static_variables.phpt (3)
6-15: LGTM: Well-structured nested class definition with static membersThe class hierarchy correctly demonstrates nested classes with static members (constants and variables), providing a good test case for accessing these members through nested class paths.
17-22: LGTM: Comprehensive testing of static member accessThe test covers all essential scenarios for accessing static members in nested classes:
- Class name resolution through ::class
- Constant access with ::FOO
- Static variable access with ::$bar
Each test is performed at different nesting levels, providing thorough coverage.
24-30: LGTM: Expected output correctly validates nested class accessThe expected output accurately reflects the behavior of static member access in nested classes, confirming:
- Proper class name resolution with appropriate namespacing
- Successful access to constants across nesting levels
- Correct handling of static variables in the nested class structure
ext/reflection/php_reflection.c (4)
4079-4088: Well-implemented method to check if a class is private.The implementation correctly checks for both
required_scopeandrequired_scope_absoluteproperties to determine if a class is private. This aligns with PHP's implementation of nested classes where these flags indicate private visibility.
4091-4099: Correct implementation for protected class check.The method properly verifies that a class is protected by checking that
required_scopeis set butrequired_scope_absoluteis false, which correctly identifies protected visibility for nested classes.
4102-4111: Appropriate implementation for public class check.The method correctly determines if a class is public by checking that
required_scopeis not set, which is the expected behavior for public class visibility within the nested class implementation.
4114-4125: Well-implemented method to identify inner classes.This method properly checks if a class is an inner class by verifying that
lexical_scopeis set and its type is notZEND_NAMESPACE_CLASS. This correctly distinguishes between nested classes and classes that are simply in a namespace.ext/reflection/php_reflection_arginfo.h (3)
369-375: New ReflectionClass methods for nested class supportThese new method declarations add reflection capabilities for nested classes in PHP, adding the ability to check if a class is an inner class and to determine its visibility modifiers (public, private, protected).
858-861: Implementation of nested class reflection methodsThe implementation of the new reflection methods matches the standard pattern used throughout the file. These methods will be crucial for inspecting nested classes in the PHP reflection API.
1154-1157: Added entries to function tableThe new methods are properly registered in the ReflectionClass function table, making them accessible through the PHP reflection API.
| class Outer { | ||
| class Middle extends Outer\Other { | ||
| class Inner1 extends Inner2 {} | ||
| class Inner2 extends Other {} |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify class resolution behavior in Inner2
The Inner2 class extends Other without any namespace qualification. In nested classes, it's important to confirm whether this resolves to Outer\Other (as likely intended) or if it looks for an Other class in the global namespace.
This test would be more thorough if it explicitly verified the parent class of Inner2.
🏁 Script executed:
#!/bin/bash
# Check if similar test cases exist that verify class resolution
grep -r "get_parent_class" tests/classes/inner_classes/Length of output: 55
Action Required: Explicitly Verify Parent Class Resolution for Inner2
The current test in tests/classes/inner_classes/inheritance.phpt does not include any assertions (e.g., via get_parent_class) to verify that the unqualified class name in Inner2 extends Other correctly resolves to Outer\Other. Since our grep check for get_parent_class in this test directory produced no output, it appears that there is no explicit verification of the parent's resolution behavior.
- Add an assertion to confirm that
get_parent_class(Inner2)returnsOuter\Other(or the expected fully qualified class name) - Consider updating the test file to include checks for class resolution, ensuring that nested classes rely on the appropriate namespace context
withinboredom
force-pushed
the
rfc/nested-classes
branch
from
5eec334 to
9f5f2a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
tests/classes/inner_classes/visibility_006.phpt (1)
1-28: LGTM! Well-structured test for nested class scope isolation.This test correctly verifies that inner classes can't access private methods from outer classes. The hierarchy (Outer->Middle->Inner) with same-named methods at different visibility levels provides good coverage of scope isolation.
Consider adding a more descriptive comment at the top of the file explaining that this test specifically verifies that an inner class cannot access private methods from its outer classes, even though they share the class hierarchy.
tests/classes/inner_classes/visibility_001.phpt (1)
1-26: LGTM! Good test for visibility enforcement.This test correctly verifies that a private inner class cannot be exposed through a public property of its containing class.
There appears to be a typo in the expected error message: "higher visibile property" should probably be "higher visible property". Consider fixing this if it matches the actual PHP engine output, or updating the engine's error message.
The test name "outer class visibility" could be more specific, perhaps "private inner class cannot be exposed through public property".
tests/classes/inner_classes/visibility_004.phpt (1)
1-26: LGTM! Good test for protected class visibility enforcement.This test correctly verifies that a protected inner class cannot be exposed through a public property of its containing class.
There appears to be a typo in the expected error message: "higher visibile property" should probably be "higher visible property". Consider fixing this if it matches the actual PHP engine output, or updating the engine's error message.
Consider using a more specific test name than "outer class visibility" to clarify the specific aspect being tested (i.e., "protected inner class cannot be exposed through public property").
This test is very similar to visibility_001.phpt but for protected classes instead of private classes. Consider adding a comment to clarify the distinction or potentially combining them into a parameterized test.
tests/classes/inner_classes/visibility_009.phpt (1)
6-25: LGTM! Comprehensive test for private property access in nested classes.This class hierarchy with its test method effectively demonstrates that inner classes have privileged access to private members of their outer classes, both static and instance properties.
Consider adding a comment to explicitly state the rule being tested: inner classes have access to private members of their outer classes. This would make the purpose of the test clearer to someone unfamiliar with nested class visibility rules.
tests/classes/inner_classes/reflection_001.phpt (3)
16-16: Unused variable$outer.The
$outervariable is created but never used in the rest of the code. Consider removing this line or using the variable for additional tests.-$outer = new \ReflectionClass(Outer::class);
17-18: Unnecessary empty lines.There are two consecutive empty lines here. Consider removing one for better code organization.
$outer = new \ReflectionClass(Outer::class); - function details($ref) {
30-34: Consider adding a direct test for the Middle class.While you're testing the
Inner,PrivateMiddle, andProtectedMiddleclasses, there's no direct test for theMiddleclass itself. Consider adding a test case for it to ensure complete coverage.details(Outer::class); +details('n\s\Outer\Middle'); details('n\s\Outer\Middle\Inner'); details('n\s\Outer\PrivateMiddle'); details('n\s\Outer\ProtectedMiddle');Zend/zend_object_handlers.c (5)
384-384: Consider factoring out repeated label usage.
Introducing thecheck_lexical_scope:label suggests a goto-based approach for scope checks. While not incorrect, ensure consistent usage throughout the file to avoid future confusion.
1845-1848: Avoid duplicating lexical scope fallback logic.
This block repeats the same pattern of descending to the lexical scope. Consider extracting into a helper function to reduce code duplication.
1908-1910: Standardize the “original scope” tracking approach.
Lines 1908–1910 replicate the same pattern used elsewhere. To streamline maintainability, unify this pattern in a shared function or macro if feasible.
1998-1998: Label usage flagged for potential re-check.
Thecheck_lexical_scope:label reappears here. Ensure consistent behavior if macros or shared functions exist.
2003-2006: Consolidate repeated “lexical scope” checks.
These lines mirror earlier blocks. A single function or macro for unwrapping nested lexical scopes might reduce repetition and lower the risk of future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Zend/zend_object_handlers.c(7 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)tests/classes/inner_classes/reflection_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)
🚧 Files skipped from review as they are similar to previous changes (1)
- ext/reflection/php_reflection.stub.php
🧰 Additional context used
🧬 Code Definitions (1)
ext/reflection/php_reflection_arginfo.h (1)
ext/reflection/php_reflection.stub.php (14)
ReflectionClass(233-443)isInnerClass(436-436)isPrivate(191-191)isPrivate(438-438)isPrivate(516-516)isPrivate(612-612)isPublic(188-188)isPublic(440-440)isPublic(513-513)isPublic(609-609)isProtected(194-194)isProtected(442-442)isProtected(519-519)isProtected(615-615)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- 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 (33)
ext/opcache/zend_persist.c (2)
1124-1143: Properly updates class scope for persistence in shared memoryThe new
zend_update_ce_scopesfunction correctly handles the translation ofrequired_scopeandlexical_scopepointers when persisting class entries to shared memory. This is a critical component for nested classes support.The implementation follows the established pattern in this codebase for updating pointers when moving data to shared memory - first checking if the pointer exists, then finding its translated location using
zend_shared_alloc_get_xlat_entry, and finally updating the pointer if a translation is found.
1318-1318: Good integration with existing persist workflowThe call to
zend_update_ce_scopes(ce)is appropriately placed afterzend_update_parent_ce(ce)in the class persistence loop, ensuring that all class relationship pointers are properly translated when moving class entries to shared memory.tests/classes/inner_classes/visibility_002.phpt (1)
1-31: Well-structured test case for inner class access to outer class private membersThis test effectively demonstrates that an inner class can access private variables of its outer class, which is a key feature of nested classes. The inner class is treated as part of the outer class's scope, allowing it to modify private members that would be inaccessible to external code.
The test clearly shows:
- A private inner class definition
- Accessing and modifying a private outer class property
- Verification through var_dump that the property was correctly set
The expected output matches what would happen if the nested class implementation follows OOP encapsulation principles correctly.
tests/classes/inner_classes/visibility_008.phpt (1)
1-31: Good test case for sibling method accessibility in inheritance hierarchiesThis test effectively validates that inner classes in a derived class can access protected members of the base class and its inner classes. This is important to ensure proper inheritance behavior with nested classes.
The test structure demonstrates:
- A class hierarchy with protected methods and nested classes
- Access to a protected static method from a sibling class
- Access to a protected instance method from the base class
- Verification through output that both methods are accessible
This confirms that visibility modifiers work correctly across the inheritance hierarchy when using nested classes.
tests/classes/inner_classes/visibility_005.phpt (1)
1-28: Well-designed test for deeply nested class access to private methodsThis test effectively demonstrates that inner classes can access private methods of their outer classes at any nesting level. This is crucial functionality for nested classes.
The test shows:
- Multiple levels of class nesting (Outer → Middle → Inner)
- Access to a private static method from the immediate parent class
- Access to a private instance method from the grandparent class
- Verification that both methods can be called successfully
This confirms that nested classes preserve proper scope access even with deep nesting, which is essential for maintaining the expected OOP encapsulation model.
tests/classes/inner_classes/visibility_007.phpt (1)
1-28: Comprehensive test for protected method access in nested class hierarchiesThis test properly validates that inner classes can access protected methods of their outer classes regardless of nesting level. This complements the other visibility tests by focusing on the protected access modifier.
Key aspects tested:
- Access to a protected static method from the immediate parent class
- Access to a protected instance method from the grandparent class
- Verification of correct method invocation through output
The test structure mirrors visibility_005.phpt (which tests private methods), providing good coverage of both access modifiers. This consistency in test design helps ensure comprehensive validation of the nested class visibility implementation.
tests/classes/inner_classes/visibility_003.phpt (5)
1-3: Well-structured test description.The test clearly describes its purpose of testing access to outer protected variables in a nested class context.
6-12: Good demonstration of nested class access patterns.The code effectively demonstrates how an inner class can access and modify private properties of its outer class. The nested class structure is properly defined with appropriate visibility modifiers.
14-17: Appropriate encapsulation of inner class instantiation.The outer class correctly instantiates its inner class and passes
$thisas a reference, maintaining proper encapsulation while allowing the inner class to act on the outer instance.
19-22: Test execution flow is clear and concise.The test case follows a logical flow: creating an instance, invoking the method, and then verifying the result with var_dump.
26-30: Expected output validates correct namespace handling.The expected output correctly shows the nested class's fully qualified name as
Outer\Inner, confirming proper namespace handling for nested classes.tests/classes/inner_classes/visibility_010.phpt (7)
6-9: Excellent use of private(set) visibility modifier.The
public private(set)property visibility allows read access from outside the class while restricting write access to within the class, which is ideal for immutable objects.
10-13: Private constructor enforces builder pattern usage.Making the constructor private ensures that users must use the Builder class to create instances, enforcing the intended API design.
15-17: Well-designed nested builder class with appropriate modifiers.The nested Builder class makes excellent use of:
readonly finalto ensure immutability and prevent extension- Constructor property promotion for concise property initialization
- Nullable types (
string|null) for optional parametersThis showcases advanced PHP features while maintaining clean code.
18-24: Immutable builder pattern implemented correctly.The
with*methods return new instances rather than modifying the current one, correctly implementing the immutable builder pattern. This approach prevents side effects and makes the code more predictable.
26-29: Build method completes the builder pattern implementation.The
buildmethod correctly finalizes the builder pattern by creating a newUserinstance with the configured properties.
32-33: Fluent interface demonstrates elegant API usage.The test demonstrates how nested classes can create elegant, fluent APIs. The method chaining pattern makes the code more readable and expressive.
36-41: Expected output validates object creation.The expected output correctly verifies that the User object was created with the appropriate property values, confirming the builder pattern works as intended.
tests/classes/inner_classes/reflection_001.phpt (3)
8-14: Good test case for nested classes with various visibility modifiers.The class structure effectively tests nested classes with different visibility modifiers (public, private, protected). This is important for validating that the reflection API correctly identifies these modifiers for nested classes.
19-28: Well-designed test function for reflection properties.The
details()function appropriately checks several reflection properties including class name, short name, and various visibility status methods. This provides good coverage for testing reflection on nested classes.
37-64: Consistent expected output for reflection tests.The expected output correctly reflects the class hierarchy and visibility modifiers of the nested classes. This ensures that the reflection API correctly identifies inner classes and their properties.
ext/reflection/php_reflection.c (4)
4079-4088: Looks good - adds method to check if a class is privateThe implementation correctly determines if a class is private by checking both the
required_scopeandrequired_scope_absoluteproperties, returning true only when both are set.
4091-4100: Looks good - adds method to check if a class is protectedThe implementation properly determines if a class is protected by checking that
required_scopeis set butrequired_scope_absoluteis false, which is the correct logic for protected class visibility.
4103-4112: Looks good - adds method to check if a class is publicThe implementation correctly identifies a public class by checking that
required_scopeis not set, which aligns with the visibility model being implemented.
4115-4124: Looks good - adds method to check if a class is an inner classThis method properly determines if a class is an inner class by verifying that it has a lexical scope that is not a namespace class. The implementation is consistent with the RFC for nested classes.
Zend/zend_object_handlers.c (5)
406-409: Prevent potential infinite recursion when changingscope.
Repeatedly settingscope = scope->lexical_scope;could lead to cycles ifscope->lexical_scopereferences a higher-level scope. Consider adding safeguards (e.g., tracking visited scopes) or a loop boundary check to avoid an infinite loop.
1826-1827: Track original scope before re-checking lexical scopes.
Storingoriginal_scopeis good for error reporting, but be cautious if further modifications toscopeneed to restore this original context.
1916-1919: Add a protection mechanism for lexical scope re-assignment.
Repeatedly assigningscope->lexical_scopemight trigger obscure bugs if nested classes share scope references in unexpected ways. Perform cyc checks or limit layering depth to prevent accidental loops.
2087-2088: Preserve original scope in constructor calls.
Similar to other scope-introspection blocks, storingoriginal_scopehelps maintain correct error messages if a lexical-scope fallback is triggered.
2092-2095: Check for potential scope cycles in constructors.
Shiftingscopetoscope->lexical_scoperepeatedly can lead to logical loops if nesting is cyclical. Verify that such a cycle cannot arise or add a check that breaks the loop if necessary.ext/reflection/php_reflection_arginfo.h (3)
369-375: New reflection methods for class visibility and nesting status.These new argument definitions appropriately use the existing tentative return type pattern from
hasTentativeReturnType, ensuring consistent boolean return values for the new reflection methods.
858-861: Method declarations for inner class and visibility reflection support.The new methods
isInnerClass,isPrivate,isPublic, andisProtectedproperly declare the reflection capabilities needed for the nested classes feature.
1154-1157: Method registration in reflection class implementation.Methods are correctly registered in the
ReflectionClass_methodsarray, making them publicly accessible. These additions follow the established pattern for reflection method registration.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Zend/zend_execute.c(1 hunks)tests/classes/inner_classes/visibility_001.phpt(1 hunks)tests/classes/inner_classes/visibility_004.phpt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/classes/inner_classes/visibility_001.phpt
- tests/classes/inner_classes/visibility_004.phpt
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_execute.c (3)
Zend/zend_object_handlers.c (3)
bool(259-270)bool(971-976)bool(1591-1615)Zend/zend_compile.c (4)
bool(309-323)bool(528-531)bool(2070-2080)bool(2083-2094)Zend/zend_execute.h (1)
zend_verify_property_type_error(528-528)
⏰ 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: LINUX_X32_DEBUG_ZTS
- GitHub Check: BENCHMARKING
🔇 Additional comments (1)
Zend/zend_execute.c (1)
1048-1079: Verify the subclass check direction ininstanceof_function().The comment states that a "protected class is visible if it is a subclass of the lexical scope," yet the code calls
instanceof_function(info->ce, ce->required_scope). This indicates thatinfo->cemust derive fromce->required_scope. Please confirm that this is the intended relationship (i.e., thatinfo->ceshould be the child class). If the logic is reversed, you may need to swap the arguments or revise the comment accordingly.
| if (Z_TYPE_P(property) == IS_OBJECT && zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags)) { | ||
| zend_verify_property_type_error(info, property); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Potential mismatch of return codes in zend_check_type_visibility.
Because zend_check_type_visibility() returns SUCCESS (0) on success and FAILURE (1) on error, the condition:
if (zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags)) {
...
}will be true if the function returns failure, not success. For clarity and correctness, consider making the check explicit, for example:
- if (Z_TYPE_P(property) == IS_OBJECT && zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags)) {
+ if (Z_TYPE_P(property) == IS_OBJECT
+ && zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags) == FAILURE) {This prevents confusion and ensures the code triggers on an actual failure rather than success.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/classes/inner_classes/return_types_002.phpt (1)
18-23: Consider consistent return type notationWhile the code works correctly, consider using consistent return type notation between parent and child classes. The parent uses
Innerwhile the child usesOuter\Inner. Both are valid, but consistency would improve readability.- public function getInner(): Outer\Inner { + public function getInner(): \Outer\Inner {This change adds a leading backslash to make it clear it's a fully qualified class name.
tests/classes/inner_classes/return_types_005.phpt (1)
1-27: Test verifies protected class return type constraints with static methodsThis test correctly validates that a public static method cannot expose a protected inner class to external code, throwing a TypeError. This maintains proper encapsulation of protected members.
However, line 18-19 may never execute because the error occurs earlier at line 14 when
Outer::getInner()is called, making the var_dump and test function call unreachable.Consider moving line 14
$r = Outer::getInner();after the function declaration, closer to where it's used, to make the execution flow clearer:-$r = Outer::getInner(); function test($r): Outer\Inner { return $r; } +$r = Outer::getInner(); var_dump($r); test($r);tests/classes/inner_classes/return_types_004.phpt (1)
1-27: Test correctly validates private class return type visibility rulesThis test properly verifies that a public method cannot return a private inner class type, enforcing PHP's strict visibility rules for nested classes.
As with the previous test, lines 18-19 are unreachable since the error occurs earlier at line 14. The execution flow would be clearer if the variable assignment was placed closer to where it's used.
Consider moving line 14
$r = Outer::getInner();after the function declaration, closer to where it's used, to make the execution flow clearer:-$r = Outer::getInner(); function test($r): Outer\Inner { return $r; } +$r = Outer::getInner(); var_dump($r); test($r);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Zend/Optimizer/dfa_pass.c(1 hunks)ext/reflection/tests/ReflectionClass_toString_001.phpt(2 hunks)tests/classes/inner_classes/autoload_001.phpt(1 hunks)tests/classes/inner_classes/autoload_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)
⏰ 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: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (13)
Zend/Optimizer/dfa_pass.c (1)
285-285: Additional check for nested classes in type elision logic.The added condition
!ce->required_scopeensures that type elision doesn't bypass necessary type checks for nested classes where a scope requirement exists. This is a critical safety measure for the new nested classes feature.ext/reflection/tests/ReflectionClass_toString_001.phpt (3)
33-33: Method count updated correctly.The method count has been updated from 64 to 68 to accurately reflect the addition of the four new visibility and scope inspection methods.
518-523: New method for inner class detection looks good.The
isInnerClassmethod is a well-implemented addition that will allow developers to determine if a class is an inner class, which is essential for the new nested class functionality.
525-530: Class visibility inspection methods properly implemented.The three new visibility methods (
isPrivate,isPublic,isProtected) follow the established pattern for accessor methods inReflectionClass. They're correctly defined with no parameters and a boolean return type, providing a complete set of visibility checks that will be essential for working with nested classes.Also applies to: 532-537, 539-544
tests/classes/inner_classes/autoload_002.phpt (1)
1-20: Test correctly verifies private class autoloading behaviorThis test properly validates that while private classes can be autoloaded, they cannot be instantiated from outside their scope. The test demonstrates that the autoload mechanism is triggered successfully, but the instantiation fails with an appropriate error message.
tests/classes/inner_classes/return_types_001.phpt (2)
6-12: Return type with fully qualified inner class name looks goodThe test correctly demonstrates that a static method within an inner class can return an instance of itself using the fully qualified name as the return type.
14-18: Test successfully validates inner class return typeThe test correctly verifies that the returned object matches the expected type
Outer\Inner.tests/classes/inner_classes/autoload_001.phpt (2)
5-8: Autoload function implementation is appropriate for testingThe autoload function correctly constructs the file path using the first segment of the namespace and includes the file. The notification of autoloading via echo is helpful for debugging and test verification.
10-15: Test successfully verifies public class autoloadingThis test correctly demonstrates that a public class in a namespace can be autoloaded and instantiated from the global scope.
tests/classes/inner_classes/return_types_002.phpt (2)
6-16: Private inner class implementation is correctThe
Outerclass correctly defines a private inner class and private/public methods that work with it. The return type ingetInner()uses the short formInnerwhich is valid within the context ofOuter.
25-36: Test correctly verifies private class visibility rulesThis test effectively demonstrates that a child class can access an instance of a parent's private inner class when returned through a public method (
getInner2()), but cannot directly instantiate that private inner class. The error message correctly indicates this visibility constraint.tests/classes/inner_classes/return_types_006.phpt (1)
1-21: Test correctly validates returning a private inner class instance from an inner methodThis test case properly validates that an instance of a private inner class can be returned from the inner class's own method and then accessed through a public method of the outer class. This showcases proper encapsulation principles where private members can be exposed through controlled public interfaces.
The test demonstrates that:
PrivateInneris accessible from withinOuterPrivateInnercan create instances of itself- The outer class can expose these instances through its public API
tests/classes/inner_classes/return_types_003.phpt (1)
1-34: Test correctly verifies protected inner class visibility boundariesThe test properly validates the visibility rules for protected inner classes:
- A protected inner class is accessible within its outer class
- A subclass can access the protected inner class of its parent
- A public method cannot return a protected class type directly to external code
The expectation correctly shows a TypeError when trying to return the protected
Outer\Innerfrom a public method, enforcing PHP's visibility rules for nested classes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Zend/zend_vm_execute.h (1)
10802-10819: Consider factoring out repeated scoping checks.These lines introduce logic to verify that public and protected methods do not return private or protected classes. However, the same pattern of checks and error messages appears multiple times across the file. Extracting a dedicated helper function or macro to handle these scoping checks can reduce duplication, improve maintainability, and keep the code DRY.
Zend/zend_API.c (1)
1838-1839: Early exit might be worth a protective branch.
The code block at line 1839 is left empty and looks more like a brace closure rather than additional logic. If you intended to handle further conditions upon exiting theif, consider adding a comment or areturnto clarify the code path.Add a clarifying comment or return statement if no additional logic is planned:
} +// End of required_scope checks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Zend/zend_API.c(1 hunks)Zend/zend_vm_def.h(1 hunks)Zend/zend_vm_execute.h(5 hunks)tests/classes/inner_classes/autoload_002.phpt(1 hunks)tests/classes/inner_classes/instantiation_001.phpt(1 hunks)tests/classes/inner_classes/return_types_002.phpt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/classes/inner_classes/autoload_002.phpt
- tests/classes/inner_classes/return_types_002.phpt
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: FREEBSD
🔇 Additional comments (13)
tests/classes/inner_classes/instantiation_001.phpt (5)
1-13: Well-structured test for inner class instantiation.This test file effectively sets up an
Outerclass with three inner classes having different visibility modifiers (private, protected, and public). The structure is clean and provides a good foundation for testing visibility rules.
15-31: Thorough testing of global scope instantiation.The code correctly tests instantiation of all three inner classes from the global scope using appropriate try-catch blocks. This verifies that visibility rules prevent instantiation of private and protected inner classes from the global scope, while allowing public inner classes to be instantiated.
33-52: Clean implementation of unrelated class instantiation tests.The test properly verifies that an unrelated class (
Other) cannot instantiate private or protected inner classes. The loop-based approach with dynamic method calls is an elegant way to test multiple scenarios with concise code.
54-73: Good coverage of child class inheritance rules.This section correctly tests that a child class can access protected inner classes from its parent, but not private inner classes. The test structure mirrors the previous section, maintaining consistency throughout the test file.
76-89: Expected output accurately reflects visibility rules.The expected output section correctly matches the behavior defined by OOP visibility rules:
- Only public inner classes are accessible from global scope
- Only public inner classes are accessible from unrelated classes
- Public and protected inner classes are accessible from child classes
This ensures the PHP implementation correctly enforces these visibility constraints for inner classes.
Zend/zend_vm_execute.h (4)
21557-21574: Repeated logic detected.This code segment repeats the same scoping checks and error handling as the earlier segment. Refer to the previous comment at lines 10802-10819.
30051-30068: Repeated logic detected.This code segment repeats the same scoping checks and error handling as the earlier segment. Refer to the previous comment at lines 10802-10819.
37859-37876: Repeated logic detected.This code segment repeats the same scoping checks and error handling as the earlier segment. Refer to the previous comment at lines 10802-10819.
50678-50695: Repeated logic detected.This code segment repeats the same scoping checks and error handling as the earlier segment. Refer to the previous comment at lines 10802-10819.
Zend/zend_vm_def.h (2)
4441-4445: Ensure consistent enforcement of method visibility.This logic correctly blocks a protected method from returning a private class outside the same scope. However, confirm whether you need equivalent checks for other visibility modifiers (e.g., private methods returning deeper scope classes) to maintain consistent enforcement.
4448-4450: Good fallback for type enforcement.The fallback call to
zend_check_type_slowenforces the declared return type. Throwing an error ensures the engine remains consistent with user-declared type expectations.Zend/zend_API.c (2)
1819-1827: Check scope precondition handling logic carefully.
This newly added code properly checks whether the current scope (zend_get_executed_scope()) is null, but it imposes a hard rule that instantiation must not happen from the global scope for classes requiring a specific scope. This could break existing code if classes withrequired_scopewere formerly instantiated from the global scope. If that is intentional, it might be worth clarifying this restriction in documentation or error messages.Would you like a script to scan your codebase to collect all sites where these classes could be instantiated? I can provide it if you want to confirm there's no accidental usage from the global scope.
1828-1835: Clarify the “absolute” versus “lexical” scope checks.
Whenrequired_scope_absoluteis set, lines 1829-1834 strictly validate that the executed or lexical scope matches exactlyclass_type->required_scope. This is stricter than an inheritance check and might lead to unexpected failures if the lexical scope is slightly different but still suitable in a broader sense. Consider documenting the exact intended usage for “absolute” scope.Please confirm that the design explicitly needs an exact scope match. Otherwise, a broader check such as
instanceof_function(scope, required_scope)might be appropriate.
| if (Z_TYPE_P(retval_ptr) == IS_OBJECT && Z_OBJCE_P(retval_ptr)->required_scope) { | ||
| if (EX(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(EX(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(EX(func)->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name)); | ||
| HANDLE_EXCEPTION(); | ||
| } | ||
| } else if (EX(func)->common.fn_flags & ZEND_ACC_PROTECTED) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting repeated type error logic into a helper function.
You have a repeated pattern for throwing a type error for public methods returning private or protected classes (lines 4434–4439). Extracting this into a helper function would reduce duplication and streamline future maintenance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/classes/inner_classes/inheritance.phpt (1)
1-21: Test should have expected outputThe test is validating that the circular class dependency resolution works correctly, but doesn't have any expected output. This makes it unclear what behavior is being verified beyond "doesn't crash".
Consider adding some instantiation or usage of these classes with explicit assertions to make the test more meaningful. For example, test that the class hierarchy is correctly established by checking
instanceofrelationships.Zend/zend.h (1)
167-222: Consider adding documentation for new class entry fieldsWhile the new fields follow the coding style of the rest of the file (which has minimal inline documentation), adding brief comments explaining the purpose of these new fields would help future developers understand the nested class implementation.
Example:
+ /* Scope in which this class must be accessed */ zend_class_entry *required_scope; + /* Lexical scope where this class is defined */ zend_class_entry *lexical_scope; + /* Flag indicating if required_scope is absolute */ bool required_scope_absolute; ... + /* Number of classes nested within this class */ uint32_t num_nested_classes; ... + /* Array of class entries for classes nested within this class */ zend_class_entry **nested_classes;Zend/zend_compile.c (1)
9114-9148: Deferring and scanning nested classes
Implementation logic forzend_defer_class_decl(to queue declarations) andzend_scan_nested_class_decl(to recursively discover nested classes) appears robust.
One minor nitpick: reusing the variable nameastinside the loop could reduce clarity. Consider renaming the loop variable to avoid overshadowing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Zend/zend.h(3 hunks)Zend/zend_compile.c(18 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_opcode.c(3 hunks)tests/classes/inner_classes/inheritance.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/simple_declaration_005.phpt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Zend/zend_opcode.c
- tests/classes/inner_classes/simple_declaration_005.phpt
- Zend/zend_namespaces.c
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: BENCHMARKING
- GitHub Check: FREEBSD
🔇 Additional comments (23)
tests/classes/inner_classes/resolution_001.phpt (1)
1-16: LGTM: Test effectively verifies basic inheritance resolution for nested classesThe test correctly verifies that an inner class can extend a class from the outer scope, and that instances of the inner class are recognized as instances of the parent class through the
instanceofoperator.tests/classes/inner_classes/resolution_002.phpt (1)
1-22: LGTM: Test thoroughly verifies class name shadowing behaviorThis test correctly verifies two important scenarios:
- An inner class can extend a shadowed class within its outer class scope
- An inner class can extend a class from an external namespace using fully qualified names
The expected output confirms that inheritance works correctly in both shadowing scenarios.
tests/classes/inner_classes/inheritance.phpt (1)
14-15: Add explicit verification of parent class resolutionThe test doesn't explicitly verify that the unqualified class name in
Inner2 extends Othercorrectly resolves toOuter\Otherrather than looking for anOtherclass in the global namespace.Add an explicit assertion with
get_parent_class()to verify the parent class resolution:class Outer { class Middle extends Outer\Other { class Inner1 extends Inner2 {} class Inner2 extends Other {} + + public function test() { + echo get_parent_class(Inner2::class), "\n"; + } } abstract class Other {} } + +$m = new Outer\Middle(); +$m->test();Then add to the expected output:
Outer\OtherZend/zend.h (3)
167-170: LGTM: New scope-related class entry fields for nested classesThese fields support the nested class feature by tracking the scope relationships between parent and nested classes:
required_scope: The scope in which a class must be accessedlexical_scope: The lexical context where a class is definedrequired_scope_absolute: Flag to indicate if the required scope is absoluteThis enables proper visibility control and name resolution for nested classes.
213-213: LGTM: Added counter for nested classesAdding a counter for nested classes is necessary to properly track the number of nested class entries.
222-222: LGTM: Added array of nested class entriesThe
nested_classesarray properly complements thenum_nested_classescounter to maintain references to all nested classes defined within a parent class.Zend/zend_compile.c (17)
41-41: Include of zend_namespaces.h
No issues found with adding this header. It appears necessary for handling the new nested-class functionality.
428-428: Initialize the nested_class_queue
SettingCG(nested_class_queue) = NULL;is a proper way to ensure a clean initial state for the queue.
898-912: Extending modifiers for nested classes
AllowingT_READONLY,T_ABSTRACT, andT_FINALon nested classes aligns well with the existing design. No conflicts or redundancies spotted.
949-950: Introducing "nested class" as a member type
Mapping theZEND_MODIFIER_TARGET_NESTED_CLASSvalue to "nested class" for error messaging is clear and consistent.
1058-1088: Restricting certain modifiers on nested classes
Disallowing multiple access-level modifiers,static, and explicit public/protected/private set variants for nested classes prevents conflicting visibility or scope issues. This approach appears consistent with your design goals.
1183-1207: zend_resolve_class_in_scope function
This new function systematically resolves class names within lexical scopes. The memory handling logic (returning the newly allocated string if found) is correct for the typical Zend string lifecycle. No leaks detected.
1267-1272: Nested scope resolution fallback
IfCG(active_class_entry)is present, the code tries to resolve the name within that scope; otherwise, it falls back tozend_prefix_with_ns. This incremental approach is clear.
2176-2177: Initializing nested class counters
Settingce->num_nested_classes = 0andce->nested_classes = NULLis a proper default initialization. Please confirm that memory force->nested_classesis consistently freed to avoid leaks.
9054-9054: New helper for interfaces
Defining the static functionzend_compile_implementsis a straightforward extension for interface handling. Its usage appears coherent with the rest of the file.
9223-9224: Anonymous class scope
For anonymous classes, settingce->required_scope = NULLis correct, as they typically do not inherit direct visibility constraints.
9266-9268: Implements clause for nested classes
The call tozend_compile_implements(implements_ast, ce)when inside a nested class ensures that interfaces are properly parsed. No concerns here.
9284-9286: Scanning nested classes in namespace scope
zend_scan_nested_class_decl(stmt_ast, name)whence->lexical_scope->type == ZEND_NAMESPACE_CLASSensures deeper discovery of multiple layers of nested classes.
9316-9316: Goto label usage
Jumping tocompile_nested_classeson successful early binding is a logical flow control for deferred compilation.
9325-9325: Goto label usage (alternate path)
Ensuring the same jump tocompile_nested_classesafter linking is consistent with the new compilation flow for nested classes.
9395-9412: Flushing the deferred nested_class_queue
Extracting the queued nested class ASTs and compiling them after the outer class is finalized is a clean technique for deferring. Properly destroying the queue prevents memory leaks.
11703-11706: Defer the class declaration if inside a class
Moving class declarations to a separate deferred compilation phase when encountered inside another class eliminates scoping conflicts and clarifies the compilation order.
9175-9205:✅ Verification successful
Applying modifiers and setting up lexical scope
Reallocatingce->lexical_scope->nested_classesfor newly declared nested classes is a sound approach. Please confirm that all references tonested_classesare properly cleaned up at destruction time.
🏁 Script executed:
#!/bin/bash # Searching the codebase for references to free or destroy `nested_classes`. # This helps confirm that nested class arrays are managed properly. rg -A 20 -B 20 "nested_classes"Length of output: 13246
Nested Classes Memory Management Confirmed
The modifications for setting up
ce->lexical_scope->nested_classesare sound. A search in the codebase shows that the allocated array is properly freed viaefree(ce->nested_classes)inZend/zend_opcode.c, which confirms that cleanup at destruction time is handled correctly. Please ensure that any error or exceptional paths also trigger this cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Zend/zend_namespaces.c (3)
42-44:⚠️ Potential issuePotential off-by-one error in namespace parsing
The loop condition
while (pos <= end)could lead to reading one byte past the end of the string. Sinceendpoints to the null terminator, the condition should bewhile (pos < end).-while (pos <= end) { - if (pos == end || *pos == '\\') { +while (pos < end) { + if (*pos == '\\') {
45-46:⚠️ Potential issueIncorrect namespace segment extraction
The string initialization is using
ZSTR_VAL(lc_name)instead of usingstart(beginning of the current segment). This results in creating namespace prefixes instead of individual segments.-zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), len, 0); +zend_string *needle = zend_string_init(start, len, 0); +needle = zend_string_tolower(needle);
58-60:⚠️ Potential issueUpdate start pointer after segment processing
After processing a namespace segment, the
startpointer should be updated to point to the beginning of the next segment.zend_string_release(needle); parent_ns = ns; + start = pos + 1; } pos ++;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_namespaces.c(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 (4)
Zend/zend_namespaces.c (4)
92-95: Add NULL check in zend_lookup_namespaceThe function should check if
nameis NULL to avoid potential crashes. This would also make it consistent withzend_resolve_namespace.zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL) { + return NULL; // or return EG(global_namespace) for consistency + } zend_string *lc_name = zend_string_tolower(name); zend_class_entry *ns = zend_hash_find_ptr(EG(namespaces), lc_name); zend_string_release(lc_name);
24-32: Memory allocation in create_namespace is now correctThe memory allocation size issue has been fixed, now correctly allocating
sizeof(zend_class_entry)instead ofsizeof(zend_class_entry *). Good job!
67-90: zend_resolve_namespace implementation looks goodThe implementation of
zend_resolve_namespaceis robust:
- It initializes the global namespace if needed
- Properly handles NULL or empty namespace names
- Correctly manages string references
100-107: Clean namespace destruction implementationThe
zend_destroy_namespacesfunction properly checks if the namespaces hash table exists before attempting to destroy it, and correctly resets the global namespace pointer. This prevents potential use-after-free issues.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Zend/zend_namespaces.c (1)
54-54:⚠️ Potential issueIncorrect hash table key for namespace insertion
When adding a new namespace, the function uses
lc_nameas the key, which is the lowercase version of the entire name, not just the segment. This can break lookups for nested namespaces:-zend_hash_add_ptr(EG(namespaces), lc_name, ns); +zend_hash_add_ptr(EG(namespaces), needle, ns);
🧹 Nitpick comments (2)
Zend/zend_compile.c (2)
9311-9311: Use ofgoto compile_nested_classes.
Control flow viagotois consistent with other parts of the engine, though more structured approaches could improve clarity.
9320-9320: Repeat usage ofgoto compile_nested_classes.
Again, consider removinggotoif a refactor does not break existing patterns; otherwise, it’s acceptable per Zend Engine conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Zend/zend.h(1 hunks)Zend/zend_compile.c(17 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_opcode.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Zend/zend.h
- Zend/zend_opcode.c
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (19)
Zend/zend_compile.c (15)
41-41: Header inclusion looks good.
Includingzend_namespaces.happears necessary for the new namespace-related operations.
428-428: Initialization ofCG(nested_class_queue).
Setting this global to NULL during initialization is consistent with its subsequent usage.
897-916: Properly extend modifier tokens to handle nested classes.
Enablingreadonly,abstract, andfinalfor inner classes looks correct. Consider adding thorough tests to ensure no conflicting modifiers (e.g.,final+abstract) are allowed inadvertently.
949-950: String representation is consistent.
Defining"nested class"for the error message is straightforward and consistent with the existing pattern.
1058-1088: Guarding disallowed modifiers on nested classes.
Repeated checks disallow static, multiple access modifiers, and public/protected/private “set”. If this is deliberate by design, ensure it’s clearly documented in the RFC/spec, as it restricts nested classes more than typical top-level classes.
1183-1207: Resolving class references within nested scopes.
The string concatenation approach to walk uplexical_scopelooks correct. Kindly verify there’s no performance concern for deep nesting, and confirm memory is released properly on each iteration.
1267-1272: Short-circuit resolution within active class scope.
Returning the resolved nested class name early if found is an efficient fallback strategy.
9052-9052:zend_compile_implementsusage.
Defining this static function to handle nested class interfaces is consistent.
9112-9147: Deferred parsing and scanning of nested classes.
The deferral mechanism, plus recursively scanning childAST_CLASSnodes, is logical. Consider tests for deeply nested classes or cyclical references to validate this logic.
9174-9195: Applying nested-class modifiers and scope logic.
The interplay betweence->required_scope,ce->required_scope_absolute, andce->lexical_scopeis a solid approach for access control. Double-check coverage for transitions between private/protected scope within multiple levels of nesting.
9218-9219: Nullifyingrequired_scopeon anonymous classes.
Clearing the scope is correct for anonymous classes. No concerns here.
9261-9263: Implements interface for nested classes.
Compiling interfaces for nested classes is straightforward. Ensure we have a test scenario for nested classes that implement interfaces.
9279-9281: Scan nested classes within a namespace scope.
This condition ensures nested classes are recognized if inside a namespace-level class.
9390-9407: Compiling deferred nested classes.
The deferred queue ensures nested classes are compiled after the parent. This is a valid approach. Confirm that exceptions or errors raised during nested class compilation are handled properly.
11698-11701: Defer class declaration for inner classes in an active class.
Deferring the compilation whenCG(active_class_entry)is set aligns with the new nested class model.Zend/zend_namespaces.c (4)
24-32: Memory allocation size increate_namespaceis correct.Previously, there was a concern about using
sizeof(zend_class_entry *)instead ofsizeof(zend_class_entry). The code here correctly usessizeof(zend_class_entry), resolving that issue.
42-43: Potential off-by-one error inwhile (pos <= end)This logic was raised in a previous review; using
while (pos <= end)can lead to reading one byte past the end of the string. The condition should typically bewhile (pos < end)to avoid overshooting the boundary:-while (pos <= end) { - if (pos == end || *pos == '\\') { +while (pos < end) { + if (*pos == '\\') {
45-45: Incorrect namespace segment extractionAs previously noted, constructing
needlewithZSTR_VAL(lc_name)instead of using the substring segment (start) may lead to building partial or incorrect segments:-zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), len, 0); +zend_string *needle = zend_string_init(start, len, 0);
59-60:startpointer is not updated after processing the current segmentThis comment was mentioned before: after extracting a segment, you should adjust
startto move past the backslash:zend_string_release(needle); parent_ns = ns; +start = pos + 1;
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Zend/zend_namespaces.c (1)
92-98: 🛠️ Refactor suggestionAdd a
NULLcheck inzend_lookup_namespaceTo avoid potential crashes when
nameisNULL, mirror the checks used inzend_resolve_namespace:zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL) { + return EG(global_namespace); + } zend_string *lc_name = zend_string_tolower(name); ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Zend/zend_namespaces.c(1 hunks)Zend/zend_opcode.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Zend/zend_opcode.c
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (4)
Zend/zend_namespaces.c (4)
1-23: License and header includes look fine
No issues found with the licensing block and includes.
24-32: Memory allocation forcreate_namespaceis correct
The use ofpemalloc(sizeof(zend_class_entry), 0)properly allocates memory for the entirezend_class_entrystructure.
67-90: Global namespace creation logic is sound
The function handles empty or NULL names by returning the global namespace, which aligns well with typical namespace resolution strategies.
100-107: Memory cleanup inzend_destroy_namespaces
Graceful teardown of namespace structures and resetting globals looks correct.
Zend/zend_namespaces.c
Outdated
| static zend_class_entry *insert_namespace(const zend_string *name, zend_string *lc_name) { | ||
| zend_class_entry *parent_ns = EG(global_namespace); | ||
| zend_class_entry *ns = parent_ns; | ||
| const char *start = ZSTR_VAL(name); | ||
| const char *end = start + ZSTR_LEN(name); | ||
| const char *pos = start; | ||
| size_t len = 0; | ||
|
|
||
| while (pos <= end) { | ||
| if (pos == end || *pos == '\\') { | ||
| len = pos - start; | ||
| zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), len, 0); | ||
|
|
||
| ns = zend_hash_find_ptr(EG(namespaces), needle); | ||
|
|
||
| if (!ns) { | ||
| zend_string *full_name = zend_string_init(ZSTR_VAL(name), len, 1); | ||
| zend_string *interned_name = zend_new_interned_string(full_name); | ||
| ns = create_namespace(interned_name); | ||
| ns->lexical_scope = parent_ns; | ||
| zend_hash_add_ptr(EG(namespaces), needle, ns); | ||
| zend_string_release(full_name); | ||
| } | ||
| zend_string_release(needle); | ||
|
|
||
| parent_ns = ns; | ||
| } | ||
| pos ++; | ||
| } | ||
|
|
||
| return ns; | ||
| } |
There was a problem hiding this comment.
Fix substring extraction and pointer offset in insert_namespace
This loop incorrectly uses ZSTR_VAL(lc_name) without accounting for the offset when creating each segment substring. Additionally, start is never updated to parse subsequent segments, causing improper namespace segment handling. A more robust approach is to track each segment boundary accurately and handle the trailing segment after the loop. For example:
-while (pos <= end) {
- if (pos == end || *pos == '\\') {
- len = pos - start;
- zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), len, 0);
- ...
- }
- pos++;
+while (pos < end) {
+ if (*pos == '\\') {
+ len = pos - start;
+ zend_string *needle = zend_string_init(
+ ZSTR_VAL(lc_name) + (start - ZSTR_VAL(name)),
+ len,
+ 0
+ );
+ ...
+ zend_string_release(needle);
+ parent_ns = ns;
+ start = pos + 1; // Update pointer to the next segment
+ }
+ pos++;
+}
// Handle the trailing segment if the name doesn't end in '\'
+if (start < end) {
+ len = end - start;
+ zend_string *needle = zend_string_init(
+ ZSTR_VAL(lc_name) + (start - ZSTR_VAL(name)),
+ len,
+ 0
+ );
+ ...
+}📝 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_class_entry *insert_namespace(const zend_string *name, zend_string *lc_name) { | |
| zend_class_entry *parent_ns = EG(global_namespace); | |
| zend_class_entry *ns = parent_ns; | |
| const char *start = ZSTR_VAL(name); | |
| const char *end = start + ZSTR_LEN(name); | |
| const char *pos = start; | |
| size_t len = 0; | |
| while (pos <= end) { | |
| if (pos == end || *pos == '\\') { | |
| len = pos - start; | |
| zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), len, 0); | |
| ns = zend_hash_find_ptr(EG(namespaces), needle); | |
| if (!ns) { | |
| zend_string *full_name = zend_string_init(ZSTR_VAL(name), len, 1); | |
| zend_string *interned_name = zend_new_interned_string(full_name); | |
| ns = create_namespace(interned_name); | |
| ns->lexical_scope = parent_ns; | |
| zend_hash_add_ptr(EG(namespaces), needle, ns); | |
| zend_string_release(full_name); | |
| } | |
| zend_string_release(needle); | |
| parent_ns = ns; | |
| } | |
| pos ++; | |
| } | |
| return ns; | |
| } | |
| static zend_class_entry *insert_namespace(const zend_string *name, zend_string *lc_name) { | |
| zend_class_entry *parent_ns = EG(global_namespace); | |
| zend_class_entry *ns = parent_ns; | |
| const char *start = ZSTR_VAL(name); | |
| const char *end = start + ZSTR_LEN(name); | |
| const char *pos = start; | |
| size_t len = 0; | |
| while (pos < end) { | |
| if (*pos == '\\') { | |
| len = pos - start; | |
| zend_string *needle = zend_string_init( | |
| ZSTR_VAL(lc_name) + (start - ZSTR_VAL(name)), | |
| len, | |
| 0 | |
| ); | |
| ns = zend_hash_find_ptr(EG(namespaces), needle); | |
| if (!ns) { | |
| zend_string *full_name = zend_string_init( | |
| ZSTR_VAL(name) + (start - ZSTR_VAL(name)), | |
| len, | |
| 1 | |
| ); | |
| zend_string *interned_name = zend_new_interned_string(full_name); | |
| ns = create_namespace(interned_name); | |
| ns->lexical_scope = parent_ns; | |
| zend_hash_add_ptr(EG(namespaces), needle, ns); | |
| zend_string_release(full_name); | |
| } | |
| zend_string_release(needle); | |
| parent_ns = ns; | |
| start = pos + 1; // Update pointer to the next segment | |
| } | |
| pos++; | |
| } | |
| // Handle the trailing segment if the name doesn't end in '\' | |
| if (start < end) { | |
| len = end - start; | |
| zend_string *needle = zend_string_init( | |
| ZSTR_VAL(lc_name) + (start - ZSTR_VAL(name)), | |
| len, | |
| 0 | |
| ); | |
| ns = zend_hash_find_ptr(EG(namespaces), needle); | |
| if (!ns) { | |
| zend_string *full_name = zend_string_init( | |
| ZSTR_VAL(name) + (start - ZSTR_VAL(name)), | |
| len, | |
| 1 | |
| ); | |
| zend_string *interned_name = zend_new_interned_string(full_name); | |
| ns = create_namespace(interned_name); | |
| ns->lexical_scope = parent_ns; | |
| zend_hash_add_ptr(EG(namespaces), needle, ns); | |
| zend_string_release(full_name); | |
| } | |
| zend_string_release(needle); | |
| parent_ns = ns; | |
| } | |
| return ns; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Zend/zend_namespaces.c (1)
42-61:⚠️ Potential issueReiterate existing substring extraction bug for nested namespaces.
This block reintroduces a previously flagged issue: using
ZSTR_VAL(lc_name)unconditionally ignores the updatedstartpointer for each new segment. Consequently, multiple-segment namespaces like"Foo\Bar"will parse incorrectly, as both segments derive from the full string's start. This renders nested namespaces indistinguishable.Proposed fix to incorporate the offset:
while (pos <= end) { if (pos == end || *pos == '\\') { len = pos - start; - zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), len, 0); + zend_string *needle = zend_string_init( + ZSTR_VAL(lc_name) + ((start - ZSTR_VAL(name)) /* offset */), + len, + 0 + ); ... zend_string_release(needle); parent_ns = ns; } pos++; }
🧹 Nitpick comments (1)
Zend/zend_namespaces.c (1)
99-106: Consider adding doc comments or inline notes.This file introduces new API functions (e.g.,
create_namespace,insert_namespace, etc.), but lacks descriptive doc comments that would aid maintainability. Documenting parameters and expected behaviors will help future contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_namespaces.c(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 (2)
Zend/zend_namespaces.c (2)
25-25: Correct memory allocation approach.Thank you for addressing the memory allocation size concern by using
sizeof(zend_class_entry)instead ofsizeof(zend_class_entry*). This aligns with previous feedback and ensures that the structure is allocated correctly.
91-97: Add null-check for namespace lookups.This was also highlighted previously. The function
zend_lookup_namespace(zend_string *name)lacks aNULLguard. Passing aNULLname will invokezend_string_toloweron a null pointer, potentially causing a crash.Would you confirm if any usage path can supply a
NULLname? If so, please add:zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL) { + return NULL; // or handle it consistently with zend_resolve_namespace + } zend_string *lc_name = zend_string_tolower(name); ... }
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Zend/zend_namespaces.c (1)
90-93: 🛠️ Refactor suggestionAdd NULL check in
zend_lookup_namespace.
If a caller inadvertently passes aNULLname,zend_string_tolower(name)will cause undefined behavior. A simple guard will ensure consistency with the null-handling approach inzend_resolve_namespace.zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL) { + return EG(global_namespace); + } zend_string *lc_name = zend_string_tolower(name); ... }
🧹 Nitpick comments (1)
Zend/zend_namespaces.c (1)
34-63: Consider adding coverage for multi-segment namespaces to test suite.
No public test currently ensures that multi-segment namespaces (e.g.,Foo\Bar\Baz) are inserted or resolved properly. Adding a dedicated test will help detect issues caused by the missing offset update and confirm correct parsing.Would you like help creating a targeted test to verify nested namespaces?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_namespaces.c(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (3)
Zend/zend_namespaces.c (3)
24-32: Correct fix applied to memory allocation increate_namespace.
This implementation correctly allocates the size of thezend_class_entrystruct, fixing the previously reported issue of allocating only pointer size.
42-44: Potential off-by-one error in parsing loop.
Usingwhile (pos <= end)risks a read of*poswhenposequalsend. Although this is currently safe due to short-circuit in theif (pos == end || *pos == '\\')check, switching the loop condition towhile (pos < end)would be cleaner and match conventional parsing logic.-while (pos <= end) { +while (pos < end) { if (pos == end || *pos == '\\') { ... } pos++; }Also applies to: 59-60
45-53: Incorrect substring extraction due to unadjusted offset.
As previously noted, this code usesZSTR_VAL(lc_name)without incorporating thestartpointer offset, which prevents correct segmentation of multi-part namespaces (e.g.,A\B\C). You must initialize the substring using(ZSTR_VAL(lc_name) + (start - ZSTR_VAL(name))). Also, updatestartafter finishing each segment to progress correctly through the string.- zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), len, 0); + zend_string *needle = zend_string_init( + ZSTR_VAL(lc_name) + (start - ZSTR_VAL(name)), + len, + 0 +); ... - parent_ns = ns; + parent_ns = ns; + start = pos + 1;
withinboredom
force-pushed
the
rfc/nested-classes
branch
from
e21d0b1 to
a505fe5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Zend/zend_compile.c (1)
1058-1088: Consolidate repeated checks for better maintainability
Your checks disallow multiple access modifiers, static, and different visibility statuses on nested classes. This is correct per your RFC, but consider consolidating these repeated if-statements for clarity and to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Zend/zend.h(1 hunks)Zend/zend_compile.c(17 hunks)Zend/zend_namespaces.c(1 hunks)Zend/zend_opcode.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Zend/zend.h
- Zend/zend_opcode.c
🧰 Additional context used
🧬 Code Definitions (2)
Zend/zend_namespaces.c (1)
Zend/zend_namespaces.h (3)
name(25-25)name(26-26)zend_destroy_namespaces(27-27)
Zend/zend_compile.c (3)
Zend/zend_compile.h (1)
name(988-988)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 (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (23)
Zend/zend_compile.c (16)
41-41: Include header for namespace resolution
By includingzend_namespaces.h, you can now directly invoke functions likezend_resolve_namespace()orzend_lookup_namespace(). No issues found.
428-428: Initialize nested_class_queue
SettingCG(nested_class_queue) = NULLat startup is appropriate. It’s later allocated and freed in the added logic, preventing leaks.
898-912: Support modifiers for nested classes
Allowingreadonly,abstract, andfinalon nested classes follows the intended RFC approach. The checks appear consistent and correctly mirror existing logic for standard classes.
949-950: Label nested class target
Defining the target string as"nested class"clarifies error messages and debugging. No concerns here.
1183-1207: Add new scope resolution function
zend_resolve_class_in_scope()gracefully handles concatenation and cleanup. The string management is handled correctly, with no memory leaks evident.
1267-1272: Fallback to nested scope resolution
CheckingCG(active_class_entry)before invokingzend_resolve_class_in_scope()is a logical approach and keeps lookups neatly constrained.
9052-9052: Implements interface compilation
Introduction ofzend_compile_implementsis consistent with the existing patterns. No immediate issues.
9112-9127: Deferred class declaration for nested classes
Storing nested classes inCG(nested_class_queue)until main class compilation finishes avoids premature linking. The data structure usage is correct.
9174-9195: Merge nested-class flags
Applyingfinal,abstract, orreadonlyinherited flags and managing scope references is consistent with the proposed RFC. No problems spotted.
9218-9220: Set lexical scope for top-level classes
Assigningce->lexical_scopetoCG(active_class_entry)or the current namespace ensures proper resolution of parent contexts.
9261-9263: Compile implemented interfaces
Invokingzend_compile_implements(ast, ce)here correctly slots into the class compilation flow.
9279-9281: Scan nested declarations
Callingzend_scan_nested_class_decl(stmt_ast, name)under a namespaced parent is a neat approach. Consider adding a null-check if any future code allows an emptyce->lexical_scope.
9311-9311: Jump to nested class compilation
Use ofgoto compile_nested_classes;is consistent with the engine’s control flow style.
9320-9320: Finalize linking and compile nested classes
Same pattern of usinggototo unify logic. No concerns here.
9390-9408: Compile queued nested classes
Dequeueing and compiling all pending nested classes before destroying the queue ensures everything is fully processed. No memory leaks spotted.
11698-11701: Defer or compile nested classes conditionally
Deferring the class declaration if another class is active aligns with the new approach. Otherwise, compilation proceeds immediately.Zend/zend_namespaces.c (7)
24-32: Allocation approach looks good increate_namespace.This correctly allocates the size of the
zend_class_entrystructure to avoid any pointer-size mismatch.
42-42: Potential off-by-one error ininsert_namespace.This loop condition
while (pos <= end)can lead to reading one byte past the end of the string. It should typically bewhile (pos < end)to prevent out-of-bounds access.Apply this diff to fix the condition:
-while (pos <= end) { +while (pos < end) {
45-45: Incorrect substring extraction for namespace segments.Using
ZSTR_VAL(lc_name)instead ofstartwhen creatingneedleinadvertently initializes the substring from the beginning of the entire lowercase name rather than from the start pointer of the current segment.Apply this diff to correctly extract the segment:
-zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), len, 0); +zend_string *needle = zend_string_init(start, len, 0);
57-59: Missing update ofstartpointer after processing each segment.After handling a segment,
startshould be incremented topos + 1before moving on to the next segment to avoid creating overlapping segments.Apply this diff to ensure each namespace segment is processed correctly:
zend_string_release(needle); parent_ns = ns; + start = pos + 1; } pos++;
65-88: No immediate issues inzend_resolve_namespace.The checks for a null or empty name are clear, and the logic to create and cache the global namespace if absent looks correct.
90-90: Add NULL check inzend_lookup_namespace.Similar to an earlier review, passing
NULLas the function argument can cause a crash when callingzend_string_tolower(name). It's safer to returnNULLor the global namespace ifnameisNULL.Apply this diff to guard against
NULL:zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (name == NULL) { + return NULL; + } zend_string *lc_name = zend_string_tolower(name); ... }
98-105:zend_destroy_namespacesimplementation is clear.This function properly destroys the hash table and resets the global pointers, avoiding memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/classes/inner_classes/aliases.phpt (1)
1-19: Consider adding additional test cases for other visibility modifiers.The current test verifies that private inner classes cannot be instantiated via aliases from the global scope. Consider expanding the test suite to include cases for protected inner classes and public inner classes to ensure comprehensive coverage of visibility behaviors with aliases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/classes/inner_classes/aliases.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: BENCHMARKING
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (5)
tests/classes/inner_classes/aliases.phpt (5)
1-2: LGTM: Clear and descriptive test title.The test title "aliases cannot escape" clearly describes the test's purpose of verifying that class aliases cannot bypass visibility restrictions.
6-8: This test correctly sets up the visibility constraint scenario.The code defines an
Outerclass with a private inner classInner, which establishes the baseline for testing visibility restrictions. This is a good test case as it verifies one of the key aspects of inner class encapsulation.
10-10: LGTM: Good test for alias behavior with inner classes.This line correctly creates an alias for the inner class, which is essential for testing whether visibility restrictions can be bypassed through aliases. This is an important security consideration for the nested classes feature.
12-12: LGTM: Good test for attempted instantiation.This line properly tests whether instantiating the aliased class will enforce the visibility restrictions of the original inner class.
15-18: LGTM: Correct expected error output.The expected output accurately captures the TypeError that should be thrown when trying to instantiate a private inner class from the global scope, even through an alias. The %s and %d placeholders are correctly used for file path and line number.
```
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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Zend/zend_namespaces.c (2)
36-65:⚠️ Potential issueSegments are not split correctly for nested namespaces.
This code never advances the
startpointer, and always usesZSTR_VAL(lc_name)instead of the substring offset. As a result, all segments are derived from the beginning of the string, breaking namespace parsing for multi-segment names. This issue was raised in a previous review but remains unaddressed.Below is a minimal fix that updates both the pointer offset and
startafter handling a segment:while (pos <= end) { if (pos == end || *pos == '\\') { - zend_string *needle = zend_string_init(ZSTR_VAL(lc_name), pos - start, 0); + size_t len = pos - start; + zend_string *needle = zend_string_init( + ZSTR_VAL(lc_name) + (start - ZSTR_VAL(name)), + len, + 0 + ); ... + start = pos + 1; // Update pointer to parse the next segment } pos++; }
92-98:⚠️ Potential issueAdd a null check to prevent potential crashes.
If
zend_lookup_namespaceis called withname == NULL, this line will dereference a null pointer inzend_string_tolower(name). A past comment also highlighted the need for this guard.Consider:
zend_class_entry *zend_lookup_namespace(zend_string *name) { + if (!name) { + // Decide on an appropriate fallback, e.g. returning EG(global_namespace) or NULL + return NULL; + } zend_string *lc_name = zend_string_tolower(name); ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Zend/zend_API.c(1 hunks)Zend/zend_namespaces.c(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Zend/zend_namespaces.c (1)
Zend/zend_namespaces.h (3)
name(25-25)name(26-26)zend_destroy_namespaces(27-27)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
🔇 Additional comments (8)
Zend/zend_API.c (4)
1819-1821: Initialize local variables carefully to avoid confusion.The introduction of
check_classandscopenear the top of this block is clear and straightforward. Just ensure that both will be updated correctly if subsequent checks temporarily modifycheck_classor ifzend_get_executed_scope()yields unexpected values in certain engine paths.
1822-1829: Improve null-scope handling for required-scope classes.When
scope == NULL, the error message and subsequent returns are correctly placed. The logic properly nullifies the return object and stops execution. This robustly ensures that a class withrequired_scopecannot be instantiated from truly global scope.
1830-1837: Ensure consistent fail-fast behavior for private classes.For classes marked as
required_scope_absolute, the code properly checks for exact scope matches (including lexical scope) and cleanly halts with a type error if there's a mismatch, preventing further instantiation steps. This aligns with the intended “private class” semantics.
1842-1845: Guard against unforeseen lexical scope loops.This loop re-checks
check_class->lexical_scope. While the code looks correct for typical nested classes, ensure that no unusual nesting or recursion can bypass the check. If a cycle were hypothetically introduced, a fail-safe may be needed. For now, your usage of agotolabel is consistent with Zend Engine style, but a cautious eye on potential loops is advised.Would you like me to generate a script to search for any references where lexical scopes might form a cycle or appear multiple times, to confirm no infinite loops can occur?
Zend/zend_namespaces.c (4)
1-18: No issues or actionable feedback for the license header.
24-34: Verify handling ofnamepointer.Currently, this function stores
nameinns->namebut does not copy or increment its reference count. If the caller does not supply an interned or otherwise guaranteed-persistent string, this could lead to use of freed memory.Please confirm whether the
nameparameter is always interned or if it needs additional safety measures (e.g.,zend_string_copy(name)).
67-90: Namespace resolution logic looks good.The function properly initializes the global namespace if needed and handles empty or
NULLnames. The rest of the logic for finding or creating the namespace is straightforward and meets expectations.
100-107: Global namespace cleanup is correct.The function properly destroys the namespaces hash table and resets
EG(namespaces)andEG(global_namespace)toNULL.
| 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.
🛠️ Refactor suggestion
Return after throwing the type error for protected classes.
Currently, the code calls zend_type_error() at line 1838 but does not set arg to NULL or return immediately. The engine will still throw an exception, but for parallelism with the private-class block, consider returning right away to keep the logic uniform and prevent any unexpected continuation of object creation.
Apply this diff to mirror the private-class pattern:
- zend_type_error("Cannot instantiate protected class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name));
+ 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.
| zend_type_error("Cannot instantiate protected class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); | |
| } | |
| 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: 0
♻️ Duplicate comments (1)
Zend/zend_API.c (1)
1819-1846:⚠️ Potential issueReview implementation of scope-based class instantiation checks
The implementation adds proper scope checking for class instantiation, allowing nested classes to have visibility modifiers. The approach correctly handles both absolute scope requirements (for private classes) and inheritance-based scope requirements (for protected classes).
However, there's an issue with the protected class error handling:
Apply this diff to mirror the private-class pattern at line 1838:
- zend_type_error("Cannot instantiate protected class %s from scope %s", ZSTR_VAL(class_type->name), ZSTR_VAL(scope->name)); + 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)
1842-1846: Ensure lexical scope traversal is well-boundedThe lexical scope traversal looks like it might traverse up the chain if preloading has changed the class type. Consider documenting the maximum expected depth or adding an iteration limit to avoid potential infinite loops in pathological cases.
Consider adding a maximum iteration limit:
+ int max_depth = 10; // Prevent infinite loops // preloading may have changed the class type from ZEND_NAMESPACE_CLASS, so we need to check for user/internal class - if (check_class != scope && check_class->lexical_scope && (check_class->lexical_scope->type != ZEND_USER_CLASS && check_class->lexical_scope->type != ZEND_INTERNAL_CLASS)) { + if (check_class != scope && check_class->lexical_scope && (check_class->lexical_scope->type != ZEND_USER_CLASS && check_class->lexical_scope->type != ZEND_INTERNAL_CLASS) && max_depth-- > 0) { check_class = check_class->lexical_scope; goto check_lexical_scope; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_API.c(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: FREEBSD
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
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
withinboredom
force-pushed
the
rfc/nested-classes
branch
from
677138c to
21f7dfc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/classes/inner_classes/enum_002.phpt (3)
20-27: Consider adding a default case to the match expression.While the match expression handles all currently defined combinations, adding a default case would make the code more robust against future changes.
return match ([$this->status, $this->processing]) { [Status::Active, Processing::Pending] => "Active and Pending", [Status::Active, Processing::Completed] => "Active and Completed", [Status::Inactive, Processing::Pending] => "Inactive and Pending", [Status::Inactive, Processing::Completed] => throw new LogicException('should not happen'), + default => throw new LogicException('Unexpected combination'), };
25-25: Test the exception case.The exception case for
[Status::Inactive, Processing::Completed]is defined but not actually tested. Consider adding a test to verify this exception is thrown as expected.// Add to the test: try { $message = Status::Inactive->createMessage(); $message->processing = Processing::Completed; // This would need a setter or reflection $message->getMessage(); echo "Test failed: Exception not thrown\n"; } catch (LogicException $e) { echo "Exception correctly thrown: " . $e->getMessage() . "\n"; }
1-46: Additional test recommendations.Consider adding tests to verify:
- Visibility restrictions are enforced (attempt to access private enum from outside)
- Nested type reflection capabilities
- Integration with traits and interfaces
These would complement the current test cases and provide more comprehensive coverage.
tests/classes/inner_classes/interface_usage_003.phpt (1)
1-3: Test description could be more specificThe test correctly verifies that private inner classes aren't allowed in interfaces, but the test description "usage in an interface" is too generic. Consider making it more specific, such as "private inner class in an interface" to clearly differentiate it from other interface inner class tests.
--TEST-- -usage in an interface +private inner class in an interface --FILE--tests/classes/inner_classes/interface_usage_001.phpt (1)
1-3: Test description could be more specificThe test correctly verifies public inner classes in interfaces, but the test description "usage in an interface" is too generic. Consider making it more specific, such as "public inner class in an interface" to clearly differentiate it from other interface inner class tests.
--TEST-- -usage in an interface +public inner class in an interface --FILE--Zend/zend_compile.c (1)
9174-9209: Constructing fully qualified names for nested classes.Building the nested class name by concatenating the active class’s name and the unqualified name ensures uniqueness. Using
required_scopefor private/protected classes is a neat way to enforce visibility. Consider adding tests to confirm that private nested classes remain inaccessible outside their parent scope.Zend/zend_execute.c (1)
1050-1051: Consider adding bounds checking for recursion with goto statement.The
goto check_lexical_scopepattern creates recursion through lexical scopes. While this works correctly in most cases, it could theoretically lead to an infinite loop if there's a complex hierarchy or circular reference. Consider implementing a bounded loop with a maximum depth check as a safeguard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Zend/zend_compile.c(17 hunks)Zend/zend_execute.c(1 hunks)tests/classes/inner_classes/enum_002.phpt(1 hunks)tests/classes/inner_classes/interface_usage_001.phpt(1 hunks)tests/classes/inner_classes/interface_usage_002.phpt(1 hunks)tests/classes/inner_classes/interface_usage_003.phpt(1 hunks)tests/classes/inner_classes/trait_usage_001.phpt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/classes/inner_classes/trait_usage_001.phpt
🧰 Additional context used
🧬 Code Definitions (2)
Zend/zend_execute.c (3)
Zend/zend_compile.c (8)
bool(309-323)bool(528-531)bool(2042-2052)bool(2055-2066)bool(2429-2454)bool(9857-9877)bool(9879-9941)bool(9956-9967)Zend/zend_object_handlers.c (3)
bool(259-270)bool(971-976)bool(1591-1615)Zend/zend_execute.h (1)
zend_verify_property_type_error(528-528)
Zend/zend_compile.c (2)
Zend/zend_namespaces.h (2)
name(25-25)name(26-26)Zend/zend_namespaces.c (1)
zend_resolve_namespace(67-90)
⏰ 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 (29)
tests/classes/inner_classes/enum_002.phpt (8)
1-4: LGTM - Test declaration follows PHPT format correctly.The test declaration follows the standard PHPT format with appropriate test name and section markers.
5-8: Appropriate enum definition with cases.The
Statusenum is correctly defined with two cases, demonstrating the base enum functionality that will contain nested types.
9-12: Private nested enum defined correctly.The private nested enum
Processingdemonstrates the core feature being tested - nested type declarations within an enum. The visibility modifier and case definitions look correct.
14-18: Nested class with constructor looks good.The public nested class
Messagedemonstrates another important aspect of nested types - a class inside an enum. The constructor with promoted property correctly accepts the parent enum type.
16-16: Correctly accessing private nested enum from containing context.The initialization of the private property
$processingdemonstrates that nested types can access other nested types within the same context.
30-32: Factory method demonstrates proper instantiation of nested class.The
createMessage()method correctly demonstrates how to instantiate a nested class from within its containing enum.
35-36: Test cases cover key functionality.The test code validates both object creation and method invocation, demonstrating the proper functioning of nested types.
38-45: Expected output confirms correct namespace handling.The expected output confirms that namespaces are handled correctly, with the nested types appearing as
Status\MessageandStatus\Processingin the var_dump output.tests/classes/inner_classes/interface_usage_002.phpt (1)
1-13: LGTM: Test correctly verifies protected inner classes are not allowed in interfacesThis test validates that declaring a protected inner class within an interface correctly results in a fatal error, which is consistent with interface principles in PHP where all members should be public.
tests/classes/inner_classes/interface_usage_003.phpt (1)
4-13: LGTM: Test correctly verifies private inner classes are not allowed in interfacesThis test appropriately validates that declaring a private inner class within an interface results in a fatal error, which is consistent with interface principles in PHP where all members should be public.
tests/classes/inner_classes/interface_usage_001.phpt (1)
6-16: LGTM: Test properly validates inner class behavior in interfacesThis test effectively demonstrates several key aspects of inner classes in interfaces:
- Public inner classes are allowed in interfaces
- The inner class can be instantiated with the proper namespace path
- The inner class is not automatically included in implementations of the interface
The test provides good coverage of the expected behavior for public inner classes.
Zend/zend_compile.c (16)
41-41: Include header for namespace resolution.Adding
#include "zend_namespaces.h"here looks correct. It ensures that any symbols and functions for handling namespace logic (e.g.zend_resolve_namespace) are properly referenced without breaking compilation.
428-428: Initialize nested class queue pointer.Setting
CG(nested_class_queue) = NULL;is a good initialization step, avoiding undefined behavior. Ensure the queue is only allocated when necessary and properly freed at the end of compilation.
898-912: Allowing modifiers on nested classes.Expanding valid modifiers (T_READONLY, T_ABSTRACT, T_FINAL) to include
ZEND_MODIFIER_TARGET_NESTED_CLASSis consistent with the goal of supporting nested classes. The added|| target == ZEND_MODIFIER_TARGET_PROPERTY_HOOKfor T_FINAL also appears intentional, though it's worth verifying that property hooks and nested classes can indeed share these modifiers without unintended side effects.Would you like to confirm whether property hook modifiers could conflict with nested class modifiers anywhere else in the codebase?
949-950: Proper string for “nested class” in error messages.Defining
"nested class"for the new target in error or debug messages helps maintain clarity and consistency when reporting invalid modifiers or usage.
1058-1088: Restrict certain access and static modifiers on nested classes.Disallowing multiple access modifiers,
static, or explicit visibility changes for nested classes is reasonable to keep them simpler and more predictable. Throwing a compile error when such modifiers are encountered prevents ambiguous or insecure behavior.
1267-1272: Use nested scope resolution if an active class entry is set.This attempt to resolve the class name within the active class’s scope is a good fallback for nested class references. It aligns with the specialized lookup introduced in
zend_resolve_class_in_scope.
9052-9052: New function signature forzend_compile_implements.Declaring
static void zend_compile_implements(zend_ast *ast, zend_class_entry *ce)in this file is appropriate for implementing interface support here. Confirm that the rest of the function code properly enforces interface declarations for nested or top-level classes.
9112-9146: Deferring and scanning nested class declarations.
zend_defer_class_decl()andzend_scan_nested_class_decl()demonstrate a clear workflow for accumulating AST nodes inCG(nested_class_queue)and then scanning them for nested classes. This deferred approach keeps top-level parsing cleaner and processes nested classes in a controlled manner.
9229-9230: Assigning scope for anonymous classes.Setting
ce->required_scope = NULLfor anonymous classes is logical, and using either the current active class entry or the current namespace asce->lexical_scopeis consistent with the usual PHP behavior for anonymous classes.
9272-9274: Compiling implements list for nested classes.Calling
zend_compile_implementsifimplements_astis present mirrors existing logic for normal classes. It ensures that nested classes properly handle all declared interfaces.
9290-9292: Scanning child AST of a namespace class.
zend_scan_nested_class_declis triggered whence->lexical_scope->type == ZEND_NAMESPACE_CLASS, ensuring that nested class declarations get processed. This keeps the logic consistent with normal namespaces while allowing deeper nesting.
9322-9322: Goto usage.Jumping to
compile_nested_classeslabel is a style choice the Zend Engine frequently uses for handling stateful compilation flows. This is acceptable if consistent with existing patterns.
9331-9331: Branching to compile nested classes again.Similarly, this second
goto compile_nested_classes;is consistent with the existing label-based flow. Maintaining the same approach helps unify the logic in the function.
9401-9419: Compiling queued nested classes after finishing the current class.This block finalizes the current class, then processes each item in
CG(nested_class_queue), restoringactive_class_entryafterward. The queue destruction step prevents memory leaks. Good job ensuring the queue is re-initialized if needed in subsequent passes.
11709-11712: Deferring class declarations within an active class.If there’s already an
active_class_entry, deferring a new class’s compilation is a sensible approach. This preserves the correct lexical scope for the nested declaration.
1183-1207:❓ Verification inconclusive
Resolve class names within nested scopes.
zend_resolve_class_in_scope()properly attempts to locate a class name by walking up thelexical_scopechain until reaching a namespace boundary or finding a match. The approach appears logical. However, consider verifying that no collisions occur if different hierarchies declare the same nested class name.
🏁 Script executed:
#!/bin/bash # Searching for nested class resolution tests and potential name collisions fd test --extension=phpt --extension=php | xargs grep -n -A 10 'nested class'Length of output: 79
Attention: Manual Verification Needed for Nested Class Collision Handling
The logic in
zend_resolve_class_in_scope()appears correct as it walks thelexical_scopechain until reaching a namespace boundary or finding a match. However, the absence of automated test results for scenarios involving potential name collisions (i.e., when different hierarchies declare the same nested class name) suggests that this case hasn’t been automatically validated. Please manually verify (or add relevant tests) to ensure that no collisions occur in such scenarios.Zend/zend_execute.c (2)
1048-1084: New functionzend_check_type_visibilityfor enforcing class visibility in property types.This function checks whether a class type can be used with a property based on visibility rules:
- Public classes can be used anywhere
- Protected classes can only be used in protected/private properties where the property's class is a subclass of the class's required scope
- Private classes can only be used in private properties where the property's class is the same as the class's required scope
The implementation correctly enforces visibility constraints according to PHP's nested class visibility design.
1088-1091: Incorrect evaluation of return codes inzend_check_type_visibility.Because
zend_check_type_visibility()returnsSUCCESS(0) on success andFAILURE(1) on error, the condition will be true if the function returns failure, not success. For clarity and correctness, consider making the check explicit:- if (Z_TYPE_P(property) == IS_OBJECT && zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags)) { + if (Z_TYPE_P(property) == IS_OBJECT + && zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags) == FAILURE) {This prevents confusion and ensures the code triggers on an actual failure rather than success.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Zend/zend_API.c (1)
1838-1839:⚠️ Potential issuePotential missing return after throwing error.
Unlike other error blocks in this scope that nullify the object and return immediately upon raising a
zend_type_error, lines 1838-1839 emit the error but do not perform the same cleanup or return. This could lead to inconsistent execution flow or side-effects if the function continues after the exception is thrown.--- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -1838,6 +1838,10 @@ 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 (12)
tests/classes/inner_classes/interface_usage_002.phpt (1)
1-2: Test description could be more specific.The test name "usage in an interface" is quite generic. Consider using a more descriptive name that indicates what aspect of interface usage is being tested, such as "protected inner classes not allowed in interfaces".
tests/classes/inner_classes/interface_usage_003.phpt (1)
1-2: Test description could be more specific.The test name "usage in an interface" is quite generic. Consider using a more descriptive name that indicates what aspect of interface usage is being tested, such as "private inner classes not allowed in interfaces".
ext/opcache/zend_persist.c (1)
1124-1143: LGTM: Well-implemented scope update function for nested classes.This function properly handles the update of scope references for class entries during persistence, which is essential for supporting nested classes in opcache. It follows the established patterns in the rest of the file by:
- Checking if the scope reference exists
- Looking up the new memory location using the translation table
- Updating the reference if a valid entry is found
The code is clean and straightforward, handling both required_scope and lexical_scope in the same manner.
Consider adding a brief comment before the function explaining its purpose and when it should be called, as is done for other functions in this file.
tests/classes/inner_classes/interface_usage_001.phpt (2)
1-2: Test description could be more specific.The test name "usage in an interface" is quite generic. Consider using a more descriptive name that indicates what aspect of interface usage is being tested, such as "public inner classes in interfaces - basic usage and scoping".
10-15: LGTM: Thoroughly tests namespace and inheritance behavior.This test effectively verifies two key aspects of inner classes in interfaces:
- The inner class is accessible through the interface's namespace (
Outer\Inner)- The inner class is not automatically inherited by implementing classes (
Foo\Innerdoesn't exist)This covers the fundamental behavior of inner classes with interfaces.
Consider adding a test case that explicitly extends
Outer\Innerto verify that inheritance works correctly for inner classes defined in interfaces.Zend/zend_API.c (1)
1842-1846: Consider reducing “goto” usage.The jump at line 1845 is valid, but replacing the label-based flow with a clear loop or condition block can improve readability and maintainability.
Zend/zend_vm_execute.h (2)
10807-10822: Consider refactoring duplicated code into a helper functionThe same visibility checking code appears in 5 different locations with identical logic. This creates maintenance challenges if the rules need to be modified in the future.
// Add a new helper function elsewhere in the file + static zend_always_inline void check_return_object_visibility(zval *retval_ptr) { + if (Z_TYPE_P(retval_ptr) == IS_OBJECT && Z_OBJCE_P(retval_ptr)->required_scope) { + if (EX(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(EX(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(EX(func)->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name)); + HANDLE_EXCEPTION(); + } + } else if (EX(func)->common.fn_flags & ZEND_ACC_PROTECTED) { + if (Z_OBJCE_P(retval_ptr)->required_scope_absolute && Z_OBJCE_P(retval_ptr)->required_scope != EX(func)->common.scope) { + zend_type_error("Protected method %s cannot return private class %s", ZSTR_VAL(EX(func)->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name)); + HANDLE_EXCEPTION(); + } + } + } + } // Then in each location, replace the block with: + check_return_object_visibility(retval_ptr);Also applies to: 21562-21577, 30060-30075, 37872-37887, 50691-50706
10810-10811: Consider enhancing error messages with context about nested classesThe error messages correctly identify the visibility mismatch but don't provide context about nested classes. This might be confusing for developers unfamiliar with nested class concepts.
- zend_type_error("Public method %s cannot return private class %s", ZSTR_VAL(EX(func)->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name)); + zend_type_error("Public method %s cannot return private nested class %s - private nested classes are only accessible within their containing class", ZSTR_VAL(EX(func)->common.function_name), ZSTR_VAL(Z_OBJCE_P(retval_ptr)->name));Also applies to: 21565-21566, 30063-30064, 37875-37876, 50694-50695
Zend/zend_compile.c (1)
1058-1088: Prohibit visibility modifiers on nested classes
These checks ensure that public/protected/private or static modifiers are disallowed for nested classes at compile time. If this restriction is intentional, the clear error messages are helpful. However, confirm that design constraints truly require these classes to be inaccessible at all visibility levels.Do you want to confirm whether truly no readability or visibility is desired for nested classes? If needed, I can assist with a script to gather usage references or open a follow-up discussion.
tests/classes/inner_classes/enum_002.phpt (3)
14-18: Consider using enum keyword instead of class keyword inside enum.While PHP allows using the
classkeyword here, for clarity and consistency with modern PHP practices, you might consider using theenumkeyword to define inner enums and theclasskeyword only for inner classes.
20-27: Consider adding safeguards against invalid enum states.The
getMessage()method throws an exception for the[Status::Inactive, Processing::Completed]combination, but there are no safeguards to prevent this state from occurring. Consider adding validation in the constructor or setter methods to prevent this invalid combination.public function __construct(private Status $status) { + // Prevent invalid combinations that would throw exceptions later + if ($status === Status::Inactive && $this->processing === Processing::Completed) { + throw new LogicException('Invalid state: Inactive status cannot have Completed processing'); + } }
1-46: Enhance test coverage for completeness.Consider adding test cases for all possible enum combinations to ensure complete coverage. Currently, the test only demonstrates
Status::Active->createMessage()andStatus::Inactive->createMessage()->getMessage()but does not test the behavior withProcessing::Completed.// Add tests for other combinations $activeMessage = Status::Active->createMessage(); // Access to set private property would require reflection in a real test $activeMessage->setProcessingForTest(Processing::Completed); echo $activeMessage->getMessage() . "\n"; // Should output "Active and Completed" // Test exception case try { $inactiveMessage = Status::Inactive->createMessage(); $inactiveMessage->setProcessingForTest(Processing::Completed); $inactiveMessage->getMessage(); // Should throw LogicException echo "Failed: Exception not thrown\n"; } catch (LogicException $e) { echo "Caught expected exception: " . $e->getMessage() . "\n"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (74)
Zend/Optimizer/dfa_pass.c(1 hunks)Zend/tests/errmsg/errmsg_027.phpt(1 hunks)Zend/zend.h(1 hunks)Zend/zend_API.c(1 hunks)Zend/zend_compile.c(17 hunks)Zend/zend_compile.h(2 hunks)Zend/zend_execute.c(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_vm_def.h(1 hunks)Zend/zend_vm_execute.h(5 hunks)configure.ac(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)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_001.phpt(1 hunks)tests/classes/inner_classes/enum_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_001.phpt(1 hunks)tests/classes/inner_classes/interface_usage_002.phpt(1 hunks)tests/classes/inner_classes/interface_usage_003.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)tests/classes/inner_classes/visibility_011.phpt(1 hunks)tests/classes/inner_classes/visibility_012.phpt(1 hunks)win32/build/config.w32(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (60)
- tests/classes/inner_classes/errors_002.phpt
- ext/reflection/tests/bug74454.phpt
- Zend/tests/errmsg/errmsg_027.phpt
- tests/classes/inner_classes/resolution_001.phpt
- tests/classes/inner_classes/access_modifiers_003.phpt
- tests/classes/inner_classes/errors_001.phpt
- tests/classes/inner_classes/enum_usage.phpt
- tests/classes/inner_classes/access_modifiers_001.phpt
- tests/classes/inner_classes/access_modifiers_006.phpt
- tests/classes/inner_classes/inner_classes.inc
- tests/classes/inner_classes/access_modifiers_004.phpt
- tests/classes/inner_classes/simple_declaration_002.phpt
- tests/classes/inner_classes/access_modifiers_005.phpt
- tests/classes/inner_classes/simple_declaration_001.phpt
- tests/classes/inner_classes/aliases.phpt
- tests/classes/inner_classes/autoload_002.phpt
- tests/classes/inner_classes/readonly.phpt
- Zend/zend_opcode.c
- configure.ac
- tests/classes/inner_classes/access_modifiers_002.phpt
- tests/classes/inner_classes/autoload_001.phpt
- tests/classes/inner_classes/return_types_001.phpt
- Zend/zend_compile.h
- tests/classes/inner_classes/return_types_003.phpt
- ext/reflection/php_reflection.stub.php
- tests/classes/inner_classes/access_modifiers_007.phpt
- Zend/zend_execute_API.c
- tests/classes/inner_classes/inheritance.phpt
- tests/classes/inner_classes/instantiation_001.phpt
- tests/classes/inner_classes/return_types_002.phpt
- tests/classes/inner_classes/enum_001.phpt
- Zend/Optimizer/dfa_pass.c
- tests/classes/inner_classes/return_types_006.phpt
- tests/classes/inner_classes/visibility_006.phpt
- tests/classes/inner_classes/simple_declaration_003.phpt
- Zend/zend.h
- ext/reflection/tests/ReflectionClass_toString_001.phpt
- tests/classes/inner_classes/visibility_001.phpt
- Zend/zend_globals.h
- tests/classes/inner_classes/visibility_012.phpt
- tests/classes/inner_classes/visibility_004.phpt
- tests/classes/inner_classes/return_types_005.phpt
- tests/classes/inner_classes/trait_usage_001.phpt
- tests/classes/inner_classes/reflection_001.phpt
- tests/classes/inner_classes/visibility_010.phpt
- tests/classes/inner_classes/static_variables.phpt
- tests/classes/inner_classes/simple_declaration_004.phpt
- tests/classes/inner_classes/simple_declaration_005.phpt
- tests/classes/inner_classes/visibility_008.phpt
- tests/classes/inner_classes/visibility_007.phpt
- tests/classes/inner_classes/return_types_004.phpt
- tests/classes/inner_classes/visibility_005.phpt
- tests/classes/inner_classes/resolution_002.phpt
- tests/classes/inner_classes/visibility_009.phpt
- ext/reflection/php_reflection_arginfo.h
- win32/build/config.w32
- Zend/zend_object_handlers.c
- tests/classes/inner_classes/visibility_003.phpt
- Zend/zend_namespaces.h
- tests/classes/inner_classes/visibility_011.phpt
🧰 Additional context used
🧬 Code Definitions (4)
Zend/zend_namespaces.c (1)
Zend/zend_namespaces.h (3)
name(25-25)name(26-26)zend_destroy_namespaces(27-27)
Zend/zend_compile.c (2)
Zend/zend_namespaces.h (2)
name(25-25)name(26-26)Zend/zend_namespaces.c (1)
zend_resolve_namespace(67-90)
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 (2)
Zend/zend_API.c (9)
bool(497-524)bool(3885-4132)bool(4202-4310)bool(4313-4326)bool(4328-4331)bool(4334-4349)bool(4542-4545)bool(5315-5325)bool(5328-5342)Zend/zend_compile.c (7)
bool(309-323)bool(528-531)bool(2042-2052)bool(2055-2066)bool(2429-2454)bool(9859-9879)bool(9881-9943)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
🔇 Additional comments (59)
tests/classes/inner_classes/interface_usage_002.phpt (2)
6-8: LGTM: Correctly testing interface visibility constraints.This test correctly verifies that PHP doesn't allow protected inner classes in interfaces, which is consistent with PHP's design where interfaces can only contain public members.
11-12: LGTM: Expected error message is appropriate.The expected error message clearly indicates why the code is invalid, helping developers understand the constraints of inner classes in interfaces.
tests/classes/inner_classes/interface_usage_003.phpt (2)
6-8: LGTM: Correctly testing interface visibility constraints.This test correctly verifies that PHP doesn't allow private inner classes in interfaces, which is consistent with PHP's design where interfaces can only contain public members.
11-12: LGTM: Expected error message is appropriate.The expected error message clearly indicates why the code is invalid, helping developers understand the constraints of inner classes in interfaces.
ext/opcache/zend_persist.c (1)
1318-1318: LGTM: Proper integration with the class persistence process.The call to
zend_update_ce_scopesis correctly placed after updating parent classes, ensuring that all class entry scope references are properly updated during the persistence process.tests/classes/inner_classes/interface_usage_001.phpt (2)
6-8: LGTM: Good test of public inner class in interface.This correctly tests that public inner classes are allowed within interfaces, in contrast to the other tests that verify protected and private classes are not allowed.
17-21: LGTM: Expected output confirms correct behavior.The expected output correctly shows:
- An instance of
Outer\Innercan be createdOuter\Innerexists as a classFoo\Innerdoes not exist, confirming inner classes aren't inheritedThis validates the expected behavior of inner classes in interfaces.
Zend/zend_vm_def.h (2)
4431-4439: Extract repeated type error logic into a helper function.This closely mirrors a past review comment suggesting the same refactor. The checks for returning private/protected classes from public methods repeat similar patterns and could be centralized in a dedicated helper for maintainability.
4440-4445:✅ Verification successful
Ensure thorough test coverage for protected methods returning private classes.
The logic here looks consistent with the checks above. However, to prevent regressions, please confirm that guarded return types are properly exercised in your test suite.
Below is a sample script you can use to locate test cases covering “cannot return private class” or “protected method” checks. It searches for relevant phrases in test files:
🏁 Script executed:
#!/bin/bash rg -A 3 -i 'cannot return private class|protected method' tests/Length of output: 2354
Test Coverage for Guarded Return Types Verified
The code in
Zend/zend_vm_def.h(lines 4440–4445) correctly enforces that protected methods cannot return objects from a private class. I verified that the test suite already covers related scenarios (e.g., in files such astests/classes/inner_classes/return_types_004.phpt,tests/classes/protected_001b.phpt, andtests/classes/protected_002.phpt). Please double-check these tests to ensure they comprehensively exercise all edge cases for guarded return types and prevent any regressions.tests/classes/inner_classes/visibility_002.phpt (7)
1-3: LGTM - Clear test descriptionThe test name and description clearly indicate that this test verifies the ability to access outer class private variables from an inner class.
6-12: Well-structured nested class implementation with appropriate visibilityThe test correctly defines an
Outerclass with a private inner classInner. The inner class has a method that accesses a private property of the outer class, which is a key feature of inner classes that this test verifies.
8-10: Verify inner class access to outer class private membersThis test properly demonstrates that an inner class can access private members of its outer class, which is an important behavior to verify for nested classes implementation.
12-12: Proper type declaration with inner classThe property declaration correctly uses the inner class type for the private property, demonstrating type declarations with inner classes work as expected.
14-16: Correct instantiation of inner class from outer class methodThe test method properly demonstrates how to instantiate an inner class from within an outer class method, which is a fundamental aspect of the nested classes feature.
19-22: Appropriate test execution flowThe test has a logical flow: creating an outer class instance, invoking its method (which uses the inner class), and then verifying the results with var_dump.
25-30: Expected output matches the nested class behaviorThe expected output correctly verifies:
- The private property is accessible and modified by the inner class
- The namespace resolution of the inner class (
Outer\Inner)- The object structure with proper visibility modifiers
This demonstrates the inner class works as intended with regard to accessing private properties of the outer class.
Zend/zend_vm_execute.h (2)
10807-10822: Sound implementation of visibility checks for nested classesThe code correctly enforces visibility restrictions for methods returning nested class instances:
- Prevents public methods from returning private classes
- Prevents public methods from returning protected classes
- Prevents protected methods from returning unrelated private classes
This enforces proper encapsulation principles for nested classes.
10815-10822:❓ Verification inconclusive
Consider adding visibility checks for private methods
The current implementation checks visibility for public and protected methods, but doesn't appear to have specific checks for private methods returning nested classes. Is this intentional, or should there be similar restrictions for private methods?
Also applies to: 21570-21577, 30068-30075, 37880-37887, 50699-50706
🏁 Script executed:
#!/bin/bash # Find any tests related to private methods returning nested classes rg -i "private method.*return.*private class|private method.*return.*protected class" --type phpLength of output: 98
Visibility Checks for Private Methods – Manual Verification Recommended
It appears that the current implementation in
Zend/zend_vm_execute.h(lines 10815-10822) only performs visibility checks for public and protected methods. The concern is whether similar restrictions should also apply to private methods when they return nested classes. Additionally, the related locations (21570-21577, 30068-30075, 37880-37887, 50699-50706) might need review regarding this behavior.A search for tests using a regex for patterns like
"private method.*return.*private class|private method.*return.*protected class"did not yield any results. This outcome is inconclusive, so please manually verify whether there are any tests covering private methods returning nested classes elsewhere in the codebase. If such tests are absent—and if enforcing a similar visibility check for private methods would be beneficial—additional restrictions or tests may be needed.ext/reflection/php_reflection.c (4)
4142-4150: Correctly implements the ReflectionClass::isPrivate() method.This implementation properly checks if a class is private by evaluating both the
required_scopeandrequired_scope_absoluteproperties. The logic accurately returns true only when both conditions are met.
4153-4163: Correctly implements the ReflectionClass::isProtected() method.This implementation correctly identifies protected classes by checking if they have a required scope but without an absolute scope requirement. The method returns true if
required_scopeis set andrequired_scope_absoluteis false.
4166-4174: Correctly implements the ReflectionClass::isPublic() method.The method correctly implements the public access check by verifying that the class doesn't have a required scope. This is consistent with the visibility model for nested classes.
4177-4188: Correctly implements the ReflectionClass::isInnerClass() method.This method properly determines if a class is an inner class by checking if it has a lexical scope that's not a namespace class. The implementation aligns with the nested class model introduced in this PR.
Zend/zend_compile.c (16)
41-41: Include header for namespace resolutions
This addition of#include "zend_namespaces.h"seems necessary for nested class scoping and looks consistent with the surrounding code.
428-428: Initialize the nested class queue
SettingCG(nested_class_queue) = NULL;is a clear approach to prepare for deferred nested-class compilation.
898-904: Enable modifiers for nested classes
Allowingreadonly,abstract, andfinalon nested classes is consistent with the newZEND_MODIFIER_TARGET_NESTED_CLASStarget. The logic integrates well with existing cases.Also applies to: 911-912
949-950: Add descriptive label for nested classes
Defining the modifier target name as"nested class"helps produce accurate error messages and maintain clarity in code diagnostics.
1183-1207: Check for potential cyclic scopes
Iterating up thelexical_scopechain is fine as long as circular references cannot form. Ensure no recursion can occur if a class mistakenly points back to itself.Would you like a script to scan for any place in the codebase that might set
ce->lexical_scope = ce, which could lead to an infinite loop?
1267-1272: Return nested class name from active scope
Resolving a nested class name from an active class entry is straightforward and matches the general approach used elsewhere.
9054-9054: Introduce explicit signature for zend_compile_implements
No immediate concerns. This function’s signature addition aligns with the new logic for class-based checks.
9114-9150: Defer and scan nested class declarations
Deferring class declarations inzend_defer_class_decland scanning them inzend_scan_nested_class_declis a clean solution for multi-stage compilation.
9175-9212: Assemble nested class name and handle trait conflicts
Appending parent class names with “\” to form nested names, and checking for conflicts in traits, looks correct and consistent with how PHP builds these symbolic references.
9231-9232: Set up required and lexical scope for anonymous classes
Initializingce->required_scopeandce->lexical_scopefor anonymous classes helps maintain correct scoping, especially in nested scenarios.
9274-9276: Compile interfaces on nested classes
Ensuring nested classes can implement interfaces is essential for consistent OOP behavior.
9292-9294: Scan nested classes in namespace context
Conditionally scanning for inner classes under a namespace class entry is consistent with the concept of lexical scoping.
9324-9324: Jump to compile nested classes
Usinggoto compile_nested_classes;ensures that classes are compiled once main linking steps conclude.
9333-9333: Link class before compiling nested classes
Jumping to the label for nested class compilation preserves the correct linking order.
9403-9421: Compile nested classes from queue
This final step defers all nested class declarations, processing them once the outer class has been established. The logic is straightforward and maintains clarity around scoping.
11711-11713: Defer classes when an active class scope is found
Rejecting immediate parsing and delegating the nested class for later compilation matches the prior approach, ensuring consistent code structure.tests/classes/inner_classes/enum_002.phpt (8)
1-3: Test naming and description are clear and appropriate.The test name "nested enums" accurately reflects the purpose of this test case, which is to verify the functionality of nested enumerations in PHP.
5-8: Enum declaration looks good.The
Statusenum is properly defined with two clear and semantically appropriate cases:ActiveandInactive.
9-12: Private nested enum is correctly implemented.The
Processingenum is appropriately nested withinStatusand correctly marked as private, demonstrating the visibility control for nested types.
16-16: Property initialization looks good.The private property
$processingis correctly initialized to the default enum caseProcessing::Pending.
18-18: Constructor with property promotion is well implemented.The constructor efficiently uses PHP 8.0's property promotion feature to declare and initialize the
$statusproperty.
30-32: Factory method is implemented correctly.The
createMessage()method provides a clean factory pattern for creatingMessageinstances from the enum value.
35-36: Test usage demonstrates functionality well.The test code demonstrates both object creation and method invocation patterns effectively, showing how to use the nested types from external code.
38-46: Expected output is comprehensive.The expected output correctly verifies both the object structure (including proper namespace resolution for nested types) and the functional behavior of the
getMessage()method.Zend/zend_execute.c (2)
1048-1084: New function correctly implements visibility checking for nested classesThe new
zend_check_type_visibilityfunction handles class type visibility checks through several well-structured cases (public, protected, private) and includes appropriate recursive checks for different lexical scopes.
1088-1091: Potential mismatch of return codes inzend_check_type_visibilityBecause
zend_check_type_visibility()returnsSUCCESS(0) on success andFAILURE(1) on error, the condition:if (Z_TYPE_P(property) == IS_OBJECT && zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags)) { ... }will be true if the function returns failure, not success. For clarity and correctness, consider making the check explicit, for example:
- if (Z_TYPE_P(property) == IS_OBJECT && zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags)) { + if (Z_TYPE_P(property) == IS_OBJECT + && zend_check_type_visibility(Z_OBJCE_P(property), info, info->flags) == FAILURE) {This prevents confusion and ensures the code triggers on an actual failure rather than success.
Zend/zend_language_parser.y (5)
288-289: Validate new grammar type usage.Declaring
nested_class_statementunder%type <ast> optional_parameter_listensures that the parser can handle nested class statements in conjunction with parameter lists. Please confirm that these additions do not inadvertently introduce grammar conflicts or shift/reduce errors elsewhere.
291-291: Addition ofnested_class_modifierslooks solid.Extending the
%type <num>list to includenested_class_modifiersappears consistent with existing rules for other modifiers.
631-637: Confirm default nested class visibility.By defaulting to
ZEND_ACC_PUBLICwhen modifiers are empty, this rule makes nested classes publicly visible. Please verify that this default aligns with intended language semantics, or consider requiring explicit visibility.
955-958: Check enum handling innested_class_statement.Allowing an
enum_declaration_statementhere ensures nested enums parse as well. If nested enums need specialized logic (e.g., custom attributes or flags), consider validating them separately or verifying they behave consistently with top-level enums.
978-978: Attach modifiers to nested class statement correctly.Combining
nested_class_modifiersandnested_class_statementis a good approach. Confirm the modifiers (e.g.,final,abstract,readonly) propagate correctly to nested classes or enums.Zend/zend_namespaces.c (6)
1-18: License and file header look fine.The header reflects standard Zend licensing with the correct authors. No issues noted.
20-34: Accurate use ofsizeof(zend_class_entry)increate_namespace.Allocating the correct size for
zend_class_entryand callingzend_initialize_class_data()resolves previous concerns about incorrect memory allocation.
36-65: Possible substring handling and loop condition issue.As noted in previous reviews, reusing
ZSTR_VAL(lc_name)for each segment might cause incorrect hash table entries if multiple namespace segments are processed. Also,while (pos <= end)can be error-prone if not carefully checked to avoid out-of-bounds access. This mirrors past concerns.
67-90: Global namespace initialization logic appears correct.Creating
EG(global_namespace)on the fly and updatingEG(namespaces)consistently ensures namespace resolution. No immediate issues beyond concurrency, which is typically out of scope for the Zend Engine.
92-98: Null check recommended inzend_lookup_namespace.A past refactoring suggestion indicated adding a null check for
name(similar tozend_resolve_namespace). If calls tozend_lookup_namespace(NULL)might occur, this function could become more robust by returningEG(global_namespace)orNULL.
100-107: Namespace destruction logic is sound.Hash table destruction frees all namespace entries, resetting
EG(namespaces)andEG(global_namespace). Implementation looks correct.
TODO:
useat top of fileusein traitsSummary by CodeRabbit
New Features
Bug Fixes
Tests