From 5c76ab35c8952e6f8f6363c66e0a0f721b7b99cd Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Fri, 1 Mar 2019 17:30:55 -0500 Subject: [PATCH] Allow properties to be set regardless of visibility --- psalm.xml.dist | 1 + src/Psalm/Internal/Analyzer/ClassAnalyzer.php | 2 +- src/Psalm/Internal/Analyzer/FileAnalyzer.php | 8 ++- .../Internal/Analyzer/ProjectAnalyzer.php | 2 +- .../Statements/Expression/CallAnalyzer.php | 66 +++++++++++-------- tests/MethodMutationTest.php | 3 + tests/PropertyTypeTest.php | 57 +++++++++++++++- 7 files changed, 104 insertions(+), 35 deletions(-) diff --git a/psalm.xml.dist b/psalm.xml.dist index 88f1e0983..b3ee281b0 100644 --- a/psalm.xml.dist +++ b/psalm.xml.dist @@ -23,6 +23,7 @@ + diff --git a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php index fb273c4e8..2c071950f 100644 --- a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php @@ -965,7 +965,7 @@ class ClassAnalyzer extends ClassLikeAnalyzer if (IssueBuffer::accepts( new PropertyNotSetInConstructor( 'Property ' . $property_id . ' is not defined in constructor of ' . - $this->fq_class_name . ' or in any private methods called in the constructor', + $this->fq_class_name . ' or in any methods called in the constructor', $property_storage->location, $property_id ), diff --git a/src/Psalm/Internal/Analyzer/FileAnalyzer.php b/src/Psalm/Internal/Analyzer/FileAnalyzer.php index a9dbcfa4f..b843842e0 100644 --- a/src/Psalm/Internal/Analyzer/FileAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FileAnalyzer.php @@ -269,20 +269,22 @@ class FileAnalyzer extends SourceAnalyzer implements StatementsSource * * @return void */ - public function getMethodMutations($method_id, Context $this_context) + public function getMethodMutations($method_id, Context $this_context, bool $from_project_analyzer = false) { list($fq_class_name, $method_name) = explode('::', $method_id); if (isset($this->class_analyzers_to_analyze[strtolower($fq_class_name)])) { $class_analyzer_to_examine = $this->class_analyzers_to_analyze[strtolower($fq_class_name)]; } else { - $this->project_analyzer->getMethodMutations($method_id, $this_context); + if (!$from_project_analyzer) { + $this->project_analyzer->getMethodMutations($method_id, $this_context); + } return; } $call_context = new Context($this_context->self); - $call_context->collect_mutations = true; + $call_context->collect_mutations = $this_context->collect_mutations; $call_context->collect_initializations = $this_context->collect_initializations; $call_context->initialized_methods = $this_context->initialized_methods; $call_context->include_location = $this_context->include_location; diff --git a/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php b/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php index 66db07f28..4cdc4a1b0 100644 --- a/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php @@ -846,7 +846,7 @@ class ProjectAnalyzer $this_context->vars_in_scope['$this'] = Type::parseString($fq_class_name); } - $file_analyzer->getMethodMutations($appearing_method_id, $this_context); + $file_analyzer->getMethodMutations($appearing_method_id, $this_context, true); $file_analyzer->class_analyzers_to_analyze = []; $file_analyzer->interface_analyzers_to_analyze = []; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php index f6ebb6b93..6736d69c3 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php @@ -105,43 +105,48 @@ class CallAnalyzer $declaring_method_id = $codebase->methods->getDeclaringMethodId($method_id); - if (!$declaring_method_id) { - if (isset($context->vars_in_scope['$this'])) { - foreach ($context->vars_in_scope['$this']->getTypes() as $atomic_type) { - if ($atomic_type instanceof TNamedObject) { + if (isset($context->vars_in_scope['$this'])) { + foreach ($context->vars_in_scope['$this']->getTypes() as $atomic_type) { + if ($atomic_type instanceof TNamedObject) { + if ($fq_class_name === $atomic_type->value) { + $alt_declaring_method_id = $declaring_method_id; + } else { $fq_class_name = $atomic_type->value; + $method_id = $fq_class_name . '::' . strtolower($method_name); - $declaring_method_id = $codebase->methods->getDeclaringMethodId($method_id); + $alt_declaring_method_id = $codebase->methods->getDeclaringMethodId($method_id); + } - if ($declaring_method_id) { - break; - } + if ($alt_declaring_method_id) { + $declaring_method_id = $alt_declaring_method_id; + break; + } - if (!$atomic_type->extra_types) { - continue; - } + if (!$atomic_type->extra_types) { + continue; + } - foreach ($atomic_type->extra_types as $intersection_type) { - if ($intersection_type instanceof TNamedObject) { - $fq_class_name = $intersection_type->value; - $method_id = $fq_class_name . '::' . strtolower($method_name); + foreach ($atomic_type->extra_types as $intersection_type) { + if ($intersection_type instanceof TNamedObject) { + $fq_class_name = $intersection_type->value; + $method_id = $fq_class_name . '::' . strtolower($method_name); - $declaring_method_id = $codebase->methods->getDeclaringMethodId($method_id); + $alt_declaring_method_id = $codebase->methods->getDeclaringMethodId($method_id); - if ($declaring_method_id) { - break; - } + if ($alt_declaring_method_id) { + $declaring_method_id = $alt_declaring_method_id; + break 2; } } } } } + } - if (!$declaring_method_id) { - // can happen for __call - return; - } + if (!$declaring_method_id) { + // can happen for __call + return; } if (isset($context->initialized_methods[$declaring_method_id])) { @@ -158,9 +163,7 @@ class CallAnalyzer $class_analyzer = $source->getSource(); - if ($class_analyzer instanceof ClassLikeAnalyzer && - ($method_storage->visibility === ClassLikeAnalyzer::VISIBILITY_PRIVATE || $method_storage->final) - ) { + if ($class_analyzer instanceof ClassLikeAnalyzer && !$method_storage->is_static) { $local_vars_in_scope = []; $local_vars_possibly_in_scope = []; @@ -176,7 +179,16 @@ class CallAnalyzer } } - $class_analyzer->getMethodMutations(strtolower($method_name), $context); + if ($fq_class_name === $source->getFQCLN()) { + $class_analyzer->getMethodMutations(strtolower($method_name), $context); + } else { + list($declaring_fq_class_name) = explode('::', $declaring_method_id); + + $old_self = $context->self; + $context->self = $declaring_fq_class_name; + $project_analyzer->getMethodMutations($declaring_method_id, $context); + $context->self = $old_self; + } foreach ($local_vars_in_scope as $var => $type) { $context->vars_in_scope[$var] = $type; diff --git a/tests/MethodMutationTest.php b/tests/MethodMutationTest.php index a3af9256e..342c79f40 100644 --- a/tests/MethodMutationTest.php +++ b/tests/MethodMutationTest.php @@ -92,6 +92,7 @@ class MethodMutationTest extends TestCase new FileAnalyzer($this->project_analyzer, 'somefile.php', 'somefile.php'); $this->project_analyzer->getCodebase()->scanFiles(); $method_context = new Context(); + $method_context->collect_mutations = true; $this->project_analyzer->getMethodMutations('FooController::barBar', $method_context); $this->assertSame('UserViewData', (string)$method_context->vars_in_scope['$this->user_viewdata']); @@ -129,6 +130,7 @@ class MethodMutationTest extends TestCase new FileAnalyzer($this->project_analyzer, 'somefile.php', 'somefile.php'); $this->project_analyzer->getCodebase()->scanFiles(); $method_context = new Context(); + $method_context->collect_mutations = true; $this->project_analyzer->getMethodMutations('FooController::__construct', $method_context); $this->assertSame('Foo', (string)$method_context->vars_in_scope['$this->foo']); @@ -165,6 +167,7 @@ class MethodMutationTest extends TestCase new FileAnalyzer($this->project_analyzer, 'somefile.php', 'somefile.php'); $this->project_analyzer->getCodebase()->scanFiles(); $method_context = new Context(); + $method_context->collect_mutations = true; $this->project_analyzer->getMethodMutations('FooController::__construct', $method_context); $this->assertSame('Foo', (string)$method_context->vars_in_scope['$this->foo']); diff --git a/tests/PropertyTypeTest.php b/tests/PropertyTypeTest.php index d1d13e49b..c8b2b9757 100644 --- a/tests/PropertyTypeTest.php +++ b/tests/PropertyTypeTest.php @@ -613,8 +613,7 @@ class PropertyTypeTest extends TestCase } class B extends A { - public function foo(): void - { + public function foo(): void { $this->bar = "hello"; } }', @@ -623,6 +622,29 @@ class PropertyTypeTest extends TestCase 'PropertyNotSetInConstructor' => Config::REPORT_INFO, ], ], + 'callsPrivateParentMethodThenUsesParentInitializedProperty' => [ + 'setBar(); + } + + private function setBar(): void { + $this->bar = "hello"; + } + } + + class B extends A { + public function __construct() { + parent::__construct(); + + echo $this->bar; + } + }', + ], 'setInFinalMethod' => [ ' [ + 'foo(); + } + + protected function foo(): void { + $this->a = 5; + } + } + + class B extends A { + const HELLO = "HELLO"; + + protected function foo() : void { + $this->a = 6; + + echo self::HELLO; + } + }', + ], ]; } @@ -1843,6 +1890,10 @@ class PropertyTypeTest extends TestCase protected function foo(): void { $this->a = 5; } + } + + class B extends A { + protected function foo() : void {} }', 'error_message' => 'PropertyNotSetInConstructor', ], @@ -1934,7 +1985,7 @@ class PropertyTypeTest extends TestCase } } }', - 'error_message' => 'PropertyNotSetInConstructor', + 'error_message' => 'InaccessibleProperty', ], 'privatePropertySetInParentConstructor' => [ '