From 9784367cf63862d7c50cfa0aadff69b43110de5d Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Sun, 5 May 2024 20:26:43 +0200 Subject: [PATCH] Fix PHP notices and add error for implicit float to int precision loss Fix https://github.com/vimeo/psalm/issues/10950 Implement https://github.com/vimeo/psalm/issues/10949 --- .../BinaryOp/ArithmeticOpAnalyzer.php | 180 +++++++++++++++--- .../Expression/BitwiseNotAnalyzer.php | 19 +- tests/BinaryOperationTest.php | 87 ++++++++- 3 files changed, 251 insertions(+), 35 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/ArithmeticOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/ArithmeticOpAnalyzer.php index 7fc47efd872..40d91daaefd 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/ArithmeticOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/ArithmeticOpAnalyzer.php @@ -47,14 +47,17 @@ use function array_diff_key; use function array_values; use function count; +use function filter_var; use function get_class; use function is_int; use function is_numeric; use function max; use function min; -use function preg_match; use function strtolower; +use const FILTER_VALIDATE_FLOAT; +use const FILTER_VALIDATE_INT; + /** * @internal */ @@ -311,6 +314,9 @@ private static function analyzeOperands( bool &$has_string_increment, ?Union &$result_type = null ): ?Union { + $implicit_left = self::noFloatLikeToIntPrecisionLoss($parent, $left_type_part, $invalid_left_messages); + $implicit_right = self::noFloatLikeToIntPrecisionLoss($parent, $right_type_part, $invalid_right_messages); + if (($left_type_part instanceof TLiteralInt || $left_type_part instanceof TLiteralFloat) && ($right_type_part instanceof TLiteralInt || $right_type_part instanceof TLiteralFloat) && ( @@ -349,8 +355,8 @@ private static function analyzeOperands( $result_type, ); - $has_valid_left_operand = true; - $has_valid_right_operand = true; + $has_valid_left_operand = $implicit_left; + $has_valid_right_operand = $implicit_right; return null; } @@ -406,6 +412,10 @@ private static function analyzeOperands( if (count($combined_atomic_types) <= 2) { $left_type_part = $combined_atomic_types[0]; $right_type_part = $combined_atomic_types[1] ?? $combined_atomic_types[0]; + + // check again, since the type changed + self::noFloatLikeToIntPrecisionLoss($parent, $left_type_part, $invalid_left_messages); + self::noFloatLikeToIntPrecisionLoss($parent, $right_type_part, $invalid_right_messages); } } @@ -476,7 +486,9 @@ private static function analyzeOperands( $statements_source->getSuppressedIssues(), ); } - } elseif ($right_type_part instanceof TTemplateParam + } + + if ($right_type_part instanceof TTemplateParam && !$right_type_part->as->isInt() && !$right_type_part->as->isFloat() ) { @@ -491,6 +503,18 @@ private static function analyzeOperands( } } + if ($left_type_part instanceof TTemplateParam && !$left_type_part->as->isInt()) { + foreach ($left_type_part->as->getAtomicTypes() as $atomic) { + self::noFloatLikeToIntPrecisionLoss($parent, $atomic, $invalid_left_messages); + } + } + + if ($right_type_part instanceof TTemplateParam && !$right_type_part->as->isInt()) { + foreach ($right_type_part->as->getAtomicTypes() as $atomic) { + self::noFloatLikeToIntPrecisionLoss($parent, $atomic, $invalid_right_messages); + } + } + return null; } @@ -711,19 +735,63 @@ private static function analyzeOperands( } } + $left_from_literal_string = false; if ($left_type_part instanceof TLiteralString) { - if (preg_match('/^\-?\d+$/', $left_type_part->value)) { - $left_type_part = new TLiteralInt((int) $left_type_part->value); - } elseif (preg_match('/^\-?\d?\.\d+$/', $left_type_part->value)) { - $left_type_part = new TLiteralFloat((float) $left_type_part->value); - } + $left_from_literal_string = $left_type_part; + $left_type_part = self::literalStringToIntFloat($left_type_part); } + $right_from_literal_string = false; if ($right_type_part instanceof TLiteralString) { - if (preg_match('/^\-?\d+$/', $right_type_part->value)) { - $right_type_part = new TLiteralInt((int) $right_type_part->value); - } elseif (preg_match('/^\-?\d?\.\d+$/', $right_type_part->value)) { - $right_type_part = new TLiteralFloat((float) $right_type_part->value); + $right_from_literal_string = $right_type_part; + $right_type_part = self::literalStringToIntFloat($right_type_part); + } + + if (($left_from_literal_string || $right_from_literal_string) + && ($left_type_part instanceof TLiteralInt || $left_type_part instanceof TLiteralFloat) + && ($right_type_part instanceof TLiteralInt || $right_type_part instanceof TLiteralFloat) + && ( + //we don't try to do arithmetics on variables in loops + $context === null + || $context->inside_loop === false + || (!$left instanceof PhpParser\Node\Expr\Variable && !$right instanceof PhpParser\Node\Expr\Variable) + ) + ) { + // get_class is fine here because both classes are final. + if ($statements_source !== null + && $config->strict_binary_operands + && (!$left_from_literal_string + || !$right_from_literal_string + || get_class($left_from_literal_string) !== get_class($right_from_literal_string)) + ) { + IssueBuffer::maybeAdd( + new InvalidOperand( + 'Cannot process numeric types together in strict operands mode, '. + 'please cast explicitly', + new CodeLocation($statements_source, $parent), + ), + $statements_source->getSuppressedIssues(), + ); + } + + // time for some arithmetic! + $calculated_type = self::arithmeticOperation( + $parent, + $left_type_part->value, + $right_type_part->value, + true, + ); + + if ($calculated_type) { + $result_type = Type::combineUnionTypes( + $calculated_type, + $result_type, + ); + + $has_valid_left_operand = $implicit_left; + $has_valid_right_operand = $implicit_right; + + return null; } } @@ -752,8 +820,8 @@ private static function analyzeOperands( $result_type = Type::combineUnionTypes($new_result_type, $result_type); - $has_valid_right_operand = true; - $has_valid_left_operand = true; + $has_valid_right_operand = $implicit_right; + $has_valid_left_operand = $implicit_left; return null; } @@ -847,8 +915,8 @@ private static function analyzeOperands( $result_type = Type::combineUnionTypes(Type::getFloat(), $result_type); } - $has_valid_right_operand = true; - $has_valid_left_operand = true; + $has_valid_right_operand = $implicit_right; + $has_valid_left_operand = $implicit_left; return null; } @@ -875,8 +943,8 @@ private static function analyzeOperands( $result_type = Type::combineUnionTypes(Type::getFloat(), $result_type); } - $has_valid_right_operand = true; - $has_valid_left_operand = true; + $has_valid_right_operand = $implicit_right; + $has_valid_left_operand = $implicit_left; return null; } @@ -901,8 +969,8 @@ private static function analyzeOperands( $result_type = new Union([new TInt, new TFloat]); } - $has_valid_right_operand = true; - $has_valid_left_operand = true; + $has_valid_right_operand = $implicit_right; + $has_valid_left_operand = $implicit_left; return null; } @@ -910,11 +978,11 @@ private static function analyzeOperands( if (!$left_type_part->isNumericType()) { $invalid_left_messages[] = 'Cannot perform a numeric operation with a non-numeric type ' . $left_type_part; - $has_valid_right_operand = true; + $has_valid_right_operand = $implicit_right; } else { $invalid_right_messages[] = 'Cannot perform a numeric operation with a non-numeric type ' . $right_type_part; - $has_valid_left_operand = true; + $has_valid_left_operand = $implicit_left; } } else { $invalid_left_messages[] = @@ -944,21 +1012,21 @@ public static function arithmeticOperation( return Type::getNever(); } - $result = $operand1 % $operand2; + $result = (int) $operand1 % (int) $operand2; } elseif ($operation instanceof PhpParser\Node\Expr\BinaryOp\Mul) { $result = $operand1 * $operand2; } elseif ($operation instanceof PhpParser\Node\Expr\BinaryOp\Pow) { $result = $operand1 ** $operand2; } elseif ($operation instanceof PhpParser\Node\Expr\BinaryOp\BitwiseOr) { - $result = $operand1 | $operand2; + $result = (int) $operand1 | (int) $operand2; } elseif ($operation instanceof PhpParser\Node\Expr\BinaryOp\BitwiseAnd) { - $result = $operand1 & $operand2; + $result = (int) $operand1 & (int) $operand2; } elseif ($operation instanceof PhpParser\Node\Expr\BinaryOp\BitwiseXor) { - $result = $operand1 ^ $operand2; + $result = (int) $operand1 ^ (int) $operand2; } elseif ($operation instanceof PhpParser\Node\Expr\BinaryOp\ShiftLeft) { - $result = $operand1 << $operand2; + $result = (int) $operand1 << (int) $operand2; } elseif ($operation instanceof PhpParser\Node\Expr\BinaryOp\ShiftRight) { - $result = $operand1 >> $operand2; + $result = (int) $operand1 >> (int) $operand2; } elseif ($operation instanceof PhpParser\Node\Expr\BinaryOp\Div) { if ($operand2 === 0 || $operand2 === 0.0) { return Type::getNever(); @@ -1413,4 +1481,58 @@ private static function analyzeModBetweenIntRange( $result_type, ); } + + /** @return TLiteralInt|TLiteralFloat|TLiteralString */ + private static function literalStringToIntFloat( + TLiteralString $literal_atomic + ) { + $int = filter_var($literal_atomic->value, FILTER_VALIDATE_INT); + if ($int !== false) { + return new TLiteralInt($int); + } + + $float = filter_var($literal_atomic->value, FILTER_VALIDATE_FLOAT); + if ($float !== false) { + return new TLiteralFloat($float); + } + + return $literal_atomic; + } + + /** + * @param string[] $invalid_messages + */ + public static function noFloatLikeToIntPrecisionLoss( + PhpParser\Node $parent, + Atomic $type_part, + array &$invalid_messages + ): bool { + if ($type_part instanceof TLiteralString) { + $type_part = self::literalStringToIntFloat($type_part); + } + + if ($type_part instanceof TLiteralFloat + && (int) $type_part->value === filter_var($type_part->value, FILTER_VALIDATE_INT) + ) { + // if we don't have precision loss, e.g. 2.0 + return true; + } + + if (($type_part instanceof TFloat + || $type_part instanceof TNumeric + || $type_part instanceof TNumericString) + && ($parent instanceof PhpParser\Node\Expr\BinaryOp\Mod + || $parent instanceof PhpParser\Node\Expr\BinaryOp\BitwiseOr + || $parent instanceof PhpParser\Node\Expr\BinaryOp\BitwiseAnd + || $parent instanceof PhpParser\Node\Expr\BinaryOp\BitwiseXor + || $parent instanceof PhpParser\Node\Expr\BinaryOp\ShiftLeft + || $parent instanceof PhpParser\Node\Expr\BinaryOp\ShiftRight) + ) { + // deprecated since PHP 8, will trigger a notice + $invalid_messages[] = 'Implicit conversion from float to int'; + return false; + } + + return true; + } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BitwiseNotAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BitwiseNotAnalyzer.php index e9b16763e37..3e955ce5408 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BitwiseNotAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BitwiseNotAnalyzer.php @@ -21,6 +21,10 @@ use Psalm\Type\Atomic\TString; use Psalm\Type\Union; +use function filter_var; + +use const FILTER_VALIDATE_INT; + /** * @internal */ @@ -56,8 +60,21 @@ public static function analyze( $acceptable_types[] = $type_part; $has_valid_operand = true; } elseif ($type_part instanceof TFloat) { + if (!$type_part instanceof TLiteralFloat + || (int) $type_part->value !== filter_var($type_part->value, FILTER_VALIDATE_INT) + ) { + // deprecated since PHP 8, will trigger a notice + IssueBuffer::maybeAdd( + new InvalidOperand( + 'Implicit conversion from float to int', + new CodeLocation($statements_analyzer, $stmt->expr), + ), + $statements_analyzer->getSuppressedIssues(), + ); + } + $type_part = ($type_part instanceof TLiteralFloat) ? - new TLiteralInt(~$type_part->value) : + new TLiteralInt(~((int) $type_part->value)) : new TInt; $stmt_expr_type->removeType($type_string); diff --git a/tests/BinaryOperationTest.php b/tests/BinaryOperationTest.php index e2dafc18724..e7d7e78d6d3 100644 --- a/tests/BinaryOperationTest.php +++ b/tests/BinaryOperationTest.php @@ -333,17 +333,31 @@ public function providerValidCodeParse(): iterable 'modulo' => [ 'code' => ' [ + '$a' => 'int', + '$e' => 'int', + '$f' => 'int', + '$g' => 'int', + ], + ], + 'moduloPrecisionFloat' => [ + 'code' => ' [ - '$a' => 'int', '$b' => 'int', '$c' => 'int', '$d' => 'int', - '$e' => 'int', + '$f' => 'int', + '$g' => 'int', ], + 'ignored_issues' => ['InvalidOperand'], ], 'numericAddition' => [ 'code' => ' ' [ '$a' => 'int', '$b' => 'int', - '$c' => 'int', + '$c' => 'string', '$d' => 'string', ], ], + 'bitwiseNotPrecisionFloat' => [ + 'code' => ' [ + '$c' => 'int', + ], + 'ignored_issues' => ['InvalidOperand'], + ], 'stringIncrementSuppressed' => [ 'code' => ' 'InvalidOperand', ], + 'invalidBitwiseNotPrecisionFloat' => [ + 'code' => ' 'InvalidOperand', + ], 'possiblyInvalidBitwiseNot' => [ 'code' => ' 'InvalidOperand', ], + 'invalidFloatModulo1' => [ + 'code' => ' 'InvalidOperand', + ], + 'invalidFloatModulo2' => [ + 'code' => ' 'InvalidOperand', + ], + 'invalidFloatModulo3' => [ + 'code' => ' 'InvalidOperand', + ], + 'invalidFloatModulo4' => [ + 'code' => ' 'InvalidOperand', + ], + 'invalidFloatModulo5' => [ + 'code' => ' 'InvalidOperand', + ], + 'invalidFloatModulo6' => [ + 'code' => ' 'InvalidOperand', + ], + 'invalidFloatModulo7' => [ + 'code' => ' 'PossiblyInvalidOperand', + ], + 'invalidFloatModulo8' => [ + 'code' => ' 'InvalidOperand', + ], + 'invalidFloatModulo9' => [ + 'code' => ' 'InvalidOperand', + ], 'substrImpossible' => [ 'code' => '