diff --git a/src/Analyser/ArrayByRefItemSlots.php b/src/Analyser/ArrayByRefItemSlots.php new file mode 100644 index 0000000000..9ca302ddb0 --- /dev/null +++ b/src/Analyser/ArrayByRefItemSlots.php @@ -0,0 +1,92 @@ + pairs of [referenced expression, slot expression] + */ + public static function resolve(Scope $scope, Expr\Array_ $array, Expr $rootExpr): array + { + $slots = []; + self::collect($scope, $array, $rootExpr, $slots); + + return $slots; + } + + /** + * @param list $slots + * @param-out list $slots + */ + private static function collect(Scope $scope, Expr\Array_ $array, Expr $parentExpr, array &$slots): void + { + $implicitIndex = 0; + foreach ($array->items as $arrayItem) { + if ($arrayItem->unpack) { + // The unpacked array shifts every subsequent implicit index by an + // unknown amount, and its own items cannot be referenced from here. + $implicitIndex = null; + continue; + } + + if ($arrayItem->key !== null) { + $keyType = $scope->getType($arrayItem->key)->toArrayKey(); + + if ($implicitIndex !== null) { + $keyValues = $keyType->getConstantScalarValues(); + if (count($keyValues) === 1) { + $keyValue = $keyValues[0]; + if (is_int($keyValue) && $keyValue >= $implicitIndex) { + $implicitIndex = $keyValue + 1; + } + } elseif (!$keyType->isInteger()->no()) { + // Key could be an integer, but we don't know which one, + // so subsequent implicit indices are unpredictable + $implicitIndex = null; + } + } + + $dimExpr = $arrayItem->key; + } elseif ($implicitIndex !== null) { + $dimExpr = new Node\Scalar\Int_($implicitIndex); + $implicitIndex++; + } else { + $dimExpr = new TypeExpr(new IntegerType()); + } + + $dimFetchExpr = new ArrayDimFetch($parentExpr, $dimExpr); + + if ($arrayItem->value instanceof Expr\Array_) { + self::collect($scope, $arrayItem->value, $dimFetchExpr, $slots); + continue; + } + + if (!$arrayItem->byRef) { + continue; + } + + $slots[] = [$arrayItem->value, $dimFetchExpr]; + } + } + +} diff --git a/src/Analyser/ExprHandler/AssignHandler.php b/src/Analyser/ExprHandler/AssignHandler.php index 4c52ec6abe..7bc39da9ed 100644 --- a/src/Analyser/ExprHandler/AssignHandler.php +++ b/src/Analyser/ExprHandler/AssignHandler.php @@ -21,6 +21,7 @@ use PhpParser\Node\Expr\Variable; use PhpParser\Node\Name; use PhpParser\Node\Stmt; +use PHPStan\Analyser\ArrayByRefItemSlots; use PHPStan\Analyser\AssignTargetWalkMode; use PHPStan\Analyser\ConditionalExpressionHolder; use PHPStan\Analyser\ExpressionContext; @@ -68,7 +69,6 @@ use PHPStan\Type\ConstantTypeHelper; use PHPStan\Type\ErrorType; use PHPStan\Type\IntegerRangeType; -use PHPStan\Type\IntegerType; use PHPStan\Type\MixedType; use PHPStan\Type\NeverType; use PHPStan\Type\NullType; @@ -86,7 +86,6 @@ use function array_slice; use function count; use function in_array; -use function is_int; use function is_string; /** @@ -1695,44 +1694,12 @@ private function isImplicitArrayCreation(array $dimFetchStack, Scope $scope): Tr private function processArrayByRefItems(MutatingScope $scope, string $rootVarName, Expr\Array_ $arrayExpr, Expr $parentExpr): MutatingScope { - $implicitIndex = 0; - foreach ($arrayExpr->items as $arrayItem) { - if ($arrayItem->key !== null) { - $keyType = $scope->getType($arrayItem->key)->toArrayKey(); - - if ($implicitIndex !== null) { - $keyValues = $keyType->getConstantScalarValues(); - if (count($keyValues) === 1) { - $keyValue = $keyValues[0]; - if (is_int($keyValue) && $keyValue >= $implicitIndex) { - $implicitIndex = $keyValue + 1; - } - } elseif (!$keyType->isInteger()->no()) { - // Key could be an integer, but we don't know which one, - // so subsequent implicit indices are unpredictable - $implicitIndex = null; - } - } - - $dimExpr = $arrayItem->key; - } elseif ($implicitIndex !== null) { - $dimExpr = new Node\Scalar\Int_($implicitIndex); - $implicitIndex++; - } else { - $dimExpr = new TypeExpr(new IntegerType()); - } - - if ($arrayItem->value instanceof Expr\Array_) { - $dimFetchExpr = new ArrayDimFetch($parentExpr, $dimExpr); - $scope = $this->processArrayByRefItems($scope, $rootVarName, $arrayItem->value, $dimFetchExpr); - } - - if (!$arrayItem->byRef || !$arrayItem->value instanceof Variable || !is_string($arrayItem->value->name)) { + foreach (ArrayByRefItemSlots::resolve($scope, $arrayExpr, $parentExpr) as [$referencedExpr, $dimFetchExpr]) { + if (!$referencedExpr instanceof Variable || !is_string($referencedExpr->name)) { continue; } - $refVarName = $arrayItem->value->name; - $dimFetchExpr = new ArrayDimFetch($parentExpr, $dimExpr); + $refVarName = $referencedExpr->name; $refType = $scope->getType(new Variable($refVarName)); $refNativeType = $scope->getNativeType(new Variable($refVarName)); diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 59e7d30a0d..31e2c263da 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -2880,6 +2880,45 @@ public function isUndefinedExpressionAllowed(Expr $expr): bool return array_key_exists($exprString, $this->currentlyAllowedUndefinedExpressions); } + /** + * Types of the expressions aliased by `&` array items (`$array = [&$v]`) of $variableName, + * resolved from $arrayType as if that was the array's new value. The slot keeps aliasing + * the referenced expression even after the array is copied, so a write into the slot is + * a write into the referenced expression. + * + * @return list pairs of [referenced expression, slot type] + */ + public function resolveByRefArrayItemTypes(string $variableName, Type $arrayType): array + { + $itemTypes = []; + foreach ($this->expressionTypes as $expressionType) { + $expr = $expressionType->getExpr(); + if (!$expr instanceof IntertwinedVariableByReferenceWithExpr) { + continue; + } + if (!$expressionType->getCertainty()->yes()) { + continue; + } + if ($expr->getVariableName() !== $variableName) { + continue; + } + $assignedExpr = $expr->getAssignedExpr(); + if ( + !$assignedExpr instanceof Expr\ArrayDimFetch + || ScopeOps::getIntertwinedRefRootVariableName($assignedExpr) !== $variableName + ) { + continue; + } + + $itemTypes[] = [ + $expr->getExpr(), + $this->resolveIntertwinedAssignedType($this, $arrayType, $assignedExpr, $variableName, false), + ]; + } + + return $itemTypes; + } + /** * @param list $intertwinedPropagatedFrom */ diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 56dc083900..a5b24cf02f 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -2326,6 +2326,37 @@ public function processArgs( } } + foreach ($args as $i => $arg) { + if ($arg->unpack) { + // spread elements land on parameters this loop cannot map, and PHP + // does not carry the reference through the unpacking anyway + continue; + } + + $currentParameter = null; + if ($writebackParameters !== null) { + if (isset($writebackParameters[$i])) { + $currentParameter = $writebackParameters[$i]; + } elseif (count($writebackParameters) > 0 && $writebackAcceptor->isVariadic()) { + $currentParameter = array_last($writebackParameters); + } + } + + if ($currentParameter !== null && $currentParameter->passedByReference()->createsNewVariable()) { + // the by-reference writeback above already propagates through the slots + continue; + } + + $scope = $this->processByRefArrayItemsPassedByValue( + $scope, + $storage, + $stmt, + $arg->value, + $currentParameter !== null ? $currentParameter->getType() : new MixedType(), + $nodeCallback, + ); + } + // not storing this, it's scope after processing all args return new ArgsResult( $this->expressionResultFactory->create($scope, $scope, $callLike, $hasYield, $isAlwaysTerminating, $throwPoints, $impurePoints), @@ -2612,6 +2643,88 @@ private function getParameterOutExtensionsType(CallLike $callLike, $calleeReflec return null; } + /** + * A `&$v` item in an array literal keeps aliasing $v after the array is copied into + * the callee, so anything the callee writes into that slot lands in $v. The same holds + * for an array variable built with by-reference items and only then passed to the call. + * + * @param callable(Node $node, Scope $scope): void $nodeCallback + */ + private function processByRefArrayItemsPassedByValue( + MutatingScope $scope, + ExpressionResultStorage $storage, + Node\Stmt $stmt, + Expr $argValue, + Type $parameterType, + callable $nodeCallback, + ): MutatingScope + { + foreach ($this->resolveByRefArrayItemTypes($scope, $argValue, $parameterType) as [$referencedExpr, $slotType]) { + if ($referencedExpr instanceof Variable && $referencedExpr->name === 'this') { + continue; + } + + if ($scope->hasExpressionType($referencedExpr)->yes()) { + // the callee does not have to write into the slot at all + $slotType = TypeCombinator::union($scope->getType($referencedExpr), $slotType); + } + + $scope = $this->processVirtualAssign( + $scope, + $storage, + $stmt, + $referencedExpr, + new TypeExpr($slotType), + $nodeCallback, + )->getScope(); + } + + return $scope; + } + + /** + * By-reference array item slots of $argValue, each with the type the callee can + * write through the reference - the slot read off $parameterType. + * + * @return list pairs of [referenced expression, slot type] + */ + private function resolveByRefArrayItemTypes(MutatingScope $scope, Expr $argValue, Type $parameterType): array + { + if ($argValue instanceof Expr\Array_) { + // rooting the slot expressions at the parameter type resolves the offsets + // through the usual dim fetch reading + $slots = []; + foreach (ArrayByRefItemSlots::resolve($scope, $argValue, new TypeExpr($parameterType)) as [$referencedExpr, $slotExpr]) { + if (!$this->isByRefArrayItemWritable($referencedExpr)) { + continue; + } + + $slots[] = [$referencedExpr, $scope->getType($slotExpr)]; + } + + return $slots; + } + + if ($argValue instanceof Variable && is_string($argValue->name)) { + // the array was built with by-reference items earlier - the slots are + // already recorded in the scope + return $scope->resolveByRefArrayItemTypes($argValue->name, $parameterType); + } + + return []; + } + + private function isByRefArrayItemWritable(Expr $expr): bool + { + if ($expr instanceof Variable) { + return is_string($expr->name); + } + + return $expr instanceof PropertyFetch + || $expr instanceof StaticPropertyFetch + || $expr instanceof ArrayDimFetch; + } + /** * @param callable(Node $node, Scope $scope): void $nodeCallback */ diff --git a/tests/PHPStan/Analyser/nsrt/array-by-ref-item-passed-to-call.php b/tests/PHPStan/Analyser/nsrt/array-by-ref-item-passed-to-call.php new file mode 100644 index 0000000000..05c078fe40 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/array-by-ref-item-passed-to-call.php @@ -0,0 +1,144 @@ +> $a */ +function takesNested(array $a): void {} + +function takesUntypedArray(array $a): void {} + +final class Holder +{ + + public bool $property = false; + + public static bool $staticProperty = false; + + /** @param array{bool} $a */ + public function __construct(array $a = [false]) + { + } + + /** @param array{bool} $a */ + public function method(array $a): void + { + } + + /** @param array{bool} $a */ + public static function staticMethod(array $a): void + { + } + +} + +function funcCall(): void +{ + $retry = false; + takesShape([&$retry]); + assertType('bool', $retry); +} + +function methodCall(Holder $h): void +{ + $retry = false; + $h->method([&$retry]); + assertType('bool', $retry); +} + +function staticCall(): void +{ + $retry = false; + Holder::staticMethod([&$retry]); + assertType('bool', $retry); +} + +function instantiation(): void +{ + $retry = false; + new Holder([&$retry]); + assertType('bool', $retry); +} + +/** @param callable(array{bool}): void $c */ +function closureCall(callable $c): void +{ + $retry = false; + $c([&$retry]); + assertType('bool', $retry); +} + +function stringKey(): void +{ + $retry = false; + takesKeyedShape(['x' => &$retry]); + assertType('bool', $retry); +} + +function nestedArrayLiteral(): void +{ + $retry = false; + takesNested([[&$retry]]); + assertType('bool', $retry); +} + +function propertyByRef(Holder $h): void +{ + if (!$h->property) { + takesShape([&$h->property]); + assertType('bool', $h->property); + } +} + +function staticPropertyByRef(): void +{ + if (!Holder::$staticProperty) { + takesShape([&Holder::$staticProperty]); + assertType('bool', Holder::$staticProperty); + } +} + +function offsetByRef(): void +{ + $arr = ['k' => false]; + takesShape([&$arr['k']]); + assertType('bool', $arr['k']); +} + +function unknownValueType(): void +{ + $retry = false; + takesUntypedArray([&$retry]); + assertType('mixed', $retry); +} + +function arrayVariablePassedToCall(): void +{ + $retry = false; + $args = [&$retry]; + assertType('false', $retry); + takesShape($args); + assertType('bool', $retry); +} + +function localWriteStillPrecise(): void +{ + $retry = false; + $args = [&$retry]; + $args[0] = true; + assertType('true', $retry); +} + +function byRefParameterNotAffected(): void +{ + $retry = false; + takesShape([$retry]); + assertType('false', $retry); +} diff --git a/tests/PHPStan/Analyser/nsrt/bug-14333.php b/tests/PHPStan/Analyser/nsrt/bug-14333.php index a01178586b..cece092dd8 100644 --- a/tests/PHPStan/Analyser/nsrt/bug-14333.php +++ b/tests/PHPStan/Analyser/nsrt/bug-14333.php @@ -195,3 +195,17 @@ function moreTest(bool $bool, int $int) { assertType("1|2|3|4|5|'a0'|'a1'|'a2'|'a3'|'a4'|'a5'", $e); assertType("'aKey'", $f); } + +/** @param array $arr */ +function testImplicitIndexAfterUnpack(array $arr): void +{ + $a = 1; + + $b = [...$arr, &$a]; + assertType('1', $a); + + // the unpacked array is of unknown length, so the byref slot's index is + // unknown too - a write to any int key might have hit it + $b[1] = 'one'; + assertType('1|string', $a); +} diff --git a/tests/PHPStan/Analyser/nsrt/bug-15116.php b/tests/PHPStan/Analyser/nsrt/bug-15116.php new file mode 100644 index 0000000000..1b213bf0f8 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-15116.php @@ -0,0 +1,45 @@ + 1) { + // tell the caller that they can retry (they wanted more cups) + $args[1] = true; + } else { + $args[1] = false; + } + } else { + // for all other types of coffee, make all the cups in the same call + $cupsMade = $args[2]; + $args[1] = false; + } + return $cupsMade; + } +} + +function () { + $retry = false; + $cupsWanted = 10; + $cb = new CoffeeBreak(); + $cupsMade = $cb->makeCoffee(["cappucino", &$retry, $cupsWanted]); + assertType('bool', $retry); + if ($retry) { + $cupsRemaining = $cupsWanted - $cupsMade; + echo "still need $cupsRemaining cups of coffee\n"; + } +};