Skip to content

Commit

Permalink
Support template property invariance (fixes #5371) (#5414)
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@AndrolGenhald
AndrolGenhald authored Mar 20, 2021
1 parent 17f23cc commit 843305c
Show file tree
Hide file tree
Showing 4 changed files with 258 additions and 73 deletions.
78 changes: 71 additions & 7 deletions src/Psalm/Internal/Analyzer/ClassAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -71,6 +72,7 @@
use function array_merge;
use function array_filter;
use function in_array;
use function assert;

/**
* @internal
Expand Down Expand Up @@ -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
Expand Down
33 changes: 0 additions & 33 deletions src/Psalm/Internal/TypeVisitor/ContainsTemplateVisitor.php
View file

This file was deleted.

12 changes: 0 additions & 12 deletions src/Psalm/Type/Union.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<TTemplateParam>
*/
Expand Down
208 changes: 187 additions & 21 deletions tests/PropertyTypeInvarianceTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class FooCollection extends ItemCollection
protected $items;
}',
],
'allowTemplatedInvarianceWithClassString' => [
'allowTemplatedInvarianceWithClassStringTemplate' => [
'<?php
abstract class Item {}
class Foo extends Item {}
Expand All @@ -118,6 +118,118 @@ class FooTypes extends ItemType
protected $type;
}',
],
'templatedInvarianceGrandchild' => [
'<?php
abstract class Item {}
class Foo extends Item {}
class Bar extends Foo {}
/** @template T of Item */
abstract class ItemCollection
{
/** @var list<T> */
protected $items = [];
}
/**
* @template T of Foo
* @extends ItemCollection<T>
*/
class FooCollection extends ItemCollection
{
/** @var list<T> */
protected $items = [];
}
/** @extends FooCollection<Bar> */
class BarCollection extends FooCollection
{
/** @var list<Bar> */
protected $items = [];
}',
],
'allowTemplateCovariant' => [
'<?php
class Foo {}
class Bar extends Foo {}
class Baz extends Bar {}
/** @template-covariant T */
class Pair
{
/** @var T|null */
public $a;
/** @var T|null */
public $b;
}
/** @extends Pair<Foo> */
class FooPair extends Pair
{
/** @var Bar|null */
public $a;
/** @var Baz|null */
public $b;
}',
],
'allowTemplateCovariantManyTemplates' => [
'<?php
class A {}
class B extends A {}
class C extends B {}
/**
* @template Ta
* @template Tb
* @template-covariant Tc
* @template Td
*/
class Foo {
/** @var Ta|null */
public $a;
/** @var Tb|null */
public $b;
/** @var Tc|null */
public $c;
/** @var Td|null */
public $d;
}
/**
* @template Ta
* @template Tb
* @template-covariant Tc
* @template Td
* @extends Foo<Ta, Tb, Tc, Td>
*/
class Bar extends Foo {}
/**
* @template Ta
* @template Tb
* @template-covariant Tc
* @template Td
* @extends Bar<A, B, A, C>
*/
class Baz extends Bar {
/** @var A|null */
public $a;
/** @var B|null */
public $b;
/** @var C|null */
public $c;
/** @var C|null */
public $d;
}',
],
];
}

Expand Down Expand Up @@ -155,26 +267,80 @@ class ChildClass extends ParentClass
}',
'error_message' => 'NonInvariantPropertyType',
],
// TODO support property invariance checks with templates
// 'disallowInvalidTemplatedInvariance' => [
// '<?php
// /**
// * @template T as string|null
// */
// abstract class A {
// /** @var T */
// public $foo;
// }

// /**
// * @extends A<string>
// */
// class AChild extends A {
// /** @var int */
// public $foo = 0;
// }',
// 'error_message' => 'NonInvariantPropertyType',
// ],
'variantTemplatedProperties' => [
'<?php
/**
* @template T as string|null
*/
abstract class A {
/** @var T */
public $foo;
}
/**
* @extends A<string>
*/
class AChild extends A {
/** @var int */
public $foo = 0;
}',
'error_message' => 'NonInvariantDocblockPropertyType',
],
'variantTemplatedGrandchild' => [
'<?php
abstract class Item {}
class Foo extends Item {}
class Bar extends Foo {}
/** @template T of Item */
abstract class ItemCollection
{
/** @var list<T> */
protected $items = [];
}
/**
* @template T of Foo
* @extends ItemCollection<T>
*/
class FooCollection extends ItemCollection
{
/** @var list<T> */
protected $items = [];
}
/** @extends FooCollection<Bar> */
class BarCollection extends FooCollection
{
/** @var list<Item> */ // Should be list<Bar>
protected $items = [];
}',
'error_message' => 'NonInvariantDocblockPropertyType',
],
'variantPropertiesWithTemplateNotSpecified' => [
'<?php
class Foo {}
/** @template T */
class Pair
{
/** @var T|null */
protected $a;
/** @var T|null */
protected $b;
}
class FooPair extends Pair
{
/** @var Foo|null */ // Template defaults to mixed, this is invariant
protected $a;
/** @var Foo|null */
protected $b;
}',
'error_message' => 'NonInvariantDocblockPropertyType',
],
];
}
}

0 comments on commit 843305c

Please sign in to comment.