Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions conf/bleedingEdge.neon
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,4 @@ parameters:
unusedParameters: true
reuseCollectedDataRuleErrors: true
checkWholePhpVersionRange: true
replaceCountMismatch: true
1 change: 1 addition & 0 deletions conf/config.neon
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ parameters:
unusedParameters: false
reuseCollectedDataRuleErrors: false
checkWholePhpVersionRange: false
replaceCountMismatch: false
fileExtensions:
- php
checkAdvancedIsset: false
Expand Down
1 change: 1 addition & 0 deletions conf/parametersSchema.neon
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ parametersSchema:
unusedParameters: bool()
reuseCollectedDataRuleErrors: bool()
checkWholePhpVersionRange: bool()
replaceCountMismatch: bool()
])
fileExtensions: listOf(string())
checkAdvancedIsset: bool()
Expand Down
141 changes: 141 additions & 0 deletions src/Rules/Functions/ReplaceCountMismatchRule.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\Functions;

use PhpParser\Node;
use PhpParser\Node\Expr\FuncCall;
use PHPStan\Analyser\ArgumentsNormalizer;
use PHPStan\Analyser\Scope;
use PHPStan\DependencyInjection\AutowiredParameter;
use PHPStan\DependencyInjection\Conditional;
use PHPStan\DependencyInjection\RegisteredRule;
use PHPStan\Php\PhpVersion;
use PHPStan\Reflection\ParametersAcceptorSelector;
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Type;
use function array_key_exists;
use function count;
use function sprintf;

/**
* Reports replace functions called with an array of replacements that is shorter
* than the array of search values. PHP silently replaces the remaining search values
* with an empty string, which is usually a mistake.
*
* @implements Rule<Node\Expr\FuncCall>
*/
#[Conditional(
attribute: new RegisteredRule(level: 5),
if: '%featureToggles.replaceCountMismatch%',
)]
final class ReplaceCountMismatchRule implements Rule
{

private const FUNCTIONS = [
'str_replace' => true,
'str_ireplace' => true,
'preg_replace' => true,
'preg_filter' => true,
];

public function __construct(
private ReflectionProvider $reflectionProvider,
private PhpVersion $phpVersion,
#[AutowiredParameter]
private bool $treatPhpDocTypesAsCertain,
#[AutowiredParameter(ref: '%tips.treatPhpDocTypesAsCertain%')]
private bool $treatPhpDocTypesAsCertainTip,
)
{
}

public function getNodeType(): string
{
return FuncCall::class;
}

public function processNode(Node $node, Scope $scope): array
{
if (count($node->getArgs()) < 2) {
return [];
}

if (!($node->name instanceof Node\Name)) {
return [];
}

if (!$this->reflectionProvider->hasFunction($node->name, $scope)) {
return [];
}

$functionReflection = $this->reflectionProvider->getFunction($node->name, $scope);
$functionName = $functionReflection->getName();
if (!array_key_exists($functionName, self::FUNCTIONS)) {
return [];
}

$parametersAcceptor = ParametersAcceptorSelector::selectFromArgs(
$scope,
$node->getArgs(),
$functionReflection->getVariants(),
$functionReflection->getNamedArgumentsVariants(),
);

$normalizedFuncCall = ArgumentsNormalizer::reorderFuncArguments($parametersAcceptor, $node);
if ($normalizedFuncCall === null) {
return [];
}

// named arguments can leave a gap in the normalized arguments when a required one is missing
$args = $normalizedFuncCall->getArgs();
if (!array_key_exists(0, $args) || !array_key_exists(1, $args)) {
return [];
}
Comment on lines +92 to +95

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check the args count instead and do so very early in the method - because its fast

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made all three changes staabm asked for in three local commits (not pushed). make tests, make phpstan and make cs all pass.

  1. Bail out early on argument count (fbd86667f): the rule now returns at the very start of processNode() when the call has fewer than two arguments, before looking up the function. I kept the existing key check after the arguments are reordered. With named arguments that leave out search or replace, the reordered list can have a gap at position 0 or 1, and a plain count wouldn't catch that.
  2. Identifier (75d824b6f): every reported call now uses argument.replaceCount. The strReplace/pregReplace names per function are gone, so FUNCTIONS is now just a lookup list.
  3. Error message (0bcc27082): the message is now Call to %s is using fewer replacements than search values., filled with the function name (e.g. Call to str_replace is using …). That made the parameter-name lookup and the size output unused, so I removed them, and updated the expected messages in ReplaceCountMismatchRuleTest.

The PR description still shows the old message and the strReplace.replaceCount / pregReplace.replaceCount identifiers, so it should be updated when these are pushed.


if ($this->treatPhpDocTypesAsCertain) {
$searchType = $scope->getType($args[0]->value);
$replaceType = $scope->getType($args[1]->value);
} else {
$searchType = $scope->getNativeType($args[0]->value);
$replaceType = $scope->getNativeType($args[1]->value);
}

if (!$this->hasFewerReplacements($searchType, $replaceType)) {
return [];
}

$errorBuilder = RuleErrorBuilder::message(sprintf(
'Call to %s is using fewer replacements than search values.',
$functionName,
))->identifier('argument.replaceCount');

if ($this->treatPhpDocTypesAsCertain && $this->treatPhpDocTypesAsCertainTip) {
$nativeSearchType = $scope->getNativeType($args[0]->value);
$nativeReplaceType = $scope->getNativeType($args[1]->value);
if (!$this->hasFewerReplacements($nativeSearchType, $nativeReplaceType)) {
$errorBuilder->treatPhpDocTypesAsCertainTip();
}
}

return [
$errorBuilder->build(),
];
}

private function hasFewerReplacements(Type $searchType, Type $replaceType): bool
{
if (!$searchType->isArray()->yes() || !$replaceType->isArray()->yes()) {
return false;
}

// an explicitly empty array of replacements is a deliberate way to remove all search values
if ($replaceType->isIterableAtLeastOnce()->no()) {
return false;
}

return $replaceType->getArraySize()->isSmallerThan($searchType->getArraySize(), $this->phpVersion)->yes();
}

}
65 changes: 65 additions & 0 deletions tests/PHPStan/Rules/Functions/ReplaceCountMismatchRuleTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\Functions;

