-
Notifications
You must be signed in to change notification settings - Fork 465
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
Avoid false inference with instanceof #3657
base: 1.12.x
Are you sure you want to change the base?
Conversation
0b629c6
to
649d831
Compare
@@ -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 { |
There was a problem hiding this comment.
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); | |
} |
} | ||
|
||
assertType('Throwable', $e1); | ||
assertType('bool', $e1 instanceof $e2); // could be false |
There was a problem hiding this comment.
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 stillThrowable
- the specific expression
$e1 instanceof $e2
is false because already computed.
But I'm not sure it's a big loss.
@@ -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); |
There was a problem hiding this comment.
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.
assertType('mixed~InstanceOfNamespace\Foo', $subject); | ||
assertType('false', $subject instanceof Foo); | ||
assertType('false', $subject instanceof $classString); | ||
assertType('mixed', $subject); |
There was a problem hiding this comment.
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
.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same idea.
@@ -167,11 +167,6 @@ public function testInstanceof(): void | |||
388, | |||
$tipText, | |||
], | |||
[ | |||
'Instanceof between T of Exception and Error will always evaluate to false.', |
There was a problem hiding this comment.
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.
This pull request has been marked as ready for review. |
No description provided.