From b6410e79483f69dac8bf725dfef0c8e1f5eafc5b Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Wed, 17 Apr 2024 13:57:47 +0200 Subject: [PATCH] ForbidCheckedExceptionInCallableRule: replace immediatelyCalledCallables with native param-immediately-invoked-callable --- README.md | 46 +-- composer-dependency-analyser.php | 3 +- composer.json | 3 +- rules.neon | 42 -- ...iatelyCalledCallableThrowTypeExtension.php | 251 ------------ .../ForbidCheckedExceptionInCallableRule.php | 366 +++++++++++++----- .../ImmediatelyCalledCallableVisitor.php | 171 -------- ...lyCalledCallableThrowTypeExtensionTest.php | 41 -- .../code.php | 247 ------------ .../extension.neon | 17 - ...rbidCheckedExceptionInCallableRuleTest.php | 22 +- .../allowed.neon | 7 + .../code.php | 94 +++++ .../visitor.neon | 17 - tests/RuleTestCase.php | 23 +- 15 files changed, 409 insertions(+), 941 deletions(-) delete mode 100644 src/Extension/ImmediatelyCalledCallableThrowTypeExtension.php delete mode 100644 src/Visitor/ImmediatelyCalledCallableVisitor.php delete mode 100644 tests/Extension/ImmediatelyCalledCallableThrowTypeExtensionTest.php delete mode 100644 tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/code.php delete mode 100644 tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/extension.neon create mode 100644 tests/Rule/data/ForbidCheckedExceptionInCallableRule/allowed.neon delete mode 100644 tests/Rule/data/ForbidCheckedExceptionInCallableRule/visitor.neon diff --git a/README.md b/README.md index 7c5b142..6806817 100644 --- a/README.md +++ b/README.md @@ -49,29 +49,6 @@ parameters: blacklist: ['(array)', '(object)', '(unset)'] forbidCheckedExceptionInCallable: enabled: true - immediatelyCalledCallables: - array_reduce: 1 - array_intersect_ukey: 2 - array_uintersect: 2 - array_uintersect_assoc: 2 - array_intersect_uassoc: 2 - array_uintersect_uassoc: [2, 3] - array_diff_ukey: 2 - array_udiff: 2 - array_udiff_assoc: 2 - array_diff_uassoc: 2 - array_udiff_uassoc: [2, 3] - array_filter: 1 - array_map: 0 - array_walk_recursive: 1 - array_walk: 1 - call_user_func: 0 - call_user_func_array: 0 - forward_static_call: 0 - forward_static_call_array: 0 - uasort: 1 - uksort: 1 - usort: 1 allowedCheckedExceptionCallables: [] forbidCheckedExceptionInYieldingMethod: enabled: true @@ -350,8 +327,7 @@ parameters: ### forbidCheckedExceptionInCallable - Denies throwing [checked exception](https://phpstan.org/blog/bring-your-exceptions-under-control) in callables (Closures, Arrow functions and First class callables) as those cannot be tracked as checked by PHPStan analysis, because it is unknown when the callable is about to be called -- It allows configuration of functions/methods, where the callable is called immediately, those cases are allowed and are also added to [dynamic throw type extension](https://phpstan.org/developing-extensions/dynamic-throw-type-extensions) which causes those exceptions to be tracked properly in your codebase (!) - - By default, native functions like `array_map` are present. So it is recommended not to overwrite the defaults here (by `!` char). +- It is allowed to throw checked exceptions in immediately called callables (e.g. params marked by `@param-immediately-invoked-callable`, see [docs](https://phpstan.org/writing-php-code/phpdocs-basics#callables)) - It allows configuration of functions/methods, where the callable is handling all thrown exceptions and it is safe to throw anything from there; this basically makes such calls ignored by this rule - It ignores [implicitly thrown Throwable](https://phpstan.org/blog/bring-your-exceptions-under-control#what-does-absent-%40throws-above-a-function-mean%3F) - Learn more in 🇨🇿 [talk about checked exceptions in general](https://www.youtube.com/watch?v=UQsP1U0sVZM) @@ -360,10 +336,6 @@ parameters: parameters: shipmonkRules: forbidCheckedExceptionInCallable: - immediatelyCalledCallables: - 'Doctrine\ORM\EntityManager::transactional': 0 # 0 is argument index where the closure appears, you can use list if needed - 'Symfony\Contracts\Cache\CacheInterface::get': 1 - 'Acme\my_custom_function': 0 allowedCheckedExceptionCallables: 'Symfony\Component\Console\Question::setValidator': 0 # symfony automatically converts all thrown exceptions to error output, so it is safe to throw anything here ``` @@ -384,16 +356,26 @@ parameters: ```php +class TransactionManager { + /** + * @param-immediately-invoked-callable $callback + */ + public function transactional(callable $callback): void { + // ... + $callback(); + // ... + } +} + class UserEditFacade { /** * @throws UserNotFoundException - * ^ This throws would normally be reported as never thrown in native phpstan, but we know the closure is immediately called */ public function updateUserEmail(UserId $userId, Email $email): void { - $this->entityManager->transactional(function () use ($userId, $email) { - $user = $this->userRepository->get($userId); // throws checked UserNotFoundException + $this->transactionManager->transactional(function () use ($userId, $email) { + $user = $this->userRepository->get($userId); // can throw checked UserNotFoundException $user->updateEmail($email); }) } diff --git a/composer-dependency-analyser.php b/composer-dependency-analyser.php index 339ab77..dc3aa07 100644 --- a/composer-dependency-analyser.php +++ b/composer-dependency-analyser.php @@ -8,5 +8,4 @@ require_once('phar://phpstan.phar/preload.php'); // prepends PHPStan's PharAutolaoder to composer's autoloader return (new Configuration()) - ->addPathToExclude(__DIR__ . '/tests/Rule/data') - ->addPathToExclude(__DIR__ . '/tests/Extension/data'); + ->addPathToExclude(__DIR__ . '/tests/Rule/data'); diff --git a/composer.json b/composer.json index bed8772..4194f81 100644 --- a/composer.json +++ b/composer.json @@ -34,8 +34,7 @@ "ShipMonk\\PHPStan\\": "tests/" }, "classmap": [ - "tests/Rule/data", - "tests/Extension/data" + "tests/Rule/data" ] }, "config": { diff --git a/rules.neon b/rules.neon index 62375db..dc2b5db 100644 --- a/rules.neon +++ b/rules.neon @@ -28,29 +28,6 @@ parameters: blacklist: ['(array)', '(object)', '(unset)'] forbidCheckedExceptionInCallable: enabled: true - immediatelyCalledCallables: - array_reduce: 1 - array_intersect_ukey: 2 - array_uintersect: 2 - array_uintersect_assoc: 2 - array_intersect_uassoc: 2 - array_uintersect_uassoc: [2, 3] - array_diff_ukey: 2 - array_udiff: 2 - array_udiff_assoc: 2 - array_diff_uassoc: 2 - array_udiff_uassoc: [2, 3] - array_filter: 1 - array_map: 0 - array_walk_recursive: 1 - array_walk: 1 - call_user_func: 0 - call_user_func_array: 0 - forward_static_call: 0 - forward_static_call_array: 0 - uasort: 1 - uksort: 1 - usort: 1 allowedCheckedExceptionCallables: [] forbidCheckedExceptionInYieldingMethod: enabled: true @@ -147,7 +124,6 @@ parametersSchema: ]) forbidCheckedExceptionInCallable: structure([ enabled: bool() - immediatelyCalledCallables: arrayOf(anyOf(listOf(int()), int()), string()) allowedCheckedExceptionCallables: arrayOf(anyOf(listOf(int()), int()), string()) ]) forbidCheckedExceptionInYieldingMethod: structure([ @@ -302,8 +278,6 @@ conditionalTags: ShipMonk\PHPStan\Rule\UselessPrivatePropertyNullabilityRule: phpstan.rules.rule: %shipmonkRules.uselessPrivatePropertyNullability.enabled% - ShipMonk\PHPStan\Visitor\ImmediatelyCalledCallableVisitor: - phpstan.parser.richParserNodeVisitor: %shipmonkRules.forbidCheckedExceptionInCallable.enabled% ShipMonk\PHPStan\Visitor\UnusedExceptionVisitor: phpstan.parser.richParserNodeVisitor: %shipmonkRules.forbidUnusedException.enabled% ShipMonk\PHPStan\Visitor\UnusedMatchVisitor: @@ -313,11 +287,6 @@ conditionalTags: ShipMonk\PHPStan\Visitor\ClassPropertyAssignmentVisitor: phpstan.parser.richParserNodeVisitor: %shipmonkRules.uselessPrivatePropertyNullability.enabled% - ShipMonk\PHPStan\Extension\ImmediatelyCalledCallableThrowTypeExtension: - phpstan.dynamicFunctionThrowTypeExtension: %shipmonkRules.forbidCheckedExceptionInCallable.enabled% - phpstan.dynamicMethodThrowTypeExtension: %shipmonkRules.forbidCheckedExceptionInCallable.enabled% - phpstan.dynamicStaticMethodThrowTypeExtension: %shipmonkRules.forbidCheckedExceptionInCallable.enabled% - services: - class: ShipMonk\PHPStan\Rule\AllowComparingOnlyComparableTypesRule @@ -352,7 +321,6 @@ services: - class: ShipMonk\PHPStan\Rule\ForbidCheckedExceptionInCallableRule arguments: - immediatelyCalledCallables: %shipmonkRules.forbidCheckedExceptionInCallable.immediatelyCalledCallables% allowedCheckedExceptionCallables: %shipmonkRules.forbidCheckedExceptionInCallable.allowedCheckedExceptionCallables% - class: ShipMonk\PHPStan\Rule\ForbidCheckedExceptionInYieldingMethodRule @@ -422,11 +390,6 @@ services: class: ShipMonk\PHPStan\Rule\RequirePreviousExceptionPassRule arguments: reportEvenIfExceptionIsNotAcceptableByRethrownOne: %shipmonkRules.requirePreviousExceptionPass.reportEvenIfExceptionIsNotAcceptableByRethrownOne% - - - class: ShipMonk\PHPStan\Visitor\ImmediatelyCalledCallableVisitor - arguments: - immediatelyCalledCallables: %shipmonkRules.forbidCheckedExceptionInCallable.immediatelyCalledCallables% - allowedCheckedExceptionCallables: %shipmonkRules.forbidCheckedExceptionInCallable.allowedCheckedExceptionCallables% - class: ShipMonk\PHPStan\Visitor\UnusedExceptionVisitor - @@ -435,8 +398,3 @@ services: class: ShipMonk\PHPStan\Visitor\TopLevelConstructorPropertyFetchMarkingVisitor - class: ShipMonk\PHPStan\Visitor\ClassPropertyAssignmentVisitor - - - - class: ShipMonk\PHPStan\Extension\ImmediatelyCalledCallableThrowTypeExtension - arguments: - immediatelyCalledCallables: %shipmonkRules.forbidCheckedExceptionInCallable.immediatelyCalledCallables% diff --git a/src/Extension/ImmediatelyCalledCallableThrowTypeExtension.php b/src/Extension/ImmediatelyCalledCallableThrowTypeExtension.php deleted file mode 100644 index 5079f06..0000000 --- a/src/Extension/ImmediatelyCalledCallableThrowTypeExtension.php +++ /dev/null @@ -1,251 +0,0 @@ - callable argument index(es) - * or - * function => callable argument index(es) - * - * @var array> - */ - private array $immediatelyCalledCallables; - - private NodeScopeResolver $nodeScopeResolver; - - private ReflectionProvider $reflectionProvider; - - /** - * @param array> $immediatelyCalledCallables - */ - public function __construct( - NodeScopeResolver $nodeScopeResolver, - ReflectionProvider $reflectionProvider, - array $immediatelyCalledCallables - ) - { - $this->nodeScopeResolver = $nodeScopeResolver; - $this->reflectionProvider = $reflectionProvider; - $this->immediatelyCalledCallables = $immediatelyCalledCallables; - } - - public function isFunctionSupported(FunctionReflection $functionReflection): bool - { - return $this->isCallSupported($functionReflection); - } - - public function isMethodSupported(MethodReflection $methodReflection): bool - { - return $this->isCallSupported($methodReflection); - } - - public function isStaticMethodSupported(MethodReflection $methodReflection): bool - { - return $this->isCallSupported($methodReflection); - } - - /** - * @param FunctionReflection|MethodReflection $callReflection - */ - private function isCallSupported(object $callReflection): bool - { - return $this->getClosureArgumentPositions($callReflection) !== []; - } - - public function getThrowTypeFromFunctionCall( - FunctionReflection $functionReflection, - FuncCall $functionCall, - Scope $scope - ): ?Type - { - return $this->combineCallbackAndCallThrowTypes($functionCall, $functionReflection, $scope); - } - - public function getThrowTypeFromMethodCall( - MethodReflection $methodReflection, - MethodCall $methodCall, - Scope $scope - ): ?Type - { - return $this->combineCallbackAndCallThrowTypes($methodCall, $methodReflection, $scope); - } - - public function getThrowTypeFromStaticMethodCall( - MethodReflection $methodReflection, - StaticCall $methodCall, - Scope $scope - ): ?Type - { - return $this->combineCallbackAndCallThrowTypes($methodCall, $methodReflection, $scope); - } - - /** - * @param FunctionReflection|MethodReflection $callReflection - */ - private function combineCallbackAndCallThrowTypes( - CallLike $call, - object $callReflection, - Scope $scope - ): ?Type - { - if (!$scope instanceof MutatingScope) { // @phpstan-ignore-line ignore bc promise - throw new LogicException('Unexpected scope implementation'); - } - - $argumentPositions = $this->getClosureArgumentPositions($callReflection); - - $throwTypes = $callReflection->getThrowType() !== null - ? [$callReflection->getThrowType()] - : []; - - foreach ($argumentPositions as $argumentPosition) { - $args = $call->getArgs(); - - if (!isset($args[$argumentPosition])) { - continue; - } - - $argumentValue = $args[$argumentPosition]->value; - - if ($argumentValue instanceof Closure) { - $result = $this->nodeScopeResolver->processStmtNodes( - $call, - $argumentValue->getStmts(), - $scope->enterAnonymousFunction($argumentValue), - static function (): void { - }, - ); - - foreach ($result->getThrowPoints() as $throwPoint) { - if ($throwPoint->isExplicit()) { - $throwTypes[] = $throwPoint->getType(); - } - } - } - - if ($argumentValue instanceof ArrowFunction) { - $result = $this->nodeScopeResolver->processStmtNodes( - $call, - $argumentValue->getStmts(), - $scope->enterArrowFunction($argumentValue), - static function (): void { - }, - ); - - foreach ($result->getThrowPoints() as $throwPoint) { - if ($throwPoint->isExplicit()) { - $throwTypes[] = $throwPoint->getType(); - } - } - } - - if ($argumentValue instanceof StaticCall - && $argumentValue->isFirstClassCallable() - && $argumentValue->name instanceof Identifier - && $argumentValue->class instanceof Name - ) { - $methodName = (string) $argumentValue->name; - $className = $scope->resolveName($argumentValue->class); - - $caller = $this->reflectionProvider->getClass($className); - $method = $caller->getMethod($methodName, $scope); - - if ($method->getThrowType() !== null) { - $throwTypes[] = $method->getThrowType(); - } - } - - if ($argumentValue instanceof MethodCall - && $argumentValue->isFirstClassCallable() - && $argumentValue->name instanceof Identifier - ) { - $methodName = (string) $argumentValue->name; - $callerType = $scope->getType($argumentValue->var); - - foreach ($callerType->getObjectClassReflections() as $callerReflection) { - $method = $callerReflection->getMethod($methodName, $scope); - - if ($method->getThrowType() !== null) { - $throwTypes[] = $method->getThrowType(); - } - } - } - } - - if ($throwTypes === []) { - return null; - } - - return TypeCombinator::union(...$throwTypes); - } - - /** - * @param FunctionReflection|MethodReflection $callReflection - * @return list - */ - private function getClosureArgumentPositions(object $callReflection): array - { - if ($callReflection instanceof FunctionReflection) { - return $this->normalizeArgumentIndexes($this->immediatelyCalledCallables[$callReflection->getName()] ?? []); - } - - $argumentPositions = []; - $classReflection = $callReflection->getDeclaringClass(); - - foreach ($this->immediatelyCalledCallables as $immediateCallerAndMethod => $indexes) { - if (strpos($immediateCallerAndMethod, '::') === false) { - continue; - } - - [$callerClass, $methodName] = explode('::', $immediateCallerAndMethod); - - if ($methodName !== $callReflection->getName() || !$classReflection->is($callerClass)) { - continue; - } - - $argumentPositions = array_merge($argumentPositions, $this->normalizeArgumentIndexes($indexes)); - } - - return array_values(array_unique($argumentPositions)); - } - - /** - * @param int|list $argumentIndexes - * @return list - */ - private function normalizeArgumentIndexes($argumentIndexes): array - { - return is_int($argumentIndexes) ? [$argumentIndexes] : $argumentIndexes; - } - -} diff --git a/src/Rule/ForbidCheckedExceptionInCallableRule.php b/src/Rule/ForbidCheckedExceptionInCallableRule.php index 2e0e398..e0e0eac 100644 --- a/src/Rule/ForbidCheckedExceptionInCallableRule.php +++ b/src/Rule/ForbidCheckedExceptionInCallableRule.php @@ -4,36 +4,46 @@ use LogicException; use PhpParser\Node; -use PhpParser\Node\Expr; +use PhpParser\Node\Arg; use PhpParser\Node\Expr\ArrowFunction; use PhpParser\Node\Expr\CallLike; +use PhpParser\Node\Expr\Closure; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\New_; +use PhpParser\Node\Expr\NullsafeMethodCall; use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\Identifier; use PhpParser\Node\Name; use PhpParser\Node\Stmt\Expression; +use PHPStan\Analyser\ArgumentsNormalizer; use PHPStan\Analyser\ExpressionContext; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; use PHPStan\Analyser\Scope; use PHPStan\Node\ClosureReturnStatementsNode; +use PHPStan\Node\FileNode; use PHPStan\Node\FunctionCallableNode; use PHPStan\Node\MethodCallableNode; use PHPStan\Node\StaticMethodCallableNode; +use PHPStan\Reflection\FunctionReflection; +use PHPStan\Reflection\MethodReflection; +use PHPStan\Reflection\ParameterReflection; +use PHPStan\Reflection\ParameterReflectionWithPhpDocs; +use PHPStan\Reflection\ParametersAcceptorSelector; use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Exceptions\DefaultExceptionTypeResolver; use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Type; -use ShipMonk\PHPStan\Visitor\ImmediatelyCalledCallableVisitor; use function array_map; use function array_merge; -use function array_merge_recursive; +use function array_values; use function explode; use function in_array; use function is_int; +use function spl_object_hash; use function strpos; /** @@ -49,37 +59,41 @@ class ForbidCheckedExceptionInCallableRule implements Rule private DefaultExceptionTypeResolver $exceptionTypeResolver; /** - * class::method => Closure argument index + * @var array spl_hash => true + */ + private array $allowedCallables = []; + + /** + * @var array spl_hash => methodName + */ + private array $callablesInArguments = []; + + /** + * class::method => callable argument index * or - * function => Closure argument index + * function => callable argument index * * @var array> */ private array $callablesAllowingCheckedExceptions; /** - * @param array> $immediatelyCalledCallables * @param array> $allowedCheckedExceptionCallables */ public function __construct( NodeScopeResolver $nodeScopeResolver, ReflectionProvider $reflectionProvider, DefaultExceptionTypeResolver $exceptionTypeResolver, - array $immediatelyCalledCallables, array $allowedCheckedExceptionCallables ) { - $this->checkClassExistence($reflectionProvider, $immediatelyCalledCallables, 'immediatelyCalledCallables'); - $this->checkClassExistence($reflectionProvider, $allowedCheckedExceptionCallables, 'allowedCheckedExceptionCallables'); - - /** @var array> $callablesWithAllowedCheckedExceptions */ - $callablesWithAllowedCheckedExceptions = array_merge_recursive($immediatelyCalledCallables, $allowedCheckedExceptionCallables); + $this->checkClassExistence($reflectionProvider, $allowedCheckedExceptionCallables); $this->callablesAllowingCheckedExceptions = array_map( function ($argumentIndexes): array { return $this->normalizeArgumentIndexes($argumentIndexes); }, - $callablesWithAllowedCheckedExceptions, + $allowedCheckedExceptionCallables, ); $this->exceptionTypeResolver = $exceptionTypeResolver; $this->reflectionProvider = $reflectionProvider; @@ -99,23 +113,30 @@ public function processNode( Scope $scope ): array { - if ( - $node instanceof MethodCallableNode // @phpstan-ignore-line ignore bc promise - || $node instanceof StaticMethodCallableNode // @phpstan-ignore-line ignore bc promise - || $node instanceof FunctionCallableNode // @phpstan-ignore-line ignore bc promise + $errors = []; + + if ($node instanceof FileNode) { + $this->allowedCallables = []; + $this->callablesInArguments = []; + + } elseif ($node instanceof CallLike) { + $this->whitelistAllowedCallables($node, $scope); + + } elseif ( + $node instanceof MethodCallableNode + || $node instanceof StaticMethodCallableNode + || $node instanceof FunctionCallableNode ) { return $this->processFirstClassCallable($node->getOriginalNode(), $scope); - } - if ($node instanceof ClosureReturnStatementsNode) { // @phpstan-ignore-line ignore bc promise - return $this->processClosure($node, $scope); - } + } elseif ($node instanceof ClosureReturnStatementsNode) { + return $this->processClosure($node); - if ($node instanceof ArrowFunction) { + } elseif ($node instanceof ArrowFunction) { return $this->processArrowFunction($node, $scope); } - return []; + return $errors; } /** @@ -131,29 +152,32 @@ public function processFirstClassCallable( throw new LogicException('This should be ensured by using XxxCallableNode'); } - if ($this->isAllowedToThrowCheckedException($callNode, $scope)) { + $nodeHash = spl_object_hash($callNode); + + if (isset($this->allowedCallables[$nodeHash])) { return []; } $errors = []; + $line = $callNode->getLine(); if ($callNode instanceof MethodCall && $callNode->name instanceof Identifier) { $callerType = $scope->getType($callNode->var); $methodName = $callNode->name->toString(); - $errors = array_merge($errors, $this->processCall($scope, $callerType, $methodName)); + $errors = array_merge($errors, $this->processCall($scope, $callerType, $methodName, $line, $nodeHash)); } if ($callNode instanceof StaticCall && $callNode->class instanceof Name && $callNode->name instanceof Identifier) { $callerType = $scope->resolveTypeByName($callNode->class); $methodName = $callNode->name->toString(); - $errors = array_merge($errors, $this->processCall($scope, $callerType, $methodName)); + $errors = array_merge($errors, $this->processCall($scope, $callerType, $methodName, $line, $nodeHash)); } if ($callNode instanceof FuncCall && $callNode->name instanceof Name) { $functionReflection = $this->reflectionProvider->getFunction($callNode->name, $scope); - $errors = array_merge($errors, $this->processThrowType($functionReflection->getThrowType(), $scope)); + $errors = array_merge($errors, $this->processThrowType($functionReflection->getThrowType(), $scope, $line, $nodeHash)); } return $errors; @@ -163,18 +187,12 @@ public function processFirstClassCallable( * @return list */ public function processClosure( - ClosureReturnStatementsNode $node, - Scope $scope + ClosureReturnStatementsNode $node ): array { - $closure = $node->getClosureExpr(); - $parentScope = $scope->getParentScope(); // we need to detect type of caller, so the scope outside of this closure is needed - - if ($parentScope === null) { - return []; - } + $nodeHash = spl_object_hash($node->getClosureExpr()); - if ($this->isAllowedToThrowCheckedException($closure, $parentScope)) { + if (isset($this->allowedCallables[$nodeHash])) { return []; } @@ -187,10 +205,12 @@ public function processClosure( foreach ($throwPoint->getType()->getObjectClassNames() as $exceptionClass) { if ($this->exceptionTypeResolver->isCheckedException($exceptionClass, $throwPoint->getScope())) { - $errors[] = RuleErrorBuilder::message("Throwing checked exception $exceptionClass in closure!") - ->line($throwPoint->getNode()->getLine()) - ->identifier('shipmonk.checkedExceptionInCallable') - ->build(); + $errors[] = $this->buildError( + $exceptionClass, + 'closure', + $throwPoint->getNode()->getLine(), + $this->callablesInArguments[$nodeHash] ?? null, + ); } } } @@ -206,36 +226,40 @@ public function processArrowFunction( Scope $scope ): array { - if (!$scope instanceof MutatingScope) { // @phpstan-ignore-line ignore BC promise + if (!$scope instanceof MutatingScope) { throw new LogicException('Unexpected scope implementation'); } - if ($this->isAllowedToThrowCheckedException($node, $scope)) { + $nodeHash = spl_object_hash($node); + + if (isset($this->allowedCallables[$nodeHash])) { return []; } - $result = $this->nodeScopeResolver->processExprNode( // @phpstan-ignore-line ignore BC promise + $result = $this->nodeScopeResolver->processExprNode( new Expression($node->expr), $node->expr, $scope->enterArrowFunction($node), static function (): void { }, - ExpressionContext::createDeep(), // @phpstan-ignore-line ignore BC promise + ExpressionContext::createDeep(), ); $errors = []; - foreach ($result->getThrowPoints() as $throwPoint) { // @phpstan-ignore-line ignore BC promise + foreach ($result->getThrowPoints() as $throwPoint) { if (!$throwPoint->isExplicit()) { continue; } foreach ($throwPoint->getType()->getObjectClassNames() as $exceptionClass) { if ($this->exceptionTypeResolver->isCheckedException($exceptionClass, $throwPoint->getScope())) { - $errors[] = RuleErrorBuilder::message("Throwing checked exception $exceptionClass in arrow function!") - ->line($throwPoint->getNode()->getLine()) - ->identifier('shipmonk.checkedExceptionInArrowFunction') - ->build(); + $errors[] = $this->buildError( + $exceptionClass, + 'arrow function', + $throwPoint->getNode()->getLine(), + $this->callablesInArguments[$nodeHash] ?? null, + ); } } } @@ -249,13 +273,15 @@ static function (): void { private function processCall( Scope $scope, Type $callerType, - string $methodName + string $methodName, + int $line, + string $nodeHash ): array { $methodReflection = $scope->getMethodReflection($callerType, $methodName); if ($methodReflection !== null) { - return $this->processThrowType($methodReflection->getThrowType(), $scope); + return $this->processThrowType($methodReflection->getThrowType(), $scope, $line, $nodeHash); } return []; @@ -266,7 +292,9 @@ private function processCall( */ private function processThrowType( ?Type $throwType, - Scope $scope + Scope $scope, + int $line, + string $nodeHash ): array { if ($throwType === null) { @@ -277,53 +305,99 @@ private function processThrowType( foreach ($throwType->getObjectClassNames() as $exceptionClass) { if ($this->exceptionTypeResolver->isCheckedException($exceptionClass, $scope)) { - $errors[] = RuleErrorBuilder::message("Throwing checked exception $exceptionClass in first-class-callable!") - ->identifier('shipmonk.checkedExceptionInCallable') - ->build(); + $errors[] = $this->buildError( + $exceptionClass, + 'first-class-callable', + $line, + $this->callablesInArguments[$nodeHash] ?? null, + ); } } return $errors; } - public function isAllowedToThrowCheckedException( - Node $node, - Scope $scope - ): bool + /** + * @param int|list $argumentIndexes + * @return list + */ + private function normalizeArgumentIndexes($argumentIndexes): array { - /** @var Expr|Name|null $callerNodeWithClosureAsArg */ - $callerNodeWithClosureAsArg = $node->getAttribute(ImmediatelyCalledCallableVisitor::CALLER_WITH_CALLABLE_POSSIBLY_ALLOWING_CHECKED_EXCEPTION); - /** @var string|null $methodNameWithClosureAsArg */ - $methodNameWithClosureAsArg = $node->getAttribute(ImmediatelyCalledCallableVisitor::METHOD_WITH_CALLABLE_POSSIBLY_ALLOWING_CHECKED_EXCEPTION); - /** @var int|null $argumentIndexWithClosureAsArg */ - $argumentIndexWithClosureAsArg = $node->getAttribute(ImmediatelyCalledCallableVisitor::ARGUMENT_INDEX_WITH_CALLABLE_POSSIBLY_ALLOWING_CHECKED_EXCEPTION); - /** @var true|null $isAllowedToThrow */ - $isAllowedToThrow = $node->getAttribute(ImmediatelyCalledCallableVisitor::CALLABLE_ALLOWING_CHECKED_EXCEPTION); - - if ($isAllowedToThrow === true) { - return true; + return is_int($argumentIndexes) ? [$argumentIndexes] : $argumentIndexes; + } + + /** + * @param array> $callables + */ + private function checkClassExistence( + ReflectionProvider $reflectionProvider, + array $callables + ): void + { + foreach ($callables as $call => $args) { + if (strpos($call, '::') === false) { + continue; + } + + [$className] = explode('::', $call); + + if (!$reflectionProvider->hasClass($className)) { + throw new LogicException("Class $className used in 'allowedCheckedExceptionCallables' does not exist."); + } } + } - if ($callerNodeWithClosureAsArg === null || $methodNameWithClosureAsArg === null || $argumentIndexWithClosureAsArg === null) { - return false; + /** + * Copied from phpstan https://github.com/phpstan/phpstan-src/commit/cefa296f24b8c0b7d4dc3d383cbceea35267cb3f#diff-0c3f50d118357d9cb6d6f4d0eade75b83797d57056ff3b9c58ec881a13eaa6feR4113 + * + * @param FunctionReflection|MethodReflection $reflection + */ + private function isImmediatelyInvokedCallable(object $reflection, ?ParameterReflection $parameter): bool + { + if ($parameter instanceof ParameterReflectionWithPhpDocs) { + $parameterCallImmediately = $parameter->isImmediatelyInvokedCallable(); + + if ($parameterCallImmediately->maybe()) { + return $reflection instanceof FunctionReflection; + } + + return $parameterCallImmediately->yes(); } - $callerWithClosureAsArgType = $callerNodeWithClosureAsArg instanceof Expr - ? $scope->getType($callerNodeWithClosureAsArg) - : $scope->resolveTypeByName($callerNodeWithClosureAsArg); + return $reflection instanceof FunctionReflection; + } - foreach ($callerWithClosureAsArgType->getObjectClassReflections() as $callerWithClosureAsArgClassReflection) { - foreach ($this->callablesAllowingCheckedExceptions as $immediateCallerAndMethod => $indexes) { - if (strpos($immediateCallerAndMethod, '::') === false) { + private function isAllowedCheckedExceptionCallable( + ?Type $caller, + string $calledMethodName, + int $argumentIndex + ): bool + { + if ($caller === null) { + foreach ($this->callablesAllowingCheckedExceptions as $immediateFunction => $indexes) { + if (strpos($immediateFunction, '::') !== false) { continue; } + if ( + $immediateFunction === $calledMethodName + && in_array($argumentIndex, $indexes, true) + ) { + return true; + } + } + + return false; + } + + foreach ($caller->getObjectClassReflections() as $callerReflection) { + foreach ($this->callablesAllowingCheckedExceptions as $immediateCallerAndMethod => $indexes) { [$callerClass, $methodName] = explode('::', $immediateCallerAndMethod); if ( - $methodName === $methodNameWithClosureAsArg - && in_array($argumentIndexWithClosureAsArg, $indexes, true) - && $callerWithClosureAsArgClassReflection->is($callerClass) + $methodName === $calledMethodName + && in_array($argumentIndex, $indexes, true) + && $callerReflection->is($callerClass) ) { return true; } @@ -333,35 +407,127 @@ public function isAllowedToThrowCheckedException( return false; } - /** - * @param int|list $argumentIndexes - * @return list - */ - private function normalizeArgumentIndexes($argumentIndexes): array + private function whitelistAllowedCallables(CallLike $node, Scope $scope): void { - return is_int($argumentIndexes) ? [$argumentIndexes] : $argumentIndexes; + if ($node instanceof MethodCall && $node->name instanceof Identifier) { + $callerType = $scope->getType($node->var); + $methodReflection = $scope->getMethodReflection($callerType, $node->name->name); + + } elseif ($node instanceof StaticCall && $node->name instanceof Identifier && $node->class instanceof Name) { + $callerType = $scope->resolveTypeByName($node->class); + $methodReflection = $scope->getMethodReflection($callerType, $node->name->name); + + } elseif ($node instanceof New_ && $node->class instanceof Name) { + $callerType = $scope->resolveTypeByName($node->class); + $methodReflection = $scope->getMethodReflection($callerType, '__construct'); + + } elseif ($node instanceof FuncCall && $node->name instanceof Name) { + $callerType = null; + $methodReflection = $this->reflectionProvider->getFunction($node->name, $scope); + + } elseif ($node instanceof FuncCall && $this->isFirstClassCallableOrClosureOrArrowFunction($node->name)) { // immediately called callable syntax + $this->allowedCallables[spl_object_hash($node->name)] = true; + return; + + } else { + return; + } + + if ($methodReflection === null) { + return; + } + + $parametersAcceptor = ParametersAcceptorSelector::selectFromArgs( + $scope, + $node->getArgs(), + $methodReflection->getVariants(), + $methodReflection->getNamedArgumentsVariants(), + ); + + if ($node instanceof New_) { + $arguments = (ArgumentsNormalizer::reorderNewArguments($parametersAcceptor, $node) ?? $node)->getArgs(); + + } elseif ($node instanceof FuncCall) { + $arguments = (ArgumentsNormalizer::reorderFuncArguments($parametersAcceptor, $node) ?? $node)->getArgs(); + + } elseif ($node instanceof MethodCall) { + $arguments = (ArgumentsNormalizer::reorderMethodArguments($parametersAcceptor, $node) ?? $node)->getArgs(); + + } elseif ($node instanceof StaticCall) { + $arguments = (ArgumentsNormalizer::reorderStaticCallArguments($parametersAcceptor, $node) ?? $node)->getArgs(); + + } else { + throw new LogicException('Unexpected node type'); + } + + /** @var list $args */ + $args = array_values($arguments); + $parameters = $parametersAcceptor->getParameters(); + + foreach ($args as $index => $arg) { + $parameterIndex = $this->getParameterIndex($arg, $index, $parameters); + $parameter = $parameters[$parameterIndex] ?? null; + $argHash = spl_object_hash($arg->value); + + if ( + $this->isImmediatelyInvokedCallable($methodReflection, $parameter) + || $this->isAllowedCheckedExceptionCallable($callerType, $methodReflection->getName(), $index) + ) { + $this->allowedCallables[$argHash] = true; + } + + if ($this->isFirstClassCallableOrClosureOrArrowFunction($arg->value)) { + $callerClass = $callerType !== null && $callerType->getObjectClassNames() !== [] ? $callerType->getObjectClassNames()[0] : null; + $methodReference = $callerClass !== null ? "$callerClass::{$methodReflection->getName()}" : $methodReflection->getName(); + $this->callablesInArguments[$argHash] = $methodReference; + } + } } /** - * @param array> $callables + * @param array $parameters */ - private function checkClassExistence( - ReflectionProvider $reflectionProvider, - array $callables, - string $configName - ): void + private function getParameterIndex(Arg $arg, int $argumentIndex, array $parameters): ?int { - foreach ($callables as $call => $args) { - if (strpos($call, '::') === false) { - continue; + if ($arg->name === null) { + return $argumentIndex; + } + + foreach ($parameters as $parameterIndex => $parameter) { + if ($parameter->getName() === $arg->name->toString()) { + return $parameterIndex; } + } - [$className] = explode('::', $call); + return null; + } - if (!$reflectionProvider->hasClass($className)) { - throw new LogicException("Class $className used '$configName' in does not exist."); - } + private function isFirstClassCallableOrClosureOrArrowFunction(Node $node): bool + { + return $node instanceof Closure + || $node instanceof ArrowFunction + || ($node instanceof MethodCall && $node->isFirstClassCallable()) + || ($node instanceof NullsafeMethodCall && $node->isFirstClassCallable()) + || ($node instanceof StaticCall && $node->isFirstClassCallable()) + || ($node instanceof FuncCall && $node->isFirstClassCallable()); + } + + private function buildError( + string $exceptionClass, + string $where, + int $line, + ?string $usedAsArgumentOfMethodName + ): IdentifierRuleError + { + $builder = RuleErrorBuilder::message("Throwing checked exception $exceptionClass in $where!") + ->line($line) + ->identifier('shipmonk.checkedExceptionInCallable'); + + if ($usedAsArgumentOfMethodName !== null) { + $builder->tip("If this callable is immediately called within '$usedAsArgumentOfMethodName', you should add @param-immediately-invoked-callable there. Then this error disappears and the exception will be properly propagated."); } + + return $builder->build(); } } diff --git a/src/Visitor/ImmediatelyCalledCallableVisitor.php b/src/Visitor/ImmediatelyCalledCallableVisitor.php deleted file mode 100644 index fbd1eb8..0000000 --- a/src/Visitor/ImmediatelyCalledCallableVisitor.php +++ /dev/null @@ -1,171 +0,0 @@ - callable argument indexes - * - * @var array> - */ - private array $methodsWithAllowedCheckedExceptions = []; - - /** - * function name => callable argument indexes - * - * @var array> - */ - private array $functionsWithAllowedCheckedExceptions = []; - - /** - * @param array> $immediatelyCalledCallables - * @param array> $allowedCheckedExceptionCallables - */ - public function __construct( - array $immediatelyCalledCallables = [], - array $allowedCheckedExceptionCallables = [] - ) - { - /** @var array> $callablesWithAllowedCheckedExceptions */ - $callablesWithAllowedCheckedExceptions = array_merge_recursive($immediatelyCalledCallables, $allowedCheckedExceptionCallables); - - foreach ($callablesWithAllowedCheckedExceptions as $call => $arguments) { - $normalizedArguments = $this->normalizeArgumentIndexes($arguments); - - if (strpos($call, '::') !== false) { - [, $methodName] = explode('::', $call); - $existingArguments = $this->methodsWithAllowedCheckedExceptions[$methodName] ?? []; - $this->methodsWithAllowedCheckedExceptions[$methodName] = array_values(array_unique(array_merge($existingArguments, $normalizedArguments))); - } else { - $this->functionsWithAllowedCheckedExceptions[$call] = $normalizedArguments; - } - } - } - - public function enterNode(Node $node): ?Node - { - if ($node instanceof MethodCall || $node instanceof NullsafeMethodCall || $node instanceof StaticCall) { - $this->resolveMethodCall($node); - } - - if ($node instanceof FuncCall) { - $this->resolveFuncCall($node); - } - - return null; - } - - /** - * @param StaticCall|MethodCall|NullsafeMethodCall $node - */ - private function resolveMethodCall(CallLike $node): void - { - if (!$node->name instanceof Identifier) { - return; - } - - $methodName = $node->name->name; - $argumentIndexes = $this->methodsWithAllowedCheckedExceptions[$methodName] ?? null; - - if ($argumentIndexes === null) { - return; - } - - foreach ($argumentIndexes as $argumentIndex) { - $argument = $node->getArgs()[$argumentIndex] ?? null; - - if ($argument === null) { - continue; - } - - if (!$this->isFirstClassCallableOrClosureOrArrowFunction($argument->value)) { - continue; - } - - // we cannot decide true/false like in function calls as we dont know caller type yet, this has to be resolved in Rule - $node->getArgs()[$argumentIndex]->value->setAttribute(self::CALLER_WITH_CALLABLE_POSSIBLY_ALLOWING_CHECKED_EXCEPTION, $node instanceof StaticCall ? $node->class : $node->var); - $node->getArgs()[$argumentIndex]->value->setAttribute(self::METHOD_WITH_CALLABLE_POSSIBLY_ALLOWING_CHECKED_EXCEPTION, $node->name->toString()); - $node->getArgs()[$argumentIndex]->value->setAttribute(self::ARGUMENT_INDEX_WITH_CALLABLE_POSSIBLY_ALLOWING_CHECKED_EXCEPTION, $argumentIndex); - } - } - - private function resolveFuncCall(FuncCall $node): void - { - if ($this->isFirstClassCallableOrClosureOrArrowFunction($node->name)) { - // phpcs:ignore Squiz.PHP.CommentedOutCode.Found - $node->name->setAttribute(self::CALLABLE_ALLOWING_CHECKED_EXCEPTION, true); // immediately called closure syntax, e.g. (function(){})() - return; - } - - if (!$node->name instanceof Name) { - return; - } - - $methodName = $node->name->toString(); - $argumentIndexes = $this->functionsWithAllowedCheckedExceptions[$methodName] ?? null; - - if ($argumentIndexes === null) { - return; - } - - foreach ($argumentIndexes as $argumentIndex) { - $argument = $node->getArgs()[$argumentIndex] ?? null; - - if ($argument === null) { - continue; - } - - if (!$this->isFirstClassCallableOrClosureOrArrowFunction($argument->value)) { - continue; - } - - $node->getArgs()[$argumentIndex]->value->setAttribute(self::CALLABLE_ALLOWING_CHECKED_EXCEPTION, true); - } - } - - private function isFirstClassCallableOrClosureOrArrowFunction(Node $node): bool - { - return $node instanceof Closure - || $node instanceof ArrowFunction - || ($node instanceof MethodCall && $node->isFirstClassCallable()) - || ($node instanceof NullsafeMethodCall && $node->isFirstClassCallable()) - || ($node instanceof StaticCall && $node->isFirstClassCallable()) - || ($node instanceof FuncCall && $node->isFirstClassCallable()); - } - - /** - * @param int|list $argumentIndexes - * @return list - */ - private function normalizeArgumentIndexes($argumentIndexes): array - { - return is_int($argumentIndexes) ? [$argumentIndexes] : $argumentIndexes; - } - -} diff --git a/tests/Extension/ImmediatelyCalledCallableThrowTypeExtensionTest.php b/tests/Extension/ImmediatelyCalledCallableThrowTypeExtensionTest.php deleted file mode 100644 index f7378d8..0000000 --- a/tests/Extension/ImmediatelyCalledCallableThrowTypeExtensionTest.php +++ /dev/null @@ -1,41 +0,0 @@ - - */ - public static function dataFileAsserts(): iterable - { - yield from self::gatherAssertTypes(__DIR__ . '/data/ImmediatelyCalledCallableThrowTypeExtension/code.php'); - } - - /** - * @dataProvider dataFileAsserts - * @param mixed ...$args - */ - public function testFileAsserts( - string $assertType, - string $file, - ...$args - ): void - { - $this->assertFileAsserts($assertType, $file, ...$args); - } - - /** - * @return list - */ - public static function getAdditionalConfigFiles(): array - { - return [ - __DIR__ . '/data/ImmediatelyCalledCallableThrowTypeExtension/extension.neon', - ]; - } - -} diff --git a/tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/code.php b/tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/code.php deleted file mode 100644 index 5f15830..0000000 --- a/tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/code.php +++ /dev/null @@ -1,247 +0,0 @@ - throw new \Exception()); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result); - } - } - - public function testArrowFunctionWithoutThrow(): void - { - try { - $result = Immediate::method(fn () => 42); - } finally { - assertVariableCertainty(TrinaryLogic::createYes(), $result); - } - } - - public function testFirstClassCallable(): void - { - try { - $result = Immediate::method($this->throw(...)); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result); - } - } - - public function testStaticFirstClassCallable(): void - { - try { - $result = Immediate::method(static::staticThrow(...)); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result); - } - } - - public function testFirstClassCallableNoThrow(): void - { - try { - $result = Immediate::method($this->noThrow(...)); - } finally { - assertVariableCertainty(TrinaryLogic::createYes(), $result); - } - } - - public function testInheritedMethod(): void - { - try { - $result1 = (new Immediate())->inheritedMethod($this->noThrow(...), $this->noThrow(...)); - } finally { - assertVariableCertainty(TrinaryLogic::createYes(), $result1); - } - - try { - $result2 = (new Immediate())->inheritedMethod($this->throw(...), $this->noThrow(...)); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result2); - } - - try { - $result3 = (new Immediate())->inheritedMethod($this->noThrow(...), $this->throw(...)); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result3); - } - } - -} - - -class FunctionCallExtensionTest -{ - - public function noThrow(): void - { - } - - /** @throws \Exception */ - public function throw(): void - { - throw new \Exception(); - } - - /** @throws \Exception */ - public static function staticThrow(): void - { - throw new \Exception(); - } - - public function testNoThrow(): void - { - try { - $result = array_map('ucfirst', []); - } finally { - assertVariableCertainty(TrinaryLogic::createYes(), $result); - } - } - - public function testClosure(): void - { - try { - $result = array_map(static function (): void { - throw new \Exception(); - }, []); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result); - } - } - - public function testClosureWithoutThrow(): void - { - try { - $result = array_map(static function (): void { - return; - }, []); - } finally { - assertVariableCertainty(TrinaryLogic::createYes(), $result); - } - } - - public function testArrowFunction(): void - { - try { - $result = array_map(fn () => throw new \Exception(), []); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result); - } - } - - public function testArrowFunctionWithoutThrow(): void - { - try { - $result = array_map(fn () => 42, []); - } finally { - assertVariableCertainty(TrinaryLogic::createYes(), $result); - } - } - - public function testFirstClassCallable(): void - { - try { - $result = array_map($this->throw(...), []); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result); - } - } - - public function testStaticFirstClassCallable(): void - { - try { - $result = array_map(static::staticThrow(...), []); - } finally { - assertVariableCertainty(TrinaryLogic::createMaybe(), $result); - } - } - - public function testFirstClassCallableNoThrow(): void - { - try { - $result = array_map($this->noThrow(...), []); - } finally { - assertVariableCertainty(TrinaryLogic::createYes(), $result); - } - } - -} diff --git a/tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/extension.neon b/tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/extension.neon deleted file mode 100644 index 0962298..0000000 --- a/tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/extension.neon +++ /dev/null @@ -1,17 +0,0 @@ -services: - - - class: ShipMonk\PHPStan\Extension\ImmediatelyCalledCallableThrowTypeExtension - tags: - - phpstan.dynamicFunctionThrowTypeExtension - - phpstan.dynamicMethodThrowTypeExtension - - phpstan.dynamicStaticMethodThrowTypeExtension - arguments: - immediatelyCalledCallables: - array_map: 0 - ImmediatelyCalledCallableThrowTypeExtension\ImmediateInterface::inheritedMethod: 0 - ImmediatelyCalledCallableThrowTypeExtension\BaseImmediate::inheritedMethod: 1 - ImmediatelyCalledCallableThrowTypeExtension\Immediate::method: [0, 1] - -parameters: - exceptions: - implicitThrows: false diff --git a/tests/Rule/ForbidCheckedExceptionInCallableRuleTest.php b/tests/Rule/ForbidCheckedExceptionInCallableRuleTest.php index 00fe662..d6435f2 100644 --- a/tests/Rule/ForbidCheckedExceptionInCallableRuleTest.php +++ b/tests/Rule/ForbidCheckedExceptionInCallableRuleTest.php @@ -3,7 +3,6 @@ namespace ShipMonk\PHPStan\Rule; use LogicException; -use Nette\Neon\Neon; use PHPStan\Analyser\NodeScopeResolver; use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Exceptions\DefaultExceptionTypeResolver; @@ -33,8 +32,6 @@ protected function getRule(): Rule throw new LogicException('Missing implicitThrows'); } - $visitorConfig = Neon::decodeFile(self::getVisitorConfigFilePath()); - return new ForbidCheckedExceptionInCallableRule( self::getContainer()->getByType(NodeScopeResolver::class), self::getContainer()->getByType(ReflectionProvider::class), @@ -45,8 +42,14 @@ protected function getRule(): Rule [], $this->checkedExceptions, // everything is checked when no config is provided ), - $visitorConfig['services'][0]['arguments']['immediatelyCalledCallables'], // @phpstan-ignore-line ignore mixed access - $visitorConfig['services'][0]['arguments']['allowedCheckedExceptionCallables'], // @phpstan-ignore-line ignore mixed access + [ + 'ForbidCheckedExceptionInCallableRule\CallableTest::allowThrowInInterface' => [0], + 'ForbidCheckedExceptionInCallableRule\BaseCallableTest::allowThrowInBaseClass' => [0], + 'ForbidCheckedExceptionInCallableRule\ClosureTest::allowThrow' => [0], + 'ForbidCheckedExceptionInCallableRule\FirstClassCallableTest::allowThrow' => [1], + 'ForbidCheckedExceptionInCallableRule\ArrowFunctionTest::allowThrow' => [0], + 'ForbidCheckedExceptionInCallableRule\ArrowFunctionTest::__construct' => [0], + ], ); } @@ -78,9 +81,7 @@ public function provideSetup(): iterable */ public static function getAdditionalConfigFiles(): array { - $files = [ - self::getVisitorConfigFilePath(), - ]; + $files = []; if (self::$implicitThrows === true) { $files[] = __DIR__ . '/data/ForbidCheckedExceptionInCallableRule/no-implicit-throws.neon'; @@ -89,9 +90,4 @@ public static function getAdditionalConfigFiles(): array return $files; } - private static function getVisitorConfigFilePath(): string - { - return __DIR__ . '/data/ForbidCheckedExceptionInCallableRule/visitor.neon'; - } - } diff --git a/tests/Rule/data/ForbidCheckedExceptionInCallableRule/allowed.neon b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/allowed.neon new file mode 100644 index 0000000..cd5e086 --- /dev/null +++ b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/allowed.neon @@ -0,0 +1,7 @@ +parameters: + allowedCheckedExceptionCallables: + 'ForbidCheckedExceptionInCallableRule\CallableTest::allowThrowInInterface': [0] + 'ForbidCheckedExceptionInCallableRule\BaseCallableTest::allowThrowInBaseClass': [0] + 'ForbidCheckedExceptionInCallableRule\ClosureTest::allowThrow': [0] + 'ForbidCheckedExceptionInCallableRule\FirstClassCallableTest::allowThrow': [1] + 'ForbidCheckedExceptionInCallableRule\ArrowFunctionTest::allowThrow': [0] diff --git a/tests/Rule/data/ForbidCheckedExceptionInCallableRule/code.php b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/code.php index e878af4..6d4da56 100644 --- a/tests/Rule/data/ForbidCheckedExceptionInCallableRule/code.php +++ b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/code.php @@ -101,6 +101,9 @@ private function denied(callable $callable): void } + /** + * @param-immediately-invoked-callable $callable + */ public function immediateThrow(?callable $denied, callable $callable): void { $callable(); @@ -175,6 +178,16 @@ function () { }, ); + $this->immediateThrow( + denied: function () {}, + ); + + $this->immediateThrow( + denied: function () { + throw new CheckedException(); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in closure! + }, + ); + $this->allowThrowInBaseClass(function () { $this->throws(); }); @@ -209,6 +222,9 @@ private function denied(callable $callable): void } + /** + * @param-immediately-invoked-callable $callable + */ public function immediateThrow(callable $callable, ?callable $denied = null): void { $callable(); @@ -227,6 +243,11 @@ public function allowThrow(callable $callable): void class ArrowFunctionTest extends BaseCallableTest { + public function __construct($callable) + { + new self(fn () => throw new CheckedException()); + } + public function testDeclarations(): void { $fn = fn () => throw new CheckedException(); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in arrow function! @@ -279,6 +300,9 @@ private function denied(callable $callable): void } + /** + * @param-immediately-invoked-callable $callable + */ public function immediateThrow(callable $callable): void { $callable(); @@ -294,3 +318,73 @@ public function allowThrow(callable $callable): void } } + +class ArgumentSwappingTest { + + public function test() + { + $this->call( + $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + $this->throws(...), + $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + ); + + $this->call( + second: $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + first: $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + third: $this->throws(...), + ); + + $this->call( + forth: $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + first: $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + ); + + $this->call( + $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + forth: $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + second: $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + third: $this->throws(...), + ); + + $this->call( + $this->noop(...), + $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + forth: $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + third: $this->noop(...), + ); + + // this is not yet supported, the rule do not see this as argument pass + $this->call(... [ + 'third' => $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + 'first' => $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + 'second' => $this->throws(...), // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + ]); + } + + /** + * @param-immediately-invoked-callable $third + */ + public function call( + callable $first, + ?callable $second = null, + ?callable $third = null, + ?callable $forth = null + ): void + { + + } + + private function noop(): void + { + } + + /** + * @throws CheckedException + */ + private function throws(): void + { + throw new CheckedException(); + } +} diff --git a/tests/Rule/data/ForbidCheckedExceptionInCallableRule/visitor.neon b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/visitor.neon deleted file mode 100644 index d2e27e2..0000000 --- a/tests/Rule/data/ForbidCheckedExceptionInCallableRule/visitor.neon +++ /dev/null @@ -1,17 +0,0 @@ -services: - - - class: ShipMonk\PHPStan\Visitor\ImmediatelyCalledCallableVisitor - arguments: - immediatelyCalledCallables: - 'array_map': 0 - 'ForbidCheckedExceptionInCallableRule\ClosureTest::immediateThrow': 0 - 'ForbidCheckedExceptionInCallableRule\FirstClassCallableTest::immediateThrow': 1 - 'ForbidCheckedExceptionInCallableRule\ArrowFunctionTest::immediateThrow': 0 - allowedCheckedExceptionCallables: - 'ForbidCheckedExceptionInCallableRule\CallableTest::allowThrowInInterface': [0] - 'ForbidCheckedExceptionInCallableRule\BaseCallableTest::allowThrowInBaseClass': [0] - 'ForbidCheckedExceptionInCallableRule\ClosureTest::allowThrow': [0] - 'ForbidCheckedExceptionInCallableRule\FirstClassCallableTest::allowThrow': [1] - 'ForbidCheckedExceptionInCallableRule\ArrowFunctionTest::allowThrow': [0] - tags: - - phpstan.parser.richParserNodeVisitor diff --git a/tests/RuleTestCase.php b/tests/RuleTestCase.php index 5b2a89b..b9e73d1 100644 --- a/tests/RuleTestCase.php +++ b/tests/RuleTestCase.php @@ -6,15 +6,18 @@ use PHPStan\Analyser\Error; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase as OriginalRuleTestCase; +use function array_values; use function explode; use function file_get_contents; use function file_put_contents; use function implode; +use function ksort; use function preg_match; use function preg_match_all; use function preg_replace; use function sprintf; use function trim; +use function uniqid; /** * @template TRule of Rule @@ -50,13 +53,17 @@ protected function processActualErrors(array $actualErrors): array $resultToAssert = []; foreach ($actualErrors as $error) { - $resultToAssert[] = $this->formatErrorForAssert($error->getMessage(), $error->getLine()); + $usedLine = $error->getLine() ?? -1; + $key = $usedLine . '-' . uniqid(); + $resultToAssert[$key] = $this->formatErrorForAssert($error->getMessage(), $usedLine); self::assertNotNull($error->getIdentifier(), "Missing error identifier for error: {$error->getMessage()}"); self::assertStringStartsWith('shipmonk.', $error->getIdentifier(), "Unexpected error identifier for: {$error->getMessage()}"); } - return $resultToAssert; + ksort($resultToAssert); + + return array_values($resultToAssert); } /** @@ -80,16 +87,20 @@ private function parseExpectedErrors(string $file): array } foreach ($matches[1] as $error) { - $expectedErrors[] = $this->formatErrorForAssert(trim($error), $line + 1); + $actualLine = $line + 1; + $key = $actualLine . '-' . uniqid(); + $expectedErrors[$key] = $this->formatErrorForAssert(trim($error), $actualLine); } } - return $expectedErrors; + ksort($expectedErrors); + + return array_values($expectedErrors); } - private function formatErrorForAssert(string $message, ?int $line): string + private function formatErrorForAssert(string $message, int $line): string { - return sprintf('%02d: %s', $line ?? -1, $message); + return sprintf('%02d: %s', $line, $message); } /**