Fix #6777: Bogus write-only error when appending to an array-like object in private class property#5102
Conversation
- In ClassStatementsGatherer, also record a PropertyRead when a PropertyAssignNode is for an ArrayAccess property via array dim fetch (e.g. $this->array[] = $s), because this is semantically $this->array->offsetSet() which reads the property - New regression test in tests/PHPStan/Rules/DeadCode/data/bug-6777.php - Updated existing WriteToCollection test expectations Closes phpstan/phpstan#6777
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
This doesn't only affect ArrayAccess objects, but also objects like Threaded which support array operations without implementing the interface. So the check
(new ObjectType(\ArrayAccess::class))->isSuperTypeOf($propertyType)->yes()
is not correct
Objects like Threaded support array operations without implementing ArrayAccess. Use isObject() instead of checking for ArrayAccess specifically, so that appending to any object property via array syntax is correctly treated as a read. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
All 13,936 tests pass, static analysis clean. The fix has been committed and pushed. Summary of the change: Replaced the |
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
- Add a test with
\ArrayObject<int, string>|array<int, string>. (Maybe object, maybe array) - Also add a test with
Threadedclass since it does not implements ArrayAccess - Also add a test with
\ArrayObject<int, string>|string(Maybe object)
…ccess objects - Simplify condition to `!$propertyType->isObject()->no()` so that any type that *might* be an object triggers a PropertyRead when appending via `[]` - Add test cases for Threaded (non-ArrayAccess object), ArrayObject|array union, and ArrayObject|string union Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
The background |
| public function __construct(private \ArrayObject $array){} | ||
|
|
||
| public function send(string $s) : void{ | ||
| $this->array[] = $s; |
There was a problem hiding this comment.
Add a similar test with assign by reference
There was a problem hiding this comment.
Background task confirmed — make tests completed successfully as well. Everything was already verified and pushed.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
3 participants
Summary
When appending to an ArrayAccess object stored in a private class property (e.g.
$this->array[] = $s), PHPStan incorrectly reported "Property is never read, only written." This is a false positive because appending to an ArrayAccess object is semantically$this->array->offsetSet(null, $s)— a method call that reads the property.Changes
src/Node/ClassStatementsGatherer.php: When handling aPropertyAssignNode, check if the assigned expression is aSetOffsetValueTypeExprorSetExistingOffsetValueTypeExpr(indicating array dim fetch assignment) and the property type is an ArrayAccess object. If so, also record aPropertyRead.WriteToCollectiontest expectations intests/PHPStan/Rules/DeadCode/UnusedPrivatePropertyRuleTest.phpto reflect the fix.tests/PHPStan/Rules/DeadCode/data/bug-6777.php.Root cause
In
ClassStatementsGatherer, whenNodeScopeResolverprocesses$this->array[] = $son an ArrayAccess property, it emits aPropertyAssignNodewhich was unconditionally recorded as aPropertyWrite. Meanwhile, thePropertyFetchfor$this->arraywas processed in expression-assign mode, causingClassStatementsGathererto skip recording aPropertyRead. This resulted in the property being tracked as "only written" even though it was being read to calloffsetSet()on the object.The fix detects when a property assignment comes from an array dim fetch operation on an ArrayAccess object and also records a
PropertyRead, since the object must be read from the property to invoke the method.Test
Added
tests/PHPStan/Rules/DeadCode/data/bug-6777.phpwhich reproduces the original issue: a class with a privateArrayObjectproperty that is only appended to via$this->array[] = $s. The test expects no errors.Fixes phpstan/phpstan#6777