Skip to content

Commit

Permalink
Fix #3004 - reset property types inside a closure defined in a class
Browse files Browse the repository at this point in the history
  • Loading branch information
@muglug
muglug committed Mar 22, 2020
1 parent c986cdf commit 6746f1c
Show file tree
Hide file tree
Showing 3 changed files with 172 additions and 125 deletions.
268 changes: 143 additions & 125 deletions src/Psalm/Internal/Analyzer/ClassAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];
Expand Down Expand Up @@ -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
*/
Expand Down
13 changes: 13 additions & 0 deletions src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
16 changes: 16 additions & 0 deletions tests/TypeReconciliation/RedundantConditionTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,22 @@ function returnsInt() {
if (is_int(returnsInt())) {}
if (!is_int(returnsInt())) {}',
],
'noRedundantConditionInClosureForProperty' => [
'<?php
class Queue {
private bool $closed = false;
public function enqueue(string $value): Closure {
if ($this->closed) {
return function() : void {
if ($this->closed) {}
};
}
return function() : void {};
}
}'
],
];
}

Expand Down

0 comments on commit 6746f1c

Please sign in to comment.