Add type checking for hook callbacks in add_action and add_filter (…

…#107)

* Setup the hook callback rule and test.

* Add the wp-hooks library.

* Start adding tests.

* Preparing more tests.

* If we don't have enough arguments, bail out and let PHPStan handle the error.

* If the callback is not valid, bail out and let PHPStan handle the error:

* Simplify this.

* More valid test cases.

* Update some test line numbers.

* First pass at detecting mismatching accepted and expected argument counts.

* Remove some accidental duplicates.

* Fix some logic.

* Let's split this up before it gets out of hand.

* Reduce this down.

* More tidying up.

* We're going to be needing this in multiple methods soon.

* Prep for callback return type checking, not yet functional.

* Split action return type assertion into its own method.

* Bring this message more inline with those used by PHPStan.

* Add detection for filter callbacks with no return value.

* Don't need this just yet.

* Use early exit to reduce code nesting.

* Remove unused imports.

* Yoda comparisons are disallowed.

* Coding standards.

* Remove unused code.

* Use early return to reduce code nesting.

* Remove more unused code.

* Renaming.

* Use early return to reduce code nesting.

* Tidying up.

* Correct logic introduced during refactoring.

* Exclude "maybe" callables.

* More handling for parameter counts.

* More handling for optional parameters in hook callbacks.

* Make the tests more manageable.

* Tests for more callback types.

* Tidying up.

* This isn't used.

* More test cases.

* Tests for variadic callbacks.

* More specific handling for variadic callbacks.

* More tests.

* The hooks names are not relevant to the tests.

* Remove an `else`.

* I'm still not entirely sure why this works, but it does. Props @herndlm.

* Another test for an action with a return value.

* More variadic handling.

* Turns out PHPStan handles typed and untyped functions differently.

* Partly broken callbacks will trigger an error in PHPStan so we don't need to handle them.

* Terminology.

* Refactoring.

* PHP 7.2 compatibility.

* Reorder the processing so the return type is checked first.

* More tests.

Co-authored-by: Viktor Szépe <viktor@szepe.net>

composer.json

@@ -24,6 +24,11 @@
         "phpunit/phpunit": "^8 || ^9",
         "szepeviktor/phpcs-psr-12-neutron-hybrid-ruleset": "^0.6"
     },
