-
Couldn't load subscription status.
- Fork 544
Skip function-like and anonymous class nodes when checking if a function or method is a generator #3794
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
Skip function-like and anonymous class nodes when checking if a function or method is a generator #3794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to also not descend into anonymous classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ondrejmirtes Anonymous class nodes should now be skipped as well. I submitted the fix via Roave/BetterReflection#1479.
710646b to
e75270a
Compare
e75270a to
13d7837
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized there's one more place that needs fixing. Check out CleaningParserTest. The job of CleaningParser is to erase function bodies of not-analyzed code (like in vendor/) before the AST is cached into memory.
I worry the code is faulty and it might do something like this:
Before:
function foo() { $fn = fn () => yield; };
After:
function foo() { yield; };
Changing function foo() from non-generator to a generator.
Thank you!
I just realized there's one more place that needs fixing. Check out CleaningParserTest. The job of CleaningParser is to erase function bodies of not-analyzed code (like in vendor/) before the AST is cached into memory.
Ah ok, I'll look into it.
d446d6c to
e046593
Compare
I added two more examples to tests/PHPStan/Parser/data/cleaning-1-before.php. Please also have a look on the expected outcome after cleaning in tests/PHPStan/Parser/data/cleaning-1-after.php.
With the proposed changes to src/Parser/CleaningVisitor.php the closure in ContainsClosure::doFoo() will now be erased not leading to any left-over yield statements just like the yielding array-function in ContainsClosure::doBar(). Is this the intended behavior?
I also added Baz::someGenerator3() containing yield as a subexpression which will now lead to a yield expression after cleaning.
e046593 to
866e7cb
Compare
8840a07 to
f88659c
Compare
df9dc44 to
d4fec47
Compare
d4fec47 to
390646b
Compare
Fixes: phpstan/phpstan#12462.
As is, when checking if a function (or method) is a generator (via
PhpFunctionFromParserNodeReflection::isGenerator()) all subnodes are recursively checked for containment of a yield expression. Hence any function or method itself returning a yielding closure or arrow function is falsely determined to be a generator which in turn leads to incorrect return type checks as demonstrated in https://phpstan.org/r/5970e2e2-0aad-4927-8938-3f7d83e73a95.The proposed fix therefore simply skips
FunctionLikenodes in the above mentiond recursive check.