From ac86981a796d4aeb39a660db228fd7dfd6fb9f68 Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Mon, 20 Feb 2023 23:04:40 -0400 Subject: [PATCH 1/3] Revert "#7387 Add asserting non-empty-string by strlen" This reverts commit 0ef7ec100a365fc8855047ca5ced4a9bc554e059. --- .../Statements/Expression/AssertionFinder.php | 335 +----------------- tests/TypeReconciliation/EmptyTest.php | 32 +- 2 files changed, 31 insertions(+), 336 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php index 352382720b0..cd3d910a936 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php @@ -444,12 +444,8 @@ private static function scrapeEqualityAssertions( ); } - - $count = null; $count_equality_position = self::hasCountEqualityCheck($conditional, $count); - $strlen = null; - $strlen_equality_position = self::hasStrlenEqualityCheck($conditional, $strlen); if ($count_equality_position) { $if_types = []; @@ -500,58 +496,6 @@ private static function scrapeEqualityAssertions( return $if_types ? [$if_types] : []; } - if ($strlen_equality_position) { - $if_types = []; - - if ($strlen_equality_position === self::ASSIGNMENT_TO_RIGHT) { - $strlen_expr = $conditional->left; - } elseif ($strlen_equality_position === self::ASSIGNMENT_TO_LEFT) { - $strlen_expr = $conditional->right; - } else { - throw new UnexpectedValueException('$strlen_equality_position value'); - } - - /** @var PhpParser\Node\Expr\FuncCall $strlen_expr */ - if (!isset($strlen_expr->getArgs()[0])) { - return []; - } - $var_name = ExpressionIdentifier::getExtendedVarId( - $strlen_expr->getArgs()[0]->value, - $this_class_name, - $source, - ); - - if ($source instanceof StatementsAnalyzer) { - $var_type = $source->node_data->getType($conditional->left); - $other_type = $source->node_data->getType($conditional->right); - - if ($codebase - && $other_type - && $var_type - && $conditional instanceof PhpParser\Node\Expr\BinaryOp\Identical - ) { - self::handleParadoxicalAssertions( - $source, - $var_type, - $this_class_name, - $other_type, - $codebase, - $conditional, - ); - } - } - - if ($var_name) { - if ($strlen > 0) { - $if_types[$var_name] = [[new Assertion\NonEmpty()]]; - } else { - $if_types[$var_name] = [[new Empty_()]]; - } - } - - return $if_types ? [$if_types] : []; - } - if (!$source instanceof StatementsAnalyzer) { return []; } @@ -724,9 +668,6 @@ private static function scrapeInequalityAssertions( $count = null; $count_inequality_position = self::hasCountEqualityCheck($conditional, $count); - $strlen = null; - $strlen_inequality_position = self::hasStrlenEqualityCheck($conditional, $strlen); - if ($count_inequality_position) { $if_types = []; @@ -776,58 +717,6 @@ private static function scrapeInequalityAssertions( return $if_types ? [$if_types] : []; } - if ($strlen_inequality_position) { - $if_types = []; - - if ($strlen_inequality_position === self::ASSIGNMENT_TO_RIGHT) { - $strlen_expr = $conditional->left; - } elseif ($strlen_inequality_position === self::ASSIGNMENT_TO_LEFT) { - $strlen_expr = $conditional->right; - } else { - throw new UnexpectedValueException('$strlen_inequality_position value'); - } - - /** @var PhpParser\Node\Expr\FuncCall $strlen_expr */ - if (!isset($strlen_expr->getArgs()[0])) { - return []; - } - $var_name = ExpressionIdentifier::getExtendedVarId( - $strlen_expr->getArgs()[0]->value, - $this_class_name, - $source, - ); - - if ($source instanceof StatementsAnalyzer) { - $var_type = $source->node_data->getType($conditional->left); - $other_type = $source->node_data->getType($conditional->right); - - if ($codebase - && $other_type - && $var_type - && $conditional instanceof PhpParser\Node\Expr\BinaryOp\NotIdentical - ) { - self::handleParadoxicalAssertions( - $source, - $var_type, - $this_class_name, - $other_type, - $codebase, - $conditional, - ); - } - } - - if ($var_name) { - if ($strlen > 0) { - $if_types[$var_name] = [[new Assertion\Empty_()]]; - } else { - $if_types[$var_name] = [[new Assertion\NonEmpty()]]; - } - } - - return $if_types ? [$if_types] : []; - } - if (!$source instanceof StatementsAnalyzer) { return []; } @@ -1650,34 +1539,10 @@ protected static function hasGetClassCheck( protected static function hasNonEmptyCountEqualityCheck( PhpParser\Node\Expr\BinaryOp $conditional, ?int &$min_count - ) { - return self::hasNonEmptyCountOrStrlenEqualityCheck($conditional, $min_count, ['count']); - } - - /** - * @param Greater|GreaterOrEqual|Smaller|SmallerOrEqual $conditional - * @return false|int - */ - protected static function hasNonEmptyStrlenEqualityCheck( - PhpParser\Node\Expr\BinaryOp $conditional, - ?int &$min_count - ) { - return self::hasNonEmptyCountOrStrlenEqualityCheck($conditional, $min_count, ['strlen', 'mb_strlen']); - } - - /** - * @param Greater|GreaterOrEqual|Smaller|SmallerOrEqual $conditional - * @param array $funcCallNames - * @return false|int - */ - protected static function hasNonEmptyCountOrStrlenEqualityCheck( - PhpParser\Node\Expr\BinaryOp $conditional, - ?int &$min_value, - array $funcCallNames ) { if ($conditional->left instanceof PhpParser\Node\Expr\FuncCall && $conditional->left->name instanceof PhpParser\Node\Name - && in_array(strtolower($conditional->left->name->parts[0]), $funcCallNames) + && strtolower($conditional->left->name->parts[0]) === 'count' && $conditional->left->getArgs() && ($conditional instanceof BinaryOp\Greater || $conditional instanceof BinaryOp\GreaterOrEqual) ) { @@ -1686,7 +1551,7 @@ protected static function hasNonEmptyCountOrStrlenEqualityCheck( $comparison_adjustment = $conditional instanceof BinaryOp\Greater ? 1 : 0; } elseif ($conditional->right instanceof PhpParser\Node\Expr\FuncCall && $conditional->right->name instanceof PhpParser\Node\Name - && in_array(strtolower($conditional->right->name->parts[0]), $funcCallNames) + && strtolower($conditional->right->name->parts[0]) === 'count' && $conditional->right->getArgs() && ($conditional instanceof BinaryOp\Smaller || $conditional instanceof BinaryOp\SmallerOrEqual) ) { @@ -1701,7 +1566,7 @@ protected static function hasNonEmptyCountOrStrlenEqualityCheck( if ($compare_to instanceof PhpParser\Node\Scalar\LNumber && $compare_to->value > (-1 * $comparison_adjustment) ) { - $min_value = $compare_to->value + $comparison_adjustment; + $min_count = $compare_to->value + $comparison_adjustment; return $assignment_to; } @@ -1716,34 +1581,10 @@ protected static function hasNonEmptyCountOrStrlenEqualityCheck( protected static function hasLessThanCountEqualityCheck( PhpParser\Node\Expr\BinaryOp $conditional, ?int &$max_count - ) { - return self::hasLessThanCountOrStrlenEqualityCheck($conditional, $max_count, ['count']); - } - - /** - * @param Greater|GreaterOrEqual|Smaller|SmallerOrEqual $conditional - * @return false|int - */ - protected static function hasLessThanStrlenEqualityCheck( - PhpParser\Node\Expr\BinaryOp $conditional, - ?int &$max_strlen - ) { - return self::hasLessThanCountOrStrlenEqualityCheck($conditional, $max_strlen, ['strlen', 'mb_strlen']); - } - - /** - * @param Greater|GreaterOrEqual|Smaller|SmallerOrEqual $conditional - * @param array $funcCallNames - * @return false|int - */ - protected static function hasLessThanCountOrStrlenEqualityCheck( - PhpParser\Node\Expr\BinaryOp $conditional, - ?int &$max_value, - array $funcCallNames ) { $left_count = $conditional->left instanceof PhpParser\Node\Expr\FuncCall && $conditional->left->name instanceof PhpParser\Node\Name - && in_array(strtolower($conditional->left->name->parts[0]), $funcCallNames) + && strtolower($conditional->left->name->parts[0]) === 'count' && $conditional->left->getArgs(); $operator_less_than_or_equal = @@ -1754,7 +1595,7 @@ protected static function hasLessThanCountOrStrlenEqualityCheck( && $operator_less_than_or_equal && $conditional->right instanceof PhpParser\Node\Scalar\LNumber ) { - $max_value = $conditional->right->value - + $max_count = $conditional->right->value - ($conditional instanceof PhpParser\Node\Expr\BinaryOp\Smaller ? 1 : 0); return self::ASSIGNMENT_TO_RIGHT; @@ -1762,7 +1603,7 @@ protected static function hasLessThanCountOrStrlenEqualityCheck( $right_count = $conditional->right instanceof PhpParser\Node\Expr\FuncCall && $conditional->right->name instanceof PhpParser\Node\Name - && in_array(strtolower($conditional->right->name->parts[0]), $funcCallNames) + && strtolower($conditional->right->name->parts[0]) === 'count' && $conditional->right->getArgs(); $operator_greater_than_or_equal = @@ -1773,7 +1614,7 @@ protected static function hasLessThanCountOrStrlenEqualityCheck( && $operator_greater_than_or_equal && $conditional->left instanceof PhpParser\Node\Scalar\LNumber ) { - $max_value = $conditional->left->value - + $max_count = $conditional->left->value - ($conditional instanceof PhpParser\Node\Expr\BinaryOp\Greater ? 1 : 0); return self::ASSIGNMENT_TO_LEFT; @@ -1789,49 +1630,25 @@ protected static function hasLessThanCountOrStrlenEqualityCheck( protected static function hasCountEqualityCheck( PhpParser\Node\Expr\BinaryOp $conditional, ?int &$count - ) { - return self::hasCountOrStrlenEqualityCheck($conditional, $count, ['count']); - } - - /** - * @param Equal|Identical|NotEqual|NotIdentical $conditional - * @return false|int - */ - protected static function hasStrlenEqualityCheck( - PhpParser\Node\Expr\BinaryOp $conditional, - ?int &$count - ) { - return self::hasCountOrStrlenEqualityCheck($conditional, $count, ['strlen', 'mb_strlen']); - } - - /** - * @param Equal|Identical|NotEqual|NotIdentical $conditional - * @param array $funcCallNames - * @return false|int - */ - protected static function hasCountOrStrlenEqualityCheck( - PhpParser\Node\Expr\BinaryOp $conditional, - ?int &$resultValue, - array $funcCallNames ) { $left_count = $conditional->left instanceof PhpParser\Node\Expr\FuncCall && $conditional->left->name instanceof PhpParser\Node\Name - && in_array(strtolower($conditional->left->name->parts[0]), $funcCallNames) + && strtolower($conditional->left->name->parts[0]) === 'count' && $conditional->left->getArgs(); if ($left_count && $conditional->right instanceof PhpParser\Node\Scalar\LNumber) { - $resultValue = $conditional->right->value; + $count = $conditional->right->value; return self::ASSIGNMENT_TO_RIGHT; } $right_count = $conditional->right instanceof PhpParser\Node\Expr\FuncCall && $conditional->right->name instanceof PhpParser\Node\Name - && in_array(strtolower($conditional->right->name->parts[0]), $funcCallNames) + && strtolower($conditional->right->name->parts[0]) === 'count' && $conditional->right->getArgs(); if ($right_count && $conditional->left instanceof PhpParser\Node\Scalar\LNumber) { - $resultValue = $conditional->left->value; + $count = $conditional->left->value; return self::ASSIGNMENT_TO_LEFT; } @@ -1966,37 +1783,15 @@ protected static function hasInferiorNumberCheck( protected static function hasReconcilableNonEmptyCountEqualityCheck( PhpParser\Node\Expr\BinaryOp $conditional ) { - return self::hasReconcilableNonEmptyCountOrStrlenEqualityCheck($conditional, ['count']); - } - - /** - * @param PhpParser\Node\Expr\BinaryOp\Greater|PhpParser\Node\Expr\BinaryOp\GreaterOrEqual $conditional - * @return false|int - */ - protected static function hasReconcilableNonEmptyStrlenEqualityCheck( - PhpParser\Node\Expr\BinaryOp $conditional - ) { - return self::hasReconcilableNonEmptyCountOrStrlenEqualityCheck($conditional, ['strlen', 'mb_strlen']); - } - - /** - * @param PhpParser\Node\Expr\BinaryOp\Greater|PhpParser\Node\Expr\BinaryOp\GreaterOrEqual $conditional - * @param array $funcCallNames - * @return false|int - */ - protected static function hasReconcilableNonEmptyCountOrStrlenEqualityCheck( - PhpParser\Node\Expr\BinaryOp $conditional, - array $funcCallNames - ) { - $left_value = $conditional->left instanceof PhpParser\Node\Expr\FuncCall + $left_count = $conditional->left instanceof PhpParser\Node\Expr\FuncCall && $conditional->left->name instanceof PhpParser\Node\Name - && in_array(strtolower($conditional->left->name->parts[0]), $funcCallNames); + && strtolower($conditional->left->name->parts[0]) === 'count'; $right_number = $conditional->right instanceof PhpParser\Node\Scalar\LNumber && $conditional->right->value === ( - $conditional instanceof PhpParser\Node\Expr\BinaryOp\Greater ? 0 : 1); + $conditional instanceof PhpParser\Node\Expr\BinaryOp\Greater ? 0 : 1); - if ($left_value && $right_number) { + if ($left_count && $right_number) { return self::ASSIGNMENT_TO_RIGHT; } @@ -4002,8 +3797,6 @@ private static function getGreaterAssertions( $conditional, $superior_value_comparison, ); - $min_strlen = null; - $strlen_equality_position = self::hasNonEmptyStrlenEqualityCheck($conditional, $min_strlen); if ($count_equality_position) { if ($count_equality_position === self::ASSIGNMENT_TO_RIGHT) { @@ -4034,42 +3827,6 @@ private static function getGreaterAssertions( return $if_types ? [$if_types] : []; } - if ($strlen_equality_position) { - if ($strlen_equality_position === self::ASSIGNMENT_TO_RIGHT) { - $strlened_expr = $conditional->left; - } else { - throw new UnexpectedValueException('$strlen_equality_position value'); - } - - /** @var PhpParser\Node\Expr\FuncCall $strlened_expr */ - if (!isset($strlened_expr->getArgs()[0])) { - return []; - } - $var_name = ExpressionIdentifier::getExtendedVarId( - $strlened_expr->getArgs()[0]->value, - $this_class_name, - $source, - ); - - if ($var_name) { - if (self::hasReconcilableNonEmptyStrlenEqualityCheck($conditional)) { - $if_types[$var_name] = [[new Assertion\NonEmpty()]]; - } else { - if ($min_strlen > 0) { - $if_types[$var_name] = [[new Assertion\NonEmpty()]]; - } else { - $if_types[$var_name] = [[new Empty_()]]; - } - } - } - - if ($var_name) { - $if_types[$var_name] = [[new Assertion\NonEmpty()]]; - } - - return $if_types ? [$if_types] : []; - } - if ($count_inequality_position) { if ($count_inequality_position === self::ASSIGNMENT_TO_LEFT) { $count_expr = $conditional->right; @@ -4146,12 +3903,6 @@ private static function getSmallerAssertions( $count_equality_position = self::hasNonEmptyCountEqualityCheck($conditional, $min_count); $max_count = null; $count_inequality_position = self::hasLessThanCountEqualityCheck($conditional, $max_count); - - $min_strlen = null; - $strlen_equality_position = self::hasNonEmptyStrlenEqualityCheck($conditional, $min_strlen); - $max_strlen = null; - $strlen_inequality_position = self::hasLessThanStrlenEqualityCheck($conditional, $max_strlen); - $inferior_value_comparison = null; $inferior_value_position = self::hasInferiorNumberCheck( $source, @@ -4209,62 +3960,6 @@ private static function getSmallerAssertions( return $if_types ? [$if_types] : []; } - if ($strlen_equality_position) { - if ($strlen_equality_position === self::ASSIGNMENT_TO_LEFT) { - $strlen_expr = $conditional->right; - } else { - throw new UnexpectedValueException('$strlen_equality_position value'); - } - - /** @var PhpParser\Node\Expr\FuncCall $strlen_expr */ - if (!isset($strlen_expr->getArgs()[0])) { - return []; - } - $var_name = ExpressionIdentifier::getExtendedVarId( - $strlen_expr->getArgs()[0]->value, - $this_class_name, - $source, - ); - - if ($var_name) { - if ($min_strlen > 0) { - $if_types[$var_name] = [[new Assertion\NonEmpty()]]; - } else { - $if_types[$var_name] = [[new Empty_()]]; - } - } - - return $if_types ? [$if_types] : []; - } - - if ($strlen_inequality_position) { - if ($strlen_inequality_position === self::ASSIGNMENT_TO_RIGHT) { - $strlen_expr = $conditional->left; - } else { - throw new UnexpectedValueException('$strlen_inequality_position value'); - } - - /** @var PhpParser\Node\Expr\FuncCall $strlen_expr */ - if (!isset($strlen_expr->getArgs()[0])) { - return []; - } - $var_name = ExpressionIdentifier::getExtendedVarId( - $strlen_expr->getArgs()[0]->value, - $this_class_name, - $source, - ); - - if ($var_name) { - if ($max_strlen > 0) { - $if_types[$var_name] = [[new Assertion\NonEmpty()]]; - } else { - $if_types[$var_name] = [[new Empty_()]]; - } - } - - return $if_types ? [$if_types] : []; - } - if ($inferior_value_position) { if ($inferior_value_position === self::ASSIGNMENT_TO_RIGHT) { $var_name = ExpressionIdentifier::getExtendedVarId( diff --git a/tests/TypeReconciliation/EmptyTest.php b/tests/TypeReconciliation/EmptyTest.php index ac95e53a131..628d13b3652 100644 --- a/tests/TypeReconciliation/EmptyTest.php +++ b/tests/TypeReconciliation/EmptyTest.php @@ -414,63 +414,63 @@ function foo(array $arr): void { if (empty($arr["a"])) {} }', ], - 'strlenWithGreaterZero' => [ + 'SKIPPED-strlenWithGreaterZero' => [ 'code' => ' 0 ? $str : "string"; }', ], - 'strlenRighthandWithGreaterZero' => [ + 'SKIPPED-strlenRighthandWithGreaterZero' => [ 'code' => ' [ + 'SKIPPED-strlenWithGreaterEqualsOne' => [ 'code' => '= 1 ? $str : "string"; }', ], - 'strlenRighthandWithGreaterEqualsOne' => [ + 'SKIPPED-strlenRighthandWithGreaterEqualsOne' => [ 'code' => ' [ + 'SKIPPED-strlenWithInequalZero' => [ 'code' => ' [ + 'SKIPPED-strlenRighthandWithInequalZero' => [ 'code' => ' [ + 'SKIPPED-strlenWithEqualOne' => [ 'code' => ' [ + 'SKIPPED-strlenRighthandWithEqualOne' => [ 'code' => ' [ + 'SKIPPED-mb_strlen' => [ 'code' => ' 'MixedReturnTypeCoercion', ], - 'preventStrlenGreaterMinusOne' => [ + 'SKIPPED-preventStrlenGreaterMinusOne' => [ 'code' => ' 'LessSpecificReturnStatement', ], - 'preventRighthandStrlenGreaterMinusOne' => [ + 'SKIPPED-preventRighthandStrlenGreaterMinusOne' => [ 'code' => ' 'LessSpecificReturnStatement', ], - 'preventStrlenGreaterEqualsZero' => [ + 'SKIPPED-preventStrlenGreaterEqualsZero' => [ 'code' => ' 'LessSpecificReturnStatement', ], - 'preventStrlenEqualsZero' => [ + 'SKIPPED-preventStrlenEqualsZero' => [ 'code' => ' 'InvalidReturnStatement', ], - 'preventStrlenLessThanOne' => [ + 'SKIPPED-preventStrlenLessThanOne' => [ 'code' => ' 'InvalidReturnStatement', ], - 'preventStrlenLessEqualsZero' => [ + 'SKIPPED-preventStrlenLessEqualsZero' => [ 'code' => ' 'InvalidReturnStatement', ], - 'preventStrlenWithConcatenatedString' => [ + 'SKIPPED-preventStrlenWithConcatenatedString' => [ 'code' => ' Date: Mon, 20 Feb 2023 23:13:03 -0400 Subject: [PATCH 2/3] Update baseline --- psalm-baseline.xml | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 66d23cbb8e1..c6faeb97b8d 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,5 +1,5 @@ - + tags['variablesfrom'][0]]]> @@ -238,12 +238,6 @@ $identifier_name - - - $input_path[2] - $input_path[2] - - $trait From 01c19d94ba09d6e06be5bb56638ab04f6c1c8787 Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Mon, 20 Feb 2023 23:45:01 -0400 Subject: [PATCH 3/3] Added a bunch of tests from referenced issues --- tests/TypeReconciliation/EmptyTest.php | 106 +++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/tests/TypeReconciliation/EmptyTest.php b/tests/TypeReconciliation/EmptyTest.php index 628d13b3652..c7991804454 100644 --- a/tests/TypeReconciliation/EmptyTest.php +++ b/tests/TypeReconciliation/EmptyTest.php @@ -506,6 +506,112 @@ function nonEmptyString(string $str): string { ', 'assertions' => ['$arr===' => 'list'], ], + 'issue-9205-1' => [ + 'code' => <<<'PHP' + [ + '$lastLabel===' => 'string', + ], + ], + 'issue-9205-2' => [ + 'code' => <<<'PHP' + 0) { + exit; + } + PHP, + 'assertions' => [ + '$x===' => 'string', // perhaps this should be improved in future + ], + ], + 'issue-9205-3' => [ + 'code' => <<<'PHP' + [ + '$x===' => 'string', // can't be improved really + ], + ], + 'issue-9205-4' => [ + 'code' => <<<'PHP' + [ + '$x===' => 'string', // can be improved + ], + ], + 'issue-9349' => [ + 'code' => <<<'PHP' + [ + '$str===' => 'non-falsy-string', // can't be improved + ], + ], + 'issue-9349-2' => [ + 'code' => <<<'PHP' + [ + 'code' => <<<'PHP' + [ + '$a===' => 'string', // can't be improved + ], + ], + 'issue-9341-1' => [ + 'code' => <<<'PHP' + 2) + { + exit; + } + PHP, + 'assertions' => [ + '$GLOBALS[\'sql_query\']===' => 'string', + ], + ], ]; }