Skip to content
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.

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

Improve in_array extension to get rid of related impossible check adaptions #1514

Closed
wants to merge 6 commits into from
Closed
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
63 changes: 0 additions & 63 deletions src/Rules/Comparison/ImpossibleCheckTypeHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,10 @@
use PHPStan\Analyser\TypeSpecifierContext;
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\TrinaryLogic;
use PHPStan\Type\Constant\ConstantArrayType;
use PHPStan\Type\Constant\ConstantBooleanType;
use PHPStan\Type\Constant\ConstantStringType;
use PHPStan\Type\MixedType;
use PHPStan\Type\NeverType;
use PHPStan\Type\ObjectType;
use PHPStan\Type\Type;
use PHPStan\Type\TypeUtils;
use PHPStan\Type\TypeWithClassName;
use PHPStan\Type\VerbosityLevel;
use function array_map;
Expand Down Expand Up @@ -77,65 +73,6 @@ public function findSpecifiedType(
return null;
} elseif ($functionName === 'array_search') {
return null;
} elseif ($functionName === 'in_array' && $argsCount >= 3) {
$haystackType = $scope->getType($node->getArgs()[1]->value);
if ($haystackType instanceof MixedType) {
return null;
}

if (!$haystackType->isArray()->yes()) {
return null;
}

$constantArrays = TypeUtils::getOldConstantArrays($haystackType);
$needleType = $scope->getType($node->getArgs()[0]->value);
$valueType = $haystackType->getIterableValueType();
$constantNeedleTypesCount = count(TypeUtils::getConstantScalars($needleType));
$constantHaystackTypesCount = count(TypeUtils::getConstantScalars($valueType));
$isNeedleSupertype = $needleType->isSuperTypeOf($valueType);
if (count($constantArrays) === 0) {
if ($haystackType->isIterableAtLeastOnce()->yes()) {
if ($constantNeedleTypesCount === 1 && $constantHaystackTypesCount === 1) {
if ($isNeedleSupertype->yes()) {
return true;
}
if ($isNeedleSupertype->no()) {
return false;
}
}
}
return null;
}

if (!$haystackType instanceof ConstantArrayType || count($haystackType->getValueTypes()) > 0) {
$haystackArrayTypes = TypeUtils::getArrays($haystackType);
if (count($haystackArrayTypes) === 1 && $haystackArrayTypes[0]->getIterableValueType() instanceof NeverType) {
return null;
}

if ($isNeedleSupertype->maybe() || $isNeedleSupertype->yes()) {
foreach ($haystackArrayTypes as $haystackArrayType) {
foreach (TypeUtils::getConstantScalars($haystackArrayType->getIterableValueType()) as $constantScalarType) {
if ($constantScalarType->isSuperTypeOf($needleType)->yes()) {
continue 2;
}
}

return null;
}
}

if ($isNeedleSupertype->yes()) {
$hasConstantNeedleTypes = $constantNeedleTypesCount > 0;
$hasConstantHaystackTypes = $constantHaystackTypesCount > 0;
if (
(!$hasConstantNeedleTypes && !$hasConstantHaystackTypes)
|| $hasConstantNeedleTypes !== $hasConstantHaystackTypes
) {
return null;
}
}
}
} elseif ($functionName === 'method_exists' && $argsCount >= 2) {
$objectType = $scope->getType($node->getArgs()[0]->value);
$methodType = $scope->getType($node->getArgs()[1]->value);
Expand Down
10 changes: 7 additions & 3 deletions src/Type/Accessory/HasOffsetType.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,12 @@ class HasOffsetType implements CompoundType, AccessoryType
use NonRemoveableTypeTrait;
use NonGeneralizableTypeTrait;

private Type $offsetValueType;

/** @api */
public function __construct(private Type $offsetType)
public function __construct(private Type $offsetType, ?Type $offsetValueType = null)
{
$this->offsetValueType = $offsetValueType ?? new MixedType();
}

public function getOffsetType(): Type
Expand Down Expand Up @@ -64,7 +67,8 @@ public function isSuperTypeOf(Type $type): TrinaryLogic
return TrinaryLogic::createYes();
}
return $type->isOffsetAccessible()
->and($type->hasOffsetValueType($this->offsetType));
->and($type->hasOffsetValueType($this->offsetType))
->and($this->offsetValueType->isSuperTypeOf($type->getOffsetValueType($this->offsetType)));
Comment on lines 69 to +71
Copy link
Contributor Author

@herndlm herndlm Jul 13, 2022

Choose a reason for hiding this comment

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

not sure if isSuperTypeOf and friends are really correct, e.g. I did not touch isSubTypeOf or accept at all

}

