Skip to content

Specfiy root expression from type specifying extensions #1516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Specfiy root expression from type specifying extensions #1516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 20 additions & 3 deletions src/Analyser/TypeSpecifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ public function specifyTypesInCondition(
continue;
}

return $extension->specifyTypes($functionReflection, $expr, $scope, $context);
return $this->specifyRootExpr($extension->specifyTypes($functionReflection, $expr, $scope, $context), $scope, $context);
}

if (count($expr->getArgs()) > 0) {
Expand Down Expand Up @@ -719,7 +719,7 @@ public function specifyTypesInCondition(
continue;
}

return $extension->specifyTypes($methodReflection, $expr, $scope, $context);
return $this->specifyRootExpr($extension->specifyTypes($methodReflection, $expr, $scope, $context), $scope, $context);
}

if (count($expr->getArgs()) > 0) {
Expand Down Expand Up @@ -753,7 +753,7 @@ public function specifyTypesInCondition(
continue;
}

return $extension->specifyTypes($staticMethodReflection, $expr, $scope, $context);
return $this->specifyRootExpr($extension->specifyTypes($staticMethodReflection, $expr, $scope, $context), $scope, $context);
}
}

Expand Down Expand Up @@ -1421,4 +1421,21 @@ private function getTypeSpecifyingExtensionsForType(array $extensions, string $c
return array_merge(...$extensionsForClass);
}

private function specifyRootExpr(SpecifiedTypes $specifiedTypes, Scope $scope, TypeSpecifierContext $context): SpecifiedTypes
{
$rootExpr = $specifiedTypes->getRootExpr();
if ($rootExpr === null || $context->null()) {
return $specifiedTypes;
}

$rootExprType = $scope->getType($rootExpr);
if (!$rootExprType instanceof BooleanType || $rootExprType instanceof ConstantBooleanType) {
return $specifiedTypes;
}

return $specifiedTypes->unionWith(
$this->create($rootExpr, new ConstantBooleanType(true), $context),
);
}

}
43 changes: 22 additions & 21 deletions src/Type/Php/StrContainingTypeSpecifyingExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

namespace PHPStan\Type\Php;

use PhpParser\Node\Arg;
use PhpParser\Node\Expr\BinaryOp\BooleanAnd;
use PhpParser\Node\Expr\BinaryOp\NotIdentical;
use PhpParser\Node\Expr\FuncCall;
Expand All @@ -27,17 +26,17 @@
final class StrContainingTypeSpecifyingExtension implements FunctionTypeSpecifyingExtension, TypeSpecifierAwareExtension
{

/** @var array<string, array{0: int, 1: int}> */
/** @var array<string, array{0: int, 1: int, 2: bool}> */
private array $strContainingFunctions = [
'fnmatch' => [1, 0],
'str_contains' => [0, 1],
'str_starts_with' => [0, 1],
'str_ends_with' => [0, 1],
'strpos' => [0, 1],
'strrpos' => [0, 1],
'stripos' => [0, 1],
'strripos' => [0, 1],
'strstr' => [0, 1],
'fnmatch' => [1, 0, true],
'str_contains' => [0, 1, true],
'str_starts_with' => [0, 1, true],
'str_ends_with' => [0, 1, true],
'strpos' => [0, 1, false],
'strrpos' => [0, 1, false],
'stripos' => [0, 1, false],
'strripos' => [0, 1, false],
'strstr' => [0, 1, false],
];

private TypeSpecifier $typeSpecifier;
Expand All @@ -58,7 +57,7 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n
$args = $node->getArgs();

if (count($args) >= 2) {
[$hackstackArg, $needleArg] = $this->strContainingFunctions[strtolower($functionReflection->getName())];
[$hackstackArg, $needleArg, $evaluatesToBoolean] = $this->strContainingFunctions[strtolower($functionReflection->getName())];

$haystackType = $scope->getType($args[$hackstackArg]->value);
$needleType = $scope->getType($args[$needleArg]->value);
Expand All @@ -76,21 +75,23 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n
$accessories[] = new AccessoryNumericStringType();
}

$rootExpr = $evaluatesToBoolean
? new BooleanAnd(
new NotIdentical(
$args[$needleArg]->value,
new String_(''),
),
new FuncCall(new Name('FAUX_FUNCTION_' . $functionReflection->getName()), $args),
)
: new FuncCall(new Name('FAUX_FUNCTION_' . $functionReflection->getName()), $args);
Copy link
Contributor Author

@herndlm herndlm Jul 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bit weird but needed to avoid false-positives for functions not evaluating to a boolean like strpos.
Maybe it's an indicator that we cannot evaluate root expressions in general and it should be done by the extensions specifying them themselves hmm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept thinking about this. I think this cannot be there. And to get rid of it (and the error without it, see https://github.com/phpstan/phpstan-src/runs/7373184847?check_suite_focus=true) I'd need to check if the real expression (the function/method call) returns a boolean. For other functions that don't, like strpos the root expression should not be further specified. But to do that I'd need to check the return type via reflection I guess and this feels like too much here.

I'm not sure if this PR is the right approach. Maybe root expression specification should only be done where the FAUX_FUNCTION is created (-> the extension).


return $this->typeSpecifier->create(
$args[$hackstackArg]->value,
new IntersectionType($accessories),
$context,
false,
$scope,
new BooleanAnd(
new NotIdentical(
$args[$needleArg]->value,
new String_(''),
),
new FuncCall(new Name('FAUX_FUNCTION'), [
new Arg($args[$needleArg]->value),
]),
),
$rootExpr,
);
}
}
Expand Down
11 changes: 10 additions & 1 deletion tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,16 @@ public function testNonEmptySpecifiedString(): void
{
$this->checkAlwaysTrueCheckTypeFunctionCall = true;
$this->treatPhpDocTypesAsCertain = true;
$this->analyse([__DIR__ . '/data/non-empty-string-impossible-type.php'], []);
$this->analyse([__DIR__ . '/data/non-empty-string-impossible-type.php'], [
[
'Call to function str_contains() with non-empty-string and \'foo\' will always evaluate to true.',
26,
],
[
'Call to function str_contains() with non-empty-string and \'foo\' will always evaluate to true.',
36,
],
]);
}

public function testBug2755(): void
Expand Down
26 changes: 26 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/non-empty-string-impossible-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,30 @@ private function isPrefixedInterface(string $shortClassName): bool

return ctype_lower($shortClassName[2]);
}

public function strContains(string $a, string $b, string $c, string $d, string $e): void
{
if (str_contains($a, 'foo')) {
if (str_contains($a, 'foo')) {
}
}

if (!str_contains($b, 'foo')) {
if (!str_contains($b, 'foo')) {
}
}

if (str_contains($c, 'foo')) {
if (!str_contains($c, 'foo')) {
}
}

if (!str_contains($d, 'foo')) {
if (str_contains($d, 'foo')) {
}
}

if (str_contains($e, 'bar')) {
}
}
}