From 19f6dda96d38eb7b507550e5d10929972d4fc57e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pudil?= Date: Tue, 28 Mar 2023 15:00:19 +0200 Subject: [PATCH 1/3] generics: check variance of template types in properties --- conf/bleedingEdge.neon | 1 + conf/config.level2.neon | 4 + conf/config.neon | 2 + src/Rules/Generics/PropertyVarianceRule.php | 52 +++++++ src/Rules/Generics/VarianceCheck.php | 35 +++++ .../Generics/PropertyVarianceRuleTest.php | 136 ++++++++++++++++++ .../data/property-variance-promoted.php | 93 ++++++++++++ .../data/property-variance-readonly.php | 89 ++++++++++++ .../Rules/Generics/data/property-variance.php | 102 +++++++++++++ 9 files changed, 514 insertions(+) create mode 100644 src/Rules/Generics/PropertyVarianceRule.php create mode 100644 tests/PHPStan/Rules/Generics/PropertyVarianceRuleTest.php create mode 100644 tests/PHPStan/Rules/Generics/data/property-variance-promoted.php create mode 100644 tests/PHPStan/Rules/Generics/data/property-variance-readonly.php create mode 100644 tests/PHPStan/Rules/Generics/data/property-variance.php diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 820aadd5943..85de2394f7f 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -34,3 +34,4 @@ parameters: paramOutVariance: true allInvalidPhpDocs: true strictStaticMethodTemplateTypeVariance: true + propertyVariance: true diff --git a/conf/config.level2.neon b/conf/config.level2.neon index 4b7175287a2..2637ee81df1 100644 --- a/conf/config.level2.neon +++ b/conf/config.level2.neon @@ -53,6 +53,8 @@ conditionalTags: phpstan.rules.rule: %featureToggles.illegalConstructorMethodCall% PHPStan\Rules\PhpDoc\VarTagChangedExpressionTypeRule: phpstan.rules.rule: %featureToggles.varTagType% + PHPStan\Rules\Generics\PropertyVarianceRule: + phpstan.rules.rule: %featureToggles.propertyVariance% services: - @@ -102,3 +104,5 @@ services: checkTypeAgainstNativeType: %featureToggles.varTagType% tags: - phpstan.rules.rule + - + class: PHPStan\Rules\Generics\PropertyVarianceRule diff --git a/conf/config.neon b/conf/config.neon index eecbe3a58bc..ffad338cf95 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -64,6 +64,7 @@ parameters: paramOutVariance: false allInvalidPhpDocs: false strictStaticMethodTemplateTypeVariance: false + propertyVariance: false fileExtensions: - php checkAdvancedIsset: false @@ -298,6 +299,7 @@ parametersSchema: paramOutVariance: bool() allInvalidPhpDocs: bool() strictStaticMethodTemplateTypeVariance: bool() + propertyVariance: bool() ]) fileExtensions: listOf(string()) checkAdvancedIsset: bool() diff --git a/src/Rules/Generics/PropertyVarianceRule.php b/src/Rules/Generics/PropertyVarianceRule.php new file mode 100644 index 00000000000..c60836ee6e0 --- /dev/null +++ b/src/Rules/Generics/PropertyVarianceRule.php @@ -0,0 +1,52 @@ + + */ +class PropertyVarianceRule implements Rule +{ + + public function __construct(private VarianceCheck $varianceCheck) + { + } + + public function getNodeType(): string + { + return ClassPropertyNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $classReflection = $scope->getClassReflection(); + if (!$classReflection instanceof ClassReflection) { + return []; + } + + if (!$classReflection->hasNativeProperty($node->getName())) { + return []; + } + + $propertyReflection = $classReflection->getNativeProperty($node->getName()); + + if ($propertyReflection->isPrivate()) { + return []; + } + + return $this->varianceCheck->checkProperty( + $propertyReflection, + sprintf('in property %s::$%s', SprintfHelper::escapeFormatString($classReflection->getDisplayName()), SprintfHelper::escapeFormatString($node->getName())), + $propertyReflection->isReadOnly(), + ); + } + +} diff --git a/src/Rules/Generics/VarianceCheck.php b/src/Rules/Generics/VarianceCheck.php index 432a47af4ac..66e546d86de 100644 --- a/src/Rules/Generics/VarianceCheck.php +++ b/src/Rules/Generics/VarianceCheck.php @@ -3,6 +3,7 @@ namespace PHPStan\Rules\Generics; use PHPStan\Reflection\ParametersAcceptorWithPhpDocs; +use PHPStan\Reflection\PropertyReflection; use PHPStan\Rules\RuleError; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Generic\TemplateType; @@ -87,6 +88,40 @@ public function checkParametersAcceptor( return $errors; } + public function checkProperty( + PropertyReflection $propertyReflection, + string $message, + bool $isReadOnly, + ): array + { + $readableType = $propertyReflection->getReadableType(); + $writableType = $propertyReflection->getWritableType(); + + if ($readableType->equals($writableType)) { + $variance = $isReadOnly + ? TemplateTypeVariance::createCovariant() + : TemplateTypeVariance::createInvariant(); + + return $this->check($variance, $readableType, $message); + } + + $errors = []; + + if ($propertyReflection->isReadable()) { + foreach ($this->check(TemplateTypeVariance::createCovariant(), $readableType, $message) as $error) { + $errors[] = $error; + } + } + + if ($propertyReflection->isWritable()) { + foreach ($this->check(TemplateTypeVariance::createContravariant(), $writableType, $message) as $error) { + $errors[] = $error; + } + } + + return $errors; + } + /** @return RuleError[] */ public function check(TemplateTypeVariance $positionVariance, Type $type, string $messageContext): array { diff --git a/tests/PHPStan/Rules/Generics/PropertyVarianceRuleTest.php b/tests/PHPStan/Rules/Generics/PropertyVarianceRuleTest.php new file mode 100644 index 00000000000..e2df98ad990 --- /dev/null +++ b/tests/PHPStan/Rules/Generics/PropertyVarianceRuleTest.php @@ -0,0 +1,136 @@ + + */ +class PropertyVarianceRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new PropertyVarianceRule( + self::getContainer()->getByType(VarianceCheck::class), + ); + } + + public function testRule(): void + { + $this->analyse([__DIR__ . '/data/property-variance.php'], [ + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\B::$a.', + 51, + ], + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\B::$b.', + 54, + ], + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\B::$c.', + 57, + ], + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\B::$d.', + 60, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\C::$a.', + 80, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\C::$b.', + 83, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\C::$c.', + 86, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\C::$d.', + 89, + ], + ]); + } + + public function testPromoted(): void + { + if (PHP_VERSION_ID < 80000) { + $this->markTestSkipped('Test requires PHP 8.0.'); + } + + $this->analyse([__DIR__ . '/data/property-variance-promoted.php'], [ + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\Promoted\B::$a.', + 58, + ], + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\Promoted\B::$b.', + 59, + ], + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\Promoted\B::$c.', + 60, + ], + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\Promoted\B::$d.', + 61, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\Promoted\C::$a.', + 84, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\Promoted\C::$b.', + 85, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\Promoted\C::$c.', + 86, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\Promoted\C::$d.', + 87, + ], + ]); + } + + public function testReadOnly(): void + { + if (PHP_VERSION_ID < 80100) { + $this->markTestSkipped('Test requires PHP 8.1.'); + } + + $this->analyse([__DIR__ . '/data/property-variance-readonly.php'], [ + [ + 'Template type X is declared as covariant, but occurs in contravariant position in property PropertyVariance\ReadOnly\B::$b.', + 45, + ], + [ + 'Template type X is declared as covariant, but occurs in invariant position in property PropertyVariance\ReadOnly\B::$d.', + 51, + ], + [ + 'Template type X is declared as contravariant, but occurs in covariant position in property PropertyVariance\ReadOnly\C::$a.', + 62, + ], + [ + 'Template type X is declared as contravariant, but occurs in covariant position in property PropertyVariance\ReadOnly\C::$c.', + 68, + ], + [ + 'Template type X is declared as contravariant, but occurs in invariant position in property PropertyVariance\ReadOnly\C::$d.', + 71, + ], + [ + 'Template type X is declared as contravariant, but occurs in covariant position in property PropertyVariance\ReadOnly\D::$a.', + 86, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/Generics/data/property-variance-promoted.php b/tests/PHPStan/Rules/Generics/data/property-variance-promoted.php new file mode 100644 index 00000000000..fa56e56822c --- /dev/null +++ b/tests/PHPStan/Rules/Generics/data/property-variance-promoted.php @@ -0,0 +1,93 @@ += 8.0 + +namespace PropertyVariance\Promoted; + +/** @template-contravariant T */ +interface In { +} + +/** @template-covariant T */ +interface Out { +} + +/** @template T */ +interface Invariant { +} + +/** + * @template X + */ +class A { + /** + * @param X $a + * @param In $b + * @param Out $c + * @param Invariant $d + * @param X $e + * @param In $f + * @param Out $g + * @param Invariant $h + */ + public function __construct( + public $a, + public $b, + public $c, + public $d, + private $e, + private $f, + private $g, + private $h, + ) {} +} + +/** + * @template-covariant X + */ +class B { + /** + * @param X $a + * @param In $b + * @param Out $c + * @param Invariant $d + * @param X $e + * @param In $f + * @param Out $g + * @param Invariant $h + */ + public function __construct( + public $a, + public $b, + public $c, + public $d, + private $e, + private $f, + private $g, + private $h, + ) {} +} + +/** + * @template-contravariant X + */ +class C { + /** + * @param X $a + * @param In $b + * @param Out $c + * @param Invariant $d + * @param X $e + * @param In $f + * @param Out $g + * @param Invariant $h + */ + public function __construct( + public $a, + public $b, + public $c, + public $d, + private $e, + private $f, + private $g, + private $h, + ) {} +} diff --git a/tests/PHPStan/Rules/Generics/data/property-variance-readonly.php b/tests/PHPStan/Rules/Generics/data/property-variance-readonly.php new file mode 100644 index 00000000000..faefc2bd8ba --- /dev/null +++ b/tests/PHPStan/Rules/Generics/data/property-variance-readonly.php @@ -0,0 +1,89 @@ += 8.1 + +namespace PropertyVariance\ReadOnly; + +/** @template-contravariant T */ +interface In { +} + +/** @template-covariant T */ +interface Out { +} + +/** @template T */ +interface Invariant { +} + +/** + * @template X + */ +class A { + /** @var X */ + public readonly mixed $a; + + /** @var In */ + public readonly mixed $b; + + /** @var Out */ + public readonly mixed $c; + + /** @var Invariant */ + public readonly mixed $d; + + /** @var Invariant */ + private readonly mixed $e; +} + +/** + * @template-covariant X + */ +class B { + /** @var X */ + public readonly mixed $a; + + /** @var In */ + public readonly mixed $b; + + /** @var Out */ + public readonly mixed $c; + + /** @var Invariant */ + public readonly mixed $d; + + /** @var Invariant */ + private readonly mixed $e; +} + +/** + * @template-contravariant X + */ +class C { + /** @var X */ + public readonly mixed $a; + + /** @var In */ + public readonly mixed $b; + + /** @var Out */ + public readonly mixed $c; + + /** @var Invariant */ + public readonly mixed $d; + + /** @var Invariant */ + private readonly mixed $e; +} + +/** + * @template-contravariant X + */ +class D { + /** + * @param X $a + * @param X $b + */ + public function __construct( + public readonly mixed $a, + private readonly mixed $b, + ) {} +} diff --git a/tests/PHPStan/Rules/Generics/data/property-variance.php b/tests/PHPStan/Rules/Generics/data/property-variance.php new file mode 100644 index 00000000000..a7c9406b8e9 --- /dev/null +++ b/tests/PHPStan/Rules/Generics/data/property-variance.php @@ -0,0 +1,102 @@ + */ + public $b; + + /** @var Out */ + public $c; + + /** @var Invariant */ + public $d; + + /** @var X */ + private $e; + + /** @var In */ + private $f; + + /** @var Out */ + private $g; + + /** @var Invariant */ + private $h; +} + +/** + * @template-covariant X + */ +class B { + /** @var X */ + public $a; + + /** @var In */ + public $b; + + /** @var Out */ + public $c; + + /** @var Invariant */ + public $d; + + /** @var X */ + private $e; + + /** @var In */ + private $f; + + /** @var Out */ + private $g; + + /** @var Invariant */ + private $h; +} + +/** + * @template-contravariant X + */ +class C { + /** @var X */ + public $a; + + /** @var In */ + public $b; + + /** @var Out */ + public $c; + + /** @var Invariant */ + public $d; + + /** @var X */ + private $e; + + /** @var In */ + private $f; + + /** @var Out */ + private $g; + + /** @var Invariant */ + private $h; +} From f44336fc897f99096abcfd854f4110589b7e2c5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pudil?= Date: Sat, 1 Apr 2023 22:45:15 +0200 Subject: [PATCH 2/3] fix build --- src/Rules/Generics/PropertyVarianceRule.php | 2 +- src/Rules/Generics/VarianceCheck.php | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Rules/Generics/PropertyVarianceRule.php b/src/Rules/Generics/PropertyVarianceRule.php index c60836ee6e0..d13c8cfb38f 100644 --- a/src/Rules/Generics/PropertyVarianceRule.php +++ b/src/Rules/Generics/PropertyVarianceRule.php @@ -2,7 +2,7 @@ namespace PHPStan\Rules\Generics; -use PHPParser\Node; +use PhpParser\Node; use PHPStan\Analyser\Scope; use PHPStan\Internal\SprintfHelper; use PHPStan\Node\ClassPropertyNode; diff --git a/src/Rules/Generics/VarianceCheck.php b/src/Rules/Generics/VarianceCheck.php index 66e546d86de..6445d038c10 100644 --- a/src/Rules/Generics/VarianceCheck.php +++ b/src/Rules/Generics/VarianceCheck.php @@ -88,6 +88,7 @@ public function checkParametersAcceptor( return $errors; } + /** @return RuleError[] */ public function checkProperty( PropertyReflection $propertyReflection, string $message, From 47cc3a2a8d1fe22a5d1933487fecdf8d8d807743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pudil?= Date: Sun, 2 Apr 2023 16:29:23 +0200 Subject: [PATCH 3/3] simplify property variance check by only supporting native properties --- src/Rules/Generics/VarianceCheck.php | 34 ++++++---------------------- 1 file changed, 7 insertions(+), 27 deletions(-) diff --git a/src/Rules/Generics/VarianceCheck.php b/src/Rules/Generics/VarianceCheck.php index 6445d038c10..b59aaee5ffb 100644 --- a/src/Rules/Generics/VarianceCheck.php +++ b/src/Rules/Generics/VarianceCheck.php @@ -3,7 +3,7 @@ namespace PHPStan\Rules\Generics; use PHPStan\Reflection\ParametersAcceptorWithPhpDocs; -use PHPStan\Reflection\PropertyReflection; +use PHPStan\Reflection\Php\PhpPropertyReflection; use PHPStan\Rules\RuleError; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Generic\TemplateType; @@ -90,37 +90,17 @@ public function checkParametersAcceptor( /** @return RuleError[] */ public function checkProperty( - PropertyReflection $propertyReflection, + PhpPropertyReflection $propertyReflection, string $message, bool $isReadOnly, ): array { - $readableType = $propertyReflection->getReadableType(); - $writableType = $propertyReflection->getWritableType(); + $type = $propertyReflection->getReadableType(); + $variance = $isReadOnly + ? TemplateTypeVariance::createCovariant() + : TemplateTypeVariance::createInvariant(); - if ($readableType->equals($writableType)) { - $variance = $isReadOnly - ? TemplateTypeVariance::createCovariant() - : TemplateTypeVariance::createInvariant(); - - return $this->check($variance, $readableType, $message); - } - - $errors = []; - - if ($propertyReflection->isReadable()) { - foreach ($this->check(TemplateTypeVariance::createCovariant(), $readableType, $message) as $error) { - $errors[] = $error; - } - } - - if ($propertyReflection->isWritable()) { - foreach ($this->check(TemplateTypeVariance::createContravariant(), $writableType, $message) as $error) { - $errors[] = $error; - } - } - - return $errors; + return $this->check($variance, $type, $message); } /** @return RuleError[] */