Skip to content

Narrow string on strlen() == and === comparison with integer-range #3342

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

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Narrow string on strlen() == and === comparison with integer-range #3342

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
64 changes: 42 additions & 22 deletions src/Analyser/TypeSpecifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,13 +1056,19 @@ private function turnListIntoConstantArray(FuncCall $countFuncCall, Type $type,

private function specifyTypesForConstantBinaryExpression(
Expr $exprNode,
ConstantScalarType $constantType,
Type $constantType,
TypeSpecifierContext $context,
Scope $scope,
?Expr $rootExpr,
): ?SpecifiedTypes
{
if (!$context->null() && $constantType->getValue() === false) {
$scalarValues = $constantType->getConstantScalarValues();
if (count($scalarValues) !== 1) {
return null;
}
$constValue = $scalarValues[0];

if (!$context->null() && $constValue === false) {
$types = $this->create($exprNode, $constantType, $context, false, $scope, $rootExpr);
if ($exprNode instanceof Expr\NullsafeMethodCall || $exprNode instanceof Expr\NullsafePropertyFetch) {
return $types;
Expand All @@ -1076,7 +1082,7 @@ private function specifyTypesForConstantBinaryExpression(
));
}

if (!$context->null() && $constantType->getValue() === true) {
if (!$context->null() && $constValue === true) {
$types = $this->create($exprNode, $constantType, $context, false, $scope, $rootExpr);
if ($exprNode instanceof Expr\NullsafeMethodCall || $exprNode instanceof Expr\NullsafePropertyFetch) {
return $types;
Expand All @@ -1090,10 +1096,6 @@ private function specifyTypesForConstantBinaryExpression(
));
}

if ($constantType->getValue() === null) {
return $this->create($exprNode, $constantType, $context, false, $scope, $rootExpr);
}

Comment on lines -1093 to -1096
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looked like dead code

if (
!$context->null()
&& $exprNode instanceof FuncCall
Expand All @@ -1102,6 +1104,10 @@ private function specifyTypesForConstantBinaryExpression(
&& in_array(strtolower((string) $exprNode->name), ['count', 'sizeof'], true)
&& $constantType instanceof ConstantIntegerType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in a followup PR I would factor out the 2 $constantType instanceof ConstantIntegerType if-blocks of this method into the parent caller and do a proper integer range check instead, which will fix phpstan/phpstan#11558

I don't do it now to keep this PR simpler and the next one will also be easier to review

) {
if ($constantType->getValue() < 0) {
return $this->create($exprNode->getArgs()[0]->value, new NeverType(), $context, false, $scope, $rootExpr);
}

$argType = $scope->getType($exprNode->getArgs()[0]->value);

if ($argType instanceof UnionType) {
Expand Down Expand Up @@ -1146,6 +1152,10 @@ private function specifyTypesForConstantBinaryExpression(
&& in_array(strtolower((string) $exprNode->name), ['strlen', 'mb_strlen'], true)
&& $constantType instanceof ConstantIntegerType
) {
if ($constantType->getValue() < 0) {
return $this->create($exprNode->getArgs()[0]->value, new NeverType(), $context, false, $scope, $rootExpr);
}

if ($context->truthy() || $constantType->getValue() === 0) {
$newContext = $context;
if ($constantType->getValue() === 0) {
Expand All @@ -1172,12 +1182,18 @@ private function specifyTypesForConstantBinaryExpression(

private function specifyTypesForConstantStringBinaryExpression(
Expr $exprNode,
ConstantStringType $constantType,
Type $constantType,
TypeSpecifierContext $context,
Scope $scope,
?Expr $rootExpr,
): ?SpecifiedTypes
{
$scalarValues = $constantType->getConstantScalarValues();
if (count($scalarValues) !== 1 || !is_string($scalarValues[0])) {
return null;
}
$constantStringValue = $scalarValues[0];

if (
$context->truthy()
&& $exprNode instanceof FuncCall
Expand All @@ -1188,12 +1204,12 @@ private function specifyTypesForConstantStringBinaryExpression(
'ucwords', 'mb_convert_case', 'mb_convert_kana',
], true)
&& isset($exprNode->getArgs()[0])
&& $constantType->getValue() !== ''
&& $constantStringValue !== ''
) {
$argType = $scope->getType($exprNode->getArgs()[0]->value);

if ($argType->isString()->yes()) {
if ($constantType->getValue() !== '0') {
if ($constantStringValue !== '0') {
return $this->create(
$exprNode->getArgs()[0]->value,
TypeCombinator::intersect($argType, new AccessoryNonFalsyStringType()),
Expand All @@ -1220,28 +1236,28 @@ private function specifyTypesForConstantStringBinaryExpression(
&& isset($exprNode->getArgs()[0])
) {
$type = null;
if ($constantType->getValue() === 'string') {
if ($constantStringValue === 'string') {
$type = new StringType();
}
if ($constantType->getValue() === 'array') {
if ($constantStringValue === 'array') {
$type = new ArrayType(new MixedType(), new MixedType());
}
if ($constantType->getValue() === 'boolean') {
if ($constantStringValue === 'boolean') {
$type = new BooleanType();
}
if (in_array($constantType->getValue(), ['resource', 'resource (closed)'], true)) {
if (in_array($constantStringValue, ['resource', 'resource (closed)'], true)) {
$type = new ResourceType();
}
if ($constantType->getValue() === 'integer') {
if ($constantStringValue === 'integer') {
$type = new IntegerType();
}
if ($constantType->getValue() === 'double') {
if ($constantStringValue === 'double') {
$type = new FloatType();
}
if ($constantType->getValue() === 'NULL') {
if ($constantStringValue === 'NULL') {
$type = new NullType();
}
if ($constantType->getValue() === 'object') {
if ($constantStringValue === 'object') {
$type = new ObjectWithoutClassType();
}

Expand All @@ -1260,7 +1276,7 @@ private function specifyTypesForConstantStringBinaryExpression(
&& isset($exprNode->getArgs()[0])
) {
$argType = $scope->getType($exprNode->getArgs()[0]->value);
$objectType = new ObjectType($constantType->getValue());
$objectType = new ObjectType($constantStringValue);
$classStringType = new GenericClassStringType($objectType);

if ($argType->isString()->yes()) {
Expand Down Expand Up @@ -2149,10 +2165,14 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty
}
}

if (count($rightType->getConstantStrings()) > 0) {
if ($rightType->isInteger()->yes() || $rightType->isString()->yes()) {
$types = null;
foreach ($rightType->getConstantStrings() as $constantString) {
$specifiedType = $this->specifyTypesForConstantStringBinaryExpression($unwrappedLeftExpr, $constantString, $context, $scope, $rootExpr);
foreach ($rightType->getFiniteTypes() as $finiteType) {
if ($finiteType->isString()->yes()) {
$specifiedType = $this->specifyTypesForConstantStringBinaryExpression($unwrappedLeftExpr, $finiteType, $context, $scope, $rootExpr);
} else {
$specifiedType = $this->specifyTypesForConstantBinaryExpression($unwrappedLeftExpr, $finiteType, $context, $scope, $rootExpr);
}
if ($specifiedType === null) {
continue;
}
Expand Down
46 changes: 46 additions & 0 deletions tests/PHPStan/Analyser/nsrt/count-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,50 @@ public function doFoo(
assertType('int<1, max>', sizeof($nonEmpty));
}

/**
* @param int<3,5> $range
* @param int<0,5> $maybeZero
* @param int<-10,-5> $negative
*/
public function doFooBar(
array $arr,
int $range,
int $maybeZero,
int $negative
)
{
if (count($arr) == $range) {
assertType('non-empty-array', $arr);
} else {
assertType('array', $arr);
}
if (count($arr) === $range) {
assertType('non-empty-array', $arr);
} else {
assertType('array', $arr);
}

if (count($arr) == $maybeZero) {
assertType('array', $arr);
} else {
assertType('non-empty-array', $arr);
}
if (count($arr) === $maybeZero) {
assertType('array', $arr);
} else {
assertType('non-empty-array', $arr);
}

if (count($arr) == $negative) {
assertType('*NEVER*', $arr);
} else {
assertType('array', $arr);
}
if (count($arr) === $negative) {
assertType('*NEVER*', $arr);
} else {
assertType('array', $arr);
}
}

}
65 changes: 63 additions & 2 deletions tests/PHPStan/Analyser/nsrt/strlen-int-range.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@
* @param int<2, 3> $twoOrThree
* @param int<2, max> $twoOrMore
* @param int<min, 3> $maxThree
* @param int<10, 11> $tenOrEleven
* @param 10|11 $tenOrEleven
* @param 0|11 $zeroOrEleven
* @param int<-10,-5> $negative
*/
function doFoo(string $s, $zeroToThree, $twoOrThree, $twoOrMore, int $maxThree, $tenOrEleven): void
function doFoo(string $s, $zeroToThree, $twoOrThree, $twoOrMore, int $maxThree, $tenOrEleven, $zeroOrEleven, int $negative): void
{
if (strlen($s) >= $zeroToThree) {
assertType('string', $s);
Expand Down Expand Up @@ -51,4 +53,63 @@ function doFoo(string $s, $zeroToThree, $twoOrThree, $twoOrMore, int $maxThree,
if (strlen($s) > $tenOrEleven) {
assertType('non-falsy-string', $s);
}

if (strlen($s) == $zeroToThree) {
assertType('string', $s);
}
if (strlen($s) === $zeroToThree) {
assertType('string', $s);
}

if (strlen($s) == $twoOrThree) {
assertType('non-falsy-string', $s);
}
if (strlen($s) === $twoOrThree) {
assertType('non-falsy-string', $s);
}

if (strlen($s) == $oneOrMore) {
assertType('string', $s); // could be non-empty-string
}
if (strlen($s) === $oneOrMore) {
assertType('string', $s); // could be non-empty-string
}

if (strlen($s) == $tenOrEleven) {
assertType('non-falsy-string', $s);
}
if (strlen($s) === $tenOrEleven) {
assertType('non-falsy-string', $s);
}
if ($tenOrEleven == strlen($s)) {
assertType('non-falsy-string', $s);
}
if ($tenOrEleven === strlen($s)) {
assertType('non-falsy-string', $s);
}

if (strlen($s) == $maxThree) {
assertType('string', $s);
}
if (strlen($s) === $maxThree) {
assertType('string', $s);
}

if (strlen($s) == $zeroOrEleven) {
assertType('string', $s);
}
if (strlen($s) === $zeroOrEleven) {
assertType('string', $s);
}

if (strlen($s) == $negative) {
assertType('*NEVER*', $s);
} else {
assertType('string', $s);
}
if (strlen($s) === $negative) {
assertType('*NEVER*', $s);
} else {
assertType('string', $s);
}
}
Loading