From b15384bbff22b9f80aa257bb17f9b4c049c71dff Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Tue, 8 Dec 2020 16:39:06 -0500 Subject: [PATCH] Tighten up rules arouund when mutation-free methods get memoised --- src/Psalm/Context.php | 10 +- .../Expression/Call/FunctionCallAnalyzer.php | 2 +- .../Method/AtomicMethodCallAnalysisResult.php | 5 + .../ExistingAtomicMethodCallAnalyzer.php | 5 +- .../Call/Method/MethodCallPurityAnalyzer.php | 22 +-- .../Expression/Call/MethodCallAnalyzer.php | 11 +- .../Expression/Call/NewAnalyzer.php | 2 +- .../Expression/Call/StaticCallAnalyzer.php | 2 +- .../Statements/Expression/CallAnalyzer.php | 8 +- .../Expression/ExpressionIdentifier.php | 2 +- .../Fetch/AtomicPropertyFetchAnalyzer.php | 5 + src/Psalm/Internal/Codebase/Populator.php | 1 + .../Type/SimpleAssertionReconciler.php | 2 +- src/Psalm/Storage/MethodStorage.php | 5 + src/Psalm/Type/Union.php | 5 + tests/PropertyTypeTest.php | 133 ++++++++++++++++-- tests/PureAnnotationTest.php | 3 +- 17 files changed, 183 insertions(+), 40 deletions(-) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index f180f23ef..d11d042b3 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -647,6 +647,8 @@ class Context return; } + $existing_type->allow_mutations = true; + $this->removeVarFromConflictingClauses( $remove_var_id, $existing_type->hasMixed() @@ -671,12 +673,14 @@ class Context } } - public function removeAllObjectVars(): void + public function removeMutableObjectVars(): void { $vars_to_remove = []; - foreach ($this->vars_in_scope as $var_id => $_) { - if (strpos($var_id, '->') !== false || strpos($var_id, '::') !== false) { + foreach ($this->vars_in_scope as $var_id => $type) { + if ($type->has_mutations + && (strpos($var_id, '->') !== false || strpos($var_id, '::') !== false) + ) { $vars_to_remove[] = $var_id; } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php index 40f4219c5..de397649f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php @@ -968,7 +968,7 @@ class FunctionCallAnalyzer extends CallAnalyzer } if (!$config->remember_property_assignments_after_call) { - $context->removeAllObjectVars(); + $context->removeMutableObjectVars(); } } elseif ($function_call_info->function_id && (($function_call_info->function_storage diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/AtomicMethodCallAnalysisResult.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/AtomicMethodCallAnalysisResult.php index 93220efbf..df47cc423 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/AtomicMethodCallAnalysisResult.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/AtomicMethodCallAnalysisResult.php @@ -84,4 +84,9 @@ class AtomicMethodCallAnalysisResult * @var bool */ public $can_memoize = false; + + /** + * @var bool + */ + public $immutable_call = false; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php index 1cbc9f154..0e6edce09 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php @@ -251,7 +251,7 @@ class ExistingAtomicMethodCallAnalyzer extends CallAnalyzer if ($method_storage) { if (!$context->collect_mutations && !$context->collect_initializations) { - $result->can_memoize = MethodCallPurityAnalyzer::analyze( + MethodCallPurityAnalyzer::analyze( $statements_analyzer, $codebase, $stmt, @@ -261,7 +261,8 @@ class ExistingAtomicMethodCallAnalyzer extends CallAnalyzer $method_storage, $class_storage, $context, - $config + $config, + $result ); } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallPurityAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallPurityAnalyzer.php index 592757e56..c61c8a5d8 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallPurityAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallPurityAnalyzer.php @@ -23,10 +23,9 @@ class MethodCallPurityAnalyzer \Psalm\Storage\MethodStorage $method_storage, \Psalm\Storage\ClassLikeStorage $class_storage, Context $context, - \Psalm\Config $config - ) : bool { - $can_memoize = false; - + \Psalm\Config $config, + AtomicMethodCallAnalysisResult $result + ) : void { $method_pure_compatible = $method_storage->external_mutation_free && $statements_analyzer->node_data->isPureCompatible($stmt->var); @@ -81,16 +80,23 @@ class MethodCallPurityAnalyzer if ($method_storage->mutation_free && (!$method_storage->mutation_free_inferred || $method_storage->final) + && ($method_storage->immutable || $config->remember_property_assignments_after_call) ) { if ($context->inside_conditional && !$method_storage->assertions && !$method_storage->if_true_assertions ) { /** @psalm-suppress UndefinedPropertyAssignment */ - $stmt->pure = true; + $stmt->memoizable = true; + + if ($method_storage->immutable) { + /** @psalm-suppress UndefinedPropertyAssignment */ + $stmt->pure = true; + } } - $can_memoize = true; + $result->can_memoize = true; + $result->immutable_call = $method_storage->immutable; } if ($codebase->find_unused_variables @@ -128,7 +134,7 @@ class MethodCallPurityAnalyzer && !$method_storage->mutation_free && !$method_pure_compatible ) { - $context->removeAllObjectVars(); + $context->removeMutableObjectVars(); } elseif ($method_storage->this_property_mutations) { foreach ($method_storage->this_property_mutations as $name => $_) { $mutation_var_id = $lhs_var_id . '->' . $name; @@ -144,7 +150,5 @@ class MethodCallPurityAnalyzer } } } - - return $can_memoize; } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/MethodCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/MethodCallAnalyzer.php index 7cad12661..b37ef8457 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/MethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/MethodCallAnalyzer.php @@ -205,14 +205,17 @@ class MethodCallAnalyzer extends \Psalm\Internal\Analyzer\Statements\Expression\ if (!$stmt->args && $lhs_var_id && $stmt->name instanceof PhpParser\Node\Identifier) { if ($codebase->config->memoize_method_calls || $result->can_memoize) { $method_var_id = $lhs_var_id . '->' . strtolower($stmt->name->name) . '()'; + if (isset($context->vars_in_scope[$method_var_id])) { $result->return_type = clone $context->vars_in_scope[$method_var_id]; - if ($result->can_memoize) { - /** @psalm-suppress UndefinedPropertyAssignment */ - $stmt->pure = true; - } } elseif ($result->return_type !== null) { $context->vars_in_scope[$method_var_id] = $result->return_type; + $context->vars_in_scope[$method_var_id]->has_mutations = false; + } + + if ($result->can_memoize) { + /** @psalm-suppress UndefinedPropertyAssignment */ + $stmt->memoizable = true; } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php index 9fbc0a342..de3dd1a97 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php @@ -222,7 +222,7 @@ class NewAnalyzer extends \Psalm\Internal\Analyzer\Statements\Expression\CallAna } if (!$config->remember_property_assignments_after_call && !$context->collect_initializations) { - $context->removeAllObjectVars(); + $context->removeMutableObjectVars(); } return true; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php index 22c1edf9e..42db9703b 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php @@ -225,7 +225,7 @@ class StaticCallAnalyzer extends CallAnalyzer } if (!$config->remember_property_assignments_after_call && !$context->collect_initializations) { - $context->removeAllObjectVars(); + $context->removeMutableObjectVars(); } if (!$statements_analyzer->node_data->getType($stmt)) { diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php index 9593a332b..b179fba4c 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php @@ -874,10 +874,10 @@ class CallAnalyzer $readonly_template_result = new TemplateResult($template_type_map, $template_type_map); \Psalm\Internal\Type\TemplateInferredTypeReplacer::replace( - $op_vars_in_scope[$var_id], - $readonly_template_result, - $codebase - ); + $op_vars_in_scope[$var_id], + $readonly_template_result, + $codebase + ); } $op_vars_in_scope[$var_id]->from_docblock = true; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/ExpressionIdentifier.php b/src/Psalm/Internal/Analyzer/Statements/Expression/ExpressionIdentifier.php index 9d141a34e..e89713548 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/ExpressionIdentifier.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/ExpressionIdentifier.php @@ -199,7 +199,7 @@ class ExpressionIdentifier ) { $config = \Psalm\Config::getInstance(); - if ($config->memoize_method_calls || isset($stmt->pure)) { + if ($config->memoize_method_calls || isset($stmt->memoizable)) { $lhs_var_name = self::getArrayVarId( $stmt->var, $this_class_name, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php index 3c29bbb36..067635864 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -425,6 +425,10 @@ class AtomicPropertyFetchAnalyzer $in_assignment ); + if ($class_storage->mutation_free) { + $class_property_type->has_mutations = false; + } + if ($stmt_type = $statements_analyzer->node_data->getType($stmt)) { $statements_analyzer->node_data->setType( $stmt, @@ -1087,6 +1091,7 @@ class AtomicPropertyFetchAnalyzer ); } } + return $class_property_type; } } diff --git a/src/Psalm/Internal/Codebase/Populator.php b/src/Psalm/Internal/Codebase/Populator.php index eb1e48559..a19c11e80 100644 --- a/src/Psalm/Internal/Codebase/Populator.php +++ b/src/Psalm/Internal/Codebase/Populator.php @@ -219,6 +219,7 @@ class Populator if (!$method->is_static && !$method->external_mutation_free) { $method->mutation_free = $storage->mutation_free; $method->external_mutation_free = $storage->external_mutation_free; + $method->immutable = $storage->mutation_free; } } diff --git a/src/Psalm/Internal/Type/SimpleAssertionReconciler.php b/src/Psalm/Internal/Type/SimpleAssertionReconciler.php index 3db3e8ada..17933ca65 100644 --- a/src/Psalm/Internal/Type/SimpleAssertionReconciler.php +++ b/src/Psalm/Internal/Type/SimpleAssertionReconciler.php @@ -1669,7 +1669,7 @@ class SimpleAssertionReconciler extends \Psalm\Type\Reconciler $did_remove_type = true; } elseif ($type instanceof TTemplateParam) { if ($type->as->hasArray() || $type->as->hasMixed()) { - $type = clone $type; + $type = clone $type; $type->as = self::reconcileArray( $type->as, diff --git a/src/Psalm/Storage/MethodStorage.php b/src/Psalm/Storage/MethodStorage.php index 625249f04..801d2a4e4 100644 --- a/src/Psalm/Storage/MethodStorage.php +++ b/src/Psalm/Storage/MethodStorage.php @@ -65,6 +65,11 @@ class MethodStorage extends FunctionLikeStorage */ public $external_mutation_free = false; + /** + * @var bool + */ + public $immutable = false; + /** * @var bool */ diff --git a/src/Psalm/Type/Union.php b/src/Psalm/Type/Union.php index 0b4e38103..7270cbecd 100644 --- a/src/Psalm/Type/Union.php +++ b/src/Psalm/Type/Union.php @@ -174,6 +174,11 @@ class Union implements TypeNode */ public $allow_mutations = true; + /** + * @var bool + */ + public $has_mutations = true; + /** @var null|string */ private $id; diff --git a/tests/PropertyTypeTest.php b/tests/PropertyTypeTest.php index 7b75cbc37..800556d3c 100644 --- a/tests/PropertyTypeTest.php +++ b/tests/PropertyTypeTest.php @@ -31,8 +31,7 @@ class PropertyTypeTest extends TestCase } class X { - /** @var ?int **/ - public $x; + public ?int $x = null; public function getX(): int { $this->x = 5; @@ -64,8 +63,7 @@ class PropertyTypeTest extends TestCase } class X { - /** @var ?int **/ - public $x; + public ?int $x = null; public function getX(): int { $this->x = 5; @@ -80,7 +78,7 @@ class PropertyTypeTest extends TestCase $this->analyzeFile('somefile.php', new Context()); } - public function testForgetPropertyAssignmentsInBranchWithThrow(): void + public function testForgetPropertyAssignmentsInBranch(): void { Config::getInstance()->remember_property_assignments_after_call = false; @@ -99,19 +97,130 @@ class PropertyTypeTest extends TestCase } class X { - /** @var ?int **/ - public $x; + public ?int $x = null; + } - public function getX(bool $b): int { - $this->x = 5; + function testX(X $x): void { + $x->x = 5; - if ($b) { - XCollector::modify(); - throw new \Exception("bad"); + if (rand(0, 1)) { + XCollector::modify(); + } + + if ($x->x === null) {} + }' + ); + + $this->analyzeFile('somefile.php', new Context()); + } + + public function testForgetFinalMethodCalls(): void + { + Config::getInstance()->remember_property_assignments_after_call = false; + + $this->addFile( + 'somefile.php', + 'x = null; } + } + } + class X { + public ?int $x = null; + + public function __construct(?int $x) { + $this->x = $x; + } + + public final function getX() : ?int { return $this->x; } + } + + function testX(X $x): void { + if ($x->getX()) { + XCollector::modify(); + if ($x->getX() === null) {} + } + }' + ); + + $this->analyzeFile('somefile.php', new Context()); + } + + public function testRememberImmutableMethodCalls(): void + { + Config::getInstance()->remember_property_assignments_after_call = false; + + $this->expectExceptionMessage('TypeDoesNotContainNull - somefile.php:22:29'); + $this->expectException(\Psalm\Exception\CodeException::class); + + $this->addFile( + 'somefile.php', + 'x = $x; + } + + public function getX() : ?int { + return $this->x; + } + } + + function testX(X $x): void { + if ($x->getX()) { + XCollector::modify(); + if ($x->getX() === null) {} + } + }' + ); + + $this->analyzeFile('somefile.php', new Context()); + } + + public function testRememberImmutableProperties(): void + { + Config::getInstance()->remember_property_assignments_after_call = false; + + $this->expectExceptionMessage('TypeDoesNotContainNull - somefile.php:18:29'); + $this->expectException(\Psalm\Exception\CodeException::class); + + $this->addFile( + 'somefile.php', + 'x = $x; + } + } + + function testX(X $x): void { + if ($x->x) { + XCollector::modify(); + if ($x->x === null) {} + } }' ); diff --git a/tests/PureAnnotationTest.php b/tests/PureAnnotationTest.php index 84bc23ccc..3c0a997e2 100644 --- a/tests/PureAnnotationTest.php +++ b/tests/PureAnnotationTest.php @@ -333,7 +333,8 @@ class PureAnnotationTest extends TestCase public function foo() : void {} public function doSomething(): void { - if ($this->checkNotNullNested() && $this->other->foo()) {} + $this->checkNotNullNested(); + $this->other->foo(); } }' ],