+    "autoload-dev": {
+        "classmap": [
+            "tests/"
+        ]
+    },
     "autoload": {
         "psr-4": {
             "SzepeViktor\\PHPStan\\WordPress\\": "src/"

extension.neon

@@ -92,6 +92,7 @@ services:
         tags:
             - phpstan.parser.richParserNodeVisitor
 rules:
+    - SzepeViktor\PHPStan\WordPress\HookCallbackRule
     - SzepeViktor\PHPStan\WordPress\HookDocsRule
     - SzepeViktor\PHPStan\WordPress\IsWpErrorRule
 parameters:

src/HookCallbackException.php

@@ -0,0 +1,14 @@
+<?php
+
+/**
+ * Exception thrown when validating a hook callback.
+ */
+
+declare(strict_types=1);
+
+namespace SzepeViktor\PHPStan\WordPress;
+
+// phpcs:ignore SlevomatCodingStandard.Classes.SuperfluousExceptionNaming.SuperfluousSuffix
+class HookCallbackException extends \DomainException
+{
+}

src/HookCallbackRule.php

@@ -0,0 +1,218 @@
+<?php
+
+/**
+ * Custom rule to validate the callback function for WordPress core actions and filters.
+ */
+
+declare(strict_types=1);
+
+namespace SzepeViktor\PHPStan\WordPress;
+
+use PhpParser\Node;
+use PhpParser\Node\Arg;
+use PhpParser\Node\Expr\FuncCall;
+use PHPStan\Analyser\Scope;
+use PHPStan\Reflection\ParametersAcceptor;
+use PHPStan\Reflection\ParametersAcceptorSelector;
+use PHPStan\Rules\RuleErrorBuilder;
+use PHPStan\Rules\RuleLevelHelper;
+use PHPStan\Type\Constant\ConstantIntegerType;
+use PHPStan\Type\VerbosityLevel;
+use PHPStan\Type\VoidType;
+
+/**
+ * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Expr\FuncCall>
+ */
+class HookCallbackRule implements \PHPStan\Rules\Rule
+{
+    private const SUPPORTED_FUNCTIONS = [
+        'add_filter',
+        'add_action',
+    ];
+
+    /** @var \PHPStan\Rules\RuleLevelHelper */
+    protected $ruleLevelHelper;
+
+    /** @var \PHPStan\Analyser\Scope */
+    protected $currentScope;
+
+    public function __construct(
+        RuleLevelHelper $ruleLevelHelper
+    ) {
+        $this->ruleLevelHelper = $ruleLevelHelper;
+    }
+
+    public function getNodeType(): string
+    {
+        return FuncCall::class;
+    }
+
+    /**
+     * @param \PhpParser\Node\Expr\FuncCall $node
+     * @param \PHPStan\Analyser\Scope       $scope
+     * @return array<int, \PHPStan\Rules\RuleError>
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        $name = $node->name;
+        $this->currentScope = $scope;
+
+        if (!($name instanceof \PhpParser\Node\Name)) {
+            return [];
+        }
+
+        if (!in_array($name->toString(), self::SUPPORTED_FUNCTIONS, true)) {
+            return [];
+        }
+
+        $args = $node->getArgs();
+
+        // If we don't have enough arguments, let PHPStan handle the error:
+        if (count($args) < 2) {
+            return [];
+        }
+
+        $callbackType = $scope->getType($args[1]->value);
+
+        // If the callback is not valid, let PHPStan handle the error:
+        if (! $callbackType->isCallable()->yes()) {
+            return [];
+        }
+
+        $callbackAcceptor = ParametersAcceptorSelector::selectSingle($callbackType->getCallableParametersAcceptors($scope));
+
+        try {
+            if ($name->toString() === 'add_action') {
+                $this->validateActionReturnType($callbackAcceptor);
+            } else {
+                $this->validateFilterReturnType($callbackAcceptor);
+            }
+
+            $this->validateParamCount($callbackAcceptor, $args[3] ?? null);
+        } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) {
+            return [RuleErrorBuilder::message($e->getMessage())->build()];
+        }
+
+        return [];
+    }
+
+    protected function getAcceptedArgs(?Arg $acceptedArgsParam): ?int
+    {
+        $acceptedArgs = 1;
+
+        if (isset($acceptedArgsParam)) {
+            $acceptedArgs = null;
+            $argumentType = $this->currentScope->getType($acceptedArgsParam->value);
+
+            if ($argumentType instanceof ConstantIntegerType) {
+                $acceptedArgs = $argumentType->getValue();
+            }
+        }
+
+        return $acceptedArgs;
+    }
+
+    protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $acceptedArgsParam): void
+    {
+        $acceptedArgs = $this->getAcceptedArgs($acceptedArgsParam);
+
+        if ($acceptedArgs === null) {
+            return;
+        }
+
+        $allParameters = $callbackAcceptor->getParameters();
+        $requiredParameters = array_filter(
+            $allParameters,
+            static function (\PHPStan\Reflection\ParameterReflection $parameter): bool {
+                return ! $parameter->isOptional();
+            }
+        );
+        $maxArgs = count($allParameters);
+        $minArgs = count($requiredParameters);
+
+        if (($acceptedArgs >= $minArgs) && ($acceptedArgs <= $maxArgs)) {
+            return;
+        }
+
+        if (($acceptedArgs >= $minArgs) && $callbackAcceptor->isVariadic()) {
+            return;
+        }
+
+        if ($minArgs === 0 && $acceptedArgs === 1) {
+            return;
+        }
+
+        throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException(
+            self::buildParameterCountMessage(
+                $minArgs,
+                $maxArgs,
+                $acceptedArgs,
+                $callbackAcceptor
+            )
+        );
+    }
+
+    protected static function buildParameterCountMessage(int $minArgs, int $maxArgs, int $acceptedArgs, ParametersAcceptor $callbackAcceptor): string
+    {
+        $expectedParametersMessage = $minArgs;
+
+        if ($maxArgs !== $minArgs) {
+            $expectedParametersMessage = sprintf(
+                '%1$d-%2$d',
+                $minArgs,
+                $maxArgs
+            );
+        }
+
+        $message = ($expectedParametersMessage === 1)
+            ? 'Callback expects %1$d parameter, $accepted_args is set to %2$d.'
+            : 'Callback expects %1$s parameters, $accepted_args is set to %2$d.';
+
+        if ($callbackAcceptor->isVariadic()) {
+            $message = ($minArgs === 1)
+                ? 'Callback expects at least %1$d parameter, $accepted_args is set to %2$d.'
+                : 'Callback expects at least %1$s parameters, $accepted_args is set to %2$d.';
+        }
+
+        return sprintf(
+            $message,
+            $expectedParametersMessage,
+            $acceptedArgs
+        );
+    }
+
+    protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor): void
+    {
+        $acceptedType = $callbackAcceptor->getReturnType();
+        $accepted = $this->ruleLevelHelper->accepts(
+            new VoidType(),
+            $acceptedType,
+            true
+        );
+
+        if ($accepted) {
+            return;
+        }
+
+        throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException(
+            sprintf(
+                'Action callback returns %s but should not return anything.',
+                $acceptedType->describe(VerbosityLevel::getRecommendedLevelByType($acceptedType))
+            )
+        );
+    }
+
+    protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor): void
+    {
+        $returnType = $callbackAcceptor->getReturnType();
+        $isVoidSuperType = $returnType->isSuperTypeOf(new VoidType());
+
+        if (! $isVoidSuperType->yes()) {
+            return;
+        }
+
+        throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException(
+            'Filter callback return statement is missing.'
+        );
+    }
+}

tests/HookCallbackRuleTest.php

@@ -0,0 +1,127 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SzepeViktor\PHPStan\WordPress\Tests;
+
+use PHPStan\Rules\RuleLevelHelper;
+use SzepeViktor\PHPStan\WordPress\HookCallbackRule;
+
+/**
+ * @extends \PHPStan\Testing\RuleTestCase<\SzepeViktor\PHPStan\WordPress\HookCallbackRule>
+ */
+class HookCallbackRuleTest extends \PHPStan\Testing\RuleTestCase
+{
+    protected function getRule(): \PHPStan\Rules\Rule
+    {
+        /** @var \PHPStan\Rules\RuleLevelHelper */
+        $ruleLevelHelper = self::getContainer()->getByType(RuleLevelHelper::class);
+
+        // getRule() method needs to return an instance of the tested rule
+        return new HookCallbackRule($ruleLevelHelper);
+    }
+
+    // phpcs:ignore NeutronStandard.Functions.LongFunction.LongFunction
+    public function testRule(): void
+    {
+        // first argument: path to the example file that contains some errors that should be reported by HookCallbackRule
+        // second argument: an array of expected errors,
+        // each error consists of the asserted error message, and the asserted error file line
+        $this->analyse(
+            [
+                __DIR__ . '/data/hook-callback.php',
+            ],
+            [
+                [
+                    'Filter callback return statement is missing.',
+                    17,
+                ],
+                [
+                    'Filter callback return statement is missing.',
+                    20,
+                ],
+                [
+                    'Filter callback return statement is missing.',
+                    23,
+                ],
+                [
+                    'Callback expects 1 parameter, $accepted_args is set to 0.',
+                    26,
+                ],
+                [
+                    'Callback expects 1 parameter, $accepted_args is set to 2.',
+                    31,
+                ],
+                [
+                    'Callback expects 0-1 parameters, $accepted_args is set to 2.',
+                    36,
+                ],
+                [
+                    'Callback expects 2 parameters, $accepted_args is set to 1.',
+                    41,
+                ],
+                [
+                    'Callback expects 2-4 parameters, $accepted_args is set to 1.',
+                    46,
+                ],
+                [
+                    'Callback expects 2-3 parameters, $accepted_args is set to 4.',
+                    51,
+                ],
+                [
+                    'Action callback returns true but should not return anything.',
+                    56,
+                ],
+                [
+                    'Action callback returns true but should not return anything.',
+                    61,
+                ],
+                [
+                    'Filter callback return statement is missing.',
+                    68,
+                ],
+                [
+                    'Action callback returns false but should not return anything.',
+                    71,
+                ],
+                [
+                    'Action callback returns int but should not return anything.',
+                    74,
+                ],
+                [
+                    'Callback expects at least 1 parameter, $accepted_args is set to 0.',
+                    77,
+                ],
+                [
+                    'Callback expects at least 1 parameter, $accepted_args is set to 0.',
+                    80,
+                ],
+                [
+                    'Callback expects 0-3 parameters, $accepted_args is set to 4.',
+                    85,
+                ],
+                [
+                    'Action callback returns int but should not return anything.',
+                    90,
+                ],
+                [
+                    'Callback expects 0 parameters, $accepted_args is set to 2.',
+                    93,
+                ],
+                [
+                    'Action callback returns false but should not return anything.',
+                    96,
+                ],
+                [
+                    'Filter callback return statement is missing.',
+                    99,
+                ],
+            ]
+        );
+    }
+
+    public static function getAdditionalConfigFiles(): array
+    {
+        return [dirname(__DIR__) . '/vendor/szepeviktor/phpstan-wordpress/extension.neon'];
+    }
+}