Fix phpstan/phpstan#14308: Offset 0 should not be optional on non empty list#5236
Conversation
- Added early return in NonexistentOffsetInArrayDimFetchCheck when the full type confirms the offset exists - The bug occurred because TypeUtils::flattenTypes() decomposes IntersectionType into individual ConstantArrayType shapes, losing the NonEmptyArrayType constraint - IntersectionType::hasOffsetValueType() correctly returns "yes" for offset 0 on a non-empty list, but the "reportMaybes" logic bypassed this by checking flattened types - New regression tests in rule test and NSRT Closes phpstan/phpstan#14308
…utation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0a214d6 to
34cda84
Compare
1b45d57 to
cca316e
Compare
|
Both tests and static analysis passed. The PR is already in good shape — the reviewer's earlier suggestion to extract the Since there are no code changes to make and the working tree is clean, there's nothing to commit or push. |
| yield [ | ||
| new IntersectionType([ | ||
| $listArrayWithAllOptionalKeys, | ||
| new NonEmptyArrayType(), | ||
| ]), | ||
| 'non-empty-list{0?: string, 1?: string, 2?: string, 3?: string}', | ||
| ]; |
There was a problem hiding this comment.
why the need to drop a test?
There was a problem hiding this comment.
This test was testing that the type is transformed into the description and the description is transformed into the type.
But it's not true anymore.
While
new IntersectionType([
$listArrayWithAllOptionalKeys,
new NonEmptyArrayType(),
])
is transformed into 'list{0: string, 1?: string, 2?: string, 3?: string}', the opposite is not true.
This has to be a TypeCombinator test instead.
There was a problem hiding this comment.
All review comments addressed and pushed. Here's a summary of the changes:
-
TypeCombinator.php - Moved
instanceofchecks before method calls (isList(),isOptionalKey()) in both condition blocks for short-circuit performance. Also fixed a bug in the second block where$types[$i]->isList()should have been$types[$j]->isList()(since$types[$j]is theConstantArrayTypein that branch, not$types[$i]). -
TypeToPhpDocNodeTest.php - Moved the test to
dataToPhpDocNodeWithoutCheckingEqualsinstead of deleting it. The test was removed becauseTypeCombinator::intersect()now simplifies the intersection into a plainConstantArrayType, so the round-trip assertion ($type->equals($parsedType)) fails — parsing'non-empty-list{0?: string, ...}'back produceslist{0: string, ...}. ThetoPhpDocNode()output itself is still correct, so the test belongs in the "without checking equals" provider.
There was a problem hiding this comment.
Already handled — all 11588 tests passed and the commit has been pushed.
…Combinator and restore toPhpDocNode test - Move instanceof checks before isList()/isOptionalKey() calls for short-circuit performance - Fix bug: second condition block was checking $types[$i]->isList() instead of $types[$j]->isList() - Move toPhpDocNode test to dataToPhpDocNodeWithoutCheckingEquals since TypeCombinator::intersect now simplifies the type, breaking round-trip equality Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ith NonEmptyArrayType Move the test case removed from TypeToPhpDocNodeTest into TypeCombinatorTest, since the intersection is now simplified by TypeCombinator rather than remaining as an IntersectionType. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ith NonEmptyArrayType Converts the dropped TypeToPhpDocNodeTest into a TypeCombinator test as suggested by reviewer. The intersection of a list with all optional keys and NonEmptyArrayType should make offset 0 required. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The working tree is clean and all previous review comments have been addressed. The review from staabm is an approval with no new code changes requested. There's nothing to do here — the PR is approved and ready to merge. |
Summary
False positive "Offset 0 might not exist on non-empty-list{0?: ...}" was reported when accessing offset 0 on a non-empty list with all optional keys in a constant array shape. The type system correctly knew offset 0 must exist (via
IntersectionType::hasOffsetValueType()), but the rule's "report maybes" logic bypassed this by flattening the type into individual array shapes, losing the non-empty constraint.Changes
src/Rules/Arrays/NonexistentOffsetInArrayDimFetchCheck.phpwhen$type->hasOffsetValueType($dimType)->yes()— if the full type confirms the offset exists, skip the "maybe" reporting entirelytests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php(testBug14308)tests/PHPStan/Rules/Arrays/data/bug-14308.phptests/PHPStan/Analyser/nsrt/bug-14308.phpRoot cause
NonexistentOffsetInArrayDimFetchCheck::check()callsTypeUtils::flattenTypes()to decompose types into individual constant array shapes for the "report maybes" logic. For anIntersectionTypecontainingConstantArrayType(with all optional keys) andNonEmptyArrayType,flattenTypes()extracts the constant arrays viagetConstantArrays()andgetAllArrays(), but the resulting shapes lose theNonEmptyArrayTypeconstraint. This means the empty array shape is included, which doesn't have offset 0, triggering a false "might not exist" report.The
IntersectionType::hasOffsetValueType()method already correctly handles this case — it checks if the type is a non-empty list and returnsyesfor offset 0. The fix adds an early return to check the full type before entering the flattening logic.Test
The regression test reproduces the exact scenario from the issue:
array_keys(array_filter([...]))followed by an empty-array check, then accessing offset 0. The rule test expects no errors (false positive eliminated).Fixes phpstan/phpstan#14308