From 1f2db5f31ca0921db004103b167b79e81b8dd61b Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Thu, 1 Jun 2023 01:05:46 +0200 Subject: [PATCH 1/4] Allow if/false assert for same variable to allow array/list distinction Fix https://github.com/vimeo/psalm/issues/9037 --- src/Psalm/Internal/Algebra.php | 58 +++ .../Statements/Expression/AssertionFinder.php | 71 +++- tests/AssertAnnotationTest.php | 342 ++++++++++++++++++ 3 files changed, 465 insertions(+), 6 deletions(-) diff --git a/src/Psalm/Internal/Algebra.php b/src/Psalm/Internal/Algebra.php index 0f581fc850f..e9bd6fae8ab 100644 --- a/src/Psalm/Internal/Algebra.php +++ b/src/Psalm/Internal/Algebra.php @@ -5,6 +5,9 @@ use Psalm\Exception\ComplicatedExpressionException; use Psalm\Storage\Assertion; use UnexpectedValueException; +use Psalm\Type\Atomic\TArray; +use Psalm\Type\Atomic\TKeyedArray; +use Psalm\Type\Atomic\TList; use function array_filter; use function array_intersect_key; @@ -388,6 +391,61 @@ public static function getTruthsFromFormula( } } + foreach ($truths as $var => $anded_types) { + $has_list_or_array = false; + foreach ($anded_types as $orred_types) { + foreach ($orred_types as $assertion) { + if ($assertion->isNegation()) { + continue; + } + + if (!isset($assertion->type)) { + continue; + } + + if ($assertion->type instanceof TList + || $assertion->type instanceof TArray + || $assertion->type instanceof TKeyedArray) { + $has_list_or_array = true; + // list/array are collapsed, therefore there can only be 1 and we can abort + // otherwise we would have to remove them all individually e.g. array cannot be array + break 2; + } + } + } + + if ($has_list_or_array === false) { + continue; + } + + $is_array = $is_list = $has_list_or_array; + foreach ($anded_types as $key => $orred_types) { + foreach ($orred_types as $index => $assertion) { + // we only need to check negations + // due to type collapsing, any negations for arrays are irrelevant + if (!$assertion->isNegation()) { + continue; + } + + if (!isset($assertion->type)) { + continue; + } + + if ($assertion->type instanceof TList + || $assertion->type instanceof TArray + || $assertion->type instanceof TKeyedArray) { + unset($truths[$var][$key][$index]); + } + } + + if ($truths[$var][$key] === []) { + unset($truths[$var][$key]); + } else { + $truths[$var][$key] = array_values($truths[$var][$key]); + } + } + } + return $truths; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php index 3396deae0e1..83407f2205c 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php @@ -2325,15 +2325,43 @@ private static function getTrueInequalityAssertions( } if (count($notif_types) === 1) { - $notif_types = $notif_types[0]; + $notif_type = $notif_types[0]; - if (count($notif_types) === 1) { - $if_types = Algebra::negateTypes($notif_types); + if (count($notif_type) === 1) { + $if_types = Algebra::negateTypes($notif_type); } } $if_types = $if_types ? [$if_types] : []; + if ($if_types === [] && count($notif_types) === 2) { + foreach ($notif_types as $notif_type) { + foreach ($notif_type as $var => $assertions) { + if (count($assertions) !== 1 || count($assertions[0]) !== 1) { + $if_types = []; + break 2; + } + + $is_not_assertion = $assertions[0][0] instanceof IsNotType ? true : false; + if (!isset($check_var)) { + $check_var_assertion = $is_not_assertion; + $check_var = $var; + continue; + } + + // only if we have 1 IsType and 1 IsNotType assertion for same variable + if ($check_var !== $var + || !isset($check_var_assertion) + || $check_var_assertion === $is_not_assertion) { + $if_types = []; + break 2; + } + } + + $if_types[] = Algebra::negateTypes($notif_type); + } + } + if ($codebase && $source instanceof StatementsAnalyzer && ($var_type = $source->node_data->getType($base_conditional)) @@ -3022,15 +3050,46 @@ private static function getFalseEqualityAssertions( } if (count($notif_types) === 1) { - $notif_types = $notif_types[0]; + $notif_type = $notif_types[0]; - if (count($notif_types) === 1) { - $if_types = Algebra::negateTypes($notif_types); + if (count($notif_type) === 1) { + $if_types = Algebra::negateTypes($notif_type); } } $if_types = $if_types ? [$if_types] : []; + // @psalm-assert-if-true and @psalm-assert-if-false for same variable in same function, e.g. array/list cases + // @todo optionally extend this to arbitrary number of assert-if cases of multiple variables in the function + // same code above too + if ($if_types === [] && count($notif_types) === 2) { + foreach ($notif_types as $notif_type) { + foreach ($notif_type as $var => $assertions) { + if (count($assertions) !== 1 || count($assertions[0]) !== 1) { + $if_types = []; + break 2; + } + + $is_not_assertion = $assertions[0][0] instanceof IsNotType ? true : false; + if (!isset($check_var)) { + $check_var_assertion = $is_not_assertion; + $check_var = $var; + continue; + } + + // only if we have 1 IsType and 1 IsNotType assertion for same variable + if ($check_var !== $var + || !isset($check_var_assertion) + || $check_var_assertion === $is_not_assertion) { + $if_types = []; + break 2; + } + } + + $if_types[] = Algebra::negateTypes($notif_type); + } + } + if ($codebase && $source instanceof StatementsAnalyzer && ($var_type = $source->node_data->getType($base_conditional)) diff --git a/tests/AssertAnnotationTest.php b/tests/AssertAnnotationTest.php index 249051b1174..e8fa215d212 100644 --- a/tests/AssertAnnotationTest.php +++ b/tests/AssertAnnotationTest.php @@ -2511,6 +2511,348 @@ function testLeftMethodTernary(Either $either): void $either->isLeft() ? testLeft($either) : testRight($either); }', ], + 'assertArrayListIfTrueFalseCompareTrue' => [ + 'code' => '|list $arg + * @return bool + * + * @psalm-assert-if-false array $arg + * @psalm-assert-if-true list $arg + */ + function is_array_or_list($arg) { + // should be array_is_list($arg), but tests run in non-PHP 8 environment + if (array_values($arg) === $arg) { + return true; + } + return false; + } + /** + * @param list $arg + * @return void + */ + function takesAList($arg) {} + /** + * @param array $arg + * @return void + */ + function takesAnArray($arg) {} + /** + * @var array|list $foo + */ + $foo; + if (is_array_or_list($foo) === true) { + takesAList($foo); + } else { + takesAnArray($foo); + }', + ], + 'assertArrayListIfTrueFalseCompareTruthy' => [ + 'code' => '|list $arg + * @return bool + * + * @psalm-assert-if-false array $arg + * @psalm-assert-if-true list $arg + */ + function is_array_or_list($arg) { + // should be array_is_list($arg), but tests run in non-PHP 8 environment + if (array_values($arg) === $arg) { + return true; + } + return false; + } + /** + * @param list $arg + * @return void + */ + function takesAList($arg) {} + /** + * @param array $arg + * @return void + */ + function takesAnArray($arg) {} + /** + * @var array|list $foo + */ + $foo; + if (is_array_or_list($foo)) { + takesAList($foo); + } else { + takesAnArray($foo); + }', + ], + 'assertArrayListIfTrueFalseCompareNotTrue' => [ + 'code' => '|list $arg + * @return bool + * + * @psalm-assert-if-false array $arg + * @psalm-assert-if-true list $arg + */ + function is_array_or_list($arg) { + if (array_values($arg) === $arg) { + return true; + } + return false; + } + /** + * @param list $arg + * @return void + */ + function takesAList($arg) {} + /** + * @param array $arg + * @return void + */ + function takesAnArray($arg) {} + /** + * @var array|list $foo + */ + $foo; + if (is_array_or_list($foo) !== true) { + takesAnArray($foo); + } else { + takesAList($foo); + }', + ], + 'assertArrayListIfTrueFalseCompareFalse' => [ + 'code' => '|list $arg + * @return bool + * + * @psalm-assert-if-false array $arg + * @psalm-assert-if-true list $arg + */ + function is_array_or_list($arg) { + if (array_values($arg) === $arg) { + return true; + } + return false; + } + /** + * @param list $arg + * @return void + */ + function takesAList($arg) {} + /** + * @param array $arg + * @return void + */ + function takesAnArray($arg) {} + /** + * @var array|list $foo + */ + $foo; + if (is_array_or_list($foo) === false) { + takesAnArray($foo); + } else { + takesAList($foo); + }', + ], + 'assertArrayListIfTrueFalseCompareNotFalse' => [ + 'code' => '|list $arg + * @return bool + * + * @psalm-assert-if-false array $arg + * @psalm-assert-if-true list $arg + */ + function is_array_or_list($arg) { + if (array_values($arg) === $arg) { + return true; + } + return false; + } + /** + * @param list $arg + * @return void + */ + function takesAList($arg) {} + /** + * @param array $arg + * @return void + */ + function takesAnArray($arg) {} + /** + * @var array|list $foo + */ + $foo; + if (is_array_or_list($foo) !== false) { + takesAList($foo); + } else { + takesAnArray($foo); + }', + ], + 'assertArrayListIfTrueFalseCompareFalsy' => [ + 'code' => '|list $arg + * @return bool + * + * @psalm-assert-if-false array $arg + * @psalm-assert-if-true list $arg + */ + function is_array_or_list($arg) { + if (array_values($arg) === $arg) { + return true; + } + return false; + } + /** + * @param list $arg + * @return void + */ + function takesAList($arg) {} + /** + * @param array $arg + * @return void + */ + function takesAnArray($arg) {} + /** + * @var array|list $foo + */ + $foo; + if (!is_array_or_list($foo)) { + takesAnArray($foo); + } else { + takesAList($foo); + }', + ], + 'assertArrayArrayIfTrueFalseCompareFalsy' => [ + 'code' => '|array $arg + * @return bool + * + * @psalm-suppress InvalidReturnType + * + * @psalm-assert-if-false array $arg + * @psalm-assert-if-true array $arg + */ + function is_array_a_or_b($arg) {} + /** + * @param array $arg + * @return void + */ + function takesAnArrayA($arg) {} + /** + * @param array $arg + * @return void + */ + function takesAnArrayB($arg) {} + /** + * @var array|array $foo + */ + $foo; + if (!is_array_a_or_b($foo)) { + takesAnArrayA($foo); + } else { + takesAnArrayB($foo); + }', + ], + 'assertListListIfTrueFalseCompareFalsy' => [ + 'code' => '|list $arg + * @return bool + * + * @psalm-suppress InvalidReturnType + * + * @psalm-assert-if-false list $arg + * @psalm-assert-if-true list $arg + */ + function is_list_string_or_int($arg) {} + /** + * @param list $arg + * @return void + */ + function takesAListString($arg) {} + /** + * @param list $arg + * @return void + */ + function takesAListInt($arg) {} + /** + * @var list|list $foo + */ + $foo; + if (!is_list_string_or_int($foo)) { + takesAListString($foo); + } else { + takesAListInt($foo); + }', + ], + 'assertKeyedArrayKeyedArrayIfTrueFalseCompareFalsy' => [ + 'code' => ' [ + 'code' => ' Date: Thu, 1 Jun 2023 01:24:15 +0200 Subject: [PATCH 2/4] code style and false positive shepherd --- src/Psalm/Internal/Algebra.php | 17 ++++++++++++++--- .../Statements/Expression/AssertionFinder.php | 4 ++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/Psalm/Internal/Algebra.php b/src/Psalm/Internal/Algebra.php index e9bd6fae8ab..a39d6f212a0 100644 --- a/src/Psalm/Internal/Algebra.php +++ b/src/Psalm/Internal/Algebra.php @@ -4,10 +4,10 @@ use Psalm\Exception\ComplicatedExpressionException; use Psalm\Storage\Assertion; -use UnexpectedValueException; use Psalm\Type\Atomic\TArray; use Psalm\Type\Atomic\TKeyedArray; use Psalm\Type\Atomic\TList; +use UnexpectedValueException; use function array_filter; use function array_intersect_key; @@ -325,6 +325,8 @@ public static function simplifyCNF(array $clauses): array /** * Look for clauses with only one possible value * + * @psalm-suppress MoreSpecificReturnType + * * @param list $clauses * @param array $cond_referenced_var_ids * @param array>> $active_truths @@ -408,7 +410,8 @@ public static function getTruthsFromFormula( || $assertion->type instanceof TKeyedArray) { $has_list_or_array = true; // list/array are collapsed, therefore there can only be 1 and we can abort - // otherwise we would have to remove them all individually e.g. array cannot be array + // otherwise we would have to remove them all individually + // e.g. array cannot be array break 2; } } @@ -418,7 +421,6 @@ public static function getTruthsFromFormula( continue; } - $is_array = $is_list = $has_list_or_array; foreach ($anded_types as $key => $orred_types) { foreach ($orred_types as $index => $assertion) { // we only need to check negations @@ -438,14 +440,23 @@ public static function getTruthsFromFormula( } } + /** + * doesn't infer the "unset" correctly + * @psalm-suppress DocblockTypeContradiction + */ if ($truths[$var][$key] === []) { unset($truths[$var][$key]); } else { + /** + * doesn't infer the "unset" correctly + * @psalm-suppress RedundantFunctionCallGivenDocblockType + */ $truths[$var][$key] = array_values($truths[$var][$key]); } } } + /** @psalm-suppress LessSpecificReturnStatement */ return $truths; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php index 83407f2205c..6a6417f732e 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php @@ -2254,6 +2254,7 @@ private static function getFalseInequalityAssertions( } /** + * @psalm-suppress MoreSpecificReturnType * @param PhpParser\Node\Expr\BinaryOp\NotIdentical|PhpParser\Node\Expr\BinaryOp\NotEqual $conditional * @return list>>> */ @@ -2401,6 +2402,7 @@ private static function getTrueInequalityAssertions( } } + /** @psalm-suppress LessSpecificReturnStatement */ return $if_types; } @@ -2979,6 +2981,7 @@ private static function getTrueEqualityAssertions( } /** + * @psalm-suppress MoreSpecificReturnType * @param PhpParser\Node\Expr\BinaryOp\Identical|PhpParser\Node\Expr\BinaryOp\Equal $conditional * @return list>>> */ @@ -3125,6 +3128,7 @@ private static function getFalseEqualityAssertions( } } + /** @psalm-suppress LessSpecificReturnStatement */ return $if_types; } From 322878b1d76d01e611d854835a90d0ce02ca1d4e Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Thu, 1 Jun 2023 01:33:12 +0200 Subject: [PATCH 3/4] code style --- src/Psalm/Internal/Algebra.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Psalm/Internal/Algebra.php b/src/Psalm/Internal/Algebra.php index a39d6f212a0..2c632906e32 100644 --- a/src/Psalm/Internal/Algebra.php +++ b/src/Psalm/Internal/Algebra.php @@ -326,7 +326,6 @@ public static function simplifyCNF(array $clauses): array * Look for clauses with only one possible value * * @psalm-suppress MoreSpecificReturnType - * * @param list $clauses * @param array $cond_referenced_var_ids * @param array>> $active_truths @@ -442,6 +441,7 @@ public static function getTruthsFromFormula( /** * doesn't infer the "unset" correctly + * * @psalm-suppress DocblockTypeContradiction */ if ($truths[$var][$key] === []) { @@ -449,6 +449,7 @@ public static function getTruthsFromFormula( } else { /** * doesn't infer the "unset" correctly + * * @psalm-suppress RedundantFunctionCallGivenDocblockType */ $truths[$var][$key] = array_values($truths[$var][$key]); From fa644cbb3487a7e79d614c32857d95728cefabae Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Fri, 2 Jun 2023 08:54:00 +0200 Subject: [PATCH 4/4] code review init variables --- src/Psalm/Internal/Algebra.php | 1 + .../Analyzer/Statements/Expression/AssertionFinder.php | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/Psalm/Internal/Algebra.php b/src/Psalm/Internal/Algebra.php index 2c632906e32..3574841eaa0 100644 --- a/src/Psalm/Internal/Algebra.php +++ b/src/Psalm/Internal/Algebra.php @@ -325,6 +325,7 @@ public static function simplifyCNF(array $clauses): array /** * Look for clauses with only one possible value * + * doesn't infer the "unset" correctly * @psalm-suppress MoreSpecificReturnType * @param list $clauses * @param array $cond_referenced_var_ids diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php index 6a6417f732e..fdfa804bc29 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php @@ -2336,6 +2336,8 @@ private static function getTrueInequalityAssertions( $if_types = $if_types ? [$if_types] : []; if ($if_types === [] && count($notif_types) === 2) { + $check_var_assertion = null; + $check_var = null; foreach ($notif_types as $notif_type) { foreach ($notif_type as $var => $assertions) { if (count($assertions) !== 1 || count($assertions[0]) !== 1) { @@ -3066,6 +3068,8 @@ private static function getFalseEqualityAssertions( // @todo optionally extend this to arbitrary number of assert-if cases of multiple variables in the function // same code above too if ($if_types === [] && count($notif_types) === 2) { + $check_var_assertion = null; + $check_var = null; foreach ($notif_types as $notif_type) { foreach ($notif_type as $var => $assertions) { if (count($assertions) !== 1 || count($assertions[0]) !== 1) {