diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 89a65fca313..380c2eae3fd 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1294,6 +1294,10 @@ public function processStmtNode( $originalKeyVarExpr = null; $continueExitPointHasUnoriginalKeyType = false; + $byRefWithoutKey = $stmt->byRef + && $stmt->keyVar === null + && $stmt->valueVar instanceof Variable && is_string($stmt->valueVar->name); + if ($stmt->keyVar instanceof Variable && is_string($stmt->keyVar->name)) { $originalKeyVarExpr = new OriginalForeachKeyExpr($stmt->keyVar->name); if ($finalScope->hasExpressionType($originalKeyVarExpr)->yes()) { @@ -1301,12 +1305,30 @@ public function processStmtNode( } else { $continueExitPointHasUnoriginalKeyType = true; } + } elseif ($byRefWithoutKey) { + $originalValueExpr = new OriginalForeachValueExpr($stmt->valueVar->name); + if (!$finalScope->hasExpressionType($originalValueExpr)->yes()) { + $scopesWithIterableValueType[] = $finalScope; + } else { + $continueExitPointHasUnoriginalKeyType = true; + } } foreach ($finalScopeResult->getExitPointsByType(Continue_::class) as $continueExitPoint) { $continueScope = $continueExitPoint->getScope(); $finalScope = $continueScope->mergeWith($finalScope); - if ($originalKeyVarExpr === null || !$continueScope->hasExpressionType($originalKeyVarExpr)->yes()) { + if ($originalKeyVarExpr !== null) { + if (!$continueScope->hasExpressionType($originalKeyVarExpr)->yes()) { + $continueExitPointHasUnoriginalKeyType = true; + continue; + } + } elseif ($byRefWithoutKey) { + $originalValueExpr = new OriginalForeachValueExpr($stmt->valueVar->name); + if ($continueScope->hasExpressionType($originalValueExpr)->yes()) { + $continueExitPointHasUnoriginalKeyType = true; + continue; + } + } else { $continueExitPointHasUnoriginalKeyType = true; continue; } @@ -1327,58 +1349,82 @@ public function processStmtNode( count($breakExitPoints) === 0 && count($scopesWithIterableValueType) > 0 && !$continueExitPointHasUnoriginalKeyType - && $stmt->keyVar !== null + && ($stmt->keyVar !== null || $byRefWithoutKey) && (!$hasExpr->no() || !$stmt->expr instanceof Variable) && $exprType->isArray()->yes() && $exprType->isConstantArray()->no() ) { - $arrayExprDimFetch = new ArrayDimFetch($stmt->expr, $stmt->keyVar); - $originalValueExpr = null; - if ($stmt->valueVar instanceof Variable && is_string($stmt->valueVar->name)) { - $originalValueExpr = new OriginalForeachValueExpr($stmt->valueVar->name); - } - $arrayDimFetchLoopTypes = []; - $keyLoopTypes = []; - foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) { - $dimFetchType = $scopeWithIterableValueType->getType($arrayExprDimFetch); - // Condition-based narrowings like `is_string($type)` apply to the value - // variable but not automatically to the array dim fetch, even though the - // two describe the same element for a given iteration. If the value var - // hasn't been reassigned (OriginalForeachValueExpr still tracked) we use - // the narrowed value-var type in place of the broader dim fetch type so - // the loop's final array rewrite below picks up the sharper element type. - if ($originalValueExpr !== null && $scopeWithIterableValueType->hasExpressionType($originalValueExpr)->yes()) { - $valueVarType = $scopeWithIterableValueType->getType($stmt->valueVar); - if ($dimFetchType->isSuperTypeOf($valueVarType)->yes()) { - $dimFetchType = $valueVarType; - } + $nativeExprType = $scope->getNativeType($stmt->expr); + $arrayDimFetchLoopType = $exprType->getIterableValueType(); + $arrayDimFetchLoopNativeType = $nativeExprType->getIterableValueType(); + $keyLoopType = $exprType->getIterableKeyType(); + $keyLoopNativeType = $nativeExprType->getIterableKeyType(); + $valueTypeChanged = false; + $keyTypeChanged = false; + + if ($byRefWithoutKey) { + $arrayDimFetchLoopTypes = []; + foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) { + $arrayDimFetchLoopTypes[] = $scopeWithIterableValueType->getType($stmt->valueVar); } - $arrayDimFetchLoopTypes[] = $dimFetchType; - $keyLoopTypes[] = $scopeWithIterableValueType->getType($stmt->keyVar); - } + $arrayDimFetchLoopType = TypeCombinator::union(...$arrayDimFetchLoopTypes); - $arrayDimFetchLoopType = TypeCombinator::union(...$arrayDimFetchLoopTypes); - $keyLoopType = TypeCombinator::union(...$keyLoopTypes); + $arrayDimFetchLoopNativeTypes = []; + foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) { + $arrayDimFetchLoopNativeTypes[] = $scopeWithIterableValueType->getNativeType($stmt->valueVar); + } + $arrayDimFetchLoopNativeType = TypeCombinator::union(...$arrayDimFetchLoopNativeTypes); + + $valueTypeChanged = !$arrayDimFetchLoopType->equals($exprType->getIterableValueType()); + } elseif ($stmt->keyVar !== null) { + $arrayExprDimFetch = new ArrayDimFetch($stmt->expr, $stmt->keyVar); + $originalValueExpr = null; + if ($stmt->valueVar instanceof Variable && is_string($stmt->valueVar->name)) { + $originalValueExpr = new OriginalForeachValueExpr($stmt->valueVar->name); + } + $arrayDimFetchLoopTypes = []; + $keyLoopTypes = []; + foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) { + $dimFetchType = $scopeWithIterableValueType->getType($arrayExprDimFetch); + // Condition-based narrowings like `is_string($type)` apply to the value + // variable but not automatically to the array dim fetch, even though the + // two describe the same element for a given iteration. If the value var + // hasn't been reassigned (OriginalForeachValueExpr still tracked) we use + // the narrowed value-var type in place of the broader dim fetch type so + // the loop's final array rewrite below picks up the sharper element type. + if ($originalValueExpr !== null && $scopeWithIterableValueType->hasExpressionType($originalValueExpr)->yes()) { + $valueVarType = $scopeWithIterableValueType->getType($stmt->valueVar); + if ($dimFetchType->isSuperTypeOf($valueVarType)->yes()) { + $dimFetchType = $valueVarType; + } + } + $arrayDimFetchLoopTypes[] = $dimFetchType; + $keyLoopTypes[] = $scopeWithIterableValueType->getType($stmt->keyVar); + } - $arrayDimFetchLoopNativeTypes = []; - $keyLoopNativeTypes = []; - foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) { - $dimFetchNativeType = $scopeWithIterableValueType->getNativeType($arrayExprDimFetch); - if ($originalValueExpr !== null && $scopeWithIterableValueType->hasExpressionType($originalValueExpr)->yes()) { - $valueVarNativeType = $scopeWithIterableValueType->getNativeType($stmt->valueVar); - if ($dimFetchNativeType->isSuperTypeOf($valueVarNativeType)->yes()) { - $dimFetchNativeType = $valueVarNativeType; + $arrayDimFetchLoopType = TypeCombinator::union(...$arrayDimFetchLoopTypes); + $keyLoopType = TypeCombinator::union(...$keyLoopTypes); + + $arrayDimFetchLoopNativeTypes = []; + $keyLoopNativeTypes = []; + foreach ($scopesWithIterableValueType as $scopeWithIterableValueType) { + $dimFetchNativeType = $scopeWithIterableValueType->getNativeType($arrayExprDimFetch); + if ($originalValueExpr !== null && $scopeWithIterableValueType->hasExpressionType($originalValueExpr)->yes()) { + $valueVarNativeType = $scopeWithIterableValueType->getNativeType($stmt->valueVar); + if ($dimFetchNativeType->isSuperTypeOf($valueVarNativeType)->yes()) { + $dimFetchNativeType = $valueVarNativeType; + } } + $arrayDimFetchLoopNativeTypes[] = $dimFetchNativeType; + $keyLoopNativeTypes[] = $scopeWithIterableValueType->getType($stmt->keyVar); } - $arrayDimFetchLoopNativeTypes[] = $dimFetchNativeType; - $keyLoopNativeTypes[] = $scopeWithIterableValueType->getType($stmt->keyVar); - } - $arrayDimFetchLoopNativeType = TypeCombinator::union(...$arrayDimFetchLoopNativeTypes); - $keyLoopNativeType = TypeCombinator::union(...$keyLoopNativeTypes); + $arrayDimFetchLoopNativeType = TypeCombinator::union(...$arrayDimFetchLoopNativeTypes); + $keyLoopNativeType = TypeCombinator::union(...$keyLoopNativeTypes); - $valueTypeChanged = !$arrayDimFetchLoopType->equals($exprType->getIterableValueType()); - $keyTypeChanged = !$keyLoopType->equals($exprType->getIterableKeyType()); + $valueTypeChanged = !$arrayDimFetchLoopType->equals($exprType->getIterableValueType()); + $keyTypeChanged = !$keyLoopType->equals($exprType->getIterableKeyType()); + } if ($valueTypeChanged || $keyTypeChanged) { $newExprType = TypeTraverser::map($exprType, static function (Type $type, callable $traverse) use ($arrayDimFetchLoopType, $keyLoopType, $valueTypeChanged, $keyTypeChanged): Type { @@ -1395,7 +1441,6 @@ public function processStmtNode( $valueTypeChanged ? $arrayDimFetchLoopType : $type->getIterableValueType(), ); }); - $nativeExprType = $scope->getNativeType($stmt->expr); $newExprNativeType = TypeTraverser::map($nativeExprType, static function (Type $type, callable $traverse) use ($arrayDimFetchLoopNativeType, $keyLoopNativeType, $valueTypeChanged, $keyTypeChanged): Type { if ($type instanceof UnionType || $type instanceof IntersectionType) { return $traverse($type); diff --git a/tests/PHPStan/Analyser/nsrt/bug-13809.php b/tests/PHPStan/Analyser/nsrt/bug-13809.php index 9cdb9d5fa5e..b91c64f7fdc 100644 --- a/tests/PHPStan/Analyser/nsrt/bug-13809.php +++ b/tests/PHPStan/Analyser/nsrt/bug-13809.php @@ -13,7 +13,7 @@ function foo(array $list): void $value = 'foo'; } - assertType('list', $list); + assertType("list<'foo'>", $list); } /** @@ -56,7 +56,7 @@ function bar3(array $list): void } } - assertType("list", $list); // could be list<'foo'|'maybe'> + assertType("list<'foo'|'maybe'>", $list); } /** @@ -68,7 +68,7 @@ function baz(array $list): void $value = 'bar'; } - assertType('list', $list); + assertType("list<'bar'>", $list); } /** diff --git a/tests/PHPStan/Analyser/nsrt/bug-14083.php b/tests/PHPStan/Analyser/nsrt/bug-14083.php index 8da92d43d36..14fcbef77a3 100644 --- a/tests/PHPStan/Analyser/nsrt/bug-14083.php +++ b/tests/PHPStan/Analyser/nsrt/bug-14083.php @@ -13,7 +13,7 @@ function example(array $convert): void { foreach ($convert as &$item) { $item = strtoupper($item); } - assertType('list', $convert); + assertType('list', $convert); } /** diff --git a/tests/PHPStan/Analyser/nsrt/bug-14084.php b/tests/PHPStan/Analyser/nsrt/bug-14084.php index 3f044aa559a..006510dea47 100644 --- a/tests/PHPStan/Analyser/nsrt/bug-14084.php +++ b/tests/PHPStan/Analyser/nsrt/bug-14084.php @@ -16,7 +16,7 @@ function example(array $convert): void $val = strtoupper($val); // https://github.com/phpstan/phpstan/issues/14083 } } - assertType('array>', $convert); + assertType('array>', $convert); } /** @@ -29,7 +29,7 @@ function example2(array $convert): void $inner[$key] = strtoupper($val); } } - assertType('array>', $convert); + assertType('array>', $convert); } /** diff --git a/tests/PHPStan/Analyser/nsrt/bug-1940-with-key.php b/tests/PHPStan/Analyser/nsrt/bug-1940-with-key.php new file mode 100644 index 00000000000..0ba6d79cfc9 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-1940-with-key.php @@ -0,0 +1,59 @@ + $a + */ +function byRefWithKeyModifySubElement(array $a): void +{ + foreach ($a as $k => &$testArray) { + $testArray['two'] = 'two'; + } + + assertType("list", $a); +} + +/** + * By-ref WITHOUT key, modifying sub-element. + * Parallel case to the reviewer's example but without key variable. + * @param list $a + */ +function byRefWithoutKeyModifySubElement(array $a): void +{ + foreach ($a as &$testArray) { + $testArray['two'] = 'two'; + } + + assertType("list", $a); +} + +/** + * By-ref with key, direct overwrite (already worked before this PR) + * @param array $arr + */ +function byRefWithKeyDirectOverwrite(array $arr): void +{ + foreach ($arr as $k => &$v) { + $v = 1; + } + + assertType('array', $arr); +} + +/** + * By-ref without key, direct overwrite (this PR's main fix) + * @param array $arr + */ +function byRefWithoutKeyDirectOverwrite(array $arr): void +{ + foreach ($arr as &$v) { + $v = 1; + } + + assertType('array', $arr); +} diff --git a/tests/PHPStan/Analyser/nsrt/bug-1940.php b/tests/PHPStan/Analyser/nsrt/bug-1940.php new file mode 100644 index 00000000000..67a17e38fed --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-1940.php @@ -0,0 +1,193 @@ + $arr + */ +function byRefWithoutKey(array $arr): void +{ + foreach ($arr as &$v) { + $v = 1; + } + + assertType('array', $arr); +} + +/** + * @param array $arr + */ +function byRefWithoutKeyConditional(array $arr): void +{ + foreach ($arr as &$v) { + if (rand(0, 1)) { + $v = 1; + } + } + + assertType('array', $arr); +} + +/** + * @param array $arr + */ +function byRefWithoutKeyAlwaysOverwritten(array $arr): void +{ + foreach ($arr as &$v) { + if (rand(0, 1)) { + $v = 1; + } else { + $v = 2; + } + } + + assertType('array', $arr); +} + +/** + * @param list $arr + */ +function byRefWithoutKeyList(array $arr): void +{ + foreach ($arr as &$v) { + $v = 'replaced'; + } + + assertType("list<'replaced'>", $arr); +} + +/** + * @param array $arr + */ +function byRefWithoutKeyStringKeys(array $arr): void +{ + foreach ($arr as &$v) { + $v = 'hello'; + } + + assertType("array", $arr); +} + +/** + * @param non-empty-array $arr + */ +function byRefWithoutKeyNonEmpty(array $arr): void +{ + foreach ($arr as &$v) { + $v = 42; + } + + assertType('non-empty-array', $arr); +} + +/** + * By-ref without key with break — should NOT rewrite since not all elements may be visited. + * @param array $arr + */ +function byRefWithoutKeyBreak(array $arr): void +{ + foreach ($arr as &$v) { + $v = 1; + if (rand(0, 1)) { + break; + } + } + + assertType('array', $arr); +} + +/** + * By-ref without key with continue — should still rewrite since all elements are visited. + * @param array $arr + */ +function byRefWithoutKeyContinue(array $arr): void +{ + foreach ($arr as &$v) { + $v = 1; + if (rand(0, 1)) { + continue; + } + } + + assertType('array', $arr); +} + +/** + * By-ref without key with continue where value is overwritten in all branches. + * @param array $arr + */ +function byRefWithoutKeyContinueBranches(array $arr): void +{ + foreach ($arr as &$v) { + if (rand(0, 1)) { + $v = 1; + continue; + } + $v = 2; + } + + assertType('array', $arr); +} + +/** + * By-ref without key with continue where value is NOT overwritten in the continue branch. + * @param array $arr + */ +function byRefWithoutKeyContinuePartial(array $arr): void +{ + foreach ($arr as &$v) { + if (rand(0, 1)) { + continue; + } + $v = 1; + } + + assertType('array', $arr); +} + +class Foo +{ + /** @var array */ + private array $prop; + + /** + * By-ref without key on a property. + */ + public function byRefWithoutKeyProperty(): void + { + foreach ($this->prop as &$v) { + $v = 'hello'; + } + + assertType("array", $this->prop); + } +} + +/** + * By-ref without key with intval transformation. + * @param array $arr + */ +function byRefWithoutKeyTransform(array $arr): void +{ + foreach ($arr as &$v) { + $v = intval($v); + } + + assertType('array', $arr); +} + +/** + * By-ref without key — value not overwritten at all. + * @param array $arr + */ +function byRefWithoutKeyNoOverwrite(array $arr): void +{ + foreach ($arr as &$v) { + // just read, don't write + echo $v; + } + + assertType('array', $arr); +} diff --git a/tests/PHPStan/Analyser/nsrt/overwritten-arrays.php b/tests/PHPStan/Analyser/nsrt/overwritten-arrays.php index 878ae9d082c..1ec5dd8b9ec 100644 --- a/tests/PHPStan/Analyser/nsrt/overwritten-arrays.php +++ b/tests/PHPStan/Analyser/nsrt/overwritten-arrays.php @@ -127,7 +127,7 @@ public function doFoo7(array $a): void $v = 1; } - assertType('array', $a); // could be array + assertType('array', $a); } /** diff --git a/tests/PHPStan/Rules/Variables/data/bug-14124.php b/tests/PHPStan/Rules/Variables/data/bug-14124.php index be3ad1a8ded..3a7303fb85b 100644 --- a/tests/PHPStan/Rules/Variables/data/bug-14124.php +++ b/tests/PHPStan/Rules/Variables/data/bug-14124.php @@ -15,7 +15,7 @@ function example3a(array &$convert): void $val = strtoupper($val); } } - assertType('array>', $convert); + assertType('array>', $convert); } /** diff --git a/tests/PHPStan/Rules/Variables/data/bug-14124b.php b/tests/PHPStan/Rules/Variables/data/bug-14124b.php index 0ad84eb7a3e..822e9f9ac68 100644 --- a/tests/PHPStan/Rules/Variables/data/bug-14124b.php +++ b/tests/PHPStan/Rules/Variables/data/bug-14124b.php @@ -17,7 +17,7 @@ function example3a(array &$convert): void } } } - assertType('array>>', $convert); + assertType('array>>', $convert); } /**