Skip to content

Avoid false inference with instanceof #3657

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 2 commits into from
Jan 15, 2025
Merged

Avoid false inference with instanceof #3657

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
649d831
Avoid false inference with instanceof
VincentLanglet Nov 23, 2024
a159a52
Update baseline
VincentLanglet Nov 23, 2024
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
7 changes: 5 additions & 2 deletions src/Analyser/TypeSpecifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,17 @@ public function specifyTypesInCondition(
}

$classType = $scope->getType($expr->class);
$type = TypeTraverser::map($classType, static function (Type $type, callable $traverse): Type {
$uncertainty = false;
$type = TypeTraverser::map($classType, static function (Type $type, callable $traverse) use (&$uncertainty): Type {
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 tried to copy the logic from MutatingScope::inferType with an uncertainty boolean:

phpstan-src/src/Analyser/MutatingScope.php

Lines 960 to 989 in adcabb3

$classType = TypeTraverser::map($classType, static function (Type $type, callable $traverse) use (&$uncertainty): Type {
if ($type instanceof UnionType || $type instanceof IntersectionType) {
return $traverse($type);
}
if ($type->getObjectClassNames() !== []) {
$uncertainty = true;
return $type;
}
if ($type instanceof GenericClassStringType) {
$uncertainty = true;
return $type->getGenericType();
}
if ($type instanceof ConstantStringType) {
return new ObjectType($type->getValue());
}
return new MixedType();
});
}
if ($classType->isSuperTypeOf(new MixedType())->yes()) {
return new BooleanType();
}
$isSuperType = $classType->isSuperTypeOf($expressionType);
if ($isSuperType->no()) {
return new ConstantBooleanType(false);
} elseif ($isSuperType->yes() && !$uncertainty) {
return new ConstantBooleanType(true);
}

if ($type instanceof UnionType || $type instanceof IntersectionType) {
return $traverse($type);
}
if ($type->getObjectClassNames() !== []) {
$uncertainty = true;
return $type;
}
if ($type instanceof GenericClassStringType) {
$uncertainty = true;
return $type->getGenericType();
}
if ($type instanceof ConstantStringType) {
Expand All @@ -179,7 +182,7 @@ public function specifyTypesInCondition(
new ObjectWithoutClassType(),
);
return $this->create($exprNode, $type, $context, false, $scope, $rootExpr);
} elseif ($context->false()) {
} elseif ($context->false() && !$uncertainty) {
$exprType = $scope->getType($expr->expr);
if (!$type->isSuperTypeOf($exprType)->yes()) {
return $this->create($exprNode, $type, $context, false, $scope, $rootExpr);
Expand Down
43 changes: 43 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-12107.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php declare(strict_types = 1);

namespace Bug12107;

use LogicException;
use Throwable;

use function PHPStan\Testing\assertType;

class HelloWorld
{
public function sayHello(Throwable $e1, LogicException $e2): void
{
if ($e1 instanceof $e2) {
return;
}

assertType('Throwable', $e1);
assertType('bool', $e1 instanceof $e2); // could be false
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'm unsure to say PHPStan that

  • $e1 is not Throwable~LogicException but still Throwable
  • the specific expression $e1 instanceof $e2 is false because already computed.

But I'm not sure it's a big loss.

}

/** @param class-string<LogicException> $e2 */
public function sayHello2(Throwable $e1, string $e2): void
{
if ($e1 instanceof $e2) {
return;
}


assertType('Throwable', $e1);
assertType('bool', $e1 instanceof $e2); // could be false
}

public function sayHello3(Throwable $e1): void
{
if ($e1 instanceof LogicException) {
return;
}

assertType('Throwable~LogicException', $e1);
assertType('false', $e1 instanceof LogicException);
}
}
2 changes: 1 addition & 1 deletion tests/PHPStan/Analyser/nsrt/instanceof-class-string.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public function doBar(Foo $foo, Bar $bar): void
if ($foo instanceof $class) {
assertType(self::class, $foo);
} else {
assertType('InstanceOfClassString\Foo~InstanceOfClassString\Bar', $foo);
assertType('InstanceOfClassString\Foo', $foo);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I call doFar($foo, $superBar) with superBar a class extending bar,
$foo is only Foo~SuperBar but can still being Bar.

}
}

Expand Down
16 changes: 8 additions & 8 deletions tests/PHPStan/Analyser/nsrt/instanceof.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ public function testExprInstanceof($subject, string $classString, $union, $inter
assertType('true', $subject instanceof Foo);
assertType('bool', $subject instanceof $classString);
} else {
assertType('mixed~InstanceOfNamespace\Foo', $subject);
assertType('false', $subject instanceof Foo);
assertType('false', $subject instanceof $classString);
assertType('mixed', $subject);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I pass class-string<SuperFoo> for $classString the subject will only be mixed~SuperFoo but not mixed~Foo.

assertType('bool', $subject instanceof Foo);
assertType('bool', $subject instanceof $classString); // could be false
}

$constantString = 'InstanceOfNamespace\BarParent';
Expand Down Expand Up @@ -132,23 +132,23 @@ public function testExprInstanceof($subject, string $classString, $union, $inter
assertType('ObjectT of InstanceOfNamespace\BarInterface (method InstanceOfNamespace\Foo::testExprInstanceof(), argument)', $subject);
assertType('bool', $subject instanceof $objectT);
} else {
assertType('mixed~ObjectT of InstanceOfNamespace\BarInterface (method InstanceOfNamespace\Foo::testExprInstanceof(), argument)', $subject);
assertType('mixed', $subject);
assertType('bool', $subject instanceof $objectT); // can be false
}

if ($subject instanceof $objectTString) {
assertType('ObjectT of InstanceOfNamespace\BarInterface (method InstanceOfNamespace\Foo::testExprInstanceof(), argument)', $subject);
assertType('bool', $subject instanceof $objectTString);
} else {
assertType('mixed~ObjectT of InstanceOfNamespace\BarInterface (method InstanceOfNamespace\Foo::testExprInstanceof(), argument)', $subject);
assertType('mixed', $subject);
assertType('bool', $subject instanceof $objectTString); // can be false
}

if ($subject instanceof $mixedTString) {
assertType('MixedT (method InstanceOfNamespace\Foo::testExprInstanceof(), argument)&object', $subject);
assertType('bool', $subject instanceof $mixedTString);
} else {
assertType('mixed~MixedT (method InstanceOfNamespace\Foo::testExprInstanceof(), argument)', $subject);
assertType('mixed', $subject);
assertType('bool', $subject instanceof $mixedTString); // can be false
}

Expand Down Expand Up @@ -180,8 +180,8 @@ public function testExprInstanceof($subject, string $classString, $union, $inter
assertType('InstanceOfNamespace\Foo', $object);
assertType('bool', $object instanceof $classString);
} else {
assertType('object~InstanceOfNamespace\Foo', $object);
assertType('false', $object instanceof $classString);
assertType('object', $object);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same idea.

assertType('bool', $object instanceof $classString); // could be false
}

if ($instance instanceof $string) {
Expand Down
10 changes: 0 additions & 10 deletions tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,6 @@ public function testInstanceof(): void
388,
$tipText,
],
[
'Instanceof between T of Exception and Error will always evaluate to false.',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code is

	/**
	 * @template T of \Exception
	 * @param T $e
	 */
	public function test(\Throwable $t, $e): void {
		if ($t instanceof $e) return;
		if ($e instanceof $t) return;
	}

if I call it with test(new \Exception(), new \LogicException()) the second if is true.

404,
$tipText,
],
[
'Instanceof between class-string<DateTimeInterface> and DateTimeInterface will always evaluate to false.',
418,
Expand Down Expand Up @@ -270,11 +265,6 @@ public function testInstanceofWithoutAlwaysTrue(): void
'Instanceof between mixed and ImpossibleInstanceOf\InvalidTypeTest|int results in an error.',
362,
],*/
[
'Instanceof between T of Exception and Error will always evaluate to false.',
404,
$tipText,
],
[
'Instanceof between class-string<DateTimeInterface> and DateTimeInterface will always evaluate to false.',
418,
Expand Down
Loading