From 843305c8e05f732ef02c35e1bca5bde37acfd160 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Fri, 19 Mar 2021 20:55:50 -0500 Subject: [PATCH] Support template property invariance (fixes #5371) (#5414) * Support property invariance with templates. * Fix false positive NonInvariantDocblockPropertyType with grandchild. * Redo templated property invariance check to fix issues. * Add template covariant test. * Fix property invariance false positive with template-covariant. --- src/Psalm/Internal/Analyzer/ClassAnalyzer.php | 78 ++++++- .../TypeVisitor/ContainsTemplateVisitor.php | 33 --- src/Psalm/Type/Union.php | 12 - tests/PropertyTypeInvarianceTest.php | 208 ++++++++++++++++-- 4 files changed, 258 insertions(+), 73 deletions(-) delete mode 100644 src/Psalm/Internal/TypeVisitor/ContainsTemplateVisitor.php diff --git a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php index c6246db99b6..09143db63e5 100644 --- a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php @@ -14,6 +14,7 @@ use Psalm\CodeLocation; use Psalm\Config; use Psalm\Context; +use Psalm\Internal\Type\TemplateInferredTypeReplacer; use Psalm\Issue\DeprecatedClass; use Psalm\Issue\DeprecatedInterface; use Psalm\Issue\DeprecatedTrait; @@ -71,6 +72,7 @@ use function array_merge; use function array_filter; use function in_array; +use function assert; /** * @internal @@ -705,20 +707,82 @@ public static function addContextProperties( } } - if ($property_storage->type - && $guide_property_storage->type - && $property_storage->location - && !$property_storage->type->equals($guide_property_storage->type) - && !$guide_property_storage->type->containsTemplate() + if ($property_storage->type === null) { + // Property type not set, no need to check for docblock invariance + continue; + } + + $property_type = clone $property_storage->type; + + $guide_property_type = $guide_property_storage->type === null + ? Type::getMixed() + : clone $guide_property_storage->type; + + // Set upper bounds for all templates + $upper_bounds = []; + $extended_templates = $storage->template_extended_params ?? []; + foreach ($extended_templates as $et_name => $et_array) { + foreach ($et_array as $et_class_name => $extended_template) { + if (!isset($upper_bounds[$et_class_name][$et_name])) { + $upper_bounds[$et_class_name][$et_name] = $extended_template; + } + } + } + + // Get actual types used for templates (to support @template-covariant) + $template_standins = new \Psalm\Internal\Type\TemplateResult($upper_bounds, []); + TemplateStandinTypeReplacer::replace( + $guide_property_type, + $template_standins, + $codebase, + null, + $property_type + ); + + // Iterate over parent classes to find template-covariants, and replace the upper bound with the + // standin. Since @template-covariant allows child classes, we want to use the standin type + // instead of the template extended type. + $parent_class = $storage->parent_class; + while ($parent_class !== null) { + $parent_storage = $codebase->classlike_storage_provider->get($parent_class); + foreach ($parent_storage->template_covariants ?? [] as $pt_offset => $covariant) { + if ($covariant) { + // If template_covariants is set template_types should also be set + assert($parent_storage->template_types !== null); + $pt_name = array_keys($parent_storage->template_types)[$pt_offset]; + if (isset($template_standins->upper_bounds[$pt_name][$parent_class])) { + $upper_bounds[$pt_name][$parent_class] = + $template_standins->upper_bounds[$pt_name][$parent_class]->type; + } + } + } + $parent_class = $parent_storage->parent_class; + } + + $template_result = new \Psalm\Internal\Type\TemplateResult([], $upper_bounds); + + TemplateInferredTypeReplacer::replace( + $guide_property_type, + $template_result, + $codebase + ); + TemplateInferredTypeReplacer::replace( + $property_type, + $template_result, + $codebase + ); + + if ($property_storage->location + && !$property_type->equals($guide_property_type) && $guide_class_storage->user_defined ) { if (IssueBuffer::accepts( new NonInvariantDocblockPropertyType( 'Property ' . $fq_class_name . '::$' . $property_name - . ' has type ' . $property_storage->type->getId() + . ' has type ' . $property_type->getId() . ", not invariant with " . $guide_class_name . '::$' . $property_name . ' of type ' - . $guide_property_storage->type->getId(), + . $guide_property_type->getId(), $property_storage->location ), $property_storage->suppressed_issues diff --git a/src/Psalm/Internal/TypeVisitor/ContainsTemplateVisitor.php b/src/Psalm/Internal/TypeVisitor/ContainsTemplateVisitor.php deleted file mode 100644 index 5f99d23e136..00000000000 --- a/src/Psalm/Internal/TypeVisitor/ContainsTemplateVisitor.php +++ /dev/null @@ -1,33 +0,0 @@ -contains_template = true; - return NodeVisitor::STOP_TRAVERSAL; - } - - return null; - } - - public function matches(): bool - { - return $this->contains_template; - } -} diff --git a/src/Psalm/Type/Union.php b/src/Psalm/Type/Union.php index 7db42dccdee..15d93c80ec8 100644 --- a/src/Psalm/Type/Union.php +++ b/src/Psalm/Type/Union.php @@ -1320,18 +1320,6 @@ public function containsClassLike(string $fq_class_like_name) : bool return $classlike_visitor->matches(); } - /** - * Returns true if union contains template at any nested level. - */ - public function containsTemplate(): bool - { - $template_visitor = new \Psalm\Internal\TypeVisitor\ContainsTemplateVisitor(); - - $template_visitor->traverseArray($this->types); - - return $template_visitor->matches(); - } - /** * @return list */ diff --git a/tests/PropertyTypeInvarianceTest.php b/tests/PropertyTypeInvarianceTest.php index 455284c3f4f..2130d92be05 100644 --- a/tests/PropertyTypeInvarianceTest.php +++ b/tests/PropertyTypeInvarianceTest.php @@ -99,7 +99,7 @@ class FooCollection extends ItemCollection protected $items; }', ], - 'allowTemplatedInvarianceWithClassString' => [ + 'allowTemplatedInvarianceWithClassStringTemplate' => [ ' [ + ' */ + protected $items = []; + } + + /** + * @template T of Foo + * @extends ItemCollection + */ + class FooCollection extends ItemCollection + { + /** @var list */ + protected $items = []; + } + + /** @extends FooCollection */ + class BarCollection extends FooCollection + { + /** @var list */ + protected $items = []; + }', + ], + 'allowTemplateCovariant' => [ + ' */ + class FooPair extends Pair + { + /** @var Bar|null */ + public $a; + + /** @var Baz|null */ + public $b; + }', + ], + 'allowTemplateCovariantManyTemplates' => [ + ' + */ + class Bar extends Foo {} + + /** + * @template Ta + * @template Tb + * @template-covariant Tc + * @template Td + * @extends Bar + */ + class Baz extends Bar { + /** @var A|null */ + public $a; + + /** @var B|null */ + public $b; + + /** @var C|null */ + public $c; + + /** @var C|null */ + public $d; + }', + ], ]; } @@ -155,26 +267,80 @@ class ChildClass extends ParentClass }', 'error_message' => 'NonInvariantPropertyType', ], - // TODO support property invariance checks with templates - // 'disallowInvalidTemplatedInvariance' => [ - // ' - // */ - // class AChild extends A { - // /** @var int */ - // public $foo = 0; - // }', - // 'error_message' => 'NonInvariantPropertyType', - // ], + 'variantTemplatedProperties' => [ + ' + */ + class AChild extends A { + /** @var int */ + public $foo = 0; + }', + 'error_message' => 'NonInvariantDocblockPropertyType', + ], + 'variantTemplatedGrandchild' => [ + ' */ + protected $items = []; + } + + /** + * @template T of Foo + * @extends ItemCollection + */ + class FooCollection extends ItemCollection + { + /** @var list */ + protected $items = []; + } + + /** @extends FooCollection */ + class BarCollection extends FooCollection + { + /** @var list */ // Should be list + protected $items = []; + }', + 'error_message' => 'NonInvariantDocblockPropertyType', + ], + 'variantPropertiesWithTemplateNotSpecified' => [ + ' 'NonInvariantDocblockPropertyType', + ], ]; } }