Skip to content

FunctionUse/ArgumentFunctionsReportCurrentValue: various improvements#1892

Merged
wimg merged 31 commits intodevelopfrom
feature/argumentfunctionsreportcurrentvalue-various-improvements
Sep 1, 2025
Merged

FunctionUse/ArgumentFunctionsReportCurrentValue: various improvements#1892
wimg merged 31 commits intodevelopfrom
feature/argumentfunctionsreportcurrentvalue-various-improvements

Conversation

@jrfnl
Copy link
Copy Markdown
Member

@jrfnl jrfnl commented Aug 27, 2025

⚠️ This PR depends on PR #1891 ⚠️


FunctionUse/ArgumentFunctionsReportCurrentValue: test case file - tabs vs spaces

FunctionUse/ArgumentFunctionsReportCurrentValue: make "no false positives" data provider more robust

(and easier to maintain).

FunctionUse/ArgumentFunctionsReportCurrentValue: test case file - fix parse and compile errors

These parse errors and compile errors did not affect the tests in any way.

The changes in the function/class names are intended to make it more straight-forward to see what is being tested in each case (aside from fixing compile errors "redeclaration of function ....").

FunctionUse/ArgumentFunctionsReportCurrentValue: add some extra tests

... for code in the sniff previously uncovered.

FunctionUse/ArgumentFunctionsReportCurrentValue: add more tests with variations of FQN function calls

FunctionUse/ArgumentFunctionsReportCurrentValue: minor code simplification

A few lines earlier there is already a continue for when $afterVar would be false.

FunctionUse/ArgumentFunctionsReportCurrentValue: simplify code with PHPCSUtils [1]

FunctionUse/ArgumentFunctionsReportCurrentValue: simplify code with PHPCSUtils [2]

FunctionUse/ArgumentFunctionsReportCurrentValue: simplify code with PHPCSUtils [3]

FunctionUse/ArgumentFunctionsReportCurrentValue: track "previous non empty" for the loop

... to prevent some unnecessary token walking.

Also: as we are searching a known set of tokens, we know that the "previous token" will always exist, so no need to check against false.

FunctionUse/ArgumentFunctionsReportCurrentValue: bug fix - handling of constant in debug*backtrace()

The DEBUG_BACKTRACE_IGNORE_ARGS should only be recognized as a reason to ignore the call to the target functions if it refers to the PHP native global constant.

This commit fixes the sniff to not have false negatives if a namespaced constant is used instead of the global one.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: allow for numeric $options in debug*backtrace()

As per the PHP manual, while the use of the PHP native constants is recommended, it is also perfectly fine to use a numeric value to set the option flags and this is even documented as such in the manual.

This commit adds handling of the documented/known numeric option values which mean the "args" index will not be set, to the sniff.

Note: this handling presumes that a plain decimal number would have been passed.
It is possible to pass a non-decimal number or even a calculation, and while that is supported in PHP, I deem it extremely unlikely anyone would do so and if they do, it just means they get the warning/error, despite the numeric value being equal to one of the values we're now detecting as flags to ignore the function call, so no harm, no foul.

Includes tests.

Ref: https://www.php.net/manual/en/function.debug-backtrace.php#refsect1-function.debug-backtrace-parameters

FunctionUse/ArgumentFunctionsReportCurrentValue: bug fix - check function names case-insensitively

This was already done correctly for the target functions, but not for the check whether a call to any of the target functions was nested in a call to array_slice() or array_splice().

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: bug fix - false negatives for methods and namespaced functions

The sniff contains special casing for when any of the target functions are nested within the PHP native global array_splice() or array_splice() functions.

However, this special casing did not take namespaced functions or methods mirroring the name of the PHP native functions, into account.

This commit adds more defensive coding to verify we're really dealing with the PHP native global functions.

As the code to check this was already used elsewhere too, it has been moved to a function, so it can be used in both places.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: set redundant default value

This variable is only accessed within conditions and those conditions already sufficiently ensure that the variable will be set for the conditions path.

Even so, PHPStan cannot handle this, so we may as well declare a sensible default for this variable to make things more explicit.

#FunctionUse/ArgumentFunctionsReportCurrentValue: improve handling of list assignments

Array variables can be destructured via lists and the sniff did not take this into account until now.

The net effect of this was:

  • False positive warning "needs inspection" when a function parameter is list-destructured to variables which are not parameters.
  • Warning instead of error when a function parameter is assigned a new value via list-destructuring.
  • False positive error when a function parameter is used as a key in list-destructuring. The key is just used to retrieve the target value from the array and is not changed, so we can safely ignore these.
  • Accidentally correct handling (warning "needs inspection") for when a reference assignment is used in the list-destructuring of a function parameter.

