[staabm]

closes #347

[staabm]

only fixing the most valuable and least risky for now

[staabm]

this case shows why we don't try to auto-fix numeric-string based haystack/needle.

[Copilot]

🟡 Changes recommended

The fix can discard evaluation of an existing strict expression and alter subsequent behavior, so side-effecting expressions must remain unfixable, with regression coverage for positional and named arguments.

1 open issue

• Fix removes side effects from the existing strict argument · New

What changed in this PR

Adds automatic fixes to StrictFunctionCallsRule for selected array-search calls, addressing #347 while retaining existing strictness diagnostics.

Changes:

• Adds or replaces strict: true when native needle and haystack value types are both integers or both booleans.
• Supports positional and named arguments, while skipping unpacked calls and excluding unrelated functions.
• Adds fix tests covering eligible calls and cases that should remain unchanged.

File	Description
tests/​Rules/​StrictCalls/​StrictFunctionCallsRuleTest.php	Adds positional and named-argument fix tests.
tests/​Rules/​StrictCalls/​data/​strict-function-calls-named-arguments.php.fixed	Defines expected named-argument fixes.
tests/​Rules/​StrictCalls/​data/​strict-function-calls-named-arguments.php	Supplies named-argument and native-type eligibility cases.
tests/​Rules/​StrictCalls/​data/​strict-function-calls-fixable.php.fixed	Defines expected positional fixes and unchanged calls.
tests/​Rules/​StrictCalls/​data/​strict-function-calls-fixable.php	Supplies eligible calls and excluded comparison cases.
src/​Rules/​StrictCalls/​StrictFunctionCallsRule.php	Adds type-based fix eligibility and strict-argument rewriting.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The scalar whitelist permits interpolated strings, so replacing them with true can remove side effects such as __get() calls and change program behavior.

1 open issue

• Exclude interpolated strings from the scalar argument whitelist · New

1 resolved since last review

• Fix removes side effects from the existing strict argument

_{Review effort: Balanced}

[staabm]

//cc @SanderMuller

[SanderMuller]

I checked out b972f1a and ran the tests, the PHPStan check and a set of extra cases. The fix logic held in every case I tried. I found one fixture problem and one performance cost.

Fixture lines that do not compile. In strict-function-calls-named-arguments.php, lines 25 and 29 put a positional argument after named ones. PHP rejects that with "Cannot use positional argument after named argument", so the two Makefile lint exclusions exist only for these lines. Line 25 is the side-effect case, so that case is tested only in code that cannot run. Suggestion: write line 25 as strict: returnsFalseWithSideEffect(), drop line 29, and remove the exclusions. The // lint >= 8.2 header already skips the file on older PHP versions.

Performance. For every fixable error, PHPStan builds the fix during analysis: RuleErrorTransformer::transform() calls fix(), which processes the whole file. This also happens for errors in ignoreErrors or a baseline. This PR makes many existing errors fixable, so projects with many reported or baselined non-strict calls pay this cost. Timings for one file, PHPStan 2.3.5 phar, PHP 8.5, macOS arm64:

File	Base	PR
1816 lines, 10 fixable calls (phpstan-src 2.3.x source, 3 rounds)	0.77-0.78 s	1.33-1.34 s
100 lines, 300 errors	0.77-0.79 s	3.52-3.55 s
200 lines, 600 errors	0.77-0.78 s	12.86-13.02 s
200 lines, 600 errors, all in ignoreErrors	0.79 s	12.92 s

A file with one fixable call shows no difference. The cost is in phpstan-src, not in this rule, and phpstan/phpstan-src#6934 reduces it. With that PR, the 1816-line file takes 0.76-0.78 s with either rule, and the 600-error file takes 6.87 to 7.38 s instead of 12.97 to 13.45 s (two runs). Whether to merge this before or after that change is your decision.

Checked, no problem found:

• testFix and testFixNamedArguments fail without the change to StrictFunctionCallsRule.
• PHPStan does not mark an error as fixable when the fixer returns the call unchanged. Examples: a variable or a function call as strict, an unpacked argument.
• Not fixed: types from PHPDoc only (@var list<int> property, @var int variable) and static::X or $this::X constants.
• Not fixed: haystack strings that PHP can read as numbers ('1 ', ' 1', '1e3').
• Fixed correctly: self::X, final constants, '', '0x1A' and 'INF' in the haystack, is_int() narrowing, a list built in the function, and \false/FALSE.
• Named arguments in any order are fixed correctly, including array_keys([1, 2], strict: false, filter_value: $i) and in_array(haystack: [1, 2], needle: $i).
• composer.json requires phpstan/phpstan ^2.3, which has fixNode().

Tests (77) and phpstan analyse -l 8 pass on the PR head, and all 49 CI checks pass. I did not test on PHP 7.4.

[staabm]

Performance. For every fixable error, PHPStan builds the fix during analysis: RuleErrorTransformer::transform() calls fix(), which processes the whole file. This also happens for errors in ignoreErrors or a baseline. This PR makes many existing errors fixable, so projects with many reported or baselined non-strict calls pay this cost.

thats interessting. lets wait a bit whether we can speedup the fixing use-case in phpstan-src a bit

[VincentLanglet]

Performance. For every fixable error, PHPStan builds the fix during analysis: RuleErrorTransformer::transform() calls fix(), which processes the whole file. This also happens for errors in ignoreErrors or a baseline. This PR makes many existing errors fixable, so projects with many reported or baselined non-strict calls pay this cost.

thats interessting. lets wait a bit whether we can speedup the fixing use-case in phpstan-src a bit

I personally don't use PHPStan for auto fixing code, I prefer to fix it myself.

If I understand correctly the more we make rule autofixable, the slower PHPStan will run just for analyze ?

[staabm]

If I understand correctly the more we make rule autofixable, the slower PHPStan will run just for analyze ?

yeah thats what I understand. its a pretty new feature, so lets see whether @ondrejmirtes has ideas so it does not matter much perf wise