Conversation
…nly used in return/exit [2] Follow up on 1208. Improve the previously applied fix to bow out from reporting "use" of a passed parameter before a call to `func_get_args()` when the parameter is used in a `return` or `exit` statement. The previous fix would break when the `return`/`exit` statement was within an unscoped condition. Unscoped conditions (no curly braces) are a bane to the existence of sniff writers anywhere and can't be fully accounted for. Even so, there are some common situations in which the previous fix can be improved to reduce false positives. This current fix will skip over `return`/`exit` statements completely, instead of walking back from a matched variable to check if something is within a `return`/`exit` statement. This also makes the sniff more efficient. Secondly, the sniff will now also ignore parameter use in `throw` statements. Includes additional unit tests. Fixes 1240
…r empty() Those uses are innocent, will not change the value or the passed parameter and can be safely ignored. Includes unit tests.
When one of the passed parameters is used in a call to `unset()`, we know for sure it has been changed, so the sniff should throw an error, not a warning. Includes unit test.
287abd3 to
cfe5186
Compare
…nly used in simple assignment Prevent a false positive (warning) when a parameter is only _assigned_ to another variable and this is a "simple assignment". For the purposes of this check, a "simple assignment" is defined as: - The variable is not nested in parenthesis, i.e. not used in a potential function call which may adjust the variable by reference. - Not preceded by a reference operator (reference assignment). - Has an assignment operator before it and none after. Any more complex statements where a passed parameter is being assigned to another with a call to one of the argument functions after it, will still be reported as "needs inspection". Includes additional unit tests. Fixes 1240 Related to 796
cfe5186 to
5801ee3
Compare
|
@jrfnl Overall, this PR appears to do what it says on the tin quite well. I was curious about why the sniff doesn't appear to cover the following similar cases, though? switch ($stuff) { /* ... */ }
switch ($other) { case $stuff: /* ... */ }
while ($stuff) { /* ... */ }
do { /* ... */ } while ($stuff);
for ($i = 0; $i < $stuff; $i++) { /* ... */ }
require $stuff;
require_once $stuff;
include $stuff;
include_once $stuff; |
|
Hi @elazar Thanks for looking it over!
I totally agree there is room for more improvement, but it is a pretty complex sniff as it is already. Regarding the code samples:
I agree that this could be a pattern which could be covered in a future iteration on the sniff, with the definition of that pattern being "parameter used in a control structure condition without any other functional tokens in that condition", with maybe the one exception being
Here we're already moving into more complex territory as the sniff would now have to interpret the condition, which could easily become more complex than anticipated and will be very error prone. Think: for ($i = 0; $stuff ??= $i < $stuff; $i++) { /* ... */ }I know that's a bit of a contrived example, but you get where I'm going with this.
Sorry, but this one is plain invalid and should definitely not be handled by the sniff as the included file may overwrite the value of the parameter. Think: // File 1:
<?php
function foo ($nextFile) {
include_once $nextFile;
func_get_args();
}
// File 2 which would be included:
<?php
if ($something) {
$nextFile = __DIR__ . '/someotherfile.php';
}Also see #796 for more discussion around what to cover and what not. |
👉 This PR will be easiest to review by looking at the individual commits.
ArgumentFunctionsReportCurrentValue: don't report when parameter is only used in return/exit [2]
Follow up on #1208.
Improve the previously applied fix to bow out from reporting "use" of a passed parameter before a call to
func_get_args()when the parameter is used in areturnorexitstatement.The previous fix would break when the
return/exitstatement was within an unscoped condition.Unscoped conditions (no curly braces) are a bane to the existence of sniff writers anywhere and can't be fully accounted for.
Even so, there are some common situations in which the previous fix can be improved to reduce false positives.
This current fix will skip over
return/exitstatements completely, instead of walking back from a matched variable to check if something is within areturn/exitstatement.This also makes the sniff more efficient.
Secondly, the sniff will now also ignore parameter use in
throwstatements.Includes additional unit tests.
Fixes #1240
ArgumentFunctionsReportCurrentValue: minor fix to the unit test documentation
ArgumentFunctionsReportCurrentValue: ignore variable use in isset() or empty()
Those uses are innocent, will not change the value of the passed parameter and can be safely ignored.
Includes unit tests.
ArgumentFunctionsReportCurrentValue: throw an error for unset()
When one of the passed parameters is used in a call to
unset(), we know for sure it has been changed, so the sniff should throw an error, not a warning.Includes unit test.
ArgumentFunctionsReportCurrentValue: don't report when parameter is only used in simple assignment
Prevent a false positive (warning) when a parameter is only assigned to another variable and this is a "simple assignment".
For the purposes of this check, a "simple assignment" is defined as:
Any more complex statements where a passed parameter is being assigned to another with a call to one of the argument functions after it, will still be reported as "needs inspection".
Includes additional unit tests.
Fixes #1240
Related to #796