From 725c15bb748dbab104af8f8db2f1a49c78ca2c93 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Thu, 24 Aug 2023 14:55:35 +0200 Subject: [PATCH] ForbidCheckedExceptionInCallableRule (#137) --- README.md | 84 +++++- composer.json | 6 +- composer.lock | 80 +++++- phpcs.xml.dist | 16 +- phpstan.neon.dist | 13 +- rules.neon | 52 +++- ...iatelyCalledCallableThrowTypeExtension.php | 224 +++++++++++++++ .../ForbidCheckedExceptionInCallableRule.php | 256 ++++++++++++++++++ .../ImmediatelyCalledCallableVisitor.php | 160 +++++++++++ ...lyCalledCallableThrowTypeExtensionTest.php | 41 +++ .../code.php | 172 ++++++++++++ .../extension.neon | 12 + ...rbidCheckedExceptionInCallableRuleTest.php | 95 +++++++ .../code.php | 159 +++++++++++ .../no-implicit-throws.neon | 3 + .../visitor.neon | 13 + 16 files changed, 1364 insertions(+), 22 deletions(-) create mode 100644 src/Extension/ImmediatelyCalledCallableThrowTypeExtension.php create mode 100644 src/Rule/ForbidCheckedExceptionInCallableRule.php create mode 100644 src/Visitor/ImmediatelyCalledCallableVisitor.php create mode 100644 tests/Extension/ImmediatelyCalledCallableThrowTypeExtensionTest.php create mode 100644 tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/code.php create mode 100644 tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/extension.neon create mode 100644 tests/Rule/ForbidCheckedExceptionInCallableRuleTest.php create mode 100644 tests/Rule/data/ForbidCheckedExceptionInCallableRule/code.php create mode 100644 tests/Rule/data/ForbidCheckedExceptionInCallableRule/no-implicit-throws.neon create mode 100644 tests/Rule/data/ForbidCheckedExceptionInCallableRule/visitor.neon diff --git a/README.md b/README.md index add3d3f..2361092 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # ShipMonk PHPStan rules About **30 super-strict rules** we found useful in ShipMonk. -We tend to have PHPStan set up as strict as possible (bleedingEdge, strict-rules, checkUninitializedProperties, ...), but that still was not strict enough for us. +We tend to have PHPStan set up as strict as possible ([bleedingEdge](https://phpstan.org/blog/what-is-bleeding-edge), [strict-rules](https://github.com/phpstan/phpstan-strict-rules), [checkUninitializedProperties](https://phpstan.org/config-reference#checkuninitializedproperties), ...), but that still was not strict enough for us. This set of rules should fill the missing gaps we found. If you find some rules opinionated, you can easily disable them. @@ -44,6 +44,28 @@ parameters: forbidCast: enabled: true 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 + uasort: 1 + uksort: 1 + usort: 1 + allowedCheckedExceptionCallables: [] forbidCheckedExceptionInYieldingMethod: enabled: true forbidCustomFunctions: @@ -335,9 +357,69 @@ parameters: blacklist!: ['(array)', '(object)', '(unset)'] ``` +### forbidCheckedExceptionInCallable +- Denies throwing [checked exception](https://phpstan.org/blog/bring-your-exceptions-under-control) in callables (Closures 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 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) + +```neon +parameters: + shipmonkRules: + forbidCheckedExceptionInCallable: + immediatellyCalledCallables: + '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 +``` + +- We recommend using following config for checked exceptions: + - Also, [bleedingEdge](https://phpstan.org/blog/what-is-bleeding-edge) enables proper analysis of dead types in multi-catch, so we recommend enabling even that + +```neon +parameters: + exceptions: + check: + missingCheckedExceptionInThrows: true # enforce checked exceptions to be stated in @throws + tooWideThrowType: true # report invalid @throws (exceptions that are not actually thrown in annotated method) + implicitThrows: false # no @throws means nothing is thrown (otherwise Throwable is thrown) + checkedExceptionClasses: + - YourApp\TopLevelRuntimeException # track only your exceptions (children of some, typically RuntimeException) +``` + + +```php +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 + $user->updateEmail($email); + }) + } + + public function getUpdateEmailCallback(UserId $userId, Email $email): callable + { + return function () use ($userId, $email) { + $user = $this->userRepository->get($userId); // this usage is denied, it throws checked exception, but you don't know when, thus it cannot be tracked by phpstan + $user->updateEmail($email); + }; + } +} +``` + ### forbidCheckedExceptionInYieldingMethod - Denies throwing [checked exception](https://phpstan.org/blog/bring-your-exceptions-under-control) within yielding methods as those exceptions are not throw upon method call, but when generator gets iterated. - This behaviour cannot be easily reflected within PHPStan exception analysis and may cause [false negatives](https://phpstan.org/r/d07ac0f0-a49d-4f82-b1dd-1939058bbeed). +- Make sure you have enabled checked exceptions, otherwise, this rule does nothing ```php class Provider { diff --git a/composer.json b/composer.json index a0e4481..fe51ce0 100644 --- a/composer.json +++ b/composer.json @@ -12,11 +12,12 @@ "require": { "php": "^7.4 || ^8.0", "nikic/php-parser": "^4.14.0", - "phpstan/phpstan": "^1.10.0" + "phpstan/phpstan": "^1.10.30" }, "require-dev": { "editorconfig-checker/editorconfig-checker": "^10.3.0", "ergebnis/composer-normalize": "^2.28", + "nette/neon": "^3.3.1", "phpstan/phpstan-phpunit": "^1.1.1", "phpstan/phpstan-strict-rules": "^1.2.3", "phpunit/phpunit": "^9.5.20", @@ -33,7 +34,8 @@ "ShipMonk\\PHPStan\\": "tests/" }, "classmap": [ - "tests/Rule/data" + "tests/Rule/data", + "tests/Extension/data" ] }, "config": { diff --git a/composer.lock b/composer.lock index 86c416e..f36eecf 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "dead97ca307165f4458800ae24aa061b", + "content-hash": "092960c7d686a662eb7e0f4df304efda", "packages": [ { "name": "nikic/php-parser", @@ -64,16 +64,16 @@ }, { "name": "phpstan/phpstan", - "version": "1.10.26", + "version": "1.10.30", "source": { "type": "git", "url": "https://github.com/phpstan/phpstan.git", - "reference": "5d660cbb7e1b89253a47147ae44044f49832351f" + "reference": "2910afdd3fe33e5afd71c09f3fb0d0845b48c410" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/phpstan/phpstan/zipball/5d660cbb7e1b89253a47147ae44044f49832351f", - "reference": "5d660cbb7e1b89253a47147ae44044f49832351f", + "url": "https://api.github.com/repos/phpstan/phpstan/zipball/2910afdd3fe33e5afd71c09f3fb0d0845b48c410", + "reference": "2910afdd3fe33e5afd71c09f3fb0d0845b48c410", "shasum": "" }, "require": { @@ -122,7 +122,7 @@ "type": "tidelift" } ], - "time": "2023-07-19T12:44:37+00:00" + "time": "2023-08-22T13:48:25+00:00" } ], "packages-dev": [ @@ -957,6 +957,74 @@ ], "time": "2023-03-08T13:26:56+00:00" }, + { + "name": "nette/neon", + "version": "v3.4.0", + "source": { + "type": "git", + "url": "https://github.com/nette/neon.git", + "reference": "372d945c156ee7f35c953339fb164538339e6283" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/nette/neon/zipball/372d945c156ee7f35c953339fb164538339e6283", + "reference": "372d945c156ee7f35c953339fb164538339e6283", + "shasum": "" + }, + "require": { + "ext-json": "*", + "php": ">=8.0 <8.3" + }, + "require-dev": { + "nette/tester": "^2.4", + "phpstan/phpstan": "^1.0", + "tracy/tracy": "^2.7" + }, + "bin": [ + "bin/neon-lint" + ], + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "3.4-dev" + } + }, + "autoload": { + "classmap": [ + "src/" + ] + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "BSD-3-Clause", + "GPL-2.0-only", + "GPL-3.0-only" + ], + "authors": [ + { + "name": "David Grudl", + "homepage": "https://davidgrudl.com" + }, + { + "name": "Nette Community", + "homepage": "https://nette.org/contributors" + } + ], + "description": "🍸 Nette NEON: encodes and decodes NEON file format.", + "homepage": "https://ne-on.org", + "keywords": [ + "export", + "import", + "neon", + "nette", + "yaml" + ], + "support": { + "issues": "https://github.com/nette/neon/issues", + "source": "https://github.com/nette/neon/tree/v3.4.0" + }, + "time": "2023-01-13T03:08:29+00:00" + }, { "name": "phar-io/manifest", "version": "2.0.3", diff --git a/phpcs.xml.dist b/phpcs.xml.dist index dc553ee..218f511 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -18,7 +18,7 @@ src/ tests/ - tests/Rule/data/* + tests/*/data/* @@ -310,16 +310,8 @@ - - - - - - - - - - + + @@ -404,6 +396,8 @@ Doctrine\Common\Collections\Collection "/> + + diff --git a/phpstan.neon.dist b/phpstan.neon.dist index b447290..600e7d3 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -12,11 +12,18 @@ parameters: - tests excludePaths: analyseAndScan: - - tests/Rule/data/* + - tests/*/data/* tmpDir: cache/phpstan/ checkMissingCallableSignature: true checkUninitializedProperties: true checkTooWideReturnTypesInProtectedAndPublicMethods: true + exceptions: + check: + missingCheckedExceptionInThrows: true + tooWideThrowType: true + implicitThrows: false + uncheckedExceptionClasses: + - LogicException shipmonkRules: classSuffixNaming: @@ -30,3 +37,7 @@ parameters: message: "#Class BackedEnum not found\\.#" path: src/Rule/BackedEnumGenericsRule.php reportUnmatched: false # fails only for PHP < 8 https://github.com/phpstan/phpstan/issues/6290 + + - + message: "#but it's missing from the PHPDoc @throws tag\\.$#" # allow uncatched exceptions in tests + path: tests/* diff --git a/rules.neon b/rules.neon index 890ceb1..30a7214 100644 --- a/rules.neon +++ b/rules.neon @@ -23,6 +23,28 @@ parameters: forbidCast: enabled: true 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 + uasort: 1 + uksort: 1 + usort: 1 + allowedCheckedExceptionCallables: [] forbidCheckedExceptionInYieldingMethod: enabled: true forbidCustomFunctions: @@ -104,6 +126,11 @@ parametersSchema: enabled: bool() blacklist: arrayOf(string()) ]) + forbidCheckedExceptionInCallable: structure([ + enabled: bool() + immediatelyCalledCallables: arrayOf(anyOf(listOf(int()), int()), string()) + allowedCheckedExceptionCallables: arrayOf(anyOf(listOf(int()), int()), string()) + ]) forbidCheckedExceptionInYieldingMethod: structure([ enabled: bool() ]) @@ -192,6 +219,8 @@ conditionalTags: phpstan.rules.rule: %shipmonkRules.forbidAssignmentNotMatchingVarDoc.enabled% ShipMonk\PHPStan\Rule\ForbidCastRule: phpstan.rules.rule: %shipmonkRules.forbidCast.enabled% + ShipMonk\PHPStan\Rule\ForbidCheckedExceptionInCallableRule: + phpstan.rules.rule: %shipmonkRules.forbidCheckedExceptionInCallable.enabled% ShipMonk\PHPStan\Rule\ForbidCheckedExceptionInYieldingMethodRule: phpstan.rules.rule: %shipmonkRules.forbidCheckedExceptionInYieldingMethod.enabled% ShipMonk\PHPStan\Rule\ForbidCustomFunctionsRule: @@ -235,6 +264,8 @@ conditionalTags: ShipMonk\PHPStan\Rule\UselessPrivatePropertyNullabilityRule: phpstan.rules.rule: %shipmonkRules.uselessPrivatePropertyNullability.enabled% + ShipMonk\PHPStan\Visitor\ImmediatelyCalledCallableVisitor: + phpstan.parser.richParserNodeVisitor: %shipmonkRules.forbidCheckedExceptionInCallable.enabled% ShipMonk\PHPStan\Visitor\NamedArgumentSourceVisitor: phpstan.parser.richParserNodeVisitor: %shipmonkRules.allowNamedArgumentOnlyInAttributes.enabled% ShipMonk\PHPStan\Visitor\UnusedExceptionVisitor: @@ -246,6 +277,11 @@ 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 @@ -273,6 +309,11 @@ services: allowNarrowing: %shipmonkRules.forbidAssignmentNotMatchingVarDoc.allowNarrowing% - class: ShipMonk\PHPStan\Rule\ForbidCastRule + - + class: ShipMonk\PHPStan\Rule\ForbidCheckedExceptionInCallableRule + arguments: + immediatelyCalledCallables: %shipmonkRules.forbidCheckedExceptionInCallable.immediatelyCalledCallables% + allowedCheckedExceptionCallables: %shipmonkRules.forbidCheckedExceptionInCallable.allowedCheckedExceptionCallables% - class: ShipMonk\PHPStan\Rule\ForbidCheckedExceptionInYieldingMethodRule - @@ -330,7 +371,11 @@ 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\NamedArgumentSourceVisitor - @@ -341,3 +386,8 @@ 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 new file mode 100644 index 0000000..d9d2938 --- /dev/null +++ b/src/Extension/ImmediatelyCalledCallableThrowTypeExtension.php @@ -0,0 +1,224 @@ + 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 in_array( + $this->getCallNotation($callReflection), + array_keys($this->immediatelyCalledCallables), + true, + ); + } + + 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) { + $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 + { + $arguments = $this->immediatelyCalledCallables[$this->getCallNotation($callReflection)]; + + if (is_int($arguments)) { + return [$arguments]; + } + + return $arguments; + } + + /** + * @param FunctionReflection|MethodReflection $callReflection + */ + private function getCallNotation(object $callReflection): string + { + if ($callReflection instanceof MethodReflection) { + $class = $callReflection->getDeclaringClass()->getName(); + $method = $callReflection->getName(); + + return "{$class}::{$method}"; + } + + return $callReflection->getName(); + } + +} diff --git a/src/Rule/ForbidCheckedExceptionInCallableRule.php b/src/Rule/ForbidCheckedExceptionInCallableRule.php new file mode 100644 index 0000000..fd5b191 --- /dev/null +++ b/src/Rule/ForbidCheckedExceptionInCallableRule.php @@ -0,0 +1,256 @@ + + */ +class ForbidCheckedExceptionInCallableRule implements Rule +{ + + private ReflectionProvider $reflectionProvider; + + private DefaultExceptionTypeResolver $exceptionTypeResolver; + + /** + * class::method => Closure argument index + * or + * function => Closure argument index + * + * @var array> + */ + private array $callablesAllowingCheckedExceptions; + + /** + * @param array> $immediatelyCalledCallables + * @param array> $allowedCheckedExceptionCallables + */ + public function __construct( + ReflectionProvider $reflectionProvider, + DefaultExceptionTypeResolver $exceptionTypeResolver, + array $immediatelyCalledCallables, + array $allowedCheckedExceptionCallables + ) + { + $this->callablesAllowingCheckedExceptions = array_merge_recursive( + $immediatelyCalledCallables, + $allowedCheckedExceptionCallables, + ); + $this->exceptionTypeResolver = $exceptionTypeResolver; + $this->reflectionProvider = $reflectionProvider; + } + + public function getNodeType(): string + { + return Node::class; + } + + /** + * @return list + */ + public function processNode( + Node $node, + 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 + ) { + return $this->processFirstClassCallable($node->getOriginalNode(), $scope); + } + + if ($node instanceof ClosureReturnStatementsNode) { // @phpstan-ignore-line ignore bc promise + return $this->processClosure($node, $scope); + } + + return []; + } + + /** + * @param MethodCall|StaticCall|FuncCall $callNode + * @return list + */ + public function processFirstClassCallable( + CallLike $callNode, + Scope $scope + ): array + { + if (!$callNode->isFirstClassCallable()) { + throw new LogicException('This should be ensured by using XxxCallableNode'); + } + + if ($this->isAllowedToThrowCheckedException($callNode, $scope)) { + return []; + } + + $errors = []; + + 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)); + } + + 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)); + } + + if ($callNode instanceof FuncCall && $callNode->name instanceof Name) { + $functionReflection = $this->reflectionProvider->getFunction($callNode->name, $scope); + $errors = array_merge($errors, $this->processThrowType($functionReflection->getThrowType(), $scope)); + } + + return $errors; + } + + /** + * @return list + */ + public function processClosure( + ClosureReturnStatementsNode $node, + Scope $scope + ): 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 []; + } + + if ($this->isAllowedToThrowCheckedException($closure, $parentScope)) { + return []; + } + + $errors = []; + + foreach ($node->getStatementResult()->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 closure!") + ->line($throwPoint->getNode()->getLine()) + ->build(); + } + } + } + + return $errors; + } + + /** + * @return list + */ + private function processCall( + Scope $scope, + Type $callerType, + string $methodName + ): array + { + $methodReflection = $scope->getMethodReflection($callerType, $methodName); + + if ($methodReflection !== null) { + return $this->processThrowType($methodReflection->getThrowType(), $scope); + } + + return []; + } + + /** + * @return list + */ + private function processThrowType( + ?Type $throwType, + Scope $scope + ): array + { + if ($throwType === null) { + return []; + } + + $errors = []; + + foreach ($throwType->getObjectClassNames() as $exceptionClass) { + if ($this->exceptionTypeResolver->isCheckedException($exceptionClass, $scope)) { + $errors[] = RuleErrorBuilder::message("Throwing checked exception $exceptionClass in first-class-callable!")->build(); + } + } + + return $errors; + } + + public function isAllowedToThrowCheckedException( + Node $node, + Scope $scope + ): bool + { + /** @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 true|null $isAllowedToThrow */ + $isAllowedToThrow = $node->getAttribute(ImmediatelyCalledCallableVisitor::CALLABLE_ALLOWING_CHECKED_EXCEPTION); + + if ($isAllowedToThrow === true) { + return true; + } + + if ($callerNodeWithClosureAsArg === null || $methodNameWithClosureAsArg === null) { + return false; + } + + $callerWithClosureAsArgType = $callerNodeWithClosureAsArg instanceof Expr + ? $scope->getType($callerNodeWithClosureAsArg) + : $scope->resolveTypeByName($callerNodeWithClosureAsArg); + + foreach ($callerWithClosureAsArgType->getObjectClassReflections() as $callerWithClosureAsArgClassReflection) { + foreach ($this->callablesAllowingCheckedExceptions as $immediateCallerAndMethod => $indexes) { + [$callerClass, $methodName] = explode('::', $immediateCallerAndMethod); + + if ( + $callerWithClosureAsArgClassReflection->is($callerClass) + && $methodName === $methodNameWithClosureAsArg + ) { + return true; + } + } + } + + return false; + } + +} diff --git a/src/Visitor/ImmediatelyCalledCallableVisitor.php b/src/Visitor/ImmediatelyCalledCallableVisitor.php new file mode 100644 index 0000000..f58da23 --- /dev/null +++ b/src/Visitor/ImmediatelyCalledCallableVisitor.php @@ -0,0 +1,160 @@ + 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 = [] + ) + { + $callablesWithAllowedCheckedExceptions = array_merge_recursive($immediatelyCalledCallables, $allowedCheckedExceptionCallables); + + foreach ($callablesWithAllowedCheckedExceptions as $call => $arguments) { + if (strpos($call, '::') !== false) { + [, $methodName] = explode('::', $call); + $this->methodsWithAllowedCheckedExceptions[$methodName] = $arguments; + } else { + $this->functionsWithAllowedCheckedExceptions[$call] = $arguments; + } + } + } + + 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 ($this->normalizeArgumentIndexes($argumentIndexes) as $argumentIndex) { + $argument = $node->getArgs()[$argumentIndex] ?? null; + + if ($argument === null) { + continue; + } + + if (!$this->isFirstClassCallableOrClosure($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()); + } + } + + private function resolveFuncCall(FuncCall $node): void + { + if ($this->isFirstClassCallableOrClosure($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 ($this->normalizeArgumentIndexes($argumentIndexes) as $argumentIndex) { + $argument = $node->getArgs()[$argumentIndex] ?? null; + + if ($argument === null) { + continue; + } + + if (!$this->isFirstClassCallableOrClosure($argument->value)) { + continue; + } + + $node->getArgs()[$argumentIndex]->value->setAttribute(self::CALLABLE_ALLOWING_CHECKED_EXCEPTION, true); + } + } + + private function isFirstClassCallableOrClosure(Node $node): bool + { + return $node instanceof Closure + || ($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 new file mode 100644 index 0000000..f7378d8 --- /dev/null +++ b/tests/Extension/ImmediatelyCalledCallableThrowTypeExtensionTest.php @@ -0,0 +1,41 @@ + + */ + 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 new file mode 100644 index 0000000..b2e93bc --- /dev/null +++ b/tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/code.php @@ -0,0 +1,172 @@ +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); + } + } + +} + + +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 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 new file mode 100644 index 0000000..0d59cbd --- /dev/null +++ b/tests/Extension/data/ImmediatelyCalledCallableThrowTypeExtension/extension.neon @@ -0,0 +1,12 @@ +services: + - + class: ShipMonk\PHPStan\Extension\ImmediatelyCalledCallableThrowTypeExtension + tags: + - phpstan.dynamicFunctionThrowTypeExtension + - phpstan.dynamicMethodThrowTypeExtension + - phpstan.dynamicStaticMethodThrowTypeExtension + arguments: + immediatelyCalledCallables: + array_map: 0 + ImmediatelyCalledCallableThrowTypeExtension\Immediate::method: [0, 1] + diff --git a/tests/Rule/ForbidCheckedExceptionInCallableRuleTest.php b/tests/Rule/ForbidCheckedExceptionInCallableRuleTest.php new file mode 100644 index 0000000..678cdfa --- /dev/null +++ b/tests/Rule/ForbidCheckedExceptionInCallableRuleTest.php @@ -0,0 +1,95 @@ + + */ +class ForbidCheckedExceptionInCallableRuleTest extends RuleTestCase +{ + + private static ?bool $implicitThrows = null; + + /** + * @var list|null + */ + private ?array $checkedExceptions = null; + + protected function getRule(): Rule + { + if ($this->checkedExceptions === null) { + throw new LogicException('Missing checkedExceptions'); + } + + if (self::$implicitThrows === null) { + throw new LogicException('Missing implicitThrows'); + } + + $visitorConfig = Neon::decodeFile(self::getVisitorConfigFilePath()); + + return new ForbidCheckedExceptionInCallableRule( + self::getContainer()->getByType(ReflectionProvider::class), + new DefaultExceptionTypeResolver( // @phpstan-ignore-line ignore BC promise + self::getContainer()->getByType(ReflectionProvider::class), + [], + [], + [], + $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 + ); + } + + /** + * @param list $checkedExceptions + * @dataProvider provideSetup + */ + public function test(bool $implicitThrows, array $checkedExceptions): void + { + self::$implicitThrows = $implicitThrows; + $this->checkedExceptions = $checkedExceptions; + + $this->analyseFile(__DIR__ . '/data/ForbidCheckedExceptionInCallableRule/code.php'); + } + + /** + * @return iterable + */ + public function provideSetup(): iterable + { + yield ['implicitThrows' => true, 'checkedExceptions' => []]; + yield ['implicitThrows' => true, 'checkedExceptions' => ['ForbidCheckedExceptionInCallableRule\CheckedException']]; + yield ['implicitThrows' => false, 'checkedExceptions' => []]; + yield ['implicitThrows' => false, 'checkedExceptions' => ['ForbidCheckedExceptionInCallableRule\CheckedException']]; + } + + /** + * @return list + */ + public static function getAdditionalConfigFiles(): array + { + $files = [ + self::getVisitorConfigFilePath(), + ]; + + if (self::$implicitThrows === true) { + $files[] = __DIR__ . '/data/ForbidCheckedExceptionInCallableRule/no-implicit-throws.neon'; + } + + return $files; + } + + private static function getVisitorConfigFilePath(): string + { + return __DIR__ . '/data/ForbidCheckedExceptionInCallableRule/visitor.neon'; + } + +} diff --git a/tests/Rule/data/ForbidCheckedExceptionInCallableRule/code.php b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/code.php new file mode 100644 index 0000000..90587c9 --- /dev/null +++ b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/code.php @@ -0,0 +1,159 @@ +noop(...); + + $this->throws(...); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + + // $this?->throws(...); // https://github.com/phpstan/phpstan/issues/9746 + + throwing_function(...); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + + $this->denied($this->throws(...)); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in first-class-callable! + + $this->immediateThrow($this->throws(...)); + + ($this->throws(...))(); + + (throwing_function(...))(); + + array_map($this->throws(...), []); + + array_map(throwing_function(...), []); + + $this->allowThrow($this->throws(...)); + + $this->allowThrow(throwing_function(...)); + } + + private function noop(): void + { + } + + /** + * @throws CheckedException + */ + private function throws(): void + { + throw new CheckedException(); + } + + private function denied(callable $callable): void + { + + } + + public function immediateThrow(callable $callable): void + { + $callable(); + } + + public function allowThrow(callable $callable): void + { + try { + $callable(); + } catch (\Exception $e) { + + } + } + +} + +class ClosureTest { + + public function test(): void + { + $fn = function () { + throw new CheckedException(); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in closure! + }; + + $fn2 = function () { + $this->throws(); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in closure! + }; + + $fn3 = function () { + $this->noop(); // implicit throw is ignored + }; + + $fn4 = function (callable $c) { + $c(); // implicit throw is ignored (https://github.com/phpstan/phpstan/issues/9779) + }; + + $this->denied(function () { + throw new CheckedException(); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in closure! + }); + + $this?->denied(function () { + $this->throws(); // error: Throwing checked exception ForbidCheckedExceptionInCallableRule\CheckedException in closure! + }); + + $this->immediateThrow(function () { + throw new CheckedException(); + }); + + $self = $this; // self is unknown variable in scope of the closure + $self->immediateThrow(function () { + throw new CheckedException(); + }); + + (function () { + throw new CheckedException(); + })(); + + array_map(function () { + throw new CheckedException(); + }, []); + + array_map(function () { + $this->throws(); + }, []); + + $this->allowThrow(function () { + $this->throws(); + }); + } + + private function noop(): void + { + } + + /** + * @throws CheckedException + */ + private function throws(): void + { + throw new CheckedException(); + } + + private function denied(callable $callable): void + { + + } + + public function immediateThrow(callable $callable): void + { + $callable(); + } + + public function allowThrow(callable $callable): void + { + try { + $callable(); + } catch (\Exception $e) { + + } + } + +} diff --git a/tests/Rule/data/ForbidCheckedExceptionInCallableRule/no-implicit-throws.neon b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/no-implicit-throws.neon new file mode 100644 index 0000000..790db39 --- /dev/null +++ b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/no-implicit-throws.neon @@ -0,0 +1,3 @@ +parameters: + exceptions: + implicitThrows: false diff --git a/tests/Rule/data/ForbidCheckedExceptionInCallableRule/visitor.neon b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/visitor.neon new file mode 100644 index 0000000..44f8d86 --- /dev/null +++ b/tests/Rule/data/ForbidCheckedExceptionInCallableRule/visitor.neon @@ -0,0 +1,13 @@ +services: + - + class: ShipMonk\PHPStan\Visitor\ImmediatelyCalledCallableVisitor + arguments: + immediatelyCalledCallables: + 'array_map': 0 + 'ForbidCheckedExceptionInCallableRule\ClosureTest::immediateThrow': 0 + 'ForbidCheckedExceptionInCallableRule\FirstClassCallableTest::immediateThrow': 0 + allowedCheckedExceptionCallables: + 'ForbidCheckedExceptionInCallableRule\ClosureTest::allowThrow': [0] + 'ForbidCheckedExceptionInCallableRule\FirstClassCallableTest::allowThrow': [0] + tags: + - phpstan.parser.richParserNodeVisitor