Skip to content

Commit

Permalink
Check array functions which require stringish values
Browse files Browse the repository at this point in the history
  • Loading branch information
schlndh authored Jun 10, 2024
1 parent 27cbdd4 commit 7896011
Show file tree
Hide file tree
Showing 16 changed files with 803 additions and 2 deletions.
1 change: 1 addition & 0 deletions conf/bleedingEdge.neon
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,6 @@ parameters:
magicConstantOutOfContext: true
paramOutType: true
pure: true
checkParameterCastableToStringFunctions: true
stubFiles:
- ../stubs/bleedingEdge/Rule.stub
11 changes: 10 additions & 1 deletion conf/config.level5.neon
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ conditionalTags:
phpstan.rules.rule: %featureToggles.arrayValues%
PHPStan\Rules\Functions\CallUserFuncRule:
phpstan.rules.rule: %featureToggles.callUserFunc%
PHPStan\Rules\Functions\ParameterCastableToStringFunctionRule:
phpstan.rules.rule: %featureToggles.checkParameterCastableToStringFunctions%

rules:
- PHPStan\Rules\DateTimeInstantiationRule
- PHPStan\Rules\Functions\ImplodeFunctionRule

services:
-
Expand All @@ -37,3 +38,11 @@ services:

-
class: PHPStan\Rules\Functions\CallUserFuncRule
-
class: PHPStan\Rules\Functions\ImplodeFunctionRule
arguments:
disabled: %featureToggles.checkParameterCastableToStringFunctions%
tags:
- phpstan.rules.rule
-
class: PHPStan\Rules\Functions\ParameterCastableToStringFunctionRule
1 change: 1 addition & 0 deletions conf/config.neon
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ parameters:
magicConstantOutOfContext: false
paramOutType: false
pure: false
checkParameterCastableToStringFunctions: 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 @@ -83,6 +83,7 @@ parametersSchema:
magicConstantOutOfContext: bool()
paramOutType: bool()
pure: bool()
checkParameterCastableToStringFunctions: bool()
])
fileExtensions: listOf(string())
checkAdvancedIsset: bool()
Expand Down
16 changes: 16 additions & 0 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -1825,6 +1825,22 @@ parameters:
count: 1
path: tests/PHPStan/Rules/DeadCode/NoopRuleTest.php

-
message: """
#^Instantiation of deprecated class PHPStan\\\\Rules\\\\Functions\\\\ImplodeFunctionRule\\:
Replaced by PHPStan\\\\Rules\\\\Functions\\\\ParameterCastableToStringFunctionRule$#
"""
count: 1
path: tests/PHPStan/Rules/Functions/ImplodeFunctionRuleTest.php

-
message: """
#^Return type of method PHPStan\\\\Rules\\\\Functions\\\\ImplodeFunctionRuleTest\\:\\:getRule\\(\\) has typehint with deprecated class PHPStan\\\\Rules\\\\Functions\\\\ImplodeFunctionRule\\:
Replaced by PHPStan\\\\Rules\\\\Functions\\\\ParameterCastableToStringFunctionRule$#
"""
count: 1
path: tests/PHPStan/Rules/Functions/ImplodeFunctionRuleTest.php

-
message: "#^PHPDoc tag @var assumes the expression with type PHPStan\\\\Type\\\\Generic\\\\TemplateType is always PHPStan\\\\Type\\\\Generic\\\\TemplateMixedType but it's error\\-prone and dangerous\\.$#"
count: 1
Expand Down
6 changes: 6 additions & 0 deletions src/Rules/Functions/ImplodeFunctionRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use function sprintf;

/**
* @deprecated Replaced by PHPStan\Rules\Functions\ParameterCastableToStringFunctionRule
* @implements Rule<Node\Expr\FuncCall>
*/
class ImplodeFunctionRule implements Rule
Expand All @@ -25,6 +26,7 @@ class ImplodeFunctionRule implements Rule
public function __construct(
private ReflectionProvider $reflectionProvider,
private RuleLevelHelper $ruleLevelHelper,
private bool $disabled,
)
{
}
Expand All @@ -36,6 +38,10 @@ public function getNodeType(): string

