Skip to content

Passing a variable by reference to a function and method has side effects#1990

Merged
ondrejmirtes merged 7 commits intophpstan:1.9.xfrom
ryium:patch/8084
Jan 8, 2023
Merged

Passing a variable by reference to a function and method has side effects#1990
ondrejmirtes merged 7 commits intophpstan:1.9.xfrom
ryium:patch/8084

Conversation

@ryium
Copy link
Copy Markdown
Contributor

@ryium ryium commented Nov 10, 2022

@ryium ryium marked this pull request as ready for review November 10, 2022 17:14
@phpstan-bot
Copy link
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

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.

Could you please test and fix other similar functions that might have the same issue? array_push, array_unshift, array_pop, array_splice

@ondrejmirtes
Copy link
Copy Markdown
Member

And maybe it'd be a better to idea to do this in public function hasSideEffects(): TrinaryLogic in various FunctionReflection and MethodReflection implementations - to return TrinaryLogic::createYes() if the function accepts parameters as references.

@ryium ryium changed the title Mark array_shift as having side effects Passing a variable by reference to a function and method has side effects Nov 12, 2022
{
public function run(array $arr): void
{
/** @var array{a: 0, b?: 1} $arr */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, please put this type into @param above the method, here it's ambiguous what it means.

Also currently this example https://phpstan.org/r/de642079-e5b0-492a-9782-d78c324f70a7 shows Expression on left side of ?? is not nullable. so we'll need another test in NullCoalesceRuleTest to make sure this message goes away.

Thank you!

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.

Hi, thank you for your review.
Added a test, but included a merge commit to follow the latest branch.
Should I recreate?

@ondrejmirtes ondrejmirtes merged commit 1070788 into phpstan:1.9.x Jan 8, 2023
@ondrejmirtes
Copy link
Copy Markdown
Member

Brought it over the finish line :) Thank you.

@ondrejmirtes
Copy link
Copy Markdown
Member

Sorry, I had to revert this, it led to some false positives.

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.

PHP 8.0 - throw statement is now throw expression, it can throw at any point

3 participants