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

Handle references in ClosureReturnTypeOverridePlugin #3176

Merged
merged 7 commits into from Sep 1, 2019

Conversation

Daimona
Copy link
Member

@Daimona Daimona commented Sep 1, 2019

Fixes #2943.

I'm not 100% sure of this change, and I'd like to add some tests, but I don't know what's the right place for them.

Copy link
Member

@TysonAndre TysonAndre left a comment

Choose a reason for hiding this comment

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

I'm not 100% sure of this change, and I'd like to add some tests, but I don't know what's the right place for them.

Generally, new tests go in tests/files/src/ (incrementing numbers) with the corresponding expected file in tests/files/expected, unless it's specific to a non-default setting, or a plugin in .phan/plugins, or a php version.

I'd want some before merging this, to avoid breaking this functionality when refactoring in the future

src/Phan/Analysis/PostOrderAnalysisVisitor.php Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
%s:14 PhanImpossibleTypeComparison Impossible attempt to check if $y of type null is identical to 2 of type 2
Copy link
Member

Choose a reason for hiding this comment

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

Unexpected output in ./tests/files/src/0762_closure_reference.php
Failed asserting that './tests/files/src/0762_closure_reference.php:15 PhanImpossibleTypeComparison Impossible attempt to check if $y of type false is identical to 2 of type 2\n
./tests/files/src/0762_closure_reference.php:31 PhanUnusedVariable Unused definition of variable $arg\n
' matches PCRE pattern "/^$/".

The file name should be .php.expected, not just .expected

Also, the variable on line 31 is unused

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, didn't notice the .php, thanks.

As for the variable, well, it was supposed to be unused, so I'll suppress the issue.

@TysonAndre TysonAndre merged commit 9299337 into phan:master Sep 1, 2019
}

$z = false;
call_user_func_array('f762clos3', [ &$z ]);
Copy link
Member

Choose a reason for hiding this comment

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

This would probably treat call_user_func_array('f762clos3', [$z]) the same way, but fixing that seemed out of scope. A new issue type might be needed for a no-op reference.

I'd have to look into whether call_user_func_array('f762clos3', ['any literal or constant']) would emit a false positive.

There are probably edge cases with call_user_func('f762clos3', $z) as well, since $z can't be passed as a reference .

php > function setx(&$x) { $x = 2; }
php > call_user_func('setx', $x);
Notice: Undefined variable: x in php shell code on line 1
Warning: Parameter 1 to setx() expected to be a reference, value given in php shell code on line 1
php > var_export($x);
Notice: Undefined variable: x in php shell code on line 1
NULL

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.

PhanSuspiciousValueComparison false positive with call_user_func_array and pass by reference
2 participants