public function processNode(Node $node, Scope $scope): array
{
if ($this->disabled) {
return [];
}

if (!($node->name instanceof Node\Name)) {
return [];
}
Expand Down
170 changes: 170 additions & 0 deletions src/Rules/Functions/ParameterCastableToStringFunctionRule.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
<?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\Reflection\ParametersAcceptorSelector;
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Rules\RuleLevelHelper;
use PHPStan\Type\ErrorType;
use PHPStan\Type\Type;
use PHPStan\Type\VerbosityLevel;
use function array_key_exists;
use function count;
use function in_array;
use function sprintf;

/**
* @implements Rule<Node\Expr\FuncCall>
*/
class ParameterCastableToStringFunctionRule implements Rule
{

public function __construct(
private ReflectionProvider $reflectionProvider,
private RuleLevelHelper $ruleLevelHelper,
)
{
}

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

public function processNode(Node $node, Scope $scope): array
{
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();
$implodeFunctions = ['implode', 'join'];
$checkAllArgsFunctions = ['array_intersect', 'array_intersect_assoc', 'array_diff', 'array_diff_assoc'];
$checkFirstArgFunctions = [
'array_unique',
'array_combine',
'sort',
'rsort',
'asort',
'arsort',
'natcasesort',
'natsort',
'array_count_values',
'array_fill_keys',
];

if (
!in_array($functionName, $checkAllArgsFunctions, true)
&& !in_array($functionName, $checkFirstArgFunctions, true)
&& !in_array($functionName, $implodeFunctions, true)
) {
return [];
}

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

$errorMessage = 'Parameter %s of function %s expects an array of values castable to string, %s given.';
$getNormalizedArgs = static function () use ($parametersAcceptor, $node): array {
$normalizedFuncCall = ArgumentsNormalizer::reorderFuncArguments($parametersAcceptor, $node);

if ($normalizedFuncCall === null) {
return [];
}

return $normalizedFuncCall->getArgs();
};
if (in_array($functionName, $implodeFunctions, true)) {
$normalizedArgs = $getNormalizedArgs();
$errorMessage = 'Parameter %s of function %s expects array<string>, %s given.';
if (count($normalizedArgs) === 1) {
$argsToCheck = [0 => $normalizedArgs[0]];
} elseif (count($normalizedArgs) === 2) {
$argsToCheck = [1 => $normalizedArgs[1]];
} else {
return [];
}
} elseif (in_array($functionName, $checkAllArgsFunctions, true)) {
$argsToCheck = $origArgs;
} elseif (in_array($functionName, $checkFirstArgFunctions, true)) {
$normalizedArgs = $getNormalizedArgs();
if ($normalizedArgs === []) {
return [];
}
$argsToCheck = [0 => $normalizedArgs[0]];
} else {
return [];
}

$origNamedArgs = [];
foreach ($origArgs as $arg) {
if ($arg->unpack || $arg->name === null) {
continue;
}

$origNamedArgs[$arg->name->toString()] = $arg;
}

$errors = [];
$functionParameters = $parametersAcceptor->getParameters();

foreach ($argsToCheck as $argIdx => $arg) {
if ($arg->unpack) {
continue;
}

$typeResult = $this->ruleLevelHelper->findTypeToCheck(
$scope,
$arg->value,
'',
static fn (Type $type): bool => !$type->getIterableValueType()->toString() instanceof ErrorType,
);

if ($typeResult->getType() instanceof ErrorType
|| !$typeResult->getType()->getIterableValueType()->toString() instanceof ErrorType) {
continue;
}

if (in_array($functionName, $implodeFunctions, true)) {
// implode has weird variants, so $array has to be fixed. It's especially weird with named arguments.
if (array_key_exists('array', $origNamedArgs)) {
$argName = '$array';
} elseif (array_key_exists('separator', $origNamedArgs) && count($origArgs) === 1) {
$argName = '$separator';
} else {
$argName = sprintf('#%d $array', $argIdx + 1);
}
} elseif (array_key_exists($argIdx, $functionParameters)) {
$paramName = $functionParameters[$argIdx]->getName();
$argName = array_key_exists($paramName, $origNamedArgs)
? sprintf('$%s', $paramName)
: sprintf('#%d $%s', $argIdx + 1, $paramName);
} else {
$argName = sprintf('#%d', $argIdx + 1);
}

$errors[] = RuleErrorBuilder::message(
sprintf($errorMessage, $argName, $functionName, $typeResult->getType()->describe(VerbosityLevel::typeOnly())),
)->identifier('argument.type')->build();
}

return $errors;
}

}
2 changes: 1 addition & 1 deletion tests/PHPStan/Rules/Functions/ImplodeFunctionRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class ImplodeFunctionRuleTest extends RuleTestCase
protected function getRule(): Rule
{
$broker = $this->createReflectionProvider();
return new ImplodeFunctionRule($broker, new RuleLevelHelper($broker, true, false, true, false, false, true, false));
return new ImplodeFunctionRule($broker, new RuleLevelHelper($broker, true, false, true, false, false, true, false), false);
}

public function testFile(): void
Expand Down
Loading

0 comments on commit 7896011

Please sign in to comment.