Skip to content

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

Merged
ondrejmirtes merged 8 commits intophpstan:masterfrom
dantleech:issue-3669-yield-complains-missing-return-type
Jul 29, 2020
Merged

Fix false positive with Generator when yield is in a "while" condition#286
ondrejmirtes merged 8 commits intophpstan:masterfrom
dantleech:issue-3669-yield-complains-missing-return-type

Conversation

@dantleech
Copy link
Copy Markdown
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.
return 2;
}

public function yieldInExpression(): \Generator
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@dantleech
Copy link
Copy Markdown
Contributor Author

indeed, added if and switch

@ondrejmirtes ondrejmirtes merged commit af9f4e2 into phpstan:master Jul 29, 2020
@ondrejmirtes
Copy link
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

2 participants