Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hook callback false positives for mixed return type #130

Merged
merged 12 commits into from Nov 12, 2022
36 changes: 25 additions & 11 deletions src/HookCallbackRule.php
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion tests/HookCallbackRuleTest.php
Expand Up @@ -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,
],
]
Expand Down
35 changes: 33 additions & 2 deletions tests/data/hook-callback.php
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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']);
Expand All @@ -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.
johnbillion marked this conversation as resolved.
Show resolved Hide resolved
*
* This is added as a regression test for a bug.
*
* @return int|mixed
johnbillion marked this conversation as resolved.
Show resolved Hide resolved
*/
function return_value_mixed_union() {
return 123;
}

/**
* @return int
*/
function return_value_documented() {
return 123;
}

function return_value_untyped() {
return 123;
}
Expand Down