Report calls to strtr(), str_replace() and other replace functions that cannot change their subject - #6423
Open
phpstan-bot wants to merge 1 commit into
Open
Conversation
…s that cannot change their subject
* New `PHPStan\Rules\Functions\StringReplaceWithoutEffectRule` (level 4, behind the
`stringReplaceWithoutEffect` bleeding-edge feature toggle) reporting replace-function
calls that provably return their subject unchanged.
* `strtr($string, $from, $to)`: reports an empty `$from`/`$to`, a `$from`/`$to` pair that
maps every character to itself, and a constant `$string` sharing no character with a
constant `$from`. The last one catches the swapped-argument bug from the issue
(`strtr('\\', '/', $path)`); checking the whole `$from` is sound because `$to` can only
shorten the effective prefix.
* `strtr($string, $pairs)`: reports an empty `$pairs` array, pairs that map every string to
itself, and a constant `$string` containing none of the constant keys.
* `str_replace()`/`str_ireplace()`: reports an empty `$search`, a `$search` identical to
`$replace` (case-sensitive variant only, since `str_ireplace('A', 'A', …)` does rewrite
lowercase `a`), and a constant `$subject` containing none of the constant `$search`
values. Array subjects and array needles are handled too.
* `substr_replace()`: reports an empty `$replace` combined with a zero `$length`.
* `preg_replace()`/`preg_replace_callback()`/`preg_replace_callback_array()`: reports an
empty pattern array.
* Calls passing the by-reference `$count` argument are skipped - they write to it even when
nothing is replaced.
* Follows `SortWithoutEffectRule` for `treatPhpDocTypesAsCertain` handling, named-argument
reordering via `ArgumentsNormalizer` and parameter names taken from the selected
`ParametersAcceptor`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
strtr('\\', '/', $path)— the swapped-argument bug fromcomposer/class-map-generator@5bb7f1e
that inspired the issue — analysed cleanly. PHPStan can prove such a call returns its
subject unchanged: the literal
'\'shares no character with the literal'/', so noreplacement can ever happen regardless of what
$pathholds.This PR adds
StringReplaceWithoutEffectRule, which reports calls to the replace-functionfamily that provably cannot change the subject they're given. It is registered at level 4
behind the
stringReplaceWithoutEffectbleeding-edge feature toggle, mirroring howSortWithoutEffectRuleis shipped.Changes
src/Rules/Functions/StringReplaceWithoutEffectRule.php.conf/config.level4.neonwith thestringReplaceWithoutEffecttoggledeclared in
conf/config.neon,conf/parametersSchema.neonand enabled inconf/bleedingEdge.neon.tests/PHPStan/Rules/Functions/data/string-replace-without-effect.phpand.../data/string-replace-without-effect-phpdoc-types.php, test classtests/PHPStan/Rules/Functions/StringReplaceWithoutEffectRuleTest.php.Cases covered, one per member of the family (the conceptual axis here is "functions that
replace parts of a subject string", which is exactly the set PHPStan already models in
ReplaceFunctionsDynamicReturnTypeExtension):strtr($string, $from, $to)$fromis'';$tois'';$from/$tomap every character to itself; constant$stringshares no character with constant$fromstrtr($string, $pairs)$pairsis empty; every pair maps a string to itself; constant$stringcontains none of the constant keysstr_replace()$searchis an empty array or empty string;$searchis identical to$replace; constant$subjectcontains none of the constant$searchvaluesstr_ireplace()$search/$replacecase (str_ireplace('A', 'A', $s)does rewrite lowercasea), and containment is checked case-insensitivelysubstr_replace()$replaceis''and$lengthis0preg_replace(),preg_replace_callback(),preg_replace_callback_array()Siblings probed and deliberately left out:
preg_*patterns — deciding "this regex cannot match this constant subject"needs the regex engine and is a much bigger change than the rest of the family.
substr_replace()with a non-zero$length— whether a range is replaced by identicaltext depends on offset arithmetic that only rarely resolves to constants.
Root cause
Not a wrong inference — a missing check. All these functions share the same shape: a
subject, a set of needles and a set of replacements. Whenever the needles provably do not
occur in the subject (or the replacement set is empty, or every needle maps to itself), the
call is a no-op and almost always signals a typo or swapped arguments. PHPStan already
knows all the constant strings involved; nothing was asking the question.
The interesting piece of reasoning is
strtr()'s three-argument form. PHP only uses thefirst
min(strlen($from), strlen($to))characters of$from, so$tocan only shrinkthe set of replaced characters. Checking the whole
$fromagainst the subject is thereforesound even when
$tois completely unknown — which is what makes the reportedstrtr('\\', '/', $path)provable despite$pathbeing an arbitrarystring. The sameobservation gives the identity check: if the common prefix of
$fromand$tois equal,every mapped character maps to itself.
Calls passing the by-reference
$countargument are skipped, because those write0to iteven when nothing is replaced, so the call is not without effect.
Test
StringReplaceWithoutEffectRuleTest::testRuleanalyses a data file that opens with theverbatim reproducer from the issue (
strtr('\\', '/', $path)andrtrim(strtr('\\', '/', $path), '/'), both lifted from the composer commit) and asserts thenew error. Without the rule the file analyses clean, so the test fails before the change.
The same data file pins every other branch of the rule and, just as importantly, the
negative cases that must stay silent: the correct argument order, non-constant
$from,subjects that do contain the needle,
str_ireplace('A', 'A', $s),substr_replace()with anon-zero
$lengthor a missing$length, a non-emptypreg_replace_callback_array()andevery call passing
$count. Named-argument calls are covered for bothstrtr()andstr_replace().testRuleWithoutTreatPhpDocTypesAsCertainchecks that findings resting only on PHPDoc typesdisappear with
treatPhpDocTypesAsCertain: false, and the main test asserts thecorresponding tip is attached when it is on.
Fixes phpstan/phpstan#11118