From b49444b8adaefc9799eb486caa36d7e4abe42416 Mon Sep 17 00:00:00 2001 From: Brown Date: Mon, 9 Sep 2019 11:14:40 -0400 Subject: [PATCH] Allow immutable objects to be cloned Fixes #2111 --- .../Analyzer/FunctionLikeAnalyzer.php | 7 ++ .../Expression/AssignmentAnalyzer.php | 31 ++++--- .../Statements/ExpressionAnalyzer.php | 18 +++++ src/Psalm/Internal/Codebase/Populator.php | 2 +- tests/ImmutableAnnotationTest.php | 80 ++++++++++++++++++- 5 files changed, 123 insertions(+), 15 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php index 293b39c73..53e8475b1 100644 --- a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php @@ -178,6 +178,13 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements } } elseif ($context->self) { $context->vars_in_scope['$this'] = new Type\Union([new TNamedObject($context->self)]); + + if ($storage->external_mutation_free + && !$storage->mutation_free_inferred + ) { + $context->vars_in_scope['$this']->external_mutation_free = true; + } + $context->vars_possibly_in_scope['$this'] = true; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index 4db3b349d..b4e8d5238 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -568,18 +568,6 @@ class AssignmentAnalyzer $assign_value_type ); } elseif ($assign_var instanceof PhpParser\Node\Expr\PropertyFetch) { - if ($context->mutation_free && !$context->collect_mutations && !$context->collect_initializations) { - if (IssueBuffer::accepts( - new ImpurePropertyAssignment( - 'Cannot assign to a property from a mutation-free context', - new CodeLocation($statements_analyzer, $assign_var) - ), - $statements_analyzer->getSuppressedIssues() - )) { - // fall through - } - } - if (!$assign_var->name instanceof PhpParser\Node\Identifier) { // this can happen when the user actually means to type $this->, but there's // a variable on the next line @@ -635,6 +623,25 @@ class AssignmentAnalyzer if ($var_id) { $context->vars_possibly_in_scope[$var_id] = true; } + + $method_pure_compatible = !empty($assign_var->var->inferredType->external_mutation_free) + || isset($assign_var->var->pure); + + if (($context->mutation_free + || ($context->external_mutation_free && !$method_pure_compatible)) + && !$context->collect_mutations + && !$context->collect_initializations + ) { + if (IssueBuffer::accepts( + new ImpurePropertyAssignment( + 'Cannot assign to a property from a mutation-free context', + new CodeLocation($statements_analyzer, $assign_var) + ), + $statements_analyzer->getSuppressedIssues() + )) { + // fall through + } + } } elseif ($assign_var instanceof PhpParser\Node\Expr\StaticPropertyFetch && $assign_var->class instanceof PhpParser\Node\Name ) { diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php index 0b15e412a..2fede0cbc 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php @@ -1767,6 +1767,8 @@ class ExpressionAnalyzer if (isset($stmt->expr->inferredType)) { $clone_type = $stmt->expr->inferredType; + $immutable_cloned = false; + foreach ($clone_type->getTypes() as $clone_type_part) { if (!$clone_type_part instanceof TNamedObject && !$clone_type_part instanceof TObject @@ -1797,9 +1799,25 @@ class ExpressionAnalyzer return; } + + $codebase = $statements_analyzer->getCodebase(); + + if ($clone_type_part instanceof TNamedObject + && $codebase->classExists($clone_type_part->value) + ) { + $class_storage = $codebase->classlike_storage_provider->get($clone_type_part->value); + + if ($class_storage->mutation_free) { + $immutable_cloned = true; + } + } } $stmt->inferredType = $stmt->expr->inferredType; + + if ($immutable_cloned) { + $stmt->inferredType->external_mutation_free = true; + } } } diff --git a/src/Psalm/Internal/Codebase/Populator.php b/src/Psalm/Internal/Codebase/Populator.php index e66cb53eb..12ac99e4a 100644 --- a/src/Psalm/Internal/Codebase/Populator.php +++ b/src/Psalm/Internal/Codebase/Populator.php @@ -262,7 +262,7 @@ class Populator if ($storage->mutation_free || $storage->external_mutation_free) { foreach ($storage->methods as $method) { - if (!$method->is_static) { + if (!$method->is_static && !$method->external_mutation_free) { $method->mutation_free = $storage->mutation_free; $method->external_mutation_free = $storage->external_mutation_free; } diff --git a/tests/ImmutableAnnotationTest.php b/tests/ImmutableAnnotationTest.php index ef3142bc4..3ff21062c 100644 --- a/tests/ImmutableAnnotationTest.php +++ b/tests/ImmutableAnnotationTest.php @@ -112,7 +112,55 @@ class ImmutableAnnotationTest extends TestCase return new self($id . rand(0, 1)); } }' - ] + ], + 'allowPropertySetOnNewInstance' => [ + 'bar = $bar; + } + + /** + * @psalm-external-mutation-free + */ + public function withBar(string $bar): self { + $new = new Foo("hello"); + /** @psalm-suppress InaccessibleProperty */ + $new->bar = $bar; + + return $new; + } + }' + ], + 'allowClone' => [ + 'bar = $bar; + } + + /** + * @psalm-external-mutation-free + */ + public function withBar(string $bar): self { + $new = clone $this; + /** @psalm-suppress InaccessibleProperty */ + $new->bar = $bar; + + return $new; + } + }' + ], ]; } @@ -143,7 +191,7 @@ class ImmutableAnnotationTest extends TestCase $this->a = $a; } }', - 'error_message' => 'ImpurePropertyAssignment', + 'error_message' => 'InaccessibleProperty', ], 'immutablePropertyAssignmentExternally' => [ ' 'ImpureMethodCall', ], + 'cloneMutatingClass' => [ + 'bar = $bar; + } + + /** + * @psalm-external-mutation-free + */ + public function withBar(Bar $b): Bar { + $new = clone $b; + $b->a = $this->bar; + + return $new; + } + } + + class Bar { + public string $a = "hello"; + }', + 'error_message' => 'ImpurePropertyAssignment', + ], ]; } }