This commit adds dedicated handling of lists to this sniff, which fixes each of these problems.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: improve handling of array_slice/splice [1]

The offset passed to array_slice()/array_splice() can be a negative number (offset from the end of the array).

Until now, the sniff did not handle this and would threat all numbers as positives numbers, which could lead to both false positives as well as false negatives.

Fixed now.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: improve handling of array_slice/splice [2]

Both array_slice()/array_splice() take a $length parameter which affects which elements are retrieved.

Until now, the sniff did not handle this and would presume all parameter from $offset to the end of the list should be taken into account, while if we take length into account, we can prevent some more false positives.

Fixed now.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: handle non-decimal numbers

While probably rare, it is perfectly valid to pass the $arg_num parameter for func_get_arg() or the $offset or $length parameters for array_slice() and array_splice() as non-decimal numbers.

And while the second case (array_slice()/array_splice()) was fixed in a previous commit, the former ($arg_num) was not.

Fixed now.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: add tests with PHP 7.4 underscore integers

These are handled correctly since some of the fixes in the previous commit went in (same fixes were needed for other reasons).

These tests just safeguard this.

FunctionUse/ArgumentFunctionsReportCurrentValue: add tests with PHP 8.1 octal literals

These are handled correctly since support for non-decimal numbers was added (same fix was needed).

FunctionUse/ArgumentFunctionsReportCurrentValue: special case PHP 7.4+ null coalesce assignment

In contrast to all other assignment operators, the null coalesce assignment operator may or may not change the value of a variable.

With that in mind, I believe it is more appropriate to throw a warning when a parameter is being assigned to with the null coalesce operator (in contrast to the error being thrown for other assignment operators).

Includes tests, both for a variety of assignment operators as well as for the null coalesce assignment operator.

FunctionUse/ArgumentFunctionsReportCurrentValue: improve handling of simple control structure conditions

If a function parameter is only used in a control structure condition without further conditions/comparisons, we can be sure it has not been changed.

This commit adds support for detecting this to reduce false positives further.

Notes:

  • foreach() is special cased as we can ignore use of the variable before the as, but should error on use of the variable after the as.
  • catch() is special cased as any variable in catch will always be an assignment, so we should error on that.
  • In all other cases, when we're in a control structure condition without other non-empty tokens, we can ignore the use of the variable.

Includes handling of PHP 8.0+ match() control structures.

Includes tests.

Loosely related to #796

FunctionUse/ArgumentFunctionsReportCurrentValue: ignore target functions in PHP 7.4+ arrow functions

Arrow functions can engage with variables outside of their own scope, so should not blindly be skipped over, but if one of the target functions is seen within the arrow function body, it applies to the arrow function, not the outside scope, so this needs careful handling.

Now, while arrow functions can only contain a single statement in their body, they can still run into the problem this sniff flags (see example code in the tests).
Having said that, arrow functions were introduced in PHP 7.4, four years after the change this sniff checks for entered PHP, so anyone who uses any of the target functions in the body of an arrow function should expect the "new" behaviour.

With this in mind, this commit explicit does NOT add checking of arrow functions to the sniff.

However, it does change the sniff to prevent false positives on any of the target functions being seen within the body of an arrow function.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: prevent false positives on PHP 8.0+ attributes

T_STRING tokens in PHP 8.0+ attributes are either class names or possibly constant names (as a parameter for the class instantiation).
They are never function calls.

This commit ensures that T_STRING tokens in attributes are not confused with function calls.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: update tests to include PHP 8.0 trailing commas in param lists

FunctionUse/ArgumentFunctionsReportCurrentValue: ignore PHP 8.0+ $obj::class

If a the class name of an object parameter is retrieved using the $obj::class syntax, we know the parameter is not being changed, so we can safely ignore it.

Includes tests.

FunctionUse/ArgumentFunctionsReportCurrentValue: add support for PHP 8.0+ named arguments in function calls

For the array_*() functions to was incidentally and partially added in some of the previous commits, though the code still needed to be adjusted to allow for an unexpected parameter order.
For the "target functions", support was not handled so far, so this commit adds support for named parameters.

Verified the parameter name used is in line with the name as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) is the only relevant name.

Name verification reference:

Includes adding the unit tests to include tests using named parameters.

Related to #1239

👉 This commit will be more straight-forward to review while ignoring whitespace changes.

FunctionUse/ArgumentFunctionsReportCurrentValue: add test with PHP 8.0+ constructor property promotion

Shouldn't have any impact, just safeguarding this with a test.

FunctionUse/ArgumentFunctionsReportCurrentValue: add new error for use as PHP 8.1+ first class callable

