Skip to content

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
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-p6ptcmu
Open

Report calls to strtr(), str_replace() and other replace functions that cannot change their subject#6423
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-p6ptcmu

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

strtr('\\', '/', $path) — the swapped-argument bug from
composer/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 no
replacement can ever happen regardless of what $path holds.

This PR adds StringReplaceWithoutEffectRule, which reports calls to the replace-function
family that provably cannot change the subject they're given. It is registered at level 4
behind the stringReplaceWithoutEffect bleeding-edge feature toggle, mirroring how
SortWithoutEffectRule is shipped.

Changes

  • New rule src/Rules/Functions/StringReplaceWithoutEffectRule.php.
  • Registered in conf/config.level4.neon with the stringReplaceWithoutEffect toggle
    declared in conf/config.neon, conf/parametersSchema.neon and enabled in
    conf/bleedingEdge.neon.
  • Test data tests/PHPStan/Rules/Functions/data/string-replace-without-effect.php and
    .../data/string-replace-without-effect-phpdoc-types.php, test class
    tests/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):

Function Reported when
strtr($string, $from, $to) $from is ''; $to is ''; $from/$to map every character to itself; constant $string shares no character with constant $from
strtr($string, $pairs) $pairs is empty; every pair maps a string to itself; constant $string contains none of the constant keys
str_replace() $search is an empty array or empty string; $search is identical to $replace; constant $subject contains none of the constant $search values
str_ireplace() same, except the identical-$search/$replace case (str_ireplace('A', 'A', $s) does rewrite lowercase a), and containment is checked case-insensitively
substr_replace() $replace is '' and $length is 0
preg_replace(), preg_replace_callback(), preg_replace_callback_array() the pattern array is empty

Siblings probed and deliberately left out:

  • Non-empty 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 identical
    text 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 the
first min(strlen($from), strlen($to)) characters of $from, so $to can only shrink
the set of replaced characters. Checking the whole $from against the subject is therefore
sound even when $to is completely unknown — which is what makes the reported
strtr('\\', '/', $path) provable despite $path being an arbitrary string. The same
observation gives the identity check: if the common prefix of $from and $to is equal,
every mapped character maps to itself.

Calls passing the by-reference $count argument are skipped, because those write 0 to it
even when nothing is replaced, so the call is not without effect.

Test

StringReplaceWithoutEffectRuleTest::testRule analyses a data file that opens with the
verbatim reproducer from the issue (strtr('\\', '/', $path) and
rtrim(strtr('\\', '/', $path), '/'), both lifted from the composer commit) and asserts the
new 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 a
non-zero $length or a missing $length, a non-empty preg_replace_callback_array() and
every call passing $count. Named-argument calls are covered for both strtr() and
str_replace().

testRuleWithoutTreatPhpDocTypesAsCertain checks that findings resting only on PHPDoc types
disappear with treatPhpDocTypesAsCertain: false, and the main test asserts the
corresponding tip is attached when it is on.

Fixes phpstan/phpstan#11118

…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>
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.

2 participants