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

Report useless return values of function calls #3225

Merged
merged 12 commits into from
Jul 12, 2024

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 11, 2024

@staabm staabm marked this pull request as ready for review July 11, 2024 12:30
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

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.

Separate rule please. Same level the void usage is reported (not sure from the top of my head).

@staabm
Copy link
Contributor Author

staabm commented Jul 11, 2024

Same level the void usage is reported (not sure from the top of my head).

its a level 0 error: https://phpstan.org/r/42b76642-82b2-422a-bd7d-f3678eb6e1b0

src/Rules/Functions/UselessFunctionReturnValueRule.php Outdated Show resolved Hide resolved
conf/config.level0.neon Outdated Show resolved Hide resolved
@clxmstaab clxmstaab force-pushed the useless-return branch 2 times, most recently from 3d864f7 to 51d7783 Compare July 12, 2024 07:10
@staabm
Copy link
Contributor Author

staabm commented Jul 12, 2024

I have no idea why the PHP7.2 build fails with a "risky error". maybe its related that I ran the levels-tests on a windows machine.

public function testUselessReturnValuePhp8(): void
{
if (PHP_VERSION_ID < 80000) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't how to make test for >= 80000 only. You need to call markTestSkipped. See many examples in other tests. This is why it's marked as risky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch. great catch, thanks.

if (count($reorderedArgs) === 1 || (count($reorderedArgs) >= 2 && $scope->getType($reorderedArgs[1]->value)->isFalse()->yes())) {
return [RuleErrorBuilder::message(
sprintf(
'Return value of function %s is always true and the result is printed instead of being returned. Pass in true as parameter #%d $%s to return the output instead.',
Copy link
Member

Choose a reason for hiding this comment

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

Please put () after %s. So that it's print_r() and not just print_r.

@staabm
Copy link
Contributor Author

staabm commented Jul 12, 2024

in phpstorm stubs I found another function alias worth reporting: show_source()

@staabm
Copy link
Contributor Author

staabm commented Jul 12, 2024

just realized the PrestaShop/PrestaShop shows 5 real world errors this rule is targetting, simlar to the one of my initial report 💪

@ondrejmirtes
Copy link
Member

Nice! I really like this addition.

@ondrejmirtes ondrejmirtes merged commit ebce317 into phpstan:1.11.x Jul 12, 2024
450 of 454 checks passed
@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
Development

Successfully merging this pull request may close these issues.

3 participants