use PHPStan\Php\PhpVersion;
use PHPStan\Rules\Rule;
use PHPStan\Testing\RuleTestCase;
use PHPUnit\Framework\Attributes\RequiresPhp;
use const PHP_VERSION_ID;

/**
* @extends RuleTestCase<ReplaceCountMismatchRule>
*/
class ReplaceCountMismatchRuleTest extends RuleTestCase
{

protected function getRule(): Rule
{
return new ReplaceCountMismatchRule(
self::createReflectionProvider(),
new PhpVersion(PHP_VERSION_ID),
true,
true,
);
}

public function testBug3889(): void
{
$this->analyse([__DIR__ . '/data/bug-3889.php'], [
[
'Call to str_replace is using fewer replacements than search values.',
12,
],
[
'Call to str_ireplace is using fewer replacements than search values.',
36,
],
[
'Call to str_replace is using fewer replacements than search values.',
40,
'Because the type is coming from a PHPDoc, you can turn off this check by setting <fg=cyan>treatPhpDocTypesAsCertain: false</> in your <fg=cyan>%configurationFile%</>.',
],
[
'Call to preg_replace is using fewer replacements than search values.',
46,
],
[
'Call to preg_filter is using fewer replacements than search values.',
49,
],
]);
}

#[RequiresPhp('>= 8.0')]
public function testNamedArgs(): void
{
$this->analyse([__DIR__ . '/data/bug-3889-named-args.php'], [
[
'Call to str_replace is using fewer replacements than search values.',
9,
],
]);
}

}
11 changes: 11 additions & 0 deletions tests/PHPStan/Rules/Functions/data/bug-3889-named-args.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php // lint >= 8.0

declare(strict_types = 1);

namespace Bug3889NamedArgs;

function doFoo(string $s): void
{
str_replace(search: ['a', 'b'], subject: $s, replace: ['x']);
str_replace(search: ['a', 'b'], subject: $s, replace: ['x', 'y']);
}
51 changes: 51 additions & 0 deletions tests/PHPStan/Rules/Functions/data/bug-3889.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php declare(strict_types = 1);

namespace Bug3889;

class HelloWorld
{
public function sayHello(): void
{
$str = 'Hello';
$search = ['a', 'b', 'c'];
$replace = ['x', 'y'];
echo str_replace($search, $replace, $str);
}

public function replaceIsString(): void
{
$str = 'Hello';
$search = ['a', 'b', 'c'];
echo str_replace($search, 'foo', $str);
}
}

class Variants
{
/**
* @param list<string> $list
* @param non-empty-list<string> $nonEmptyList
* @param array{string, string} $two
* @param array{string, string, string}|array{string} $oneOrThree
*/
public function doFoo(string $s, array $list, array $nonEmptyList, array $two, array $oneOrThree): void
{
str_replace(['a', 'b'], ['x', 'y'], $s);
str_replace(['a', 'b'], ['x', 'y', 'z'], $s);
str_replace(['a', 'b'], [], $s);
str_ireplace(['a', 'b', 'c'], ['x'], $s);
str_replace('a', 'x', $s);
str_replace($list, ['x'], $s);
str_replace($nonEmptyList, ['x'], $s);
str_replace($two, ['x'], $s);
str_replace($two, $nonEmptyList, $s);
str_replace($oneOrThree, ['x'], $s);
str_replace($oneOrThree, [], $s);
str_replace(['a', 'b', 'c'], $oneOrThree, $s);

preg_replace(['/a/', '/b/'], ['x'], $s);
preg_replace(['/a/', '/b/'], ['x', 'y'], $s);
preg_replace(['/a/', '/b/'], 'x', $s);
preg_filter(['/a/', '/b/'], ['x'], $s);
}
}
Loading