public function isSubTypeOf(Type $otherType): TrinaryLogic
Expand Down Expand Up @@ -110,7 +114,7 @@ public function hasOffsetValueType(Type $offsetType): TrinaryLogic

public function getOffsetValueType(Type $offsetType): Type
{
return new MixedType();
return $offsetType->equals($this->offsetType) ? $this->offsetValueType : new ErrorType();
}

public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $unionValues = true): Type
Expand Down
33 changes: 33 additions & 0 deletions src/Type/Constant/ConstantArrayType.php
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,39 @@ public function getOffsetValueType(Type $offsetType): Type
return new ErrorType(); // undefined offset
}

public function getOffsetType(Type $offsetValueType): Type
{
$matchingOffsetTypes = [];
$all = true;
foreach ($this->valueTypes as $i => $valueType) {
if ($valueType->isSuperTypeOf($offsetValueType)->no()) {
$all = false;
continue;
}

$matchingOffsetTypes[] = $this->keyTypes[$i];
}

if ($all) {
if (count($this->keyTypes) === 0) {
return new ErrorType();
}

return $this->getIterableKeyType();
}

if (count($matchingOffsetTypes) > 0) {
$type = TypeCombinator::union(...$matchingOffsetTypes);
if ($type instanceof ErrorType) {
return new MixedType();
}

return $type;
}

return new ErrorType(); // undefined offset value
}

