From 20539b33bf0c33e4aa724fe5816015ace4c9cce3 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Wed, 13 Apr 2022 16:29:50 +0200 Subject: [PATCH] Fix `array_push` with an empty constant array --- src/Analyser/NodeScopeResolver.php | 125 +++++++++--------- .../Analyser/NodeScopeResolverTest.php | 2 + tests/PHPStan/Analyser/data/array-push.php | 58 ++++++++ tests/PHPStan/Analyser/data/array-unshift.php | 58 ++++++++ .../PHPStan/Rules/Variables/EmptyRuleTest.php | 15 +++ .../PHPStan/Rules/Variables/data/bug-6974.php | 32 +++++ 6 files changed, 230 insertions(+), 60 deletions(-) create mode 100644 tests/PHPStan/Analyser/data/array-push.php create mode 100644 tests/PHPStan/Analyser/data/array-unshift.php create mode 100644 tests/PHPStan/Rules/Variables/data/bug-6974.php diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 46280842f5a..9ced27d43a1 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -6,6 +6,7 @@ use Closure; use PhpParser\Comment\Doc; use PhpParser\Node; +use PhpParser\Node\Arg; use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\ArrayDimFetch; @@ -109,16 +110,15 @@ use PHPStan\Reflection\ReflectionProvider; use PHPStan\ShouldNotHappenException; use PHPStan\TrinaryLogic; -use PHPStan\Type\Accessory\NonEmptyArrayType; use PHPStan\Type\ArrayType; use PHPStan\Type\ClosureType; use PHPStan\Type\Constant\ConstantArrayType; use PHPStan\Type\Constant\ConstantArrayTypeBuilder; use PHPStan\Type\Constant\ConstantBooleanType; -use PHPStan\Type\Constant\ConstantIntegerType; use PHPStan\Type\Constant\ConstantStringType; use PHPStan\Type\ErrorType; use PHPStan\Type\FileTypeMapper; +use PHPStan\Type\GeneralizePrecision; use PHPStan\Type\Generic\GenericClassStringType; use PHPStan\Type\Generic\TemplateTypeHelper; use PHPStan\Type\Generic\TemplateTypeMap; @@ -1843,75 +1843,80 @@ function (MutatingScope $scope) use ($expr, $nodeCallback, $context): Expression && in_array($functionReflection->getName(), ['array_push', 'array_unshift'], true) && count($expr->getArgs()) >= 2 ) { - $argumentTypes = []; - foreach (array_slice($expr->getArgs(), 1) as $callArg) { - $callArgType = $scope->getType($callArg->value); - if ($callArg->unpack) { - $iterableValueType = $callArgType->getIterableValueType(); - if ($iterableValueType instanceof UnionType) { - foreach ($iterableValueType->getTypes() as $innerType) { - $argumentTypes[] = $innerType; + $arrayArg = $expr->getArgs()[0]->value; + $arrayType = $scope->getType($arrayArg); + $callArgs = array_slice($expr->getArgs(), 1); + + /** + * @param Arg[] $callArgs + * @param callable(?Type, Type, bool=): void $setOffsetValueType + */ + $setOffsetValueTypes = static function (Scope $scope, array $callArgs, callable $setOffsetValueType, ?bool &$nonConstantArrayWasUnpacked = null): void { + foreach ($callArgs as $callArg) { + $callArgType = $scope->getType($callArg->value); + if ($callArg->unpack) { + if ($callArgType->isIterableAtLeastOnce()->no()) { + continue; } - } else { - $argumentTypes[] = $iterableValueType; + if (!$callArgType instanceof ConstantArrayType) { + $nonConstantArrayWasUnpacked = true; + } + $iterableValueType = $callArgType->getIterableValueType(); + $isOptional = !$callArgType->isIterableAtLeastOnce()->yes(); + if ($iterableValueType instanceof UnionType) { + foreach ($iterableValueType->getTypes() as $innerType) { + $setOffsetValueType(null, $innerType, $isOptional); + } + } else { + $setOffsetValueType(null, $iterableValueType, $isOptional); + } + continue; } - continue; + $setOffsetValueType(null, $callArgType); } + }; - $argumentTypes[] = $callArgType; - } + if ($arrayType instanceof ConstantArrayType) { + $prepend = $functionReflection->getName() === 'array_unshift'; + $arrayTypeBuilder = $prepend ? ConstantArrayTypeBuilder::createEmpty() : ConstantArrayTypeBuilder::createFromConstantArray($arrayType); - $arrayArg = $expr->getArgs()[0]->value; - $originalArrayType = $scope->getType($arrayArg); - $constantArrays = TypeUtils::getOldConstantArrays($originalArrayType); - if ( - $functionReflection->getName() === 'array_push' - || ($originalArrayType->isArray()->yes() && count($constantArrays) === 0) - ) { - $arrayType = $originalArrayType; - foreach ($argumentTypes as $argType) { - $arrayType = $arrayType->setOffsetValueType(null, $argType); - } - - $scope = $scope->invalidateExpression($arrayArg)->specifyExpressionType($arrayArg, TypeCombinator::intersect($arrayType, new NonEmptyArrayType())); - } elseif (count($constantArrays) > 0) { - $defaultArrayBuilder = ConstantArrayTypeBuilder::createEmpty(); - foreach ($argumentTypes as $argType) { - $defaultArrayBuilder->setOffsetValueType(null, $argType); - } + $setOffsetValueTypes( + $scope, + $callArgs, + static function (?Type $offsetType, Type $valueType, bool $optional = false) use (&$arrayTypeBuilder): void { + $arrayTypeBuilder->setOffsetValueType($offsetType, $valueType, $optional); + }, + $nonConstantArrayWasUnpacked, + ); - $defaultArrayType = $defaultArrayBuilder->getArray(); - if (!$defaultArrayType instanceof ConstantArrayType) { - $arrayType = $originalArrayType; - foreach ($argumentTypes as $argType) { - $arrayType = $arrayType->setOffsetValueType(null, $argType); + if ($prepend) { + $keyTypes = $arrayType->getKeyTypes(); + $valueTypes = $arrayType->getValueTypes(); + foreach ($keyTypes as $k => $keyType) { + $arrayTypeBuilder->setOffsetValueType( + $keyType instanceof ConstantStringType ? $keyType : null, + $valueTypes[$k], + $arrayType->isOptionalKey($k), + ); } + } - $scope = $scope->invalidateExpression($arrayArg)->specifyExpressionType($arrayArg, TypeCombinator::intersect($arrayType, new NonEmptyArrayType())); - } else { - $arrayTypes = []; - foreach ($constantArrays as $constantArray) { - $arrayTypeBuilder = ConstantArrayTypeBuilder::createFromConstantArray($defaultArrayType); - foreach ($constantArray->getKeyTypes() as $i => $keyType) { - $valueType = $constantArray->getValueTypes()[$i]; - if ($keyType instanceof ConstantIntegerType) { - $keyType = null; - } - $arrayTypeBuilder->setOffsetValueType( - $keyType, - $valueType, - $constantArray->isOptionalKey($i), - ); - } - $arrayTypes[] = $arrayTypeBuilder->getArray(); - } + $arrayType = $arrayTypeBuilder->getArray(); - $scope = $scope->invalidateExpression($arrayArg)->specifyExpressionType( - $arrayArg, - TypeCombinator::union(...$arrayTypes), - ); + if ($arrayType instanceof ConstantArrayType && $nonConstantArrayWasUnpacked) { + $arrayType = $arrayType->generalize(GeneralizePrecision::lessSpecific()); } + } else { + $setOffsetValueTypes( + $scope, + $callArgs, + static function (?Type $offsetType, Type $valueType) use (&$arrayType): void { + $arrayType = $arrayType->setOffsetValueType($offsetType, $valueType); + }, + ); } + + $scope = $scope->invalidateExpression($arrayArg)->specifyExpressionType($arrayArg, $arrayType); } if ( diff --git a/tests/PHPStan/Analyser/NodeScopeResolverTest.php b/tests/PHPStan/Analyser/NodeScopeResolverTest.php index 12b63a834d1..2cc864effa7 100644 --- a/tests/PHPStan/Analyser/NodeScopeResolverTest.php +++ b/tests/PHPStan/Analyser/NodeScopeResolverTest.php @@ -561,6 +561,7 @@ public function dataFileAsserts(): iterable yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-1870.php'); yield from $this->gatherAssertTypes(__DIR__ . '/../Rules/Methods/data/bug-5562.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-5615.php'); + yield from $this->gatherAssertTypes(__DIR__ . '/data/array-unshift.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/array_map_multiple.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/range-numeric-string.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/missing-closure-native-return-typehint.php'); @@ -838,6 +839,7 @@ public function dataFileAsserts(): iterable yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6439.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6748.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/array-search-type-specifying.php'); + yield from $this->gatherAssertTypes(__DIR__ . '/data/array-push.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/array-replace.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6889.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6891.php'); diff --git a/tests/PHPStan/Analyser/data/array-push.php b/tests/PHPStan/Analyser/data/array-push.php new file mode 100644 index 00000000000..88049fbd8f6 --- /dev/null +++ b/tests/PHPStan/Analyser/data/array-push.php @@ -0,0 +1,58 @@ + $c + * @param array $d + */ +function arrayPush(array $a, array $b, array $c, array $d): void +{ + array_push($a, ...$b); + assertType('non-empty-array', $a); + + array_push($b, ...[]); + assertType('array', $b); + + array_push($c, ...[19, 'baz', false]); + assertType('non-empty-array<\'baz\'|int|false>', $c); + + /** @var array $d1 */ + $d1 = []; + array_push($d, ...$d1); + assertType('non-empty-array', $d); +} + +function arrayPushConstantArray(): void +{ + $a = ['foo' => 17, 'a', 'bar' => 18,]; + array_push($a, ...[19, 'baz', false]); + assertType('array{foo: 17, 0: \'a\', bar: 18, 1: 19, 2: \'baz\', 3: false}', $a); + + $b = ['foo' => 17, 'a', 'bar' => 18]; + array_push($b, 19, 'baz', false); + assertType('array{foo: 17, 0: \'a\', bar: 18, 1: 19, 2: \'baz\', 3: false}', $b); + + $c = ['foo' => 17, 'a', 'bar' => 18]; + array_push($c, ...[]); + assertType('array{foo: 17, 0: \'a\', bar: 18}', $c); + + $d = []; + array_push($d, ...[]); + assertType('array{}', $d); + + $e = []; + array_push($e, 19, 'baz', false); + assertType('array{19, \'baz\', false}', $e); + + $f = [17]; + /** @var array $f1 */ + $f1 = []; + array_push($f, ...$f1); + assertType('non-empty-array', $f); +} diff --git a/tests/PHPStan/Analyser/data/array-unshift.php b/tests/PHPStan/Analyser/data/array-unshift.php new file mode 100644 index 00000000000..6c768d3cf44 --- /dev/null +++ b/tests/PHPStan/Analyser/data/array-unshift.php @@ -0,0 +1,58 @@ + $c + * @param array $d + */ +function arrayUnshift(array $a, array $b, array $c, array $d): void +{ + array_unshift($a, ...$b); + assertType('non-empty-array', $a); + + array_unshift($b, ...[]); + assertType('array', $b); + + array_unshift($c, ...[19, 'baz', false]); + assertType('non-empty-array<\'baz\'|int|false>', $c); + + /** @var array $d1 */ + $d1 = []; + array_unshift($d, ...$d1); + assertType('non-empty-array', $d); +} + +function arrayUnshiftConstantArray(): void +{ + $a = ['foo' => 17, 'a', 'bar' => 18,]; + array_unshift($a, ...[19, 'baz', false]); + assertType('array{0: 19, 1: \'baz\', 2: false, foo: 17, 3: \'a\', bar: 18}', $a); + + $b = ['foo' => 17, 'a', 'bar' => 18]; + array_unshift($b, 19, 'baz', false); + assertType('array{0: 19, 1: \'baz\', 2: false, foo: 17, 3: \'a\', bar: 18}', $b); + + $c = ['foo' => 17, 'a', 'bar' => 18]; + array_unshift($c, ...[]); + assertType('array{foo: 17, 0: \'a\', bar: 18}', $c); + + $d = []; + array_unshift($d, ...[]); + assertType('array{}', $d); + + $e = []; + array_unshift($e, 19, 'baz', false); + assertType('array{19, \'baz\', false}', $e); + + $f = [17]; + /** @var array $f1 */ + $f1 = []; + array_unshift($f, ...$f1); + assertType('non-empty-array', $f); +} diff --git a/tests/PHPStan/Rules/Variables/EmptyRuleTest.php b/tests/PHPStan/Rules/Variables/EmptyRuleTest.php index 4fcb08548b3..e638b921cab 100644 --- a/tests/PHPStan/Rules/Variables/EmptyRuleTest.php +++ b/tests/PHPStan/Rules/Variables/EmptyRuleTest.php @@ -81,4 +81,19 @@ public function testBug970(): void ]); } + public function testBug6974(): void + { + $this->treatPhpDocTypesAsCertain = false; + $this->analyse([__DIR__ . '/data/bug-6974.php'], [ + [ + 'Variable $a in empty() always exists and is always falsy.', + 12, + ], + [ + 'Variable $a in empty() always exists and is not falsy.', + 30, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Variables/data/bug-6974.php b/tests/PHPStan/Rules/Variables/data/bug-6974.php new file mode 100644 index 00000000000..d7e0341dc7f --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-6974.php @@ -0,0 +1,32 @@ + $b */ + $b = []; + array_push($a, ...$b); + $c = empty($a) ? 'empty' : 'non-empty'; + } +}