From 62fb8035bf9d4c2cdab1b37104fa1e0fd114764d Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Sun, 10 Feb 2019 15:01:10 -0500 Subject: [PATCH] Fix #1309 - no PropertyNotSetInConstructor warnings for grandchild of class --- src/Psalm/Internal/Analyzer/ClassAnalyzer.php | 30 +++- .../Analyzer/FunctionLikeAnalyzer.php | 1 + .../Assignment/PropertyAssignmentAnalyzer.php | 1 - .../Expression/AssignmentAnalyzer.php | 3 +- .../Expression/Call/MethodCallAnalyzer.php | 3 +- .../Expression/Call/StaticCallAnalyzer.php | 152 +++++++++--------- src/Psalm/IssueBuffer.php | 4 + tests/PropertyTypeTest.php | 36 ++++- tests/Template/TemplateExtendsTest.php | 90 +++++++++++ 9 files changed, 238 insertions(+), 82 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php index 5c4753f3755..ed4db33ef6a 100644 --- a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php @@ -519,6 +519,27 @@ public function analyze( ) : $property_type; + /** + * @psalm-suppress ReferenceConstraintViolation + */ + $class_template_params = MethodCallAnalyzer::getClassTemplateParams( + $codebase, + $property_class_storage, + $fq_class_name, + null, + new Type\Atomic\TNamedObject($fq_class_name), + '$this' + ); + + if ($class_template_params) { + $generic_params = []; + $fleshed_out_type->replaceTemplateTypesWithStandins( + $class_template_params, + $generic_params, + $codebase + ); + } + if ($property_type_location && !$fleshed_out_type->isMixed()) { $fleshed_out_type->check( $this, @@ -834,11 +855,11 @@ function (FunctionLikeParameter $param) { $fake_constructor_stmts = [ new PhpParser\Node\Stmt\Expression( new PhpParser\Node\Expr\StaticCall( - new PhpParser\Node\Name(['parent']), + new PhpParser\Node\Name\FullyQualified($constructor_declaring_fqcln), new PhpParser\Node\Identifier('__construct'), $fake_constructor_stmt_args, [ - 'line' => $class->extends->getLine(), + 'startLine' => $class->extends->getLine(), 'startFilePos' => $class->extends->getAttribute('startFilePos'), 'endFilePos' => $class->extends->getAttribute('endFilePos'), ] @@ -852,6 +873,11 @@ function (FunctionLikeParameter $param) { 'type' => PhpParser\Node\Stmt\Class_::MODIFIER_PUBLIC, 'params' => $fake_constructor_params, 'stmts' => $fake_constructor_stmts, + ], + [ + 'startLine' => $class->extends->getLine(), + 'startFilePos' => $class->extends->getAttribute('startFilePos'), + 'endFilePos' => $class->extends->getAttribute('endFilePos'), ] ); diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php index 2808c4c3cc9..c4908604dd6 100644 --- a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php @@ -8,6 +8,7 @@ use Psalm\Codebase; use Psalm\Internal\Analyzer\FunctionLike\ReturnTypeAnalyzer; use Psalm\Internal\Analyzer\FunctionLike\ReturnTypeCollector; +use Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer; use Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer; use Psalm\Internal\Codebase\CallMap; use Psalm\CodeLocation; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/PropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/PropertyAssignmentAnalyzer.php index 8ac26389ce4..210ceda35a2 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/PropertyAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/PropertyAssignmentAnalyzer.php @@ -434,7 +434,6 @@ public static function analyzeInstance( } } - $declaring_property_class = (string) $codebase->properties->getDeclaringClassForProperty( $property_id ); diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index f2c7e60c2e1..f3b1f1be86e 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -248,7 +248,8 @@ public static function analyze( new ReferenceConstraintViolation( 'Variable ' . $var_id . ' is limited to values of type ' . $context->byref_constraints[$var_id]->type - . ' because it is passed by reference', + . ' because it is passed by reference, ' + . $assign_value_type->getId() . ' type found', new CodeLocation($statements_analyzer->getSource(), $assign_var) ), $statements_analyzer->getSuppressedIssues() diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/MethodCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/MethodCallAnalyzer.php index c622d0f9128..b5a653a2b34 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/MethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/MethodCallAnalyzer.php @@ -1087,7 +1087,7 @@ public static function getClassTemplateParams( Codebase $codebase, ClassLikeStorage $class_storage, string $fq_class_name, - string $method_name, + string $method_name = null, Type\Atomic $lhs_type_part = null, string $lhs_var_id = null ) { @@ -1097,6 +1097,7 @@ public static function getClassTemplateParams( if (!$template_types) { if ($calling_class_storage->template_type_extends + && $method_name && !empty($class_storage->overridden_method_ids[$method_name]) && !isset($class_storage->methods[$method_name]->return_type) ) { diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php index d01d708a603..759e3bc0b34 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php @@ -84,82 +84,6 @@ public static function analyze( $class_storage = $codebase->classlike_storage_provider->get($fq_class_name); $fq_class_name = $class_storage->name; - - if ($stmt->name instanceof PhpParser\Node\Identifier - && $class_storage->user_defined - && ($context->collect_mutations || $context->collect_initializations) - ) { - $method_id = $fq_class_name . '::' . strtolower($stmt->name->name); - - $appearing_method_id = $codebase->getAppearingMethodId($method_id); - - if (!$appearing_method_id) { - if (IssueBuffer::accepts( - new UndefinedMethod( - 'Method ' . $method_id . ' does not exist', - new CodeLocation($statements_analyzer->getSource(), $stmt), - $method_id - ), - $statements_analyzer->getSuppressedIssues() - )) { - return false; - } - - return; - } - - list($appearing_method_class_name) = explode('::', $appearing_method_id); - - $old_context_include_location = $context->include_location; - $old_self = $context->self; - $context->include_location = new CodeLocation($statements_analyzer->getSource(), $stmt); - $context->self = $appearing_method_class_name; - - if ($context->collect_mutations) { - $file_analyzer->getMethodMutations($method_id, $context); - } else { - // collecting initializations - $local_vars_in_scope = []; - $local_vars_possibly_in_scope = []; - - foreach ($context->vars_in_scope as $var => $_) { - if (strpos($var, '$this->') !== 0 && $var !== '$this') { - $local_vars_in_scope[$var] = $context->vars_in_scope[$var]; - } - } - - foreach ($context->vars_possibly_in_scope as $var => $_) { - if (strpos($var, '$this->') !== 0 && $var !== '$this') { - $local_vars_possibly_in_scope[$var] = $context->vars_possibly_in_scope[$var]; - } - } - - if (!isset($context->initialized_methods[$method_id])) { - if ($context->initialized_methods === null) { - $context->initialized_methods = []; - } - - $context->initialized_methods[$method_id] = true; - - $file_analyzer->getMethodMutations($method_id, $context); - - foreach ($local_vars_in_scope as $var => $type) { - $context->vars_in_scope[$var] = $type; - } - - foreach ($local_vars_possibly_in_scope as $var => $type) { - $context->vars_possibly_in_scope[$var] = $type; - } - } - } - - $context->include_location = $old_context_include_location; - $context->self = $old_self; - - if (isset($context->vars_in_scope['$this']) && $old_self) { - $context->vars_in_scope['$this'] = Type::parseString($old_self); - } - } } elseif ($context->self) { if ($stmt->class->parts[0] === 'static' && isset($context->vars_in_scope['$this'])) { $fq_class_name = (string) $context->vars_in_scope['$this']; @@ -543,6 +467,82 @@ function (PhpParser\Node\Arg $arg) { $class_storage = $codebase->classlike_storage_provider->get($fq_class_name); + if ($class_storage->user_defined + && $context->self + && ($context->collect_mutations || $context->collect_initializations) + ) { + $appearing_method_id = $codebase->getAppearingMethodId($method_id); + + if (!$appearing_method_id) { + if (IssueBuffer::accepts( + new UndefinedMethod( + 'Method ' . $method_id . ' does not exist', + new CodeLocation($statements_analyzer->getSource(), $stmt), + $method_id + ), + $statements_analyzer->getSuppressedIssues() + )) { + return false; + } + + return; + } + + list($appearing_method_class_name) = explode('::', $appearing_method_id); + + if ($codebase->classExtends($context->self, $appearing_method_class_name)) { + $old_context_include_location = $context->include_location; + $old_self = $context->self; + $context->include_location = new CodeLocation($statements_analyzer->getSource(), $stmt); + $context->self = $appearing_method_class_name; + + if ($context->collect_mutations) { + $file_analyzer->getMethodMutations($method_id, $context); + } else { + // collecting initializations + $local_vars_in_scope = []; + $local_vars_possibly_in_scope = []; + + foreach ($context->vars_in_scope as $var => $_) { + if (strpos($var, '$this->') !== 0 && $var !== '$this') { + $local_vars_in_scope[$var] = $context->vars_in_scope[$var]; + } + } + + foreach ($context->vars_possibly_in_scope as $var => $_) { + if (strpos($var, '$this->') !== 0 && $var !== '$this') { + $local_vars_possibly_in_scope[$var] = $context->vars_possibly_in_scope[$var]; + } + } + + if (!isset($context->initialized_methods[$method_id])) { + if ($context->initialized_methods === null) { + $context->initialized_methods = []; + } + + $context->initialized_methods[$method_id] = true; + + $file_analyzer->getMethodMutations($method_id, $context); + + foreach ($local_vars_in_scope as $var => $type) { + $context->vars_in_scope[$var] = $type; + } + + foreach ($local_vars_possibly_in_scope as $var => $type) { + $context->vars_possibly_in_scope[$var] = $type; + } + } + } + + $context->include_location = $old_context_include_location; + $context->self = $old_self; + + if (isset($context->vars_in_scope['$this']) && $old_self) { + $context->vars_in_scope['$this'] = Type::parseString($old_self); + } + } + } + if ($class_storage->deprecated) { if (IssueBuffer::accepts( new DeprecatedClass( diff --git a/src/Psalm/IssueBuffer.php b/src/Psalm/IssueBuffer.php index 12385a30aae..1598c2d590a 100644 --- a/src/Psalm/IssueBuffer.php +++ b/src/Psalm/IssueBuffer.php @@ -97,6 +97,10 @@ public static function accepts(CodeIssue $e, array $suppressed_issues = []) } } + if ($e->getLocation()->getLineNumber() === -1) { + return false; + } + if (self::$recording_level > 0) { self::$recorded_issues[self::$recording_level][] = $e; diff --git a/tests/PropertyTypeTest.php b/tests/PropertyTypeTest.php index 029e00afc5b..95a531bef76 100644 --- a/tests/PropertyTypeTest.php +++ b/tests/PropertyTypeTest.php @@ -1278,6 +1278,40 @@ public function foo($b) : void { } }' ], + 'propertySetInGrandparentExplicitly' => [ + 's = $s; + } + } + class B extends A {} + class C extends B { + public function __construct(string $s) { + A::__construct($s); + } + }' + ], + 'propertySetInGrandparentImplicitly' => [ + 's = $s; + } + } + class B extends A {} + class C extends B {}' + ], ]; } @@ -1655,7 +1689,7 @@ private function __construct() { } class B extends A {}', - 'error_message' => 'InaccessibleMethod', + 'error_message' => 'InaccessibleMethod - src/somefile.php:11', ], 'classInheritsPrivateConstructorWithImplementedConstructor' => [ ' 'AContainer', ], ], + 'returnParentExtendedTemplateProperty' => [ + 't = $t; + } + } + + /** + * @template-extends Container + */ + class IntContainer extends Container { + public function __construct(int $i) { + parent::__construct($i); + } + + public function getValue() : int { + return $this->t; + } + }', + ], + 'childSetInConstructor' => [ + 't = $t; + } + } + + /** + * @template T1 as object + * @template-extends Container + */ + class ObjectContainer extends Container {}', + ], + 'grandChildSetInConstructor' => [ + 't = $t; + } + } + + /** + * @template T1 as object + * @template-extends Container + */ + class ObjectContainer extends Container {} + + /** + * @template T2 as A + * @template-extends ObjectContainer + */ + class AContainer extends ObjectContainer {} + + class A {}', + ], ]; }