public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $unionValues = true): Type
{
$builder = ConstantArrayTypeBuilder::createFromConstantArray($this);
Expand Down
85 changes: 73 additions & 12 deletions src/Type/Php/InArrayFunctionTypeSpecifyingExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,22 @@
use PHPStan\Analyser\TypeSpecifierAwareExtension;
use PHPStan\Analyser\TypeSpecifierContext;
use PHPStan\Reflection\FunctionReflection;
use PHPStan\Type\Accessory\HasOffsetType;
use PHPStan\Type\Accessory\NonEmptyArrayType;
use PHPStan\Type\ArrayType;
use PHPStan\Type\Constant\ConstantArrayType;
use PHPStan\Type\Constant\ConstantArrayTypeBuilder;
use PHPStan\Type\Constant\ConstantBooleanType;
use PHPStan\Type\Constant\ConstantIntegerType;
use PHPStan\Type\ConstantScalarType;
use PHPStan\Type\FunctionTypeSpecifyingExtension;
use PHPStan\Type\IntersectionType;
use PHPStan\Type\MixedType;
use PHPStan\Type\Type;
use PHPStan\Type\TypeCombinator;
use PHPStan\Type\TypeTraverser;
use PHPStan\Type\TypeUtils;
use PHPStan\Type\UnionType;
use function count;
use function strtolower;

Expand Down Expand Up @@ -51,11 +60,21 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n

$specifiedTypes = new SpecifiedTypes();

if (
$arrayType instanceof ConstantArrayType && !$arrayType->isEmpty()
&& count(TypeUtils::getConstantScalars($needleType)) === 0 && $arrayValueType->isSuperTypeOf($needleType)->yes()
) {
// Avoid false-positives with e.g. a string needle and array{'self', string} as haystack
// For such cases there seems to be nothing more that we can specify unfortunately
return $specifiedTypes;
}

Comment on lines +63 to +71
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an edge case I'd really like to not have here. But I didn't find a way to specify anything here, that doesn't get normalized away (e.g. $needle is 'self'|string which is string and the array must have a string value for sure, which it already has). There was initially no test case for this, but it was showing up as error in slevomat-cs.
any other ideas?

if (
$context->truthy()
|| count(TypeUtils::getConstantScalars($arrayValueType)) > 0
|| count(TypeUtils::getEnumCaseObjects($arrayValueType)) > 0
) {
// Specify needle type
$specifiedTypes = $this->typeSpecifier->create(
$node->getArgs()[0]->value,
$arrayValueType,
Expand All @@ -65,27 +84,69 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n
);
}

// If e.g. needle is 'a' and haystack non-empty-array<int, 'a'> we can be sure that this always evaluates to true
// Belows HasOffset::isSuperTypeOf cannot deal with that since it calls ArrayType::hasOffsetValueType and that returns maybe at max
if ($needleType instanceof ConstantScalarType && $arrayType->isIterableAtLeastOnce()->yes() && $arrayValueType->equals($needleType)) {
return $specifiedTypes;
}

Comment on lines +87 to +92
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also dislike this edge case, maybe it is possible to adapt HasOffset::isSuperTypeOf somehow to figure this out.. Any ideas?

if (
$context->truthy()
|| count(TypeUtils::getConstantScalars($needleType)) > 0
|| count(TypeUtils::getEnumCaseObjects($needleType)) > 0
) {
if ($context->truthy()) {
$arrayType = TypeCombinator::intersect(
new ArrayType(new MixedType(), TypeCombinator::union($arrayValueType, $needleType)),
new NonEmptyArrayType(),
// Specify haystack type
if ($arrayType instanceof ConstantArrayType) {
$newArrayType = TypeTraverser::map(
$arrayType->getOffsetType($needleType),
static function (Type $offsetType, callable $traverse) use ($context, $arrayType, $needleType): Type {
if ($offsetType instanceof UnionType || $offsetType instanceof IntersectionType) {
return $traverse($offsetType);
}

$resultArray = $arrayType;
if ($context->truthy()) {
$resultArray = $resultArray->makeOffsetRequired($offsetType);
} elseif ($offsetType instanceof ConstantIntegerType && $resultArray->isOptionalKey($offsetType->getValue())) {
$resultArray = $resultArray->unsetOffset($offsetType);
}

if ($offsetType instanceof ConstantIntegerType && $resultArray instanceof ConstantArrayType && $resultArray->hasOffsetValueType($offsetType)->yes()) {
// If haystack is e.g. {string, string|null} and needle null, we can further narrow string|null
$builder = ConstantArrayTypeBuilder::createFromConstantArray($resultArray);
$builder->setOffsetValueType(
$offsetType,
$context->truthy() ? $needleType : TypeCombinator::remove($resultArray->getOffsetValueType($offsetType), $needleType),
$resultArray->isOptionalKey($offsetType->getValue()),
);
$resultArray = $builder->getArray();
}

return $resultArray;
},
);
} else {
$arrayType = new ArrayType(new MixedType(), TypeCombinator::remove($arrayValueType, $needleType));
if ($context->truthy()) {
$newArrayType = TypeCombinator::intersect(
new ArrayType(new MixedType(), TypeCombinator::union($arrayValueType, $needleType)),
new HasOffsetType(new MixedType(), $needleType),
new NonEmptyArrayType(),
);
} else {
$newArrayType = new ArrayType(
new MixedType(),
TypeCombinator::remove($arrayValueType, $needleType),
);
}
}

$specifiedTypes = $specifiedTypes->unionWith($this->typeSpecifier->create(
$node->getArgs()[1]->value,
$arrayType,
TypeSpecifierContext::createTrue(),
false,
$scope,
));
$specifiedTypes = $specifiedTypes->unionWith($this->typeSpecifier->create(
$node->getArgs()[1]->value,
$newArrayType,
TypeSpecifierContext::createTrue(),
false,
$scope,
));
}

return $specifiedTypes;
Expand Down
2 changes: 1 addition & 1 deletion tests/PHPStan/Analyser/data/bug-7153.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ function () {
assertType('array{string, string|null}', $data);

if (in_array(null, $data, true)) {
assertType('array{string, string|null}', $data);
assertType('array{string, null}', $data);
throw new Exception();
}

Expand Down
4 changes: 2 additions & 2 deletions tests/PHPStan/Analyser/data/in-array-non-empty.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ class HelloWorld
public function sayHello(array $array): void
{
if(in_array("thing", $array, true)){
assertType('non-empty-array<int, string>', $array);
assertType('non-empty-array<int, string>&hasOffset(mixed)', $array);
Copy link
Contributor Author

@herndlm herndlm Jul 13, 2022

Choose a reason for hiding this comment

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

this is obviously weird. I'd prefer to rather have an hasOffsetValue('thing') I guess. But on the other hand, the HasOffset with both the offset and offset value might be useful somewhere else?
Also, mixed is potentially confusing here, isn't it? :/

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe print offset and offsetValue type within the bracket, like
&hasOffset(mixed, 'thing') ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would be easy to do and makes sense, yeah. I did not do it yet because I was scared that it will lead to too many changes in e.g. baselines. but we can't think always about that, right? I'll check it out :)

}
}

/** @param array<int> $haystack */
public function nonConstantNeedle(int $needle, array $haystack): void
{
if (in_array($needle, $haystack, true)) {
assertType('non-empty-array<int>', $haystack);
assertType('non-empty-array<int>&hasOffset(mixed)', $haystack);
}
}
}
6 changes: 4 additions & 2 deletions tests/PHPStan/Analyser/data/in-array.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ class Foo
* @param string $r
* @param $mixed
* @param string[] $strings
* @param string[] $moreStrings
*/
public function doFoo(
string $s,
string $r,
$mixed,
array $strings
array $strings,
array $moreStrings
)
{
if (!in_array($s, ['foo', 'bar'], true)) {
Expand All @@ -26,7 +28,7 @@ public function doFoo(
return;
}

if (in_array($r, $strings, true)) {
if (in_array($r, $moreStrings, true)) {
Copy link
Contributor Author

@herndlm herndlm Jul 13, 2022

Choose a reason for hiding this comment

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

I had to do that because $strings was already type-specified from previous conditionals in the same scope which messed the result up here since types are specified better now.

return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ public function testImpossibleCheckTypeFunctionCall(): void
'Call to function in_array() with arguments \'bar\'|\'foo\', array{\'baz\', \'lorem\'} and true will always evaluate to false.',
244,
],
[
'Call to function in_array() with arguments \'bar\'|\'foo\', array{\'foo\', \'bar\'} and true will always evaluate to true.',
248,
],
[
'Call to function in_array() with arguments \'foo\', array{\'foo\'} and true will always evaluate to true.',
252,
Expand Down Expand Up @@ -233,6 +237,18 @@ public function testImpossibleCheckTypeFunctionCall(): void
'Call to function property_exists() with CheckTypeFunctionCall\Bug2221 and \'foo\' will always evaluate to true.',
786,
],
[
'Call to function in_array() with arguments \'foo\', non-empty-array<string> and true will always evaluate to true.',
890,
],
[
'Call to function in_array() with arguments \'foo\', array{\'foo\'} and true will always evaluate to true.',
896,
],
[
'Call to function in_array() with arguments \'foo\', array and true will always evaluate to false.',
900,
],
],
);
}
Expand Down Expand Up @@ -329,6 +345,10 @@ public function testImpossibleCheckTypeFunctionCallWithoutAlwaysTrue(): void
'Call to function is_numeric() with \'blabla\' will always evaluate to false.',
693,
],
[
'Call to function in_array() with arguments \'foo\', array and true will always evaluate to false.',
900,
],
],
);
}
Expand Down Expand Up @@ -483,6 +503,10 @@ public function testBugInArrayDateFormat(): void
'Call to function in_array() with arguments int, array{} and true will always evaluate to false.',
47,
],
[
'Call to function in_array() with arguments int, array<int, string> and true will always evaluate to false.',
61,
],
]);
}

