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.

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

Jump to bottom

Added error message to impossible instanceof rule to warn when comparing a trait. #1570

Merged
merged 3 commits into from
Jul 30, 2022
Merged

Added error message to impossible instanceof rule to warn when comparing a trait. #1570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c45692
Added error message to impossible instanceof rule to warn when compar…
mad-briller Jul 30, 2022
1477644
Fixed not calling hasClass before getClass in impossible instanceof r…
mad-briller Jul 30, 2022
b27cf7e
Moved instanceof trait check to the ExistingClassInInstanceOfRule.
mad-briller Jul 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Rules/Classes/ImpossibleInstanceOfRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Constant\ConstantBooleanType;
Expand All @@ -23,6 +24,7 @@ class ImpossibleInstanceOfRule implements Rule
public function __construct(
private bool $checkAlwaysTrueInstanceof,
private bool $treatPhpDocTypesAsCertain,
private ReflectionProvider $reflectionProvider,
)
{
}
Expand All @@ -36,6 +38,7 @@ public function processNode(Node $node, Scope $scope): array
{
$instanceofType = $scope->getType($node);
$expressionType = $scope->getType($node->expr);
$className = null;

if ($node->class instanceof Node\Name) {
$className = $scope->resolveName($node->class);
Expand All @@ -57,6 +60,20 @@ public function processNode(Node $node, Scope $scope): array
}
}

if ($className !== null) {
$classReflection = $this->reflectionProvider->getClass($className);
mad-briller marked this conversation as resolved.
Show resolved Hide resolved

if ($classReflection->isTrait()) {
return [
RuleErrorBuilder::message(sprintf(
'Instanceof between %s and trait %s will always evaluate to false.',
$expressionType->describe(VerbosityLevel::typeOnly()),
$classType->describe(VerbosityLevel::typeOnly()),
))->build(),
];
}
}

if (!$instanceofType instanceof ConstantBooleanType) {
return [];
}
Expand Down
4 changes: 4 additions & 0 deletions src/Type/ObjectType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,10 @@ public function isSuperTypeOf(Type $type): TrinaryLogic
$thisClassReflection = $this->getClassReflection();
$thatClassReflection = $reflectionProvider->getClass($thatClassName);

if ($thisClassReflection->isTrait() || $thatClassReflection->isTrait()) {
return TrinaryLogic::createNo();
}

if ($thisClassReflection->getName() === $thatClassReflection->getName()) {
return self::$superTypes[$thisDescription][$description] = $transformResult(TrinaryLogic::createYes());
}
Expand Down
18 changes: 17 additions & 1 deletion tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ class ImpossibleInstanceOfRuleTest extends RuleTestCase

protected function getRule(): Rule
{
return new ImpossibleInstanceOfRule($this->checkAlwaysTrueInstanceOf, $this->treatPhpDocTypesAsCertain);
return new ImpossibleInstanceOfRule(
$this->checkAlwaysTrueInstanceOf,
$this->treatPhpDocTypesAsCertain,
$this->createReflectionProvider(),
);
}

protected function shouldTreatPhpDocTypesAsCertain(): bool
Expand Down Expand Up @@ -346,4 +350,16 @@ public function testBug6213(): void
$this->analyse([__DIR__ . '/data/bug-6213.php'], []);
}

public function testBug7720(): void
{
$this->checkAlwaysTrueInstanceOf = true;
$this->treatPhpDocTypesAsCertain = true;
$this->analyse([__DIR__ . '/data/bug-7720.php'], [
[
'Instanceof between mixed and trait Bug7720\FooBar will always evaluate to false.',
17,
],
]);
}

}
21 changes: 21 additions & 0 deletions tests/PHPStan/Rules/Classes/data/bug-7720.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php declare(strict_types = 1);

namespace Bug7720;

trait FooBar
{
public function foo(): string
{
return 'abc';
}
}

class HelloWorld
{
public function sayHello(mixed $value): void
{
if ($value instanceof FooBar) {
echo $value->foo();
}
}
}
11 changes: 11 additions & 0 deletions tests/PHPStan/Type/ObjectTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use PHPStan\Type\Generic\TemplateTypeFactory;
use PHPStan\Type\Generic\TemplateTypeScope;
use PHPStan\Type\Generic\TemplateTypeVariance;
use PHPStan\Type\Traits\ConstantNumericComparisonTypeTrait;
use SimpleXMLElement;
use stdClass;
use Throwable;
Expand Down Expand Up @@ -418,6 +419,16 @@ public function dataIsSuperTypeOf(): array
new ObjectType(ExtendsThrowable::class),
TrinaryLogic::createMaybe(),
],
59 => [
new ObjectType(DateTime::class),
new ObjectType(ConstantNumericComparisonTypeTrait::class),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty sure php has no built-in traits so i used one that phpstan has, i feel like theres a better way to do this though

TrinaryLogic::createNo(),
],
60 => [
new ObjectType(ConstantNumericComparisonTypeTrait::class),
new ObjectType(DateTime::class),
TrinaryLogic::createNo(),
],
];
}

Expand Down