Using the func_get_arg*() functions as first class callables is not allowed in PHP. See: https://3v4l.org/ISt2r and https://3v4l.org/aTjjQ

However, using the debug_*backtrace() function as first class callable is allowed, but seems like a really bad idea....
See: https://3v4l.org/j8lWg and https://3v4l.org/8qbJF

This commit adds handling for this by:

  • Ignoring the use of func_get_arg*() functions when used as first class callables. That's forbidden by PHP, so not our concern.
  • And flagging the use of debug_*backtrace() functions when used as first class callables with a warning with its own error code.

Includes tests.

jrfnl added 30 commits August 27, 2025 04:44
… PHP 8.1 octal literals

Using the PHPCSUtils `Numbers` class and using the decimal value determined via that class prevents issues with PHP 7.4+ numeric literals with underscores and PHP 8.1+ octal literals when the sniffs are being run on PHP < 7.4.

Includes tests.
…ives" data provider more robust

(and easier to maintain).
… parse and compile errors

These parse errors and compile errors did not affect the tests in any way.

The changes in the function/class names are intended to make it more straight-forward to see what is being tested in each case (aside from fixing compile errors "redeclaration of function ....").
... for code in the sniff previously uncovered.
…ation

A few lines earlier there is already a `continue` for when `$afterVar` would be `false`.
…empty" for the loop

... to prevent some unnecessary token walking.

Also: as we are searching a known set of tokens, we know that the "previous token" will always exist, so no need to check against `false`.
…f constant in debug*backtrace()

The `DEBUG_BACKTRACE_IGNORE_ARGS` should only be recognized as a reason to ignore the call to the target functions if it refers to the PHP native global constant.

This commit fixes the sniff to not have false negatives if a namespaced constant is used instead of the global one.

Includes tests.
…ptions in debug*backtrace()

As per the PHP manual, while the use of the PHP native constants is recommended, it is also perfectly fine to use a numeric value to set the option flags and this is even documented as such in the manual.

This commit adds handling of the documented/known numeric option values which mean the "args" index will not be set, to the sniff.

Note: this handling presumes that a plain decimal number would have been passed.
It is possible to pass a non-decimal number or even a calculation, and while that is supported in PHP, I deem it extremely unlikely anyone would do so and if they do, it just means they get the warning/error, despite the numeric value being equal to one of the values we're now detecting as flags to ignore the function call, so no harm, no foul.

Includes tests.

Ref: https://www.php.net/manual/en/function.debug-backtrace.php#refsect1-function.debug-backtrace-parameters
…tion names case-insensitively

This was already done correctly for the target functions, but not for the check whether a call to any of the target functions was nested in a call to `array_slice()` or `array_splice()`.

Includes tests.
…tives for methods and namespaced functions

The sniff contains special casing for when any of the target functions are nested within the PHP native global `array_splice()` or `array_splice()` functions.

However, this special casing did not take namespaced functions or methods mirroring the name of the PHP native functions, into account.

This commit adds more defensive coding to verify we're really dealing with the PHP native global functions.

As the code to check this was already used elsewhere too, it has been moved to a function, so it can be used in both places.

Includes tests.
…t value

This variable is only accessed within conditions and those conditions already sufficiently ensure that the variable will be set for the conditions path.

Even so, PHPStan cannot handle this, so we may as well declare a sensible default for this variable to make things more explicit.
…list assignments

Array variables can be destructured via lists and the sniff did not take this into account until now.

The net effect of this was:
* False positive warning "needs inspection" when a function parameter is list-destructured to variables which are not parameters.
* Warning instead of error when a function parameter is assigned a new value via list-destructuring.
* False positive error when a function parameter is used as a key in list-destructuring. The key is just used to retrieve the target value from the array and is not changed, so we can safely ignore these.
* Accidentally correct handling (warning "needs inspection") for when a reference assignment is used in the list-destructuring of a function parameter.

This commit adds dedicated handling of lists to this sniff, which fixes each of these problems.

Includes tests.
…array_slice/splice [1]

The offset passed to `array_slice()`/`array_splice()` can be a negative number (offset from the end of the array).

Until now, the sniff did not handle this and would threat all numbers as positives numbers, which could lead to both false positives as well as false negatives.

Fixed now.

Includes tests.
…array_slice/splice [2]

Both `array_slice()`/`array_splice()` take a `$length` parameter which affects which elements are retrieved.

Until now, the sniff did not handle this and would presume all parameter from `$offset` to the end of the list should be taken into account, while if we take length into account, we can prevent some more false positives.

Fixed now.

Includes tests.
…umbers

While probably rare, it is perfectly valid to pass the `$arg_num` parameter for `func_get_arg()` or the `$offset` or `$length` parameters for `array_slice()` and `array_splice()` as non-decimal numbers.

