Skip to content

Fix args are mistakenly handled as immediately-invoked #4145

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

Merged
merged 20 commits into from
Aug 1, 2025

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 22, 2025

@staabm staabm marked this pull request as ready for review July 22, 2025 09:59
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

if (
$parameterType === null
|| $parameterType instanceof MixedType
|| $parameterType->isCallable()->no()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a new TypeUtils method. findCallableType perhaps. It'd return the type for isCallable()->yes(). Additionally it should recursively inspect unions for types like ?callable or ?Closure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think this logic is wrong. What mainly has to be handled is $parameterCallImmediately->maybe(). That's where you need to ask about this. And also in the else section where it's $callCallbackImmediately = $calleeReflection instanceof FunctionReflection; right now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic fails for a couple reasons:

  1. If the parameter is mixed but marked with @param-immediately-invoked-callable it'd get silently ignored.
  2. It'd still mark parameter as immediately invoked if the type is for example string (which is maybe-callable).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the in-detail feedback. there are a lot of things to consider, I did not see yet.
additionally phpstan/phpstan#13331 (comment) was useful.

I tried adding more tests and adding comments about expectations, of what I understood from the comments.

thank you

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally please add a regression test for 12119

return $type;
}

if ($type instanceof UnionType || $type instanceof IntersectionType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to go into intersections. Once one of the intersected types is callable, the whole type is callable.

@ondrejmirtes ondrejmirtes merged commit 9dbff97 into phpstan:2.1.x Aug 1, 2025
433 of 441 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the bug13288 branch August 1, 2025 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment