From f4b6cbd48f2252ab03ae643d0cf6975f9274d357 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 09:56:41 +0100 Subject: [PATCH 01/12] Increase the memory limit during unit tests. --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 23f5942..48a6e7a 100644 --- a/composer.json +++ b/composer.json @@ -56,7 +56,7 @@ "test:cs": "phpcs", "test:cs:fix": "phpcbf", "test:phpstan": "phpstan analyze", - "test:phpunit": "phpunit", + "test:phpunit": "phpunit -d memory_limit=1G", "test:syntax": "parallel-lint bootstrap.php src/ tests/" } } From a59e4120cb307aa4186f2a8b8fbbb0a5fc7ed986 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 10:07:22 +0100 Subject: [PATCH 02/12] Add some failing tests. --- tests/data/hook-callback.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index bf57d22..654fa9d 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -214,6 +214,9 @@ // Various callback types add_filter('filter', '__return_false'); +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 +243,22 @@ function return_value_typed() : int { function no_return_value_untyped( $value ) {} +/** + * Return type documented as a union that includes mixed. + * + * @return int|mixed + */ +function return_value_mixed_union() { + return 123; +} + +/** + * @return int + */ +function return_value_documented() { + return 123; +} + function return_value_untyped() { return 123; } From c28c817ab1beaffdcb86da331ecf1affae7ca513 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 10:14:53 +0100 Subject: [PATCH 03/12] More tests for `mixed`. --- tests/HookCallbackRuleTest.php | 4 ++++ tests/data/hook-callback.php | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index cc161fe..56d1af3 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -116,6 +116,10 @@ public function testRule(): void 'Filter callback return statement is missing.', 99, ], + [ + 'Action callback returns mixed but should not return anything.', + 102, + ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 654fa9d..3e0cf7b 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -98,6 +98,9 @@ // 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: * @@ -214,6 +217,7 @@ // Various callback types add_filter('filter', '__return_false'); +add_filter('filter', __NAMESPACE__ . '\\return_value_mixed'); add_filter('filter', __NAMESPACE__ . '\\return_value_mixed_union'); add_filter('filter', __NAMESPACE__ . '\\return_value_documented'); add_filter('filter', __NAMESPACE__ . '\\return_value_untyped'); @@ -243,6 +247,13 @@ 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. * From 141e5862278e98708bd1ba58ca84e658f11c2b4f Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 10:16:07 +0100 Subject: [PATCH 04/12] Boop. --- tests/data/hook-callback.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 3e0cf7b..a0a4017 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -99,7 +99,7 @@ add_filter('filter', __NAMESPACE__ . '\\no_return_value_untyped'); // Action callback returns mixed but should not return anything. -add_action('action', __NAMESPACE__ . 'return_value_mixed'); +add_action('action', __NAMESPACE__ . '\\return_value_mixed'); /** * Incorrect usage that's handled by PHPStan: From 602a64d70cc4f9d06d70da8d01462b4e9d70c5af Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 10:19:56 +0100 Subject: [PATCH 05/12] Add a test for implicit mixed return type. --- tests/data/hook-callback.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index a0a4017..ea424c1 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -218,6 +218,7 @@ // Various callback types add_filter('filter', '__return_false'); add_filter('filter', __NAMESPACE__ . '\\return_value_mixed'); +add_filter('filter', __NAMESPACE__ . '\\return_value_implicit_mixed'); add_filter('filter', __NAMESPACE__ . '\\return_value_mixed_union'); add_filter('filter', __NAMESPACE__ . '\\return_value_documented'); add_filter('filter', __NAMESPACE__ . '\\return_value_untyped'); @@ -254,6 +255,10 @@ function return_value_mixed() { return 123; } +function return_value_implicit_mixed( $value ) { + return $value; +} + /** * Return type documented as a union that includes mixed. * From 5b1a8762ff5ae32ca0845c2cfc43c9cf2297bcfb Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 10:23:12 +0100 Subject: [PATCH 06/12] Add another implicit return type test for an action callback. --- tests/HookCallbackRuleTest.php | 4 ++++ tests/data/hook-callback.php | 3 +++ 2 files changed, 7 insertions(+) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 56d1af3..b6c396b 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -120,6 +120,10 @@ public function testRule(): void 'Action callback returns mixed but should not return anything.', 102, ], + [ + 'Action callback returns mixed but should not return anything.', + 105, + ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index ea424c1..9df5544 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -101,6 +101,9 @@ // Action callback returns mixed but should not return anything. add_action('action', __NAMESPACE__ . '\\return_value_mixed'); +// Action callback returns mixed but should not return anything. +add_action('action', __NAMESPACE__ . '\\return_value_implicit_mixed'); + /** * Incorrect usage that's handled by PHPStan: * From aed9233194aa9e0455c7c79851b4025d280fa744 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 09:33:28 +0000 Subject: [PATCH 07/12] Update tests/data/hook-callback.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Viktor Szépe --- tests/data/hook-callback.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 9df5544..312a8b3 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -265,6 +265,8 @@ function return_value_implicit_mixed( $value ) { /** * 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() { From 974b07227f5ab55394175ac2e6f9c519863f3433 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 12:08:29 +0100 Subject: [PATCH 08/12] Implicit mixed is not the responsibility of this library. --- tests/HookCallbackRuleTest.php | 4 ---- tests/data/hook-callback.php | 4 +--- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index b6c396b..56d1af3 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -120,10 +120,6 @@ public function testRule(): void 'Action callback returns mixed but should not return anything.', 102, ], - [ - 'Action callback returns mixed but should not return anything.', - 105, - ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 312a8b3..6dc9a0c 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -101,9 +101,6 @@ // Action callback returns mixed but should not return anything. add_action('action', __NAMESPACE__ . '\\return_value_mixed'); -// Action callback returns mixed but should not return anything. -add_action('action', __NAMESPACE__ . '\\return_value_implicit_mixed'); - /** * Incorrect usage that's handled by PHPStan: * @@ -151,6 +148,7 @@ // Callback function with no declared return type. add_action('action', __NAMESPACE__ . '\\return_value_untyped'); +add_action('action', __NAMESPACE__ . '\\return_value_implicit_mixed'); /** * Correct usage: From 858eddbe7fbbd9806e12775e506a2214d21a518f Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 12:09:10 +0100 Subject: [PATCH 09/12] Add support for filters that return explicit mixed. Props @herndlm. --- src/HookCallbackRule.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 39bf49e..af5f9c7 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; @@ -205,9 +206,12 @@ protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor): void { $returnType = $callbackAcceptor->getReturnType(); - $isVoidSuperType = $returnType->isSuperTypeOf(new VoidType()); - if (! $isVoidSuperType->yes()) { + if ($returnType instanceof MixedType && $returnType->isExplicitMixed()) { + return; + } + + if (! $returnType->isSuperTypeOf(new VoidType())->yes()) { return; } From baaf88e81b06bbb367e7269413a675c004637106 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Fri, 11 Nov 2022 12:09:42 +0100 Subject: [PATCH 10/12] Add support for actions that incorrectly return explicit mixed. --- src/HookCallbackRule.php | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index af5f9c7..392b997 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -185,22 +185,26 @@ 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 From c558d9cbb270287f3b248339a553b8d3f6956098 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 12 Nov 2022 15:46:53 +0100 Subject: [PATCH 11/12] Fix the implicit handling. --- src/HookCallbackRule.php | 10 ++++++++-- tests/HookCallbackRuleTest.php | 6 +----- tests/data/hook-callback.php | 11 ++--------- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 392b997..6a0ad72 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -211,11 +211,17 @@ protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor { $returnType = $callbackAcceptor->getReturnType(); - if ($returnType instanceof MixedType && $returnType->isExplicitMixed()) { + if ($returnType instanceof MixedType) { return; } - if (! $returnType->isSuperTypeOf(new VoidType())->yes()) { + $accepted = $this->ruleLevelHelper->accepts( + new VoidType(), + $returnType, + true + ); + + if (! $accepted) { return; } diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 56d1af3..3cc50d3 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -112,13 +112,9 @@ public function testRule(): void 'Action callback returns false but should not return anything.', 96, ], - [ - 'Filter callback return statement is missing.', - 99, - ], [ 'Action callback returns mixed but should not return anything.', - 102, + 99, ], ] ); diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 6dc9a0c..840e4d4 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -95,9 +95,6 @@ // 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'); @@ -148,7 +145,7 @@ // Callback function with no declared return type. add_action('action', __NAMESPACE__ . '\\return_value_untyped'); -add_action('action', __NAMESPACE__ . '\\return_value_implicit_mixed'); +add_filter('filter', __NAMESPACE__ . '\\no_return_value_untyped'); /** * Correct usage: @@ -219,7 +216,7 @@ // Various callback types add_filter('filter', '__return_false'); add_filter('filter', __NAMESPACE__ . '\\return_value_mixed'); -add_filter('filter', __NAMESPACE__ . '\\return_value_implicit_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'); @@ -256,10 +253,6 @@ function return_value_mixed() { return 123; } -function return_value_implicit_mixed( $value ) { - return $value; -} - /** * Return type documented as a union that includes mixed. * From 310e0530aace4c3bbc0a03cc0027eb4e02e3df75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20Sz=C3=A9pe?= Date: Sat, 12 Nov 2022 14:55:05 +0000 Subject: [PATCH 12/12] Update composer.json --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 48a6e7a..23f5942 100644 --- a/composer.json +++ b/composer.json @@ -56,7 +56,7 @@ "test:cs": "phpcs", "test:cs:fix": "phpcbf", "test:phpstan": "phpstan analyze", - "test:phpunit": "phpunit -d memory_limit=1G", + "test:phpunit": "phpunit", "test:syntax": "parallel-lint bootstrap.php src/ tests/" } }