And while the second case (`array_slice()`/`array_splice()`) was fixed in a previous commit, the former (`$arg_num`) was not.

Fixed now.

Includes tests.
….4 underscore integers

These are handled correctly since some of the fixes in the previous commit went in (same fixes were needed for other reasons).

These tests just safeguard this.
….1 octal literals

These are handled correctly since support for non-decimal numbers was added (same fix was needed).
…+ null coalesce assignment

In contrast to all other assignment operators, the null coalesce assignment operator _may_ or _may not_ change the value of a variable.

With that in mind, I believe it is more appropriate to throw a warning when a parameter is being assigned to with the null coalesce operator (in contrast to the error being thrown for other assignment operators).

Includes tests, both for a variety of assignment operators as well as for the null coalesce assignment operator.
…simple control structure conditions

If a function parameter is only used in a control structure condition without further conditions/comparisons, we can be sure it has not been changed.

This commit adds support for detecting this to reduce false positives further.

Notes:
* `foreach()` is special cased as we can ignore use of the variable before the `as`, but should error on use of the variable after the `as`.
* `catch()` is special cased as any variable in catch will always be an assignment, so we should error on that.
* In all other cases, when we're in a control structure condition without other non-empty tokens, we can ignore the use of the variable.

Includes handling of PHP 8.0+ `match()` control structures.

Includes tests.

Loosely related to 796
…ons in PHP 7.4+ arrow functions

Arrow functions can engage with variables outside of their own scope, so should not blindly be skipped over, but if one of the target functions is seen within the arrow function body, it applies to the arrow function, not the outside scope, so this needs careful handling.

Now, while arrow functions can only contain a single statement in their body, they can still run into the problem this sniff flags (see example code in the tests).
Having said that, arrow functions were introduced in PHP 7.4, four years after the change this sniff checks for entered PHP, so anyone who uses any of the target functions in the body of an arrow function should expect the "new" behaviour.

With this in mind, this commit explicit does NOT add checking of arrow functions to the sniff.

However, it does change the sniff to prevent false positives on any of the target functions being seen within the body of an arrow function.

Includes tests.
…ves on PHP 8.0+ attributes

`T_STRING` tokens in PHP 8.0+ attributes are either class names or possibly constant names (as a parameter for the class instantiation).
They are never function calls.

This commit ensures that `T_STRING` tokens in attributes are not confused with function calls.

Includes tests.
…::class

If a the class name of an object parameter is retrieved using the `$obj::class` syntax, we know the parameter is not being changed, so we can safely ignore it.

Includes tests.
…8.0+ named arguments in function calls

For the `array_*()` functions to was incidentally and partially added in some of the previous commits, though the code still needed to be adjusted to allow for an unexpected parameter order.
For the "target functions", support was not handled so far, so this commit adds support for named parameters.

Verified the parameter name used is in line with the name as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) is the only relevant name.

Name verification reference:
* `func_get_arg`: https://3v4l.org/liZU1#veol
* `debug_backtrace`: https://3v4l.org/sFqA7#veol
* `debug_print_backtrace`: https://3v4l.org/LtnLq#veol
* `array_slice`: https://3v4l.org/7hUY7#veol
* `array_splice`: https://3v4l.org/XU9KW#veol

Includes adding the unit tests to include tests using named parameters.

Related to 1239

:point_right: This commit will be more straight-forward to review while ignoring whitespace changes.
…0+ constructor property promotion

Shouldn't have any impact, just safeguarding this with a test.
…e as PHP 8.1+ first class callable

Using the `func_get_arg*()` functions as first class callables is not allowed in PHP. See: https://3v4l.org/ISt2r and https://3v4l.org/aTjjQ

However, using the `debug_*backtrace()` function as first class callable is allowed, but seems like a really bad idea....
See: https://3v4l.org/j8lWg and https://3v4l.org/8qbJF

This commit adds handling for this by:
* Ignoring the use of `func_get_arg*()` functions when used as first class callables. That's forbidden by PHP, so not our concern.
* And flagging the use of `debug_*backtrace()` functions when used as first class callables with a warning with its own error code.

Includes tests.
@jrfnl jrfnl added this to the 10.0.0 milestone Aug 27, 2025
Base automatically changed from feature/tokengroup-isnumber-allow-for-php-7.4-and-8.1-numeric-literals to develop September 1, 2025 16:03
@jrfnl jrfnl marked this pull request as ready for review September 1, 2025 16:29
@wimg wimg merged commit e211f42 into develop Sep 1, 2025
49 checks passed
@wimg wimg deleted the feature/argumentfunctionsreportcurrentvalue-various-improvements branch September 1, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants