From 1d32097610c7dd652400ad9806f3a372e1ce2eb3 Mon Sep 17 00:00:00 2001 From: Giovanni Giacobbi Date: Thu, 8 Aug 2024 11:30:46 +0000 Subject: [PATCH 1/6] Fix and improve loose equality type narrowing --- src/Analyser/TypeSpecifier.php | 38 +++++- tests/PHPStan/Analyser/nsrt/equal-narrow.php | 136 +++++++++++++++++++ 2 files changed, 173 insertions(+), 1 deletion(-) create mode 100644 tests/PHPStan/Analyser/nsrt/equal-narrow.php diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 68864c18b6..561682d1d2 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -43,6 +43,7 @@ use PHPStan\Type\Constant\ConstantArrayType; use PHPStan\Type\Constant\ConstantArrayTypeBuilder; use PHPStan\Type\Constant\ConstantBooleanType; +use PHPStan\Type\Constant\ConstantFloatType; use PHPStan\Type\Constant\ConstantIntegerType; use PHPStan\Type\Constant\ConstantStringType; use PHPStan\Type\ConstantScalarType; @@ -1949,7 +1950,20 @@ public function resolveEqual(Expr\BinaryOp\Equal $expr, Scope $scope, TypeSpecif if ($expressions !== null) { $exprNode = $expressions[0]; $constantType = $expressions[1]; - if (!$context->null() && ($constantType->getValue() === false || $constantType->getValue() === null)) { + + if (!$context->null() && $constantType->getValue() === null) { + $trueTypes = [ + new NullType(), + new ConstantBooleanType(false), + new ConstantIntegerType(0), + new ConstantFloatType(0.0), + new ConstantStringType(''), + new ConstantArrayType([], []), + ]; + return $this->create($exprNode, new UnionType($trueTypes), $context, false, $scope, $rootExpr); + } + + if (!$context->null() && $constantType->getValue() === false) { return $this->specifyTypesInCondition( $scope, $exprNode, @@ -1967,6 +1981,28 @@ public function resolveEqual(Expr\BinaryOp\Equal $expr, Scope $scope, TypeSpecif ); } + if (!$context->null() && $constantType->getValue() === '') { + /* There is a difference between php 7.x and 8.x on the equality + * behavior between zero and the empty string, so to be conservative + * we leave it untouched regardless of the language version */ + if ($context->true()) { + $trueTypes = [ + new NullType(), + new ConstantBooleanType(false), + new ConstantIntegerType(0), + new ConstantFloatType(0.0), + new ConstantStringType(''), + ]; + } else { + $trueTypes = [ + new NullType(), + new ConstantBooleanType(false), + new ConstantStringType(''), + ]; + } + return $this->create($exprNode, new UnionType($trueTypes), $context, false, $scope, $rootExpr); + } + if ( $exprNode instanceof FuncCall && $exprNode->name instanceof Name diff --git a/tests/PHPStan/Analyser/nsrt/equal-narrow.php b/tests/PHPStan/Analyser/nsrt/equal-narrow.php new file mode 100644 index 0000000000..5d68f0b1d3 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/equal-narrow.php @@ -0,0 +1,136 @@ +|int<1, max>|non-empty-string", $y); + } + + if ($z == null) { + assertType("0|0.0|''|array{}|false|null", $z); + } else { + assertType("mixed~0|0.0|''|array{}|false|null", $z); + } +} + +/** + * @param 0|0.0|1|''|'0'|'x'|array{}|bool|object|null $x + * @param int|string|null $y + * @param mixed $z + */ +function doFalse($x, $y, $z): void +{ + if ($x == false) { + assertType("0|0.0|''|'0'|array{}|false|null", $x); + } else { + assertType("1|'x'|object|true", $x); + } + if (false != $x) { + assertType("1|'x'|object|true", $x); + } else { + assertType("0|0.0|''|'0'|array{}|false|null", $x); + } + + if ($y == false) { + assertType("0|''|'0'|null", $y); + } else { + assertType("int|int<1, max>|non-falsy-string", $y); + } + + if ($z == false) { + assertType("0|0.0|''|'0'|array{}|false|null", $z); + } else { + assertType("mixed~0|0.0|''|'0'|array{}|false|null", $z); + } +} + +/** + * @param 0|0.0|1|''|'0'|'x'|array{}|bool|object|null $x + * @param int|string|null $y + * @param mixed $z + */ +function doTrue($x, $y, $z): void +{ + if ($x == true) { + assertType("1|'x'|object|true", $x); + } else { + assertType("0|0.0|''|'0'|array{}|false|null", $x); + } + if (true != $x) { + assertType("0|0.0|''|'0'|array{}|false|null", $x); + } else { + assertType("1|'x'|object|true", $x); + } + + if ($y == true) { + assertType("int|int<1, max>|non-falsy-string", $y); + } else { + assertType("0|''|'0'|null", $y); + } + + if ($z == true) { + assertType("mixed~0|0.0|''|'0'|array{}|false|null", $z); + } else { + assertType("0|0.0|''|'0'|array{}|false|null", $z); + } +} + +/** + * @param 0|0.0|1|''|'0'|'x'|array{}|bool|object|null $x + * @param int|string|null $y + * @param mixed $z + */ +function doEmptyString($x, $y, $z): void +{ + // PHP 7.x/8.x compatibility: Keep zero in both cases + if ($x == '') { + assertType("0|0.0|''|false|null", $x); + } else { + assertType("0|0.0|1|'0'|'x'|array{}|object|true", $x); + } + if ('' != $x) { + assertType("0|0.0|1|'0'|'x'|array{}|object|true", $x); + } else { + assertType("0|0.0|''|false|null", $x); + } + + if ($y == '') { + assertType("0|''|null", $y); + } else { + assertType("int|non-empty-string", $y); + } + + if ($z == '') { + assertType("0|0.0|''|false|null", $z); + } else { + assertType("mixed~''|false|null", $z); + } +} From ed081553671764243d9b12d89ab3880c1241a356 Mon Sep 17 00:00:00 2001 From: Giovanni Giacobbi Date: Fri, 9 Aug 2024 10:42:57 +0000 Subject: [PATCH 2/6] FIXUP couple more test cases --- tests/PHPStan/Analyser/nsrt/equal-narrow.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/PHPStan/Analyser/nsrt/equal-narrow.php b/tests/PHPStan/Analyser/nsrt/equal-narrow.php index 5d68f0b1d3..3fe2733876 100644 --- a/tests/PHPStan/Analyser/nsrt/equal-narrow.php +++ b/tests/PHPStan/Analyser/nsrt/equal-narrow.php @@ -59,6 +59,12 @@ function doFalse($x, $y, $z): void assertType("0|0.0|''|'0'|array{}|false|null", $x); } + if (!$x) { + assertType("0|0.0|''|'0'|array{}|false|null", $x); + } else { + assertType("1|'x'|object|true", $x); + } + if ($y == false) { assertType("0|''|'0'|null", $y); } else { @@ -90,6 +96,12 @@ function doTrue($x, $y, $z): void assertType("1|'x'|object|true", $x); } + if ($x) { + assertType("1|'x'|object|true", $x); + } else { + assertType("0|0.0|''|'0'|array{}|false|null", $x); + } + if ($y == true) { assertType("int|int<1, max>|non-falsy-string", $y); } else { From 3ec1bd6eda3702a6031cbad0508390d2b0e9c2d7 Mon Sep 17 00:00:00 2001 From: Giovanni Giacobbi Date: Fri, 15 Nov 2024 19:54:02 +0100 Subject: [PATCH 3/6] FIXUP fixed tests --- tests/PHPStan/Analyser/TypeSpecifierTest.php | 4 ++-- tests/PHPStan/Analyser/nsrt/equal-narrow.php | 8 ++++---- tests/PHPStan/Analyser/nsrt/narrow-cast.php | 2 +- tests/PHPStan/Analyser/nsrt/non-empty-string.php | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/PHPStan/Analyser/TypeSpecifierTest.php b/tests/PHPStan/Analyser/TypeSpecifierTest.php index ced1959085..999c7169a3 100644 --- a/tests/PHPStan/Analyser/TypeSpecifierTest.php +++ b/tests/PHPStan/Analyser/TypeSpecifierTest.php @@ -477,8 +477,8 @@ public function dataCondition(): iterable new Variable('foo'), new Expr\ConstFetch(new Name('null')), ), - ['$foo' => self::SURE_NOT_TRUTHY], - ['$foo' => self::SURE_NOT_FALSEY], + ['$foo' => '0|0.0|\'\'|array{}|false|null'], + ['$foo' => '~0|0.0|\'\'|array{}|false|null'], ], [ new Expr\BinaryOp\Identical( diff --git a/tests/PHPStan/Analyser/nsrt/equal-narrow.php b/tests/PHPStan/Analyser/nsrt/equal-narrow.php index 3fe2733876..659161c933 100644 --- a/tests/PHPStan/Analyser/nsrt/equal-narrow.php +++ b/tests/PHPStan/Analyser/nsrt/equal-narrow.php @@ -37,7 +37,7 @@ function doNull($x, $y, $z): void if ($z == null) { assertType("0|0.0|''|array{}|false|null", $z); } else { - assertType("mixed~0|0.0|''|array{}|false|null", $z); + assertType("mixed~(0|0.0|''|array{}|false|null)", $z); } } @@ -74,7 +74,7 @@ function doFalse($x, $y, $z): void if ($z == false) { assertType("0|0.0|''|'0'|array{}|false|null", $z); } else { - assertType("mixed~0|0.0|''|'0'|array{}|false|null", $z); + assertType("mixed~(0|0.0|''|'0'|array{}|false|null)", $z); } } @@ -109,7 +109,7 @@ function doTrue($x, $y, $z): void } if ($z == true) { - assertType("mixed~0|0.0|''|'0'|array{}|false|null", $z); + assertType("mixed~(0|0.0|''|'0'|array{}|false|null)", $z); } else { assertType("0|0.0|''|'0'|array{}|false|null", $z); } @@ -143,6 +143,6 @@ function doEmptyString($x, $y, $z): void if ($z == '') { assertType("0|0.0|''|false|null", $z); } else { - assertType("mixed~''|false|null", $z); + assertType("mixed~(''|false|null)", $z); } } diff --git a/tests/PHPStan/Analyser/nsrt/narrow-cast.php b/tests/PHPStan/Analyser/nsrt/narrow-cast.php index 82e09e0bd3..1da8858473 100644 --- a/tests/PHPStan/Analyser/nsrt/narrow-cast.php +++ b/tests/PHPStan/Analyser/nsrt/narrow-cast.php @@ -33,7 +33,7 @@ function castString($x, string $s, bool $b) { if ((string) $x) { assertType('int<-5, 5>', $x); } else { - assertType('int<-5, 5>', $x); + assertType('0', $x); } if ((string) $b) { diff --git a/tests/PHPStan/Analyser/nsrt/non-empty-string.php b/tests/PHPStan/Analyser/nsrt/non-empty-string.php index da8426c905..5400b1d31a 100644 --- a/tests/PHPStan/Analyser/nsrt/non-empty-string.php +++ b/tests/PHPStan/Analyser/nsrt/non-empty-string.php @@ -422,8 +422,8 @@ function subtract($m) { assertType('non-falsy-string', (string) $m); } if ($m != '') { - assertType("mixed", $m); - assertType('string', (string) $m); + assertType("mixed~(''|false|null)", $m); + assertType('non-empty-string', (string) $m); } if ($m !== '') { assertType("mixed~''", $m); From f6289bd9d5b28ca8abf922aa50b433a7ac933fcc Mon Sep 17 00:00:00 2001 From: Giovanni Giacobbi Date: Sat, 16 Nov 2024 00:05:04 +0100 Subject: [PATCH 4/6] WIP Attempt to fix zero --- src/Analyser/TypeSpecifier.php | 31 ++++++++++++++++-- tests/PHPStan/Analyser/nsrt/equal-narrow.php | 32 +++++++++++++++++++ .../Analyser/nsrt/integer-range-types.php | 2 +- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 561682d1d2..aa36fac5c8 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -1611,7 +1611,7 @@ private function processBooleanNotSureConditionalTypes(Scope $scope, SpecifiedTy } /** - * @return array{Expr, ConstantScalarType}|null + * @return array{Expr, ConstantScalarType, Type}|null */ private function findTypeExpressionsFromBinaryOperation(Scope $scope, Node\Expr\BinaryOp $binaryOperation): ?array { @@ -1633,13 +1633,13 @@ private function findTypeExpressionsFromBinaryOperation(Scope $scope, Node\Expr\ && !$rightExpr instanceof ConstFetch && !$rightExpr instanceof ClassConstFetch ) { - return [$binaryOperation->right, $leftType]; + return [$binaryOperation->right, $leftType, $rightType]; } elseif ( $rightType instanceof ConstantScalarType && !$leftExpr instanceof ConstFetch && !$leftExpr instanceof ClassConstFetch ) { - return [$binaryOperation->left, $rightType]; + return [$binaryOperation->left, $rightType, $leftType]; } return null; @@ -1950,6 +1950,7 @@ public function resolveEqual(Expr\BinaryOp\Equal $expr, Scope $scope, TypeSpecif if ($expressions !== null) { $exprNode = $expressions[0]; $constantType = $expressions[1]; + $otherType = $expressions[2]; if (!$context->null() && $constantType->getValue() === null) { $trueTypes = [ @@ -1981,6 +1982,30 @@ public function resolveEqual(Expr\BinaryOp\Equal $expr, Scope $scope, TypeSpecif ); } + if (!$context->null() && $constantType->getValue() === 0 && !$otherType->isInteger()->yes()) { + /* There is a difference between php 7.x and 8.x on the equality + * behavior between zero and the empty string, so to be conservative + * we leave it untouched regardless of the language version */ + if ($context->true()) { + $trueTypes = [ + new NullType(), + new ConstantBooleanType(false), + new ConstantIntegerType(0), + new ConstantFloatType(0.0), + new StringType(), + ]; + } else { + $trueTypes = [ + new NullType(), + new ConstantBooleanType(false), + new ConstantIntegerType(0), + new ConstantFloatType(0.0), + new ConstantStringType('0'), + ]; + } + return $this->create($exprNode, new UnionType($trueTypes), $context, false, $scope, $rootExpr); + } + if (!$context->null() && $constantType->getValue() === '') { /* There is a difference between php 7.x and 8.x on the equality * behavior between zero and the empty string, so to be conservative diff --git a/tests/PHPStan/Analyser/nsrt/equal-narrow.php b/tests/PHPStan/Analyser/nsrt/equal-narrow.php index 659161c933..774377f400 100644 --- a/tests/PHPStan/Analyser/nsrt/equal-narrow.php +++ b/tests/PHPStan/Analyser/nsrt/equal-narrow.php @@ -115,6 +115,38 @@ function doTrue($x, $y, $z): void } } +/** + * @param 0|0.0|1|''|'0'|'x'|array{}|bool|object|null $x + * @param int|string|null $y + * @param mixed $z + */ +function doZero($x, $y, $z): void +{ + // PHP 7.x/8.x compatibility: Keep zero in both cases + if ($x == 0) { + assertType("0|0.0|''|'0'|'x'|false|null", $x); + } else { + assertType("1|''|'x'|array{}|object|true", $x); + } + if (0 != $x) { + assertType("1|''|'x'|array{}|object|true", $x); + } else { + assertType("0|0.0|''|'0'|'x'|false|null", $x); + } + + if ($y == 0) { + assertType("0|string|null", $y); + } else { + assertType("int|int<1, max>|string", $y); + } + + if ($z == 0) { + assertType("0|0.0|string|false|null", $z); + } else { + assertType("mixed~(0|0.0|'0'|false|null)", $z); + } +} + /** * @param 0|0.0|1|''|'0'|'x'|array{}|bool|object|null $x * @param int|string|null $y diff --git a/tests/PHPStan/Analyser/nsrt/integer-range-types.php b/tests/PHPStan/Analyser/nsrt/integer-range-types.php index cd35c77953..876a791352 100644 --- a/tests/PHPStan/Analyser/nsrt/integer-range-types.php +++ b/tests/PHPStan/Analyser/nsrt/integer-range-types.php @@ -449,7 +449,7 @@ public function zeroIssues($positive, $negative) function subtract($m) { if ($m != 0) { - assertType("mixed", $m); // could be "mixed~0|0.0|''|'0'|array{}|false|null" + assertType("mixed~(0|0.0|'0'|false|null)", $m); // could be "mixed~(0|0.0|''|'0'|false|null)" assertType('int', (int) $m); } if ($m !== 0) { From 56d954450117a9e90de5c3d613ae691b7eea58c8 Mon Sep 17 00:00:00 2001 From: Giovanni Giacobbi Date: Sat, 16 Nov 2024 11:20:30 +0100 Subject: [PATCH 5/6] WIP More attempts to fix --- src/Analyser/TypeSpecifier.php | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index aa36fac5c8..088236bac5 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -1982,7 +1982,7 @@ public function resolveEqual(Expr\BinaryOp\Equal $expr, Scope $scope, TypeSpecif ); } - if (!$context->null() && $constantType->getValue() === 0 && !$otherType->isInteger()->yes()) { + if (!$context->null() && $constantType->getValue() === 0 && !$otherType->isInteger()->yes() && !$otherType->isBoolean()->yes()) { /* There is a difference between php 7.x and 8.x on the equality * behavior between zero and the empty string, so to be conservative * we leave it untouched regardless of the language version */ @@ -2121,11 +2121,13 @@ public function resolveEqual(Expr\BinaryOp\Equal $expr, Scope $scope, TypeSpecif public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, TypeSpecifierContext $context, ?Expr $rootExpr): SpecifiedTypes { + // Normalize to: fn() === expr $leftExpr = $expr->left; $rightExpr = $expr->right; if ($rightExpr instanceof FuncCall && !$leftExpr instanceof FuncCall) { [$leftExpr, $rightExpr] = [$rightExpr, $leftExpr]; } + $unwrappedLeftExpr = $leftExpr; if ($leftExpr instanceof AlwaysRememberedExpr) { $unwrappedLeftExpr = $leftExpr->getExpr(); @@ -2134,8 +2136,10 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty if ($rightExpr instanceof AlwaysRememberedExpr) { $unwrappedRightExpr = $rightExpr->getExpr(); } + $rightType = $scope->getType($rightExpr); + // (count($a) === $b) if ( !$context->null() && $unwrappedLeftExpr instanceof FuncCall @@ -2200,6 +2204,7 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty } } + // strlen($a) === $b if ( !$context->null() && $unwrappedLeftExpr instanceof FuncCall @@ -2236,6 +2241,7 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty } } + // preg_match($a) === $b if ( $context->true() && $unwrappedLeftExpr instanceof FuncCall @@ -2251,6 +2257,7 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty ); } + // get_class($a) === 'Foo' if ( $context->true() && $unwrappedLeftExpr instanceof FuncCall @@ -2270,6 +2277,7 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty } } + // get_class($a) === 'Foo' if ( $context->truthy() && $unwrappedLeftExpr instanceof FuncCall @@ -2350,6 +2358,7 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty } } + // $a::class === 'Foo' if ( $context->true() && $unwrappedLeftExpr instanceof ClassConstFetch && @@ -2372,6 +2381,8 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty } $leftType = $scope->getType($leftExpr); + + // 'Foo' === $a::class if ( $context->true() && $unwrappedRightExpr instanceof ClassConstFetch && @@ -2417,7 +2428,11 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty $types = null; if ( count($leftType->getFiniteTypes()) === 1 - || ($context->true() && $leftType->isConstantValue()->yes() && !$rightType->equals($leftType) && $rightType->isSuperTypeOf($leftType)->yes()) + || ( + $context->true() + && $leftType->isConstantValue()->yes() + && !$rightType->equals($leftType) + && $rightType->isSuperTypeOf($leftType)->yes()) ) { $types = $this->create( $rightExpr, @@ -2440,7 +2455,12 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty } if ( count($rightType->getFiniteTypes()) === 1 - || ($context->true() && $rightType->isConstantValue()->yes() && !$leftType->equals($rightType) && $leftType->isSuperTypeOf($rightType)->yes()) + || ( + $context->true() + && $rightType->isConstantValue()->yes() + && !$leftType->equals($rightType) + && $leftType->isSuperTypeOf($rightType)->yes() + ) ) { $leftTypes = $this->create( $leftExpr, From 2273dd042dd7ccaa2653c1b06ed2aa14181549b9 Mon Sep 17 00:00:00 2001 From: Giovanni Giacobbi Date: Sat, 16 Nov 2024 11:47:18 +0100 Subject: [PATCH 6/6] WIP another fix --- .../Rules/Comparison/ElseIfConstantConditionRuleTest.php | 4 ++++ tests/PHPStan/Rules/Comparison/data/bug-11674.php | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/PHPStan/Rules/Comparison/ElseIfConstantConditionRuleTest.php b/tests/PHPStan/Rules/Comparison/ElseIfConstantConditionRuleTest.php index bf67146684..6c51033309 100644 --- a/tests/PHPStan/Rules/Comparison/ElseIfConstantConditionRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ElseIfConstantConditionRuleTest.php @@ -135,6 +135,10 @@ public function testBug11674(): void 'Elseif condition is always false.', 28, ], + [ + 'Elseif condition is always false.', + 36, + ], ]); } diff --git a/tests/PHPStan/Rules/Comparison/data/bug-11674.php b/tests/PHPStan/Rules/Comparison/data/bug-11674.php index 7af6660da4..6156b8e1cb 100644 --- a/tests/PHPStan/Rules/Comparison/data/bug-11674.php +++ b/tests/PHPStan/Rules/Comparison/data/bug-11674.php @@ -10,7 +10,7 @@ function show() : void { if ((int) $this->param) { echo 1; } elseif ($this->param) { - echo 2; + echo 2; // might be "0" } } @@ -18,7 +18,7 @@ function show2() : void { if ((float) $this->param) { echo 1; } elseif ($this->param) { - echo 2; + echo 2; // might be "0" } } @@ -26,7 +26,7 @@ function show3() : void { if ((bool) $this->param) { echo 1; } elseif ($this->param) { - echo 2; + echo 2; // not possible } } @@ -34,7 +34,7 @@ function show4() : void { if ((string) $this->param) { echo 1; } elseif ($this->param) { - echo 2; + echo 2; // not possible } } }