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

Fix false positive with Generator when yield is in a "while" condition #286

Merged
merged 8 commits into from Jul 29, 2020
Merged

Fix false positive with Generator when yield is in a "while" condition #286

merged 8 commits into from Jul 29, 2020

Conversation

dantleech
Copy link
Contributor

@dantleech dantleech commented Jul 26, 2020

Relates to: phpstan/phpstan#3669

This PR fixes the above named issue as suggested there.

Not sure about the test however.

when determining if scope has yield or not.
@@ -396,6 +396,12 @@ public function bodySpecifiedVoidTReturn3(): \Generator
return 2;
}

public function yieldInExpression(): \Generator
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue reported was detected by the MissingReturn rule. Not sure if there is a more suitable way to test (the test is that there is no error).

@dantleech dantleech changed the title WIP: Fix false positive with Generator when yield is in a condition Fix false positive with Generator when yield is in a condition Jul 26, 2020
@dantleech dantleech changed the title Fix false positive with Generator when yield is in a condition Fix false positive with Generator when yield is in a "while" condition Jul 26, 2020
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.

Thank you, looks like the right fix! (I'm sorry for the current build failures on master, will fix them once I have a working computer again this week.)

Could you please also verify that other similar statements (for loop, foreach loop, do while, switch) don't have the same problem? THanks.

$exprResult = $this->processExprNode($loopExpr, $bodyScope, static function (): void {
}, ExpressionContext::createTopLevel());
$bodyScope = $exprResult->getScope();
$hasYield = $hasYield || $exprResult->hasYield();
Copy link
Contributor Author

@dantleech dantleech Jul 27, 2020

Choose a reason for hiding this comment

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

not sure why this is a special case - i guess we could also get the ExpressionResult before this loop on $stmt->loop no, loop is an array of the 3 expressions I guess.

<?php // lint >= 7.4
<?php declare(strict_types = 1);

// lint >= 7.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the cs fixer

@dantleech
Copy link
Contributor Author

dantleech commented Jul 27, 2020

@ondrejmirtes updated for for, do and foreach - others seem to have logic to handle yield.

@ondrejmirtes
Copy link
Member

Looks like a simple if statement can't handle it either: https://phpstan.org/r/5dafc496-79af-4719-bcf2-41fa369da313

@dantleech
Copy link
Contributor Author

indeed, added if and switch

tests/e2e/magic-setter/test.php Outdated Show resolved Hide resolved
@ondrejmirtes ondrejmirtes merged commit af9f4e2 into phpstan:master Jul 29, 2020
@ondrejmirtes
Copy link
Member

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
2 participants