From 35725267f96aa0bc96b210de34e620fbfbaef87f Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Tue, 24 Apr 2018 23:02:20 -0400 Subject: [PATCH] Fix #691 - Allow comparisons to float for integer results --- .../Statements/Expression/BinaryOpChecker.php | 4 +- src/Psalm/Context.php | 3 +- src/Psalm/Type.php | 13 +++- src/Psalm/Type/Reconciler.php | 46 ++++++++++++++- src/Psalm/Type/Union.php | 7 +++ tests/RedundantConditionTest.php | 59 +++++++++++++++++++ 6 files changed, 124 insertions(+), 8 deletions(-) diff --git a/src/Psalm/Checker/Statements/Expression/BinaryOpChecker.php b/src/Psalm/Checker/Statements/Expression/BinaryOpChecker.php index 0f900d15c..e54a6e9af 100644 --- a/src/Psalm/Checker/Statements/Expression/BinaryOpChecker.php +++ b/src/Psalm/Checker/Statements/Expression/BinaryOpChecker.php @@ -768,9 +768,9 @@ class BinaryOpChecker if ($left_type_part instanceof TInt && $right_type_part instanceof TInt) { if (!$result_type) { - $result_type = Type::getInt(); + $result_type = Type::getInt(true); } else { - $result_type = Type::combineUnionTypes(Type::getInt(), $result_type); + $result_type = Type::combineUnionTypes(Type::getInt(true), $result_type); } $has_valid_right_operand = true; diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 0734275d7..950136830 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -317,7 +317,8 @@ class Context && !$this_type->isEmpty() && !$new_type->isEmpty() && ($this_type->getId() !== $new_type->getId() - || $this_type->initialized !== $new_type->initialized) + || $this_type->initialized !== $new_type->initialized + || $this_type->from_calculation !== $new_type->from_calculation) ) { $redefined_vars[$var_id] = $this_type; } diff --git a/src/Psalm/Type.php b/src/Psalm/Type.php index 07597de6c..d6d6d368f 100644 --- a/src/Psalm/Type.php +++ b/src/Psalm/Type.php @@ -541,13 +541,16 @@ abstract class Type } /** + * @param bool $from_calculation + * * @return Type\Union */ - public static function getInt() + public static function getInt($from_calculation = false) { - $type = new TInt; + $union = new Union([new TInt]); + $union->from_calculation = $from_calculation; - return new Union([$type]); + return $union; } /** @@ -752,6 +755,10 @@ abstract class Type $combined_type->from_docblock = true; } + if ($type_1->from_calculation || $type_2->from_calculation) { + $combined_type->from_calculation = true; + } + if ($type_1->ignore_nullable_issues || $type_2->ignore_nullable_issues) { $combined_type->ignore_nullable_issues = true; } diff --git a/src/Psalm/Type/Reconciler.php b/src/Psalm/Type/Reconciler.php index 98c659187..d6313e6f0 100644 --- a/src/Psalm/Type/Reconciler.php +++ b/src/Psalm/Type/Reconciler.php @@ -96,6 +96,7 @@ class Reconciler $failed_reconciliation = false; $from_docblock = $result_type && $result_type->from_docblock; $possibly_undefined = $result_type && $result_type->possibly_undefined; + $from_calculation = $result_type && $result_type->from_calculation; foreach ($new_type_parts as $new_type_part) { $new_type_part_parts = explode('|', $new_type_part); @@ -128,6 +129,7 @@ class Reconciler if ($result_type->getId() !== $before_adjustment || $result_type->from_docblock !== $from_docblock || $result_type->possibly_undefined !== $possibly_undefined + || $result_type->from_calculation !== $from_calculation || $failed_reconciliation ) { $changed_var_ids[] = $key; @@ -459,10 +461,29 @@ class Reconciler $negated_type = substr($new_var_type, 1); - if ($negated_type === 'false' && isset($existing_var_type->getTypes()['bool'])) { + $existing_var_atomic_types = $existing_var_type->getTypes(); + + if (isset($existing_var_atomic_types['int']) + && $existing_var_type->from_calculation + && ($negated_type === 'int' || $negated_type === 'float') + ) { + $existing_var_type->removeType($negated_type); + + if ($negated_type === 'int') { + $existing_var_type->addType(new Type\Atomic\TFloat); + } else { + $existing_var_type->addType(new Type\Atomic\TInt); + } + + $existing_var_type->from_calculation = false; + + return $existing_var_type; + } + + if ($negated_type === 'false' && isset($existing_var_atomic_types['bool'])) { $existing_var_type->removeType('bool'); $existing_var_type->addType(new TTrue); - } elseif ($negated_type === 'true' && isset($existing_var_type->getTypes()['bool'])) { + } elseif ($negated_type === 'true' && isset($existing_var_atomic_types['bool'])) { $existing_var_type->removeType('bool'); $existing_var_type->addType(new TFalse); } elseif (strtolower($negated_type) === 'traversable' @@ -781,6 +802,19 @@ class Reconciler return Type::getMixed(); } + $existing_var_atomic_types = $existing_var_type->getTypes(); + + if (isset($existing_var_atomic_types['int']) + && $existing_var_type->from_calculation + && ($new_var_type === 'int' || $new_var_type === 'float') + ) { + if ($new_var_type === 'int') { + return Type::getInt(); + } + + return Type::getFloat(); + } + if (substr($new_var_type, 0, 4) === 'isa-') { if ($existing_var_type->isMixed()) { return Type::getMixed(); @@ -877,6 +911,14 @@ class Reconciler $has_local_match = false; foreach ($existing_var_type->getTypes() as $existing_var_type_part) { + // special workaround because PHP allows floats to contain ints, but we don’t want this + // behaviour here + if ($existing_var_type_part instanceof Type\Atomic\TFloat + && $new_type_part instanceof Type\Atomic\TInt + ) { + continue; + } + if (TypeChecker::isAtomicContainedBy( $project_checker->codebase, $new_type_part, diff --git a/src/Psalm/Type/Union.php b/src/Psalm/Type/Union.php index cc240ec52..20ab86dfe 100644 --- a/src/Psalm/Type/Union.php +++ b/src/Psalm/Type/Union.php @@ -21,6 +21,13 @@ class Union */ public $from_docblock = false; + /** + * Whether the type originated from integer calculation + * + * @var bool + */ + public $from_calculation = false; + /** * Whether the property that this type has been derived from has been initialized in a constructor * diff --git a/tests/RedundantConditionTest.php b/tests/RedundantConditionTest.php index 2be1254b0..bee4851b1 100644 --- a/tests/RedundantConditionTest.php +++ b/tests/RedundantConditionTest.php @@ -345,6 +345,40 @@ class RedundantConditionTest extends TestCase 'assignments' => [], 'error_levels' => ['MixedAssignment'], ], + 'allowIntValueCheckAfterComparisonDueToOverflow' => [ + ' [ + ' 'TypeDoesNotContainType - src' . DIRECTORY_SEPARATOR . 'somefile.php:7', ], + 'allowIntValueCheckAfterComparisonDueToConditionalOverflow' => [ + ' 'TypeDoesNotContainType - src' . DIRECTORY_SEPARATOR . 'somefile.php:7', + ], + 'disallowTwoIntValueChecksDueToConditionalOverflow' => [ + ' 'TypeDoesNotContainType - src' . DIRECTORY_SEPARATOR . 'somefile.php:6', + ], ]; } }