From b87508fc088a0bbcb25f85b265f6daa6b0e01d03 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Mon, 30 May 2022 22:37:09 +0200 Subject: [PATCH 1/5] Add failing tests for `MissingReadOnlyPropertyAssignRule` --- ...ReadOnlyByPhpDocPropertyAssignRuleTest.php | 34 +++++++++++++++++++ .../MissingReadOnlyPropertyAssignRuleTest.php | 34 +++++++++++++++++++ ...issing-readonly-property-assign-phpdoc.php | 21 ++++++++++++ .../data/missing-readonly-property-assign.php | 16 +++++++++ 4 files changed, 105 insertions(+) create mode 100644 tests/Rules/Properties/MissingReadOnlyByPhpDocPropertyAssignRuleTest.php create mode 100644 tests/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php create mode 100644 tests/Rules/Properties/data/missing-readonly-property-assign-phpdoc.php create mode 100644 tests/Rules/Properties/data/missing-readonly-property-assign.php diff --git a/tests/Rules/Properties/MissingReadOnlyByPhpDocPropertyAssignRuleTest.php b/tests/Rules/Properties/MissingReadOnlyByPhpDocPropertyAssignRuleTest.php new file mode 100644 index 00000000..97f537a9 --- /dev/null +++ b/tests/Rules/Properties/MissingReadOnlyByPhpDocPropertyAssignRuleTest.php @@ -0,0 +1,34 @@ + + */ +class MissingReadOnlyByPhpDocPropertyAssignRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return self::getContainer()->getByType(MissingReadOnlyByPhpDocPropertyAssignRule::class); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/../../../extension.neon']; + } + + public function testRule(): void + { + if (PHP_VERSION_ID < 70400) { + self::markTestSkipped('Test requires PHP 7.4.'); + } + + $this->analyse([__DIR__ . '/data/missing-readonly-property-assign-phpdoc.php'], []); + } + +} diff --git a/tests/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php b/tests/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php new file mode 100644 index 00000000..ff01c4a3 --- /dev/null +++ b/tests/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php @@ -0,0 +1,34 @@ + + */ +class MissingReadOnlyPropertyAssignRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return self::getContainer()->getByType(MissingReadOnlyPropertyAssignRule::class); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/../../../extension.neon']; + } + + public function testRule(): void + { + if (PHP_VERSION_ID < 80100) { + self::markTestSkipped('Test requires PHP 8.1.'); + } + + $this->analyse([__DIR__ . '/data/missing-readonly-property-assign.php'], []); + } + +} diff --git a/tests/Rules/Properties/data/missing-readonly-property-assign-phpdoc.php b/tests/Rules/Properties/data/missing-readonly-property-assign-phpdoc.php new file mode 100644 index 00000000..fbe580be --- /dev/null +++ b/tests/Rules/Properties/data/missing-readonly-property-assign-phpdoc.php @@ -0,0 +1,21 @@ += 7.4 + +namespace MissingReadOnlyPropertyAssignPhpDoc; + +use Doctrine\ORM\Mapping as ORM; + +/** + * @ORM\Entity + */ +class Entity +{ + + /** + * @ORM\Id + * @ORM\GeneratedValue + * @ORM\Column + * @readonly + */ + private int $id; // ok because of PropertiesExtension + +} diff --git a/tests/Rules/Properties/data/missing-readonly-property-assign.php b/tests/Rules/Properties/data/missing-readonly-property-assign.php new file mode 100644 index 00000000..bcaf9aa2 --- /dev/null +++ b/tests/Rules/Properties/data/missing-readonly-property-assign.php @@ -0,0 +1,16 @@ += 8.1 + +namespace MissingReadOnlyPropertyAssign; + +use Doctrine\ORM\Mapping as ORM; + +#[ORM\Entity] +class Entity +{ + + #[ORM\Id] + #[ORM\GeneratedValue] + #[ORM\Column] + private readonly int $id; // ok because of PropertiesExtension + +} From 04314d406069c99d8c1041d0ea1ff7bdc440a7e9 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Mon, 30 May 2022 22:57:37 +0200 Subject: [PATCH 2/5] Require PHPStan 1.7.3 --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index a31a0059..ef81194f 100644 --- a/composer.json +++ b/composer.json @@ -7,7 +7,7 @@ ], "require": { "php": "^7.2 || ^8.0", - "phpstan/phpstan": "^1.7.0" + "phpstan/phpstan": "^1.7.3" }, "conflict": { "doctrine/collections": "<1.0", From c3834aada1fe40bb934942b7c18cabe21d136942 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Mon, 30 May 2022 22:47:06 +0200 Subject: [PATCH 3/5] Consider readonly entity properties without a constructor to be initialized --- src/Rules/Doctrine/ORM/PropertiesExtension.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/Rules/Doctrine/ORM/PropertiesExtension.php b/src/Rules/Doctrine/ORM/PropertiesExtension.php index 7e622040..d19ebca1 100644 --- a/src/Rules/Doctrine/ORM/PropertiesExtension.php +++ b/src/Rules/Doctrine/ORM/PropertiesExtension.php @@ -2,6 +2,7 @@ namespace PHPStan\Rules\Doctrine\ORM; +use Doctrine\ORM\Mapping\ClassMetadataInfo; use PHPStan\Reflection\PropertyReflection; use PHPStan\Rules\Properties\ReadWritePropertiesExtension; use PHPStan\Type\Doctrine\ObjectMetadataResolver; @@ -43,6 +44,10 @@ public function isAlwaysWritten(PropertyReflection $property, string $propertyNa return false; } + if ($this->isGeneratedIdentifier($metadata, $propertyName)) { + return true; + } + if ($metadata->isReadOnly && !$declaringClass->hasConstructor()) { return true; } @@ -82,7 +87,20 @@ public function isInitialized(PropertyReflection $property, string $propertyName return false; } + if ($this->isGeneratedIdentifier($metadata, $propertyName)) { + return true; + } + return $metadata->isReadOnly && !$declaringClass->hasConstructor(); } + private function isGeneratedIdentifier(ClassMetadataInfo $metadata, string $propertyName): bool + { + if (!in_array($propertyName, $metadata->getIdentifierFieldNames(), true)) { + return false; + } + + return $metadata->generatorType !== ClassMetadataInfo::GENERATOR_TYPE_NONE; + } + } From 6d135acb266b98c67efa999a82007e659de4b62f Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Wed, 1 Jun 2022 10:56:41 +0200 Subject: [PATCH 4/5] Add more test cases for `MissingReadOnlyPropertyAssignRule` --- ...ReadOnlyByPhpDocPropertyAssignRuleTest.php | 15 ++++- .../MissingReadOnlyPropertyAssignRuleTest.php | 15 ++++- ...issing-readonly-property-assign-phpdoc.php | 56 ++++++++++++++++++- .../data/missing-readonly-property-assign.php | 40 ++++++++++++- 4 files changed, 120 insertions(+), 6 deletions(-) diff --git a/tests/Rules/Properties/MissingReadOnlyByPhpDocPropertyAssignRuleTest.php b/tests/Rules/Properties/MissingReadOnlyByPhpDocPropertyAssignRuleTest.php index 97f537a9..2f42a9a4 100644 --- a/tests/Rules/Properties/MissingReadOnlyByPhpDocPropertyAssignRuleTest.php +++ b/tests/Rules/Properties/MissingReadOnlyByPhpDocPropertyAssignRuleTest.php @@ -28,7 +28,20 @@ public function testRule(): void self::markTestSkipped('Test requires PHP 7.4.'); } - $this->analyse([__DIR__ . '/data/missing-readonly-property-assign-phpdoc.php'], []); + $this->analyse([__DIR__ . '/data/missing-readonly-property-assign-phpdoc.php'], [ + [ + 'Class MissingReadOnlyPropertyAssignPhpDoc\EntityWithAGeneratedId has an uninitialized @readonly property $unassigned. Assign it in the constructor.', + 25, + ], + [ + '@readonly property MissingReadOnlyPropertyAssignPhpDoc\EntityWithAGeneratedId::$doubleAssigned is already assigned.', + 36, + ], + [ + 'Class MissingReadOnlyPropertyAssignPhpDoc\ReadOnlyEntityWithConstructor has an uninitialized @readonly property $id. Assign it in the constructor.', + 67, + ], + ]); } } diff --git a/tests/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php b/tests/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php index ff01c4a3..6f5c545f 100644 --- a/tests/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php +++ b/tests/Rules/Properties/MissingReadOnlyPropertyAssignRuleTest.php @@ -28,7 +28,20 @@ public function testRule(): void self::markTestSkipped('Test requires PHP 8.1.'); } - $this->analyse([__DIR__ . '/data/missing-readonly-property-assign.php'], []); + $this->analyse([__DIR__ . '/data/missing-readonly-property-assign.php'], [ + [ + 'Class MissingReadOnlyPropertyAssign\EntityWithAGeneratedId has an uninitialized readonly property $unassigned. Assign it in the constructor.', + 17, + ], + [ + 'Readonly property MissingReadOnlyPropertyAssign\EntityWithAGeneratedId::$doubleAssigned is already assigned.', + 25, + ], + [ + 'Class MissingReadOnlyPropertyAssign\ReadOnlyEntityWithConstructor has an uninitialized readonly property $id. Assign it in the constructor.', + 46, + ], + ]); } } diff --git a/tests/Rules/Properties/data/missing-readonly-property-assign-phpdoc.php b/tests/Rules/Properties/data/missing-readonly-property-assign-phpdoc.php index fbe580be..16f9d556 100644 --- a/tests/Rules/Properties/data/missing-readonly-property-assign-phpdoc.php +++ b/tests/Rules/Properties/data/missing-readonly-property-assign-phpdoc.php @@ -7,7 +7,7 @@ /** * @ORM\Entity */ -class Entity +class EntityWithAGeneratedId { /** @@ -16,6 +16,58 @@ class Entity * @ORM\Column * @readonly */ - private int $id; // ok because of PropertiesExtension + private int $id; // ok, ID is generated + + /** + * @ORM\Column + * @readonly + */ + private int $unassigned; + + /** + * @ORM\Column + * @readonly + */ + private int $doubleAssigned; + + public function __construct(int $doubleAssigned) + { + $this->doubleAssigned = $doubleAssigned; + $this->doubleAssigned = 17; + } + +} + +/** + * @ORM\Entity(readOnly=true) + */ +class ReadOnlyEntity +{ + + /** + * @ORM\Id + * @ORM\Column + * @readonly + */ + private int $id; // ok, entity is read only + +} + +/** + * @ORM\Entity(readOnly=true) + */ +class ReadOnlyEntityWithConstructor +{ + + /** + * @ORM\Id + * @ORM\Column + * @readonly + */ + private int $id; + + public function __construct() + { + } } diff --git a/tests/Rules/Properties/data/missing-readonly-property-assign.php b/tests/Rules/Properties/data/missing-readonly-property-assign.php index bcaf9aa2..edbac4d7 100644 --- a/tests/Rules/Properties/data/missing-readonly-property-assign.php +++ b/tests/Rules/Properties/data/missing-readonly-property-assign.php @@ -5,12 +5,48 @@ use Doctrine\ORM\Mapping as ORM; #[ORM\Entity] -class Entity +class EntityWithAGeneratedId { #[ORM\Id] #[ORM\GeneratedValue] #[ORM\Column] - private readonly int $id; // ok because of PropertiesExtension + private readonly int $id; // ok, ID is generated + + #[ORM\Column] + private readonly int $unassigned; + + #[ORM\Column] + private readonly int $doubleAssigned; + + public function __construct(int $doubleAssigned) + { + $this->doubleAssigned = $doubleAssigned; + $this->doubleAssigned = 17; + } + +} + +#[ORM\Entity(null, true)] +class ReadOnlyEntity +{ + + #[ORM\Id] + #[ORM\Column] + private readonly int $id; // ok, entity is readonly + +} + +#[ORM\Entity(null, true)] +class ReadOnlyEntityWithConstructor +{ + + #[ORM\Id] + #[ORM\Column] + private readonly int $id; + + public function __construct() + { + } } From 909516ad1e45ce43f73263780ef53b0683c782e3 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Wed, 1 Jun 2022 11:24:44 +0200 Subject: [PATCH 5/5] Add test cases for `UnusedPrivatePropertyRule` --- .../Doctrine/ORM/PropertiesExtension.php | 34 ++++------- .../UnusedPrivatePropertyRuleTest.php | 50 ++++++++++++++++ .../DeadCode/data/unused-private-property.php | 59 +++++++++++++++++++ 3 files changed, 121 insertions(+), 22 deletions(-) create mode 100644 tests/Rules/DeadCode/UnusedPrivatePropertyRuleTest.php create mode 100644 tests/Rules/DeadCode/data/unused-private-property.php diff --git a/src/Rules/Doctrine/ORM/PropertiesExtension.php b/src/Rules/Doctrine/ORM/PropertiesExtension.php index d19ebca1..7328d6f4 100644 --- a/src/Rules/Doctrine/ORM/PropertiesExtension.php +++ b/src/Rules/Doctrine/ORM/PropertiesExtension.php @@ -44,34 +44,15 @@ public function isAlwaysWritten(PropertyReflection $property, string $propertyNa return false; } - if ($this->isGeneratedIdentifier($metadata, $propertyName)) { - return true; - } - if ($metadata->isReadOnly && !$declaringClass->hasConstructor()) { return true; } - if ($metadata->isIdentifierNatural()) { - return false; - } - if ($metadata->versionField === $propertyName) { return true; } - try { - $identifiers = $metadata->getIdentifierFieldNames(); - } catch (Throwable $e) { - $mappingException = 'Doctrine\ORM\Mapping\MappingException'; - if (!$e instanceof $mappingException) { - throw $e; - } - - return false; - } - - return in_array($propertyName, $identifiers, true); + return $this->isGeneratedIdentifier($metadata, $propertyName); } public function isInitialized(PropertyReflection $property, string $propertyName): bool @@ -96,11 +77,20 @@ public function isInitialized(PropertyReflection $property, string $propertyName private function isGeneratedIdentifier(ClassMetadataInfo $metadata, string $propertyName): bool { - if (!in_array($propertyName, $metadata->getIdentifierFieldNames(), true)) { + if ($metadata->isIdentifierNatural()) { return false; } - return $metadata->generatorType !== ClassMetadataInfo::GENERATOR_TYPE_NONE; + try { + return in_array($propertyName, $metadata->getIdentifierFieldNames(), true); + } catch (Throwable $e) { + $mappingException = 'Doctrine\ORM\Mapping\MappingException'; + if (!$e instanceof $mappingException) { + throw $e; + } + + return false; + } } } diff --git a/tests/Rules/DeadCode/UnusedPrivatePropertyRuleTest.php b/tests/Rules/DeadCode/UnusedPrivatePropertyRuleTest.php new file mode 100644 index 00000000..ccef2e9c --- /dev/null +++ b/tests/Rules/DeadCode/UnusedPrivatePropertyRuleTest.php @@ -0,0 +1,50 @@ + + */ +class UnusedPrivatePropertyRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return self::getContainer()->getByType(UnusedPrivatePropertyRule::class); + } + + public static function getAdditionalConfigFiles(): array + { + return [__DIR__ . '/../../../extension.neon']; + } + + public function testRule(): void + { + if (PHP_VERSION_ID < 70400) { + self::markTestSkipped('Test requires PHP 7.4.'); + } + + $this->analyse([__DIR__ . '/data/unused-private-property.php'], [ + [ + 'Property UnusedPrivateProperty\EntityWithAGeneratedId::$unused is never written, only read.', + 23, + 'See: https://phpstan.org/developing-extensions/always-read-written-properties', + ], + [ + 'Property UnusedPrivateProperty\EntityWithAGeneratedId::$unused2 is unused.', + 25, + 'See: https://phpstan.org/developing-extensions/always-read-written-properties', + ], + [ + 'Property UnusedPrivateProperty\ReadOnlyEntityWithConstructor::$id is never written, only read.', + 53, + 'See: https://phpstan.org/developing-extensions/always-read-written-properties', + ], + ]); + } + +} diff --git a/tests/Rules/DeadCode/data/unused-private-property.php b/tests/Rules/DeadCode/data/unused-private-property.php new file mode 100644 index 00000000..88aca7cf --- /dev/null +++ b/tests/Rules/DeadCode/data/unused-private-property.php @@ -0,0 +1,59 @@ += 7.4 + +namespace UnusedPrivateProperty; + +use Doctrine\ORM\Mapping as ORM; + +/** + * @ORM\Entity + */ +class EntityWithAGeneratedId +{ + + /** + * @ORM\Id + * @ORM\GeneratedValue + * @ORM\Column + */ + private int $id; // ok, ID is generated + + /** + * @ORM\Column + */ + private int $unused; + + private int $unused2; + +} + +/** + * @ORM\Entity(readOnly=true) + */ +class ReadOnlyEntity +{ + + /** + * @ORM\Id + * @ORM\Column + */ + private int $id; // ok, entity is read only + +} + +/** + * @ORM\Entity(readOnly=true) + */ +class ReadOnlyEntityWithConstructor +{ + + /** + * @ORM\Id + * @ORM\Column + */ + private int $id; + + public function __construct() + { + } + +}