From 6746f1c0479d11775a28519d2799fc573b90fc8b Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Sat, 21 Mar 2020 20:46:38 -0400 Subject: [PATCH] Fix #3004 - reset property types inside a closure defined in a class --- src/Psalm/Internal/Analyzer/ClassAnalyzer.php | 268 ++++++++++-------- .../Statements/ExpressionAnalyzer.php | 13 + .../RedundantConditionTest.php | 16 ++ 3 files changed, 172 insertions(+), 125 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php index 0e0c34e38dd..b52690675b5 100644 --- a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php @@ -704,131 +704,13 @@ public function analyze( } } - foreach ($storage->appearing_property_ids as $property_name => $appearing_property_id) { - $property_class_name = $codebase->properties->getDeclaringClassForProperty( - $appearing_property_id, - true - ); - - if ($property_class_name === null) { - continue; - } - - $property_class_storage = $classlike_storage_provider->get($property_class_name); - - $property_storage = $property_class_storage->properties[$property_name]; - - if (isset($storage->overridden_property_ids[$property_name])) { - foreach ($storage->overridden_property_ids[$property_name] as $overridden_property_id) { - list($guide_class_name) = explode('::$', $overridden_property_id); - $guide_class_storage = $classlike_storage_provider->get($guide_class_name); - $guide_property_storage = $guide_class_storage->properties[$property_name]; - - if ($property_storage->visibility > $guide_property_storage->visibility - && $property_storage->location - ) { - if (IssueBuffer::accepts( - new OverriddenPropertyAccess( - 'Property ' . $guide_class_storage->name . '::$' . $property_name - . ' has different access level than ' - . $storage->name . '::$' . $property_name, - $property_storage->location - ) - )) { - return false; - } - - return null; - } - } - } - - if ($property_storage->type) { - $property_type = clone $property_storage->type; - - if (!$property_type->isMixed() - && !$property_storage->has_default - && !($property_type->isNullable() && $property_type->from_docblock) - ) { - $property_type->initialized = false; - } - } else { - $property_type = Type::getMixed(); - - if (!$property_storage->has_default) { - $property_type->initialized = false; - } - } - - $property_type_location = $property_storage->type_location; - - $fleshed_out_type = !$property_type->isMixed() - ? ExpressionAnalyzer::fleshOutType( - $codebase, - $property_type, - $this->fq_class_name, - $this->fq_class_name, - $this->parent_fq_class_name - ) - : $property_type; - - $class_template_params = ClassTemplateParamCollector::collect( - $codebase, - $property_class_storage, - $fq_class_name, - null, - new Type\Atomic\TNamedObject($fq_class_name), - '$this' - ); - - $template_result = new \Psalm\Internal\Type\TemplateResult( - $class_template_params ?: [], - [] - ); - - if ($class_template_params) { - $fleshed_out_type = \Psalm\Internal\Type\UnionTemplateHandler::replaceTemplateTypesWithStandins( - $fleshed_out_type, - $template_result, - $codebase, - null, - null, - null - ); - } - - if ($property_type_location && !$fleshed_out_type->isMixed()) { - $fleshed_out_type->check( - $this, - $property_type_location, - $storage->suppressed_issues + $this->getSuppressedIssues(), - [], - false - ); - } - - if ($property_storage->is_static) { - $property_id = $this->fq_class_name . '::$' . $property_name; - - $class_context->vars_in_scope[$property_id] = $fleshed_out_type; - } else { - $class_context->vars_in_scope['$this->' . $property_name] = $fleshed_out_type; - } - } - - foreach ($storage->pseudo_property_get_types as $property_name => $property_type) { - $fleshed_out_type = !$property_type->isMixed() - ? ExpressionAnalyzer::fleshOutType( - $codebase, - $property_type, - $this->fq_class_name, - $this->fq_class_name, - $this->parent_fq_class_name - ) - : $property_type; - - $class_context->vars_in_scope['$this->' . substr($property_name, 1)] = $fleshed_out_type; - } + self::addContextProperties( + $this, + $storage, + $class_context, + $this->fq_class_name, + $this->parent_fq_class_name + ); $constructor_analyzer = null; $member_stmts = []; @@ -1071,6 +953,142 @@ public function analyze( } } + public static function addContextProperties( + StatementsSource $statements_source, + ClassLikeStorage $storage, + Context $class_context, + string $fq_class_name, + ?string $parent_fq_class_name + ) : void { + $codebase = $statements_source->getCodebase(); + + foreach ($storage->appearing_property_ids as $property_name => $appearing_property_id) { + $property_class_name = $codebase->properties->getDeclaringClassForProperty( + $appearing_property_id, + true + ); + + if ($property_class_name === null) { + continue; + } + + $property_class_storage = $codebase->classlike_storage_provider->get($property_class_name); + + $property_storage = $property_class_storage->properties[$property_name]; + + if (isset($storage->overridden_property_ids[$property_name])) { + foreach ($storage->overridden_property_ids[$property_name] as $overridden_property_id) { + list($guide_class_name) = explode('::$', $overridden_property_id); + $guide_class_storage = $classlike_storage_provider->get($guide_class_name); + $guide_property_storage = $guide_class_storage->properties[$property_name]; + + if ($property_storage->visibility > $guide_property_storage->visibility + && $property_storage->location + ) { + if (IssueBuffer::accepts( + new OverriddenPropertyAccess( + 'Property ' . $guide_class_storage->name . '::$' . $property_name + . ' has different access level than ' + . $storage->name . '::$' . $property_name, + $property_storage->location + ) + )) { + // fall through + } + + continue; + } + } + } + + if ($property_storage->type) { + $property_type = clone $property_storage->type; + + if (!$property_type->isMixed() + && !$property_storage->has_default + && !($property_type->isNullable() && $property_type->from_docblock) + ) { + $property_type->initialized = false; + } + } else { + $property_type = Type::getMixed(); + + if (!$property_storage->has_default) { + $property_type->initialized = false; + } + } + + $property_type_location = $property_storage->type_location; + + $fleshed_out_type = !$property_type->isMixed() + ? ExpressionAnalyzer::fleshOutType( + $codebase, + $property_type, + $fq_class_name, + $fq_class_name, + $parent_fq_class_name + ) + : $property_type; + + $class_template_params = ClassTemplateParamCollector::collect( + $codebase, + $property_class_storage, + $fq_class_name, + null, + new Type\Atomic\TNamedObject($fq_class_name), + '$this' + ); + + $template_result = new \Psalm\Internal\Type\TemplateResult( + $class_template_params ?: [], + [] + ); + + if ($class_template_params) { + $fleshed_out_type = \Psalm\Internal\Type\UnionTemplateHandler::replaceTemplateTypesWithStandins( + $fleshed_out_type, + $template_result, + $codebase, + null, + null, + null + ); + } + + if ($property_type_location && !$fleshed_out_type->isMixed()) { + $fleshed_out_type->check( + $statements_source, + $property_type_location, + $storage->suppressed_issues + $statements_source->getSuppressedIssues(), + [], + false + ); + } + + if ($property_storage->is_static) { + $property_id = $fq_class_name . '::$' . $property_name; + + $class_context->vars_in_scope[$property_id] = $fleshed_out_type; + } else { + $class_context->vars_in_scope['$this->' . $property_name] = $fleshed_out_type; + } + } + + foreach ($storage->pseudo_property_get_types as $property_name => $property_type) { + $fleshed_out_type = !$property_type->isMixed() + ? ExpressionAnalyzer::fleshOutType( + $codebase, + $property_type, + $fq_class_name, + $fq_class_name, + $parent_fq_class_name + ) + : $property_type; + + $class_context->vars_in_scope['$this->' . substr($property_name, 1)] = $fleshed_out_type; + } + } + /** * @return void */ diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php index 5ad1ed1559f..3378dec9b46 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php @@ -3,6 +3,7 @@ use PhpParser; use Psalm\Codebase; +use Psalm\Internal\Analyzer\ClassAnalyzer; use Psalm\Internal\Analyzer\ClassLikeAnalyzer; use Psalm\Internal\Analyzer\ClosureAnalyzer; use Psalm\Internal\Analyzer\CommentAnalyzer; @@ -493,6 +494,18 @@ public static function analyze( } } + if ($context->self) { + $self_class_storage = $codebase->classlike_storage_provider->get($context->self); + + ClassAnalyzer::addContextProperties( + $statements_analyzer, + $self_class_storage, + $use_context, + $statements_analyzer->getFQCLN(), + $statements_analyzer->getParentFQCLN() + ); + } + foreach ($context->vars_possibly_in_scope as $var => $_) { if (strpos($var, '$this->') === 0) { $use_context->vars_possibly_in_scope[$var] = true; diff --git a/tests/TypeReconciliation/RedundantConditionTest.php b/tests/TypeReconciliation/RedundantConditionTest.php index 59c3c712e3f..cf1dafdada4 100644 --- a/tests/TypeReconciliation/RedundantConditionTest.php +++ b/tests/TypeReconciliation/RedundantConditionTest.php @@ -783,6 +783,22 @@ function returnsInt() { if (is_int(returnsInt())) {} if (!is_int(returnsInt())) {}', ], + 'noRedundantConditionInClosureForProperty' => [ + 'closed) { + return function() : void { + if ($this->closed) {} + }; + } + + return function() : void {}; + } + }' + ], ]; }