From 078ca3715973ea2a10a9ef146e8f593f904845ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Fr=C3=B6mer?= Date: Fri, 18 Jan 2019 06:53:56 +0100 Subject: [PATCH] Feature: inherit contracts (#31) * Made AbstractFetcher accept more then one annotation type * Add inherit annotation This will serve as a contract inherit flag. It will take all existing annotations (ensure, verify, invariant) and check current as well as parent classes for contracts * Update Inherit implementation - Inherit will now always inherit contracts (despite missing @inheritdoc) - provide Inherit tests - update Demo implementation - update Readme with Inherit documentation --- .gitignore | 1 + README.md | 63 +++++++-- demo/Demo/Account.php | 12 +- demo/Demo/AccountContractInterface.php | 11 ++ demo/demo.php | 8 +- src/Annotation/Inherit.php | 26 ++++ src/Aspect/AbstractContractAspect.php | 5 + src/Aspect/InheritCheckerAspect.php | 130 ++++++++++++++++++ src/Aspect/InvariantCheckerAspect.php | 2 +- src/Aspect/PostconditionCheckerAspect.php | 2 +- src/Aspect/PreconditionCheckerAspect.php | 2 +- .../Fetcher/Parent/AbstractFetcher.php | 18 +-- src/ContractApplication.php | 2 + src/Exception/ContractViolation.php | 8 +- tests/Functional/Inherit/ContractTest.php | 46 +++++++ tests/Functional/Inherit/StubInterface.php | 20 +++ tests/Functional/Inherit/StubWithInherit.php | 29 ++++ .../Functional/Inherit/StubWithoutInherit.php | 18 +++ 18 files changed, 376 insertions(+), 27 deletions(-) create mode 100644 src/Annotation/Inherit.php create mode 100644 src/Aspect/InheritCheckerAspect.php create mode 100644 tests/Functional/Inherit/ContractTest.php create mode 100644 tests/Functional/Inherit/StubInterface.php create mode 100644 tests/Functional/Inherit/StubWithInherit.php create mode 100644 tests/Functional/Inherit/StubWithoutInherit.php diff --git a/.gitignore b/.gitignore index 4f7f9e6..5aea601 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ /vendor/ composer.lock /tests/cache/ +/demo/cache/ diff --git a/README.md b/README.md index 31ef86e..7377d1c 100644 --- a/README.md +++ b/README.md @@ -135,17 +135,30 @@ class Account ``` Invariants contain assert expressions, and so when they fail, they throw a ContractViolation exception. -NOTE! The code in the invariant may not call any public non-static members of the class, either directly or +__NOTE__: The code in the invariant may not call any public non-static members of the class, either directly or indirectly. Doing so will result in a stack overflow, as the invariant will wind up being called in an infinitely recursive manner. Contract propagation ---------- -All contracts are propagated from parent classes and interfaces. - -For preconditions (Verify contracts) subclasses do not inherit contracts of parents' methods if they don't have the @inheritdoc annotation. Example: - +There a some differences in inheritance of the contracts: + +1. Ensure + - if provided `Ensure` will automatically inherit all contracts from parent class or interface +2. Verify + - if provided `Verify` will _not_ inherit contracts from parent class or interface + - to inherit contracts you will ne to provide `@inheritdoc` or the `Inherit` contract +3. Invariant + - if provided `Invariant` will inherit all contracts from parent class or interface +4. Inherit + - if provided `Inherit` will inherit all contracts from the given leven (class, method) without the + need to provide a contract on your current class or method + +__Notes__: +- The parsing of a contract only happens __IF__ you provide any given annotation from this package. +Without it your contracts won't work! +- The annotation __must not__ have curly braces (`{}`) otherwise the annotation reader can't find them. ```php @@ -175,7 +188,7 @@ class FooParent ``` -Foo::bar accepts '2' literal as a parameter and does not accept '1'. +`Foo::bar` accepts `2` literal as a parameter and does not accept `1`. With @inheritdoc: @@ -208,12 +221,10 @@ class FooParent ``` -Foo::bar does not accept '1' and '2' literals as a parameter. - - +`Foo::bar` does not accept `1` and `2` literals as a parameter. -For postconditions (Ensure and Invariants contracts) subclasses inherit contracts and they don't need @inheritdoc. Example: +For postconditions (Ensure and Invariants contracts) subclasses inherit contracts and they don't need `@inheritdoc`. Example: ```php @@ -246,7 +257,37 @@ class FooParent ``` -Foo::setBar does not accept '1' and '2' literals as a parameter. +`Foo::setBar` does not accept `1` and `2` literals as a parameter. + +If you don't want to provide a contract on your curent method/class you can use the `Inherit` annotation: + +```php +class Foo extends FooParent +{ + /** + * @param int $amount + * @Contract\Inherit + */ + public function bar($amount) + { + ... + } +} + +class FooParent +{ + /** + * @param int $amount + * @Contract\Verify("$amount != 2") + */ + public function bar($amount) + { + ... + } +} +``` + +`Foo:bar()` does accept eveything, except: `2` Integration with assertion library ---------- diff --git a/demo/Demo/Account.php b/demo/Demo/Account.php index 8116a03..3e2cde6 100644 --- a/demo/Demo/Account.php +++ b/demo/Demo/Account.php @@ -14,7 +14,7 @@ /** * Simple trade account class - * @Contract\Invariant("$this->balance > 0") + * @Contract\Invariant("$this->balance >= 0") */ class Account implements AccountContractInterface { @@ -39,10 +39,20 @@ public function deposit($amount) $this->balance += $amount; } + /** + * @Contract\Inherit() + * @param float $amount + */ + public function withdraw($amount) + { + $this->balance -= $amount; + } + /** * Returns current balance * * @Contract\Ensure("$__result == $this->balance") + * * @return float */ public function getBalance() diff --git a/demo/Demo/AccountContractInterface.php b/demo/Demo/AccountContractInterface.php index 2cd77bc..0f6332c 100644 --- a/demo/Demo/AccountContractInterface.php +++ b/demo/Demo/AccountContractInterface.php @@ -27,6 +27,17 @@ interface AccountContractInterface */ public function deposit($amount); + /** + * Withdraw amount of money from account. + * + * We don't allow withdrawal of more than 50 + * @Contract\Verify("$amount <= $this->balance") + * @Contract\Verify("$amount <= 50") + * @Contract\Ensure("$this->balance == $__old->balance-$amount") + * @param float $amount + */ + public function withdraw($amount); + /** * Returns current balance * diff --git a/demo/demo.php b/demo/demo.php index d888544..b268edd 100644 --- a/demo/demo.php +++ b/demo/demo.php @@ -13,5 +13,11 @@ include_once __DIR__.'/aspect_bootstrap.php'; $account = new Demo\Account(); + +echo 'Deposit: 100' . PHP_EOL; $account->deposit(100); -echo $account->getBalance(); +echo 'Current balance: ' . $account->getBalance(); +echo PHP_EOL; +echo 'Withdraw: 100' . PHP_EOL; +$account->withdraw(50); +echo 'Current balance: ' . $account->getBalance(); diff --git a/src/Annotation/Inherit.php b/src/Annotation/Inherit.php new file mode 100644 index 0000000..ab72210 --- /dev/null +++ b/src/Annotation/Inherit.php @@ -0,0 +1,26 @@ + + * + * This source file is subject to the license that is bundled + * with this source code in the file LICENSE. + */ +namespace PhpDeal\Annotation; + +use Doctrine\Common\Annotations\Annotation as BaseAnnotation; + +/** + * This annotation defines a contract inheritance check, applied to the method or class + * + * @Annotation + * @Target({"METHOD", "CLASS"}) + */ +class Inherit extends BaseAnnotation +{ + public function __toString() + { + return $this->value; + } +} diff --git a/src/Aspect/AbstractContractAspect.php b/src/Aspect/AbstractContractAspect.php index 692deb1..9a8bcef 100644 --- a/src/Aspect/AbstractContractAspect.php +++ b/src/Aspect/AbstractContractAspect.php @@ -67,6 +67,7 @@ protected function fetchMethodArguments(MethodInvocation $invocation) * @param array $args List of arguments for the method * * @throws DomainException + * @throws ContractViolation */ protected function ensureContracts(MethodInvocation $invocation, array $contracts, $instance, $scope, array $args) { @@ -87,6 +88,10 @@ protected function ensureContracts(MethodInvocation $invocation, array $contract try { $invocationResult = $boundInvoker->__invoke($args, $contractExpression); +// if ($invocationResult === false) { +// throw new ContractViolation($invocation, $contractExpression); +// } + // we accept as a result only true or null // null may be a result of assertions from beberlei/assert which passed if ($invocationResult !== null && $invocationResult !== true) { diff --git a/src/Aspect/InheritCheckerAspect.php b/src/Aspect/InheritCheckerAspect.php new file mode 100644 index 0000000..11b2b23 --- /dev/null +++ b/src/Aspect/InheritCheckerAspect.php @@ -0,0 +1,130 @@ +methodConditionFetcher = new MethodConditionFetcher([Ensure::class, Verify::class, Invariant::class], $reader); + $this->invariantFetcher = new InvariantFetcher([Invariant::class], $reader); + } + + /** + * Verifies inherit contracts for the method + * + * @Around("@execution(PhpDeal\Annotation\Inherit)") + * @param MethodInvocation $invocation + * + * @throws ContractViolation + * @return mixed + */ + public function inheritMethodContracts(MethodInvocation $invocation) + { + $object = $invocation->getThis(); + $args = $this->fetchMethodArguments($invocation); + $class = $invocation->getMethod()->getDeclaringClass(); + if ($class->isCloneable()) { + $args['__old'] = clone $object; + } + + $result = $invocation->proceed(); + $args['__result'] = $result; + $allContracts = $this->fetchMethodContracts($invocation); + + $this->ensureContracts($invocation, $allContracts, $object, $class->name, $args); + + return $result; + } + + /** + * @Around("@within(PhpDeal\Annotation\Inherit) && execution(public **->*(*))") + * @param MethodInvocation $invocation + * @return mixed + */ + public function inheritClassContracts(MethodInvocation $invocation) + { + $object = $invocation->getThis(); + $args = $this->fetchMethodArguments($invocation); + $class = $invocation->getMethod()->getDeclaringClass(); + if ($class->isCloneable()) { + $args['__old'] = clone $object; + } + + $result = $invocation->proceed(); + $args['__result'] = $result; + + $allContracts = $this->fetchClassContracts($class); + $this->ensureContracts($invocation, $allContracts, $object, $class->name, $args); + + return $result; + } + + /** + * @param MethodInvocation $invocation + * @return array + */ + private function fetchMethodContracts(MethodInvocation $invocation) + { + $allContracts = $this->fetchParentsMethodContracts($invocation); + + foreach ($invocation->getMethod()->getAnnotations() as $annotation) { + $annotationClass = \get_class($annotation); + + if (\in_array($annotationClass, [Ensure::class, Verify::class, Invariant::class], true)) { + $allContracts[] = $annotation; + } + } + + return array_unique($allContracts); + } + + /** + * @param MethodInvocation $invocation + * @return array + */ + private function fetchParentsMethodContracts(MethodInvocation $invocation) + { + return $this->methodConditionFetcher->getConditions( + $invocation->getMethod()->getDeclaringClass(), + $invocation->getMethod()->name + ); + } + + /** + * @param ReflectionClass $class + * @return array + */ + private function fetchClassContracts(ReflectionClass $class) + { + $allContracts = $this->invariantFetcher->getConditions($class); + foreach ($this->reader->getClassAnnotations($class) as $annotation) { + if ($annotation instanceof Invariant) { + $allContracts[] = $annotation; + } + } + + return array_unique($allContracts); + } +} diff --git a/src/Aspect/InvariantCheckerAspect.php b/src/Aspect/InvariantCheckerAspect.php index de1f5d7..d192dd2 100644 --- a/src/Aspect/InvariantCheckerAspect.php +++ b/src/Aspect/InvariantCheckerAspect.php @@ -29,7 +29,7 @@ class InvariantCheckerAspect extends AbstractContractAspect implements Aspect public function __construct(Reader $reader) { parent::__construct($reader); - $this->invariantFetcher = new InvariantFetcher(Invariant::class, $reader); + $this->invariantFetcher = new InvariantFetcher([Invariant::class], $reader); } /** diff --git a/src/Aspect/PostconditionCheckerAspect.php b/src/Aspect/PostconditionCheckerAspect.php index 105a522..766a2e0 100644 --- a/src/Aspect/PostconditionCheckerAspect.php +++ b/src/Aspect/PostconditionCheckerAspect.php @@ -28,7 +28,7 @@ class PostconditionCheckerAspect extends AbstractContractAspect implements Aspec public function __construct(Reader $reader) { parent::__construct($reader); - $this->methodConditionFetcher = new MethodConditionFetcher(Ensure::class, $reader); + $this->methodConditionFetcher = new MethodConditionFetcher([Ensure::class], $reader); } /** diff --git a/src/Aspect/PreconditionCheckerAspect.php b/src/Aspect/PreconditionCheckerAspect.php index 5358a0a..3385217 100644 --- a/src/Aspect/PreconditionCheckerAspect.php +++ b/src/Aspect/PreconditionCheckerAspect.php @@ -28,7 +28,7 @@ class PreconditionCheckerAspect extends AbstractContractAspect implements Aspect public function __construct(Reader $reader) { parent::__construct($reader); - $this->methodConditionFetcher = new MethodConditionWithInheritDocFetcher(Verify::class, $reader); + $this->methodConditionFetcher = new MethodConditionWithInheritDocFetcher([Verify::class], $reader); } /** diff --git a/src/Contract/Fetcher/Parent/AbstractFetcher.php b/src/Contract/Fetcher/Parent/AbstractFetcher.php index 18474fb..860934b 100644 --- a/src/Contract/Fetcher/Parent/AbstractFetcher.php +++ b/src/Contract/Fetcher/Parent/AbstractFetcher.php @@ -15,9 +15,9 @@ abstract class AbstractFetcher { /** - * @var string + * @var string[] */ - protected $expectedAnnotationType; + protected $expectedAnnotationTypes; /** * @var Reader @@ -25,13 +25,13 @@ abstract class AbstractFetcher protected $annotationReader; /** - * @param string $expectedAnnotationType - * @param Reader $reader + * @param string[] $expectedAnnotationTypes + * @param Reader $reader */ - public function __construct($expectedAnnotationType, Reader $reader) + public function __construct($expectedAnnotationTypes, Reader $reader) { - $this->expectedAnnotationType = $expectedAnnotationType; - $this->annotationReader = $reader; + $this->expectedAnnotationTypes = $expectedAnnotationTypes; + $this->annotationReader = $reader; } /** @@ -45,7 +45,9 @@ protected function filterContractAnnotation(array $annotations) $contractAnnotations = []; foreach ($annotations as $annotation) { - if ($annotation instanceof $this->expectedAnnotationType) { + $annotationClass = \get_class($annotation); + + if (\in_array($annotationClass, $this->expectedAnnotationTypes, true)) { $contractAnnotations[] = $annotation; } } diff --git a/src/ContractApplication.php b/src/ContractApplication.php index c5377a8..52124ea 100644 --- a/src/ContractApplication.php +++ b/src/ContractApplication.php @@ -11,6 +11,7 @@ use Go\Core\AspectContainer; use Go\Core\AspectKernel; +use PhpDeal\Aspect\InheritCheckerAspect; use PhpDeal\Aspect\InvariantCheckerAspect; use PhpDeal\Aspect\PostconditionCheckerAspect; use PhpDeal\Aspect\PreconditionCheckerAspect; @@ -34,5 +35,6 @@ protected function configureAop(AspectContainer $container) $container->registerAspect(new InvariantCheckerAspect($reader)); $container->registerAspect(new PostconditionCheckerAspect($reader)); $container->registerAspect(new PreconditionCheckerAspect($reader)); + $container->registerAspect(new InheritCheckerAspect($reader)); } } diff --git a/src/Exception/ContractViolation.php b/src/Exception/ContractViolation.php index a957d26..94af037 100644 --- a/src/Exception/ContractViolation.php +++ b/src/Exception/ContractViolation.php @@ -28,10 +28,12 @@ class ContractViolation extends \LogicException */ public function __construct(MethodInvocation $invocation, $contract, Exception $previous = null) { - $obj = $invocation->getThis(); - $objName = is_object($obj) ? get_class($obj) : $obj; - $method = $invocation->getMethod(); + $obj = $invocation->getThis(); + $objName = is_object($obj) ? get_class($obj) : $obj; + $method = $invocation->getMethod(); +// $arguments = implode(', ', $invocation->getArguments()); +// $message = "Contract {$contract} violated with {$arguments} for {$objName}->{$method->name}"; $message = "Contract {$contract} violated for {$objName}->{$method->name}"; parent::__construct($message, 0, $previous); diff --git a/tests/Functional/Inherit/ContractTest.php b/tests/Functional/Inherit/ContractTest.php new file mode 100644 index 0000000..170e2f2 --- /dev/null +++ b/tests/Functional/Inherit/ContractTest.php @@ -0,0 +1,46 @@ +stubWithoutInherit = new StubWithoutInherit(); + $this->stubWithInherit = new StubWithInherit(); + } + + public function tearDown() + { + unset($this->stubWithoutInherit, $this->stubWithInherit); + } + + public function testWithoutInheritIsValid() + { + $this->stubWithoutInherit->add("test"); + $this->stubWithoutInherit->add(0); + $this->assertEquals(10, $this->stubWithoutInherit->getAmount()); + } + + /** + * @expectedException \PhpDeal\Exception\ContractViolation + * @return void + */ + public function testWithInheritInvalid() + { + $this->stubWithInherit->add(0); + } + + public function testWithInheritValid() + { + $this->stubWithInherit->add(10); + $this->assertEquals(10, $this->stubWithInherit->getAmount()); + } +} diff --git a/tests/Functional/Inherit/StubInterface.php b/tests/Functional/Inherit/StubInterface.php new file mode 100644 index 0000000..6266e25 --- /dev/null +++ b/tests/Functional/Inherit/StubInterface.php @@ -0,0 +1,20 @@ + 0 && is_numeric($amount)") + * @param integer $amount + * @return void + */ + public function add($amount); + + /** + * @Contract\Ensure("$__result === $this->amount") + */ + public function getAmount(); +} diff --git a/tests/Functional/Inherit/StubWithInherit.php b/tests/Functional/Inherit/StubWithInherit.php new file mode 100644 index 0000000..f6157bf --- /dev/null +++ b/tests/Functional/Inherit/StubWithInherit.php @@ -0,0 +1,29 @@ +amount += $amount; + } + + /** + * @Contract\Inherit() + * @return integer + */ + public function getAmount() + { + return $this->amount; + } +} diff --git a/tests/Functional/Inherit/StubWithoutInherit.php b/tests/Functional/Inherit/StubWithoutInherit.php new file mode 100644 index 0000000..8df7f7e --- /dev/null +++ b/tests/Functional/Inherit/StubWithoutInherit.php @@ -0,0 +1,18 @@ +amount = $amount; + } + + public function getAmount() + { + return 10; + } +}