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
Merged

Hook callback false positives for mixed return type #130

merged 12 commits into from Nov 12, 2022

Conversation

johnbillion
Copy link
Sponsor Contributor

@johnbillion johnbillion commented Nov 11, 2022

Fixes #125

Seems the problem is ultimately down to mixed return types.

@herndlm
Copy link
Contributor

herndlm commented Nov 11, 2022

also looking at this right now :)

yeah, the problem is that a mixed return type currently cannot be distinguished from a missing one (which is also mixed). I think you could check mixed explicitness (the one should be explicit while the other is implicit). That means some instanceof MixedType checks which are not optimal, but I can't think of anything else right now ..

@herndlm
Copy link
Contributor

herndlm commented Nov 11, 2022

oh and btw another test case for implicit mixed is simply something like the following

function return_value_mixed_implicit( $value ) {
    return $value;
}

if you put return 123; there it's most likely not mixed anymore

my false-positive was more dynamic, with a conditional inside, but it comes down to the same thing basically

UPDATE: oh, CI is red already, nevermind then, the test cases are good enough!

composer.json Outdated Show resolved Hide resolved
@johnbillion johnbillion changed the title Hook callback false positives Hook callback false positives for mixed return type Nov 11, 2022
Co-authored-by: Viktor Szépe <viktor@szepe.net>
@herndlm
Copy link
Contributor

herndlm commented Nov 11, 2022

Fyi using the following patch seems to greatly improve it already for me. But we can still not differentiate between no return at all and mixed implicit return. Type-wise it makes sense, since PHPStan is not looking at the function body but only the definition and unfortunately I don't know if there's another way of getting the info if there's a return inside basically. Sticking with the types might be safer

     protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor): void
     {
         $returnType = $callbackAcceptor->getReturnType();
-        $isVoidSuperType = $returnType->isSuperTypeOf(new VoidType());

-        if (! $isVoidSuperType->yes()) {
+        if (
+            ($returnType instanceof MixedType && $returnType->isExplicitMixed())
+            || !$returnType->isSuperTypeOf(new VoidType())->yes()
+        ) {
             return;
         }

Give me a sec, I'm gonna ask at phpstan if there's something else..

on second thought I think you did the right thing with using RuleLevelHelper to let PHPStan and its config + level decide if it should handle mixed or not. But that likely leads to missing false negatives, for add_action this is not a big problem right? but for add_filter it is I guess..

@johnbillion
Copy link
Sponsor Contributor Author

I think we got a bit distracted by implicit mixed, which isn't the responsibility of this library to figure out. If your callback uses implicit mixed then PHPStan flags the callback as having a missing return type (these tests are in place to ensure this library doesn't trigger an error in that situation).

@szepeviktor
Copy link
Owner

You paint CI feedback green, I'll merge it right away.

@herndlm
Copy link
Contributor

herndlm commented Nov 11, 2022

good point. In that case - does it make sense to also use RuleLevelHelper then, as you do with add_action? Or turn the supertype check around like to report only for when something is surely a void return via (new VoidType())->isSuperTypeOf($returnType)->yes()? I'm not sure about the explicit mixed checks, yeah :/

@johnbillion
Copy link
Sponsor Contributor Author

Okay I removed the implicit mixed handling (which is how it should be), added support for explicit mixed handling, and CI is all green.

@johnbillion johnbillion requested review from herndlm and szepeviktor and removed request for herndlm and szepeviktor November 12, 2022 14:51
composer.json Outdated Show resolved Hide resolved
@szepeviktor
Copy link
Owner

Thank you!

I hope every user will be happy.

@szepeviktor szepeviktor merged commit 3930c97 into szepeviktor:master Nov 12, 2022
@johnbillion johnbillion deleted the hook-false-positives branch November 12, 2022 14:58
@herndlm
Copy link
Contributor

herndlm commented Nov 12, 2022

Nice, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positives in add_filter() type checking
3 participants