diff --git a/README.md b/README.md index 10d785d..ddcafe8 100644 --- a/README.md +++ b/README.md @@ -621,6 +621,14 @@ match ($foo) { // unused match result ``` +### forbidUnusedClosureParameters +- Reports unused parameters in closures and arrow functions +- Only reports trailing unused parameters (parameters that are unused and all parameters after them are also unused) +```php +fn (int $key, Item $item, string $unused) => $item->ok(); // unused parameter $unused is reported, $key not +``` + + ### requirePreviousExceptionPass - Detects forgotten exception pass-as-previous when re-throwing - Checks if caught exception can be passed as argument to the call (including constructor call) in `throw` node inside the catch block diff --git a/rules.neon b/rules.neon index d3eae5e..687dbf3 100644 --- a/rules.neon +++ b/rules.neon @@ -81,6 +81,8 @@ parameters: enabled: %shipmonkRules.enableAllRules% forbidUnusedMatchResult: enabled: %shipmonkRules.enableAllRules% + forbidUnusedClosureParameters: + enabled: %shipmonkRules.enableAllRules% requirePreviousExceptionPass: enabled: %shipmonkRules.enableAllRules% reportEvenIfExceptionIsNotAcceptableByRethrownOne: true @@ -203,6 +205,9 @@ parametersSchema: forbidUnusedMatchResult: structure([ enabled: bool() ]) + forbidUnusedClosureParameters: structure([ + enabled: bool() + ]) requirePreviousExceptionPass: structure([ enabled: bool() reportEvenIfExceptionIsNotAcceptableByRethrownOne: bool() @@ -279,6 +284,8 @@ conditionalTags: phpstan.rules.rule: %shipmonkRules.forbidUnusedException.enabled% ShipMonk\PHPStan\Rule\ForbidUnusedMatchResultRule: phpstan.rules.rule: %shipmonkRules.forbidUnusedMatchResult.enabled% + ShipMonk\PHPStan\Rule\ForbidUnusedClosureParametersRule: + phpstan.rules.rule: %shipmonkRules.forbidUnusedClosureParameters.enabled% ShipMonk\PHPStan\Rule\ForbidReturnInConstructorRule: phpstan.rules.rule: %shipmonkRules.uselessPrivatePropertyDefaultValue.enabled% ShipMonk\PHPStan\Rule\RequirePreviousExceptionPassRule: @@ -394,6 +401,8 @@ services: class: ShipMonk\PHPStan\Rule\ForbidUnusedExceptionRule - class: ShipMonk\PHPStan\Rule\ForbidUnusedMatchResultRule + - + class: ShipMonk\PHPStan\Rule\ForbidUnusedClosureParametersRule - class: ShipMonk\PHPStan\Rule\ForbidReturnInConstructorRule - diff --git a/src/Rule/ForbidUnusedClosureParametersRule.php b/src/Rule/ForbidUnusedClosureParametersRule.php new file mode 100644 index 0000000..2168a41 --- /dev/null +++ b/src/Rule/ForbidUnusedClosureParametersRule.php @@ -0,0 +1,154 @@ + + */ +final class ForbidUnusedClosureParametersRule implements Rule +{ + + public function getNodeType(): string + { + return Expr::class; + } + + /** + * @param Expr $node + * @return list + */ + public function processNode( + Node $node, + Scope $scope + ): array + { + if ($node instanceof ArrowFunction) { + $referencedVariables = $this->getReferencedVariables([$node->expr]); + + } elseif ($node instanceof Closure) { + $referencedVariables = $this->getReferencedVariables($node->stmts); + + } else { + return []; + } + + $errors = []; + $trailingUnusedParameterNames = $this->getTrailingUnusedParameterNames(array_values($node->params), $referencedVariables); + + $functionType = $node instanceof ArrowFunction ? 'Arrow function' : 'Closure'; + foreach ($trailingUnusedParameterNames as $parameterName) { + $errors[] = RuleErrorBuilder::message("{$functionType} parameter \${$parameterName} is unused") + ->identifier('shipmonk.unusedParameter') + ->build(); + } + + return $errors; + } + + /** + * @param array $nodes + * @return array + */ + private function getReferencedVariables(array $nodes): array + { + $visitor = new class extends NodeVisitorAbstract { + + /** + * @var array + */ + private array $usedVariables = []; + + public function enterNode(Node $node): ?Node + { + if ($node instanceof Variable && is_string($node->name)) { + $this->usedVariables[$node->name] = true; + } + + return null; + } + + /** + * @return array + */ + public function getUsedVariables(): array + { + return $this->usedVariables; + } + + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($nodes); + + return $visitor->getUsedVariables(); + } + + /** + * @param list $params + * @param array $referencedVariables + * @return list + */ + private function getTrailingUnusedParameterNames( + array $params, + array $referencedVariables + ): array + { + for ($i = count($params) - 1; $i >= 0; $i--) { + $param = $params[$i]; // @phpstan-ignore offsetAccess.notFound + + if (!$param->var instanceof Variable) { + continue; + } + + if (!is_string($param->var->name)) { + continue; + } + + $parameterName = $param->var->name; + + if (isset($referencedVariables[$parameterName])) { + return $this->extractParamNames(array_slice($params, $i + 1)); + } + } + + return $this->extractParamNames($params); + } + + /** + * @param array $params + * @return list + */ + private function extractParamNames(array $params): array + { + return array_values(array_map(static function (Param $param): string { + if (!$param->var instanceof Variable) { + throw new LogicException('Param variable must be a variable'); + } + if (!is_string($param->var->name)) { + throw new LogicException('Param variable name must be a string'); + } + return $param->var->name; + }, $params)); + } + +} diff --git a/tests/Rule/ForbidUnusedClosureParametersRuleTest.php b/tests/Rule/ForbidUnusedClosureParametersRuleTest.php new file mode 100644 index 0000000..0187dcc --- /dev/null +++ b/tests/Rule/ForbidUnusedClosureParametersRuleTest.php @@ -0,0 +1,24 @@ + + */ +class ForbidUnusedClosureParametersRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new ForbidUnusedClosureParametersRule(); + } + + public function testClass(): void + { + $this->analyseFile(__DIR__ . '/data/ForbidUnusedClosureParametersRule/code.php'); + } + +} diff --git a/tests/Rule/data/ForbidUnusedClosureParametersRule/code.php b/tests/Rule/data/ForbidUnusedClosureParametersRule/code.php new file mode 100644 index 0000000..1a4e099 --- /dev/null +++ b/tests/Rule/data/ForbidUnusedClosureParametersRule/code.php @@ -0,0 +1,70 @@ + $narrowAisleOccurrenceIds; // error: Arrow function parameter $occurrenceIds is unused + + // This should not trigger an error - both parameters are used + $usedCallback = fn (array $ids, string $type) => str_contains($type, 'narrow') ? $ids : []; + + // This should not trigger an error - single parameter is used + $singleUsedCallback = fn (array $data) => count($data); + + // This should trigger an error - only $unused is trailing unused (but $used is used) + $mixedCallback = fn (array $used, string $unused) => count($used); // error: Arrow function parameter $unused is unused + + // This should NOT trigger an error - $key is unused but needed for $receivingItem position + $positionCallback = fn (int $key, \stdClass $receivingItem) => $receivingItem->id === 123; + + // This should trigger an error - multiple trailing unused parameters + $multipleUnusedCallback = fn (array $used, string $first, int $second) => count($used); // error: Arrow function parameter $first is unused // error: Arrow function parameter $second is unused + + // This should trigger an error - all parameters are unused (all trailing) + $allUnusedCallback = fn (array $first, string $second, int $third) => 'constant'; // error: Arrow function parameter $first is unused // error: Arrow function parameter $second is unused // error: Arrow function parameter $third is unused + + // This should not trigger an error - no parameters + $noParamsCallback = fn () => 'constant'; + + // This should not trigger an error - unused in middle, but used at end + $unusedInMiddleCallback = fn (int $unused1, string $unused2, array $used) => count($used); + + // Regular closures - This should trigger an error - $occurrenceIds is not used (trailing unused param) + $closureCallback = function (array $occurrenceIds) { // error: Closure parameter $occurrenceIds is unused + return $narrowAisleOccurrenceIds; + }; + + // This should not trigger an error - both parameters are used + $usedClosure = function (array $ids, string $type) { + return str_contains($type, 'narrow') ? $ids : []; + }; + + // This should trigger an error - only $unused is trailing unused + $mixedClosure = function (array $used, string $unused) { // error: Closure parameter $unused is unused + return count($used); + }; + + // This should NOT trigger an error - $key is unused but needed for $receivingItem position + $positionClosure = function (int $key, \stdClass $receivingItem) { + return $receivingItem->id === 123; + }; + + // This should trigger an error - multiple trailing unused parameters + $multipleUnusedClosure = function (array $used, string $first, int $second) { // error: Closure parameter $first is unused // error: Closure parameter $second is unused + return count($used); + }; + + // This should not trigger an error - unused in middle, but used at end + $unusedInMiddleClosure = function (int $unused1, string $unused2, array $used) { + return count($used); + }; + } + +}