From 3930c977d186b1c9ee4053ad4108bcab943c2c93 Mon Sep 17 00:00:00 2001
From: John Blackbourn <john@humanmade.com>
Date: Sat, 12 Nov 2022 15:57:30 +0100
Subject: [PATCH] Hook callback false positives for `mixed` return type (#130)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Increase the memory limit during unit tests.

* Add some failing tests.

* More tests for `mixed`.

* Boop.

* Add a test for implicit mixed return type.

* Add another implicit return type test for an action callback.

* Update tests/data/hook-callback.php

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

* Implicit mixed is not the responsibility of this library.

* Add support for filters that return explicit mixed. Props @herndlm.

* Add support for actions that incorrectly return explicit mixed.

* Fix the implicit handling.

* Update composer.json

Co-authored-by: Viktor Szépe <viktor@szepe.net>
---
 src/HookCallbackRule.php       | 36 +++++++++++++++++++++++-----------
 tests/HookCallbackRuleTest.php |  2 +-
 tests/data/hook-callback.php   | 35 +++++++++++++++++++++++++++++++--
 3 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php
index 39bf49e..6a0ad72 100644
--- a/src/HookCallbackRule.php
+++ b/src/HookCallbackRule.php
@@ -17,6 +17,7 @@
 use PHPStan\Rules\RuleErrorBuilder;
 use PHPStan\Rules\RuleLevelHelper;
 use PHPStan\Type\Constant\ConstantIntegerType;
+use PHPStan\Type\MixedType;
 use PHPStan\Type\VerbosityLevel;
 use PHPStan\Type\VoidType;
 
@@ -184,30 +185,43 @@ protected static function buildParameterCountMessage(int $minArgs, int $maxArgs,
     protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor): void
     {
         $acceptedType = $callbackAcceptor->getReturnType();
+        $exception = new \SzepeViktor\PHPStan\WordPress\HookCallbackException(
+            sprintf(
+                'Action callback returns %s but should not return anything.',
+                $acceptedType->describe(VerbosityLevel::getRecommendedLevelByType($acceptedType))
+            )
+        );
+
+        if ($acceptedType instanceof MixedType && $acceptedType->isExplicitMixed()) {
+            throw $exception;
+        }
+
         $accepted = $this->ruleLevelHelper->accepts(
             new VoidType(),
             $acceptedType,
             true
         );
 
-        if ($accepted) {
-            return;
+        if (! $accepted) {
+            throw $exception;
         }
-
-        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()) {
+        if ($returnType instanceof MixedType) {
+            return;
+        }
+
+        $accepted = $this->ruleLevelHelper->accepts(
+            new VoidType(),
+            $returnType,
+            true
+        );
+
+        if (! $accepted) {
             return;
         }
 
diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php
index cc161fe..3cc50d3 100644
--- a/tests/HookCallbackRuleTest.php
+++ b/tests/HookCallbackRuleTest.php
@@ -113,7 +113,7 @@ public function testRule(): void
                     96,
                 ],
                 [
-                    'Filter callback return statement is missing.',
+                    'Action callback returns mixed but should not return anything.',
                     99,
                 ],
             ]
diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php
index bf57d22..840e4d4 100644
--- a/tests/data/hook-callback.php
+++ b/tests/data/hook-callback.php
@@ -95,8 +95,8 @@
 // Action callback returns false but should not return anything (with incorrect number of accepted args).
 add_action('action', '__return_false', 10, 2);
 
-// Filter callback return statement is missing.
-add_filter('filter', __NAMESPACE__ . '\\no_return_value_untyped');
+// Action callback returns mixed but should not return anything.
+add_action('action', __NAMESPACE__ . '\\return_value_mixed');
 
 /**
  * Incorrect usage that's handled by PHPStan:
@@ -145,6 +145,7 @@
 
 // Callback function with no declared return type.
 add_action('action', __NAMESPACE__ . '\\return_value_untyped');
+add_filter('filter', __NAMESPACE__ . '\\no_return_value_untyped');
 
 /**
  * Correct usage:
@@ -214,6 +215,11 @@
 
 // Various callback types
 add_filter('filter', '__return_false');
+add_filter('filter', __NAMESPACE__ . '\\return_value_mixed');
+add_filter('filter', __NAMESPACE__ . '\\return_value_untyped');
+add_filter('filter', __NAMESPACE__ . '\\return_value_mixed_union');
+add_filter('filter', __NAMESPACE__ . '\\return_value_documented');
+add_filter('filter', __NAMESPACE__ . '\\return_value_untyped');
 add_filter('filter', __NAMESPACE__ . '\\return_value_typed');
 add_filter('filter', new TestInvokableTyped(), 10, 2);
 add_filter('filter', [new TestClassTyped, 'foo']);
@@ -240,6 +246,31 @@ function return_value_typed() : int {
 
 function no_return_value_untyped( $value ) {}
 
+/**
+ * @return mixed
+ */
+function return_value_mixed() {
+    return 123;
+}
+
+/**
+ * Return type documented as a union that includes mixed.
+ *
+ * This is added as a regression test for a bug.
+ *
+ * @return int|mixed
+ */
+function return_value_mixed_union() {
+    return 123;
+}
+
+/**
+ * @return int
+ */
+function return_value_documented() {
+    return 123;
+}
+
 function return_value_untyped() {
     return 123;
 }