diff --git a/README.md b/README.md index bdefd73..f8df617 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,7 @@ ## Features * [Property initialization](https://patchlevel.dev/docs/event-sourcing-phpstan-extension/latest/getting-started#property-initialization) for aggregate roots and child aggregates, so PHPStan does not report false uninitialized property errors. +* [Write only properties](https://patchlevel.dev/docs/event-sourcing-phpstan-extension/latest/getting-started#write-only-properties) are reported when apply methods store state that is never read, because state that is not used to check invariants belongs in a projection. * [Recording in apply methods](https://patchlevel.dev/docs/event-sourcing-phpstan-extension/latest/getting-started#recording-in-apply-methods) is reported as an error, because recording events while replaying them leads to duplicated events. * [Writing state outside apply methods](https://patchlevel.dev/docs/event-sourcing-phpstan-extension/latest/getting-started#writing-state-outside-apply-methods) is reported as an error, because state that is not derived from an event is lost when the aggregate is reloaded. diff --git a/docs/getting-started.md b/docs/getting-started.md index b0d51a0..6902dc2 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -71,6 +71,51 @@ as initialized, so the analysis passes. This works for both aggregate roots and child aggregates: any class implementing `AggregateRoot` or `ChildAggregate` has its properties treated as initialized. ::: +## Write only properties + +The mirror image of an unused property: state that apply methods populate but that nothing ever +reads. An aggregate holds state for exactly one purpose, deciding whether a command is allowed, +so a property that is written but never read is not part of any decision. The extension reports it: + +```php +use Patchlevel\EventSourcing\Aggregate\BasicAggregateRoot; +use Patchlevel\EventSourcing\Aggregate\Uuid; +use Patchlevel\EventSourcing\Attribute\Apply; +use Patchlevel\EventSourcing\Attribute\Id; + +final class Profile extends BasicAggregateRoot +{ + #[Id] + private Uuid $id; + private string $name; + private string $lastName; // reported + + #[Apply] + protected function applyProfileCreated(ProfileCreated $event): void + { + $this->id = $event->id; + $this->name = $event->name; + $this->lastName = $event->name; + } + + public function name(): string + { + return $this->name; + } +} +``` +Running PHPStan now produces: + +``` +Property "lastName" of aggregate "Profile" is written in an #[Apply] method +but never read, so it is not used to check any invariants. +💡 Use the property to check invariants or remove it. State that only exists +for reading belongs in a projection. +``` +Any read counts: an invariant check in a command method, a read inside an apply method, or a getter. +Only private properties are checked, and properties the library itself reads, `#[Id]` and +`#[ChildAggregate]`, are skipped. + ## Recording in apply methods Apply methods are also called while an aggregate is rebuilt from its stored events. If you record a @@ -168,15 +213,16 @@ the same way PHPStan handles its own rules: parameters: patchlevelEventSourcing: propertyInitialization: false + writeOnlyProperty: false noRecordThatWhenApplying: false noStateWriteWhenNotApplying: false ``` ## Result With the extension enabled, PHPStan understands your aggregates: it stops complaining about -properties that are initialized through events, it fails the build when an apply method records -an event, and it fails the build when aggregate state is written outside an apply method. You get -accurate static analysis without writing a single annotation. +properties that are initialized through events, it reports state that is written but never used for a decision, it fails +the build when an apply method records an event, and it fails the build when aggregate state is written outside an apply +method. You get accurate static analysis without writing a single annotation. ## Learn more diff --git a/docs/introduction.md b/docs/introduction.md index dc7d98c..a651e0e 100644 --- a/docs/introduction.md +++ b/docs/introduction.md @@ -7,6 +7,7 @@ and it catches common mistakes before they ever reach runtime. ## Features * [Property initialization](getting-started.md#property-initialization) for aggregate roots and child aggregates, so PHPStan does not report false uninitialized property errors. +* [Write only properties](getting-started.md#write-only-properties) are reported when apply methods store state that is never read, because state that is not used to check invariants belongs in a projection. * [Recording in apply methods](getting-started.md#recording-in-apply-methods) is reported as an error, because recording events while replaying them leads to duplicated events. * [Writing state outside apply methods](getting-started.md#writing-state-outside-apply-methods) is reported as an error, because state that is not derived from an event is lost when the aggregate is reloaded. diff --git a/extension.neon b/extension.neon index 6e4ffd6..e64bf68 100644 --- a/extension.neon +++ b/extension.neon @@ -1,12 +1,14 @@ parameters: patchlevelEventSourcing: propertyInitialization: true + writeOnlyProperty: true noRecordThatWhenApplying: true noStateWriteWhenNotApplying: true parametersSchema: patchlevelEventSourcing: structure([ propertyInitialization: bool() + writeOnlyProperty: bool() noRecordThatWhenApplying: bool() noStateWriteWhenNotApplying: bool() ]) @@ -14,6 +16,8 @@ parametersSchema: conditionalTags: Patchlevel\EventSourcingPHPStanExtension\AggregateRootExtension: phpstan.properties.readWriteExtension: %patchlevelEventSourcing.propertyInitialization% + Patchlevel\EventSourcingPHPStanExtension\WriteOnlyPropertyRule: + phpstan.rules.rule: %patchlevelEventSourcing.writeOnlyProperty% Patchlevel\EventSourcingPHPStanExtension\DontRecordWhenApplyingExtension: phpstan.restrictedMethodUsageExtension: %patchlevelEventSourcing.noRecordThatWhenApplying% Patchlevel\EventSourcingPHPStanExtension\DontWriteStateWhenNotApplyingRule: @@ -23,6 +27,9 @@ services: - class: Patchlevel\EventSourcingPHPStanExtension\AggregateRootExtension + - + class: Patchlevel\EventSourcingPHPStanExtension\WriteOnlyPropertyRule + - class: Patchlevel\EventSourcingPHPStanExtension\DontRecordWhenApplyingExtension diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 7e952b1..557e1cc 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -47,3 +47,9 @@ parameters: identifier: patchlevel.noRecordThatWhenApplying count: 1 path: tests/Invalid/Profile.php + + - + rawMessage: 'Property "lastName" of aggregate "Patchlevel\EventSourcingPHPStanExtension\Tests\Invalid\Profile" is written in an #[Apply] method but never read, so it is not used to check any invariants.' + identifier: patchlevel.writeOnlyProperty + count: 1 + path: tests/Invalid/Profile.php diff --git a/src/AggregateRootExtension.php b/src/AggregateRootExtension.php index a7274ae..00df393 100644 --- a/src/AggregateRootExtension.php +++ b/src/AggregateRootExtension.php @@ -13,7 +13,7 @@ final class AggregateRootExtension implements ReadWritePropertiesExtension { public function isAlwaysRead(PropertyReflection $property, string $propertyName): bool { - return false; + return $this->isAggregate($property); } public function isAlwaysWritten(PropertyReflection $property, string $propertyName): bool @@ -22,6 +22,11 @@ public function isAlwaysWritten(PropertyReflection $property, string $propertyNa } public function isInitialized(PropertyReflection $property, string $propertyName): bool + { + return $this->isAggregate($property); + } + + private function isAggregate(PropertyReflection $property): bool { $interfaces = $property->getDeclaringClass()->getInterfaces(); diff --git a/src/WriteOnlyPropertyRule.php b/src/WriteOnlyPropertyRule.php new file mode 100644 index 0000000..177f009 --- /dev/null +++ b/src/WriteOnlyPropertyRule.php @@ -0,0 +1,280 @@ + */ +final class WriteOnlyPropertyRule implements Rule +{ + private NodeFinder $nodeFinder; + + public function __construct() + { + $this->nodeFinder = new NodeFinder(); + } + + public function getNodeType(): string + { + return InClassNode::class; + } + + /** @return list */ + public function processNode(Node $node, Scope $scope): array + { + $classReflection = $node->getClassReflection(); + + if ( + !$classReflection->implementsInterface(AggregateRoot::class) + && !$classReflection->implementsInterface(ChildAggregate::class) + ) { + return []; + } + + if ($classReflection->isAbstract()) { + return []; + } + + $classNode = $node->getOriginalNode(); + + $writtenInApply = []; + $read = []; + + foreach ($classNode->getMethods() as $method) { + $writeTargets = $this->writeTargets($method); + + if ($this->isApplyMethod($method)) { + foreach ($writeTargets as $writeTarget) { + foreach ($this->basePropertyFetches($writeTarget) as $propertyFetch) { + $propertyName = $this->ownPropertyName($propertyFetch); + + if ($propertyName === null) { + continue; + } + + $writtenInApply[$propertyName] = true; + } + } + } + + foreach ($this->readPropertyNames($method, $writeTargets) as $propertyName) { + $read[$propertyName] = true; + } + } + + $errors = []; + + foreach ($classNode->getProperties() as $property) { + if ($property->isStatic() || !$property->isPrivate()) { + continue; + } + + if ($this->isLibraryReadProperty($property)) { + continue; + } + + foreach ($property->props as $propertyItem) { + $propertyName = $propertyItem->name->toString(); + + if (!array_key_exists($propertyName, $writtenInApply)) { + continue; + } + + if (array_key_exists($propertyName, $read)) { + continue; + } + + $errors[] = RuleErrorBuilder::message(sprintf( + 'Property "%s" of aggregate "%s" is written in an #[Apply] method but never read, so it is not used to check any invariants.', + $propertyName, + $classReflection->getName(), + )) + ->identifier('patchlevel.writeOnlyProperty') + ->line($propertyItem->getStartLine()) + ->tip('Use the property to check invariants or remove it. State that only exists for reading belongs in a projection.') + ->build(); + } + } + + return $errors; + } + + private function isApplyMethod(ClassMethod $method): bool + { + foreach ($method->attrGroups as $attrGroup) { + foreach ($attrGroup->attrs as $attr) { + if ($attr->name->toString() === Apply::class) { + return true; + } + } + } + + return false; + } + + private function isLibraryReadProperty(Property $property): bool + { + foreach ($property->attrGroups as $attrGroup) { + foreach ($attrGroup->attrs as $attr) { + if ($attr->name->toString() === Id::class || $attr->name->toString() === ChildAggregateAttribute::class) { + return true; + } + } + } + + return false; + } + + /** + * Collects the expressions that are written to in the method: assignment + * targets, operands of increment and decrement, and unset() arguments. + * + * @return list + */ + private function writeTargets(ClassMethod $method): array + { + $targets = []; + + /** @var list $writes */ + $writes = $this->nodeFinder->find( + $method->stmts ?? [], + static fn (Node $node): bool => $node instanceof Assign + || $node instanceof AssignOp + || $node instanceof AssignRef + || $node instanceof PreInc + || $node instanceof PreDec + || $node instanceof PostInc + || $node instanceof PostDec + || $node instanceof Unset_, + ); + + foreach ($writes as $write) { + if ($write instanceof Unset_) { + foreach ($write->vars as $var) { + $targets[] = $var; + } + + continue; + } + + $targets[] = $write->var; + } + + return $targets; + } + + /** + * Collects the names of all properties the method reads: every $this + * property fetch that is not itself the target of a write. + * + * @param list $writeTargets + * + * @return list + */ + private function readPropertyNames(ClassMethod $method, array $writeTargets): array + { + $writeBaseIds = []; + + foreach ($writeTargets as $writeTarget) { + foreach ($this->basePropertyFetches($writeTarget) as $propertyFetch) { + $writeBaseIds[spl_object_id($propertyFetch)] = true; + } + } + + $names = []; + + $propertyFetches = $this->nodeFinder->findInstanceOf($method->stmts ?? [], PropertyFetch::class); + + foreach ($propertyFetches as $propertyFetch) { + if (array_key_exists(spl_object_id($propertyFetch), $writeBaseIds)) { + continue; + } + + $propertyName = $this->ownPropertyName($propertyFetch); + + if ($propertyName === null) { + continue; + } + + $names[] = $propertyName; + } + + return $names; + } + + /** @return list */ + private function basePropertyFetches(Expr $expr): array + { + while ($expr instanceof ArrayDimFetch) { + $expr = $expr->var; + } + + if ($expr instanceof PropertyFetch) { + return [$expr]; + } + + if ($expr instanceof List_) { + $fetches = []; + + foreach ($expr->items as $item) { + if ($item === null) { + continue; + } + + foreach ($this->basePropertyFetches($item->value) as $fetch) { + $fetches[] = $fetch; + } + } + + return $fetches; + } + + return []; + } + + private function ownPropertyName(PropertyFetch $propertyFetch): string|null + { + if ( + !$propertyFetch->var instanceof Variable + || $propertyFetch->var->name !== 'this' + || !$propertyFetch->name instanceof Identifier + ) { + return null; + } + + return $propertyFetch->name->toString(); + } +} diff --git a/tests/Invalid/Profile.php b/tests/Invalid/Profile.php index 5ea8e48..b8c03d7 100644 --- a/tests/Invalid/Profile.php +++ b/tests/Invalid/Profile.php @@ -15,6 +15,7 @@ class Profile extends BasicAggregateRoot #[Id] private Uuid $id; private string $name; + private string $lastName; private int $count = 0; /** @var array */ @@ -46,6 +47,7 @@ protected function applyProfileCreated(ProfileCreated $event): void protected function applyNameChanged(NameChanged $event): void { $this->name = $event->name; + $this->lastName = $event->name; $this->hiddenRecordThat($event); }