Skip to content

Commit

Permalink
Show TypeResult reasons in StrictComparisonOfDifferentTypesRule
Browse files Browse the repository at this point in the history
  • Loading branch information
ondrejmirtes committed Oct 18, 2024
1 parent a815d57 commit 34bacd7
Show file tree
Hide file tree
Showing 11 changed files with 173 additions and 50 deletions.
3 changes: 3 additions & 0 deletions conf/config.neon
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,9 @@ services:
arguments:
cacheFilePath: %resultCachePath%

-
class: PHPStan\Analyser\RicherScopeGetTypeHelper

-
class: PHPStan\Cache\Cache
arguments:
Expand Down
2 changes: 2 additions & 0 deletions src/Analyser/DirectInternalScopeFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public function __construct(
private PropertyReflectionFinder $propertyReflectionFinder,
private Parser $parser,
private NodeScopeResolver $nodeScopeResolver,
private RicherScopeGetTypeHelper $richerScopeGetTypeHelper,
private PhpVersion $phpVersion,
private bool $explicitMixedInUnknownGenericNew,
private bool $explicitMixedForGlobalVariables,
Expand Down Expand Up @@ -85,6 +86,7 @@ public function create(
$this->propertyReflectionFinder,
$this->parser,
$this->nodeScopeResolver,
$this->richerScopeGetTypeHelper,
$this->constantResolver,
$context,
$this->phpVersion,
Expand Down
1 change: 1 addition & 0 deletions src/Analyser/LazyInternalScopeFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public function create(
$this->container->getByType(PropertyReflectionFinder::class),
$this->container->getService('currentPhpVersionSimpleParser'),
$this->container->getByType(NodeScopeResolver::class),
$this->container->getByType(RicherScopeGetTypeHelper::class),
$this->container->getByType(ConstantResolver::class),
$context,
$this->container->getByType(PhpVersion::class),
Expand Down
42 changes: 4 additions & 38 deletions src/Analyser/MutatingScope.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ public function __construct(
private PropertyReflectionFinder $propertyReflectionFinder,
private Parser $parser,
private NodeScopeResolver $nodeScopeResolver,
private RicherScopeGetTypeHelper $richerScopeGetTypeHelper,
private ConstantResolver $constantResolver,
private ScopeContext $context,
private PhpVersion $phpVersion,
Expand Down Expand Up @@ -924,46 +925,11 @@ private function resolveType(string $exprString, Expr $node): Type
}

if ($node instanceof Expr\BinaryOp\Identical) {
if (
$node->left instanceof Variable
&& is_string($node->left->name)
&& $node->right instanceof Variable
&& is_string($node->right->name)
&& $node->left->name === $node->right->name
) {
return new ConstantBooleanType(true);
}

$leftType = $this->getType($node->left);
$rightType = $this->getType($node->right);

if (
(
$node->left instanceof Node\Expr\PropertyFetch
|| $node->left instanceof Node\Expr\StaticPropertyFetch
)
&& $rightType->isNull()->yes()
&& !$this->hasPropertyNativeType($node->left)
) {
return new BooleanType();
}

if (
(
$node->right instanceof Node\Expr\PropertyFetch
|| $node->right instanceof Node\Expr\StaticPropertyFetch
)
&& $leftType->isNull()->yes()
&& !$this->hasPropertyNativeType($node->right)
) {
return new BooleanType();
}

return $this->initializerExprTypeResolver->resolveIdenticalType($leftType, $rightType)->type;
return $this->richerScopeGetTypeHelper->getIdenticalResult($this, $node)->type;
}

if ($node instanceof Expr\BinaryOp\NotIdentical) {
return $this->getType(new Expr\BooleanNot(new BinaryOp\Identical($node->left, $node->right)));
return $this->richerScopeGetTypeHelper->getNotIdenticalResult($this, $node)->type;
}

if ($node instanceof Expr\Instanceof_) {
Expand Down Expand Up @@ -2654,7 +2620,7 @@ private function promoteNativeTypes(): self
/**
* @param Node\Expr\PropertyFetch|Node\Expr\StaticPropertyFetch $propertyFetch
*/
private function hasPropertyNativeType($propertyFetch): bool
public function hasPropertyNativeType($propertyFetch): bool
{
$propertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($propertyFetch, $this);
if ($propertyReflection === null) {
Expand Down
78 changes: 78 additions & 0 deletions src/Analyser/RicherScopeGetTypeHelper.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php declare(strict_types = 1);

namespace PHPStan\Analyser;

use PhpParser\Node;
use PhpParser\Node\Expr\BinaryOp\Identical;
use PhpParser\Node\Expr\Variable;
use PHPStan\Reflection\InitializerExprTypeResolver;
use PHPStan\Type\BooleanType;
use PHPStan\Type\Constant\ConstantBooleanType;
use PHPStan\Type\TypeResult;
use function is_string;

final class RicherScopeGetTypeHelper
{

public function __construct(private InitializerExprTypeResolver $initializerExprTypeResolver)
{
}

/**
* @return TypeResult<BooleanType>
*/
public function getIdenticalResult(Scope $scope, Identical $expr): TypeResult
{
if (
$expr->left instanceof Variable
&& is_string($expr->left->name)
&& $expr->right instanceof Variable
&& is_string($expr->right->name)
&& $expr->left->name === $expr->right->name
) {
return new TypeResult(new ConstantBooleanType(true), []);
}

$leftType = $scope->getType($expr->left);
$rightType = $scope->getType($expr->right);

if (
(
$expr->left instanceof Node\Expr\PropertyFetch
|| $expr->left instanceof Node\Expr\StaticPropertyFetch
)
&& $rightType->isNull()->yes()
&& !$scope->hasPropertyNativeType($expr->left)
) {
return new TypeResult(new BooleanType(), []);
}

if (
(
$expr->right instanceof Node\Expr\PropertyFetch
|| $expr->right instanceof Node\Expr\StaticPropertyFetch
)
&& $leftType->isNull()->yes()
&& !$scope->hasPropertyNativeType($expr->right)
) {
return new TypeResult(new BooleanType(), []);
}

return $this->initializerExprTypeResolver->resolveIdenticalType($leftType, $rightType);
}

/**
* @return TypeResult<BooleanType>
*/
public function getNotIdenticalResult(Scope $scope, Node\Expr\BinaryOp\NotIdentical $expr): TypeResult
{
$identicalResult = $this->getIdenticalResult($scope, new Identical($expr->left, $expr->right));
$identicalType = $identicalResult->type;
if ($identicalType instanceof ConstantBooleanType) {
return new TypeResult(new ConstantBooleanType(!$identicalType->getValue()), $identicalResult->reasons);
}

return new TypeResult(new BooleanType(), []);
}

}
18 changes: 15 additions & 3 deletions src/Rules/Comparison/StrictComparisonOfDifferentTypesRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
namespace PHPStan\Rules\Comparison;

use PhpParser\Node;
use PHPStan\Analyser\RicherScopeGetTypeHelper;
use PHPStan\Analyser\Scope;
use PHPStan\Parser\LastConditionVisitor;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\TrinaryLogic;
use PHPStan\Type\Constant\ConstantBooleanType;
use PHPStan\Type\VerbosityLevel;
use function count;
use function sprintf;

/**
Expand All @@ -19,6 +21,7 @@ final class StrictComparisonOfDifferentTypesRule implements Rule
{

public function __construct(
private RicherScopeGetTypeHelper $richerScopeGetTypeHelper,
private bool $checkAlwaysTrueStrictComparison,
private bool $treatPhpDocTypesAsCertain,
private bool $reportAlwaysTrueInLastCondition,
Expand All @@ -34,19 +37,28 @@ public function getNodeType(): string

public function processNode(Node $node, Scope $scope): array
{
if (!$node instanceof Node\Expr\BinaryOp\Identical && !$node instanceof Node\Expr\BinaryOp\NotIdentical) {
if ($node instanceof Node\Expr\BinaryOp\Identical) {
$nodeTypeResult = $this->richerScopeGetTypeHelper->getIdenticalResult($this->treatPhpDocTypesAsCertain ? $scope : $scope->doNotTreatPhpDocTypesAsCertain(), $node);
} elseif ($node instanceof Node\Expr\BinaryOp\NotIdentical) {
$nodeTypeResult = $this->richerScopeGetTypeHelper->getNotIdenticalResult($this->treatPhpDocTypesAsCertain ? $scope : $scope->doNotTreatPhpDocTypesAsCertain(), $node);
} else {
return [];
}

$nodeType = $this->treatPhpDocTypesAsCertain ? $scope->getType($node) : $scope->getNativeType($node);
$nodeType = $nodeTypeResult->type;
if (!$nodeType instanceof ConstantBooleanType) {
return [];
}

$leftType = $this->treatPhpDocTypesAsCertain ? $scope->getType($node->left) : $scope->getNativeType($node->left);
$rightType = $this->treatPhpDocTypesAsCertain ? $scope->getType($node->right) : $scope->getNativeType($node->right);

$addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node): RuleErrorBuilder {
$addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $nodeTypeResult): RuleErrorBuilder {
$reasons = $nodeTypeResult->reasons;
if (count($reasons) > 0) {
return $ruleErrorBuilder->acceptsReasonsTip($reasons);
}

if (!$this->treatPhpDocTypesAsCertain) {
return $ruleErrorBuilder;
}
Expand Down
20 changes: 12 additions & 8 deletions src/Testing/PHPStanTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use PHPStan\Analyser\Error;
use PHPStan\Analyser\MutatingScope;
use PHPStan\Analyser\NodeScopeResolver;
use PHPStan\Analyser\RicherScopeGetTypeHelper;
use PHPStan\Analyser\ScopeFactory;
use PHPStan\Analyser\TypeSpecifier;
use PHPStan\BetterReflection\Reflector\ClassReflector;
Expand Down Expand Up @@ -168,25 +169,28 @@ public static function createScopeFactory(ReflectionProvider $reflectionProvider
$reflectionProviderProvider = new DirectReflectionProviderProvider($reflectionProvider);
$constantResolver = new ConstantResolver($reflectionProviderProvider, $dynamicConstantNames);

$initializerExprTypeResolver = new InitializerExprTypeResolver(
$constantResolver,
$reflectionProviderProvider,
$container->getByType(PhpVersion::class),
$container->getByType(OperatorTypeSpecifyingExtensionRegistryProvider::class),
new OversizedArrayBuilder(),
$container->getParameter('usePathConstantsAsConstantString'),
);

return new ScopeFactory(
new DirectInternalScopeFactory(
MutatingScope::class,
$reflectionProvider,
new InitializerExprTypeResolver(
$constantResolver,
$reflectionProviderProvider,
$container->getByType(PhpVersion::class),
$container->getByType(OperatorTypeSpecifyingExtensionRegistryProvider::class),
new OversizedArrayBuilder(),
$container->getParameter('usePathConstantsAsConstantString'),
),
$initializerExprTypeResolver,
$container->getByType(DynamicReturnTypeExtensionRegistryProvider::class),
$container->getByType(ExpressionTypeResolverExtensionRegistryProvider::class),
$container->getByType(ExprPrinter::class),
$typeSpecifier,
new PropertyReflectionFinder(),
self::getParser(),
$container->getByType(NodeScopeResolver::class),
new RicherScopeGetTypeHelper($initializerExprTypeResolver),
$container->getByType(PhpVersion::class),
$container->getParameter('featureToggles')['explicitMixedInUnknownGenericNew'],
$container->getParameter('featureToggles')['explicitMixedForGlobalVariables'],
Expand Down
13 changes: 12 additions & 1 deletion src/Type/MixedType.php
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,18 @@ public function isSuperTypeOfWithReason(Type $type): IsSuperTypeOfResult
return IsSuperTypeOfResult::createMaybe();
}

return $this->subtractedType->isSuperTypeOfWithReason($type)->negate();
$result = $this->subtractedType->isSuperTypeOfWithReason($type)->negate();
if ($result->no()) {
return IsSuperTypeOfResult::createNo([
sprintf(
'Type %s has already been eliminated from %s.',
$this->subtractedType->describe(VerbosityLevel::precise()),
$this->describe(VerbosityLevel::typeOnly()),
),
]);
}

return $result;
}

public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $unionValues = true): Type
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace PHPStan\Rules\Comparison;

use PHPStan\Analyser\RicherScopeGetTypeHelper;
use PHPStan\Rules\Rule;
use PHPStan\Testing\RuleTestCase;
use const PHP_INT_SIZE;
Expand All @@ -22,6 +23,7 @@ class StrictComparisonOfDifferentTypesRuleTest extends RuleTestCase
protected function getRule(): Rule
{
return new StrictComparisonOfDifferentTypesRule(
self::getContainer()->getByType(RicherScopeGetTypeHelper::class),
$this->checkAlwaysTrueStrictComparison,
$this->treatPhpDocTypesAsCertain,
$this->reportAlwaysTrueInLastCondition,
Expand Down Expand Up @@ -216,10 +218,12 @@ public function testStrictComparison(): void
[
'Strict comparison using === between mixed and \'foo\' will always evaluate to false.',
808,
'Type 1|string has already been eliminated from mixed.',
],
[
'Strict comparison using !== between mixed and 1 will always evaluate to true.',
812,
'Type 1|string has already been eliminated from mixed.',
],
[
'Strict comparison using === between \'foo\' and \'foo\' will always evaluate to true.',
Expand Down Expand Up @@ -275,6 +279,16 @@ public function testStrictComparison(): void
1014,
$tipText,
],
[
'Strict comparison using === between mixed and null will always evaluate to false.',
1030,
'Type null has already been eliminated from mixed.',
],
[
'Strict comparison using !== between mixed and null will always evaluate to true.',
1034,
'Type null has already been eliminated from mixed.',
],
],
);
}
Expand Down Expand Up @@ -419,6 +433,7 @@ public function testStrictComparisonWithoutAlwaysTrue(): void
[
'Strict comparison using === between mixed and \'foo\' will always evaluate to false.',
808,
'Type 1|string has already been eliminated from mixed.',
],
[
'Strict comparison using === between NAN and NAN will always evaluate to false.',
Expand All @@ -433,6 +448,11 @@ public function testStrictComparisonWithoutAlwaysTrue(): void
1014,
$tipText,
],
[
'Strict comparison using === between mixed and null will always evaluate to false.',
1030,
'Type null has already been eliminated from mixed.',
],
],
);
}
Expand Down
20 changes: 20 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/strict-comparison.php
Original file line number Diff line number Diff line change
Expand Up @@ -1017,3 +1017,23 @@ public function doFoo($a): void
}

}

class SubtractedMixedAgainstNull
{

public function doFoo($m): void
{
if ($m === null) {
return;
}

if ($m === null) {

}

if ($m !== null) {

}
}

}
Loading

0 comments on commit 34bacd7

Please sign in to comment.