Expand Down Expand Up @@ -525,7 +549,12 @@ public function testSlevomatCsInArrayBug(): void
{
$this->checkAlwaysTrueCheckTypeFunctionCall = true;
$this->treatPhpDocTypesAsCertain = true;
$this->analyse([__DIR__ . '/data/slevomat-cs-in-array.php'], []);
$this->analyse([__DIR__ . '/data/slevomat-cs-in-array.php'], [
[
'Call to function in_array() with arguments \'abstract methods\'|\'constructor\'|\'destructor\'|\'final methods\'|\'magic methods\'|\'private constants\'|\'private methods\'|\'private properties\'|\'private static…\'|\'private static…\'|\'protected abstract…\'|\'protected constants\'|\'protected final…\'|\'protected methods\'|\'protected properties\'|\'protected static…\'|\'protected static…\'|\'protected static…\'|\'protected static…\'|\'public abstract…\'|\'public constants\'|\'public final methods\'|\'public methods\'|\'public properties\'|\'public static…\'|\'public static final…\'|\'public static…\'|\'public static…\'|\'static constructors\'|\'static methods\'|\'static properties\', array{0: \'final methods\'|\'private static…\'|\'protected final…\'|\'public abstract…\'|\'public constants\'|\'public final methods\'|\'public static…\'|\'static constructors\'|\'static properties\', 1: \'abstract methods\'|\'private methods\'|\'protected abstract…\'|\'protected constants\'|\'protected final…\'|\'protected static…\'|\'protected static…\'|\'public properties\'|\'public static final…\', 2?: \'private constants\'|\'private static…\'|\'protected abstract…\'|\'protected properties\'|\'protected static…\'|\'public abstract…\'|\'public static…\'|\'public static final…\'|\'static methods\', 3?: \'constructor\'|\'private properties\'|\'protected static…\'|\'protected static…\'|\'public static…\', 4?: \'destructor\'|\'protected static…\'|\'protected static…\'|\'public static…\', 5?: \'protected methods\'|\'public methods\'|\'public static…\', 6?: \'protected methods\'|\'protected static…\', 7?: \'private methods\'|\'private static…\', ...} and true will always evaluate to true.',
132,
],
Comment on lines +553 to +556
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is showing up because in_array_key is specifying constant types I think which it shouldn't. I created phpstan/phpstan#7621 for that

]);
}

public function testNonEmptySpecifiedString(): void
Expand Down
Loading