Skip to content

Commit

Permalink
Fix another BooleanOr issue
Browse files Browse the repository at this point in the history
  • Loading branch information
@ondrejmirtes
ondrejmirtes committed Feb 25, 2022
1 parent a483c11 commit cc6655f
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 28 deletions.
22 changes: 9 additions & 13 deletions src/Analyser/SpecifiedTypes.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,31 +58,28 @@ public function getNewConditionalExpressionHolders(): array
/** @api */
public function intersectWith(SpecifiedTypes $other): self
{
$normalized = $this->normalize();
$otherNormalized = $other->normalize();

$sureTypeUnion = [];
$sureNotTypeUnion = [];

foreach ($normalized->sureTypes as $exprString => [$exprNode, $type]) {
if (!isset($otherNormalized->sureTypes[$exprString])) {
foreach ($this->sureTypes as $exprString => [$exprNode, $type]) {
if (!isset($other->sureTypes[$exprString])) {
continue;
}

$sureTypeUnion[$exprString] = [
$exprNode,
TypeCombinator::union($type, $otherNormalized->sureTypes[$exprString][1]),
TypeCombinator::union($type, $other->sureTypes[$exprString][1]),
];
}

foreach ($normalized->sureNotTypes as $exprString => [$exprNode, $type]) {
if (!isset($otherNormalized->sureNotTypes[$exprString])) {
foreach ($this->sureNotTypes as $exprString => [$exprNode, $type]) {
if (!isset($other->sureNotTypes[$exprString])) {
continue;
}

$sureNotTypeUnion[$exprString] = [
$exprNode,
TypeCombinator::intersect($type, $otherNormalized->sureNotTypes[$exprString][1]),
TypeCombinator::intersect($type, $other->sureNotTypes[$exprString][1]),
];
}

Expand Down Expand Up @@ -120,21 +117,20 @@ public function unionWith(SpecifiedTypes $other): self
return new self($sureTypeUnion, $sureNotTypeUnion);
}

private function normalize(): self
public function normalize(Scope $scope): self
{
$sureTypes = $this->sureTypes;
$sureNotTypes = [];

foreach ($this->sureNotTypes as $exprString => [$exprNode, $sureNotType]) {
if (!isset($sureTypes[$exprString])) {
$sureNotTypes[$exprString] = [$exprNode, $sureNotType];
$sureTypes[$exprString] = [$exprNode, TypeCombinator::remove($scope->getType($exprNode), $sureNotType)];
continue;
}

$sureTypes[$exprString][1] = TypeCombinator::remove($sureTypes[$exprString][1], $sureNotType);
}

return new self($sureTypes, $sureNotTypes, $this->overwrite, $this->newConditionalExpressionHolders);
return new self($sureTypes, [], $this->overwrite, $this->newConditionalExpressionHolders);
}

}
14 changes: 8 additions & 6 deletions src/Analyser/TypeSpecifier.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -674,8 +674,9 @@ public function specifyTypesInCondition(
throw new ShouldNotHappenException();
}
$leftTypes = $this->specifyTypesInCondition($scope, $expr->left, $context);
$rightTypes = $this->specifyTypesInCondition($scope->filterByTruthyValue($expr->left), $expr->right, $context);
$types = $context->true() ? $leftTypes->unionWith($rightTypes) : $leftTypes->intersectWith($rightTypes);
$rightScope = $scope->filterByTruthyValue($expr->left);
$rightTypes = $this->specifyTypesInCondition($rightScope, $expr->right, $context);
$types = $context->true() ? $leftTypes->unionWith($rightTypes) : $leftTypes->normalize($scope)->intersectWith($rightTypes->normalize($rightScope));
if ($context->false()) {
return new SpecifiedTypes(
$types->getSureTypes(),
Expand All @@ -694,8 +695,9 @@ public function specifyTypesInCondition(
throw new ShouldNotHappenException();
}
$leftTypes = $this->specifyTypesInCondition($scope, $expr->left, $context);
$rightTypes = $this->specifyTypesInCondition($scope->filterByFalseyValue($expr->left), $expr->right, $context);
$types = $context->true() ? $leftTypes->intersectWith($rightTypes) : $leftTypes->unionWith($rightTypes);
$rightScope = $scope->filterByFalseyValue($expr->left);
$rightTypes = $this->specifyTypesInCondition($rightScope, $expr->right, $context);
$types = $context->true() ? $leftTypes->normalize($scope)->intersectWith($rightTypes->normalize($rightScope)) : $leftTypes->unionWith($rightTypes);
if ($context->true()) {
return new SpecifiedTypes(
$types->getSureTypes(),
Expand Down Expand Up @@ -849,7 +851,7 @@ public function specifyTypesInCondition(
);

$nullSafeTypes = $this->handleDefaultTruthyOrFalseyContext($context, $expr, $scope);
return $context->true() ? $types->unionWith($nullSafeTypes) : $types->intersectWith($nullSafeTypes);
return $context->true() ? $types->unionWith($nullSafeTypes) : $types->normalize($scope)->intersectWith($nullSafeTypes->normalize($scope));
} elseif ($expr instanceof Expr\NullsafeMethodCall && !$context->null()) {
$types = $this->specifyTypesInCondition(
$scope,
Expand All @@ -861,7 +863,7 @@ public function specifyTypesInCondition(
);

$nullSafeTypes = $this->handleDefaultTruthyOrFalseyContext($context, $expr, $scope);
return $context->true() ? $types->unionWith($nullSafeTypes) : $types->intersectWith($nullSafeTypes);
return $context->true() ? $types->unionWith($nullSafeTypes) : $types->normalize($scope)->intersectWith($nullSafeTypes->normalize($scope));
} elseif (!$context->null()) {
return $this->handleDefaultTruthyOrFalseyContext($context, $expr, $scope);
}
Expand Down
2 changes: 2 additions & 0 deletions tests/PHPStan/Analyser/NodeScopeResolverTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,8 @@ public function dataFileAsserts(): iterable

require_once __DIR__ . '/data/countable.php';
yield from $this->gatherAssertTypes(__DIR__ . '/data/countable.php');

yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6696.php');
}

/**
Expand Down
12 changes: 6 additions & 6 deletions tests/PHPStan/Analyser/TypeSpecifierTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ public function dataCondition(): array
$this->createFunctionCall('random'),
),
[],
['$foo' => '~*NEVER*'],
['$foo' => 'mixed'],
],
[
new Expr\BinaryOp\BooleanOr(
Expand Down Expand Up @@ -342,7 +342,7 @@ public function dataCondition(): array
),
$this->createFunctionCall('random'),
),
['$foo' => '~*NEVER*'],
['$foo' => 'mixed'],
[],
],

Expand Down Expand Up @@ -975,7 +975,7 @@ public function dataCondition(): array
new Identical(new Expr\ConstFetch(new Name('null')), new Variable('a')),
),
['$a' => 'non-empty-string|null'],
['$a' => '~null'],
['$a' => 'mixed~non-empty-string & ~null'],
],
[
new Expr\BinaryOp\BooleanOr(
Expand All @@ -989,7 +989,7 @@ public function dataCondition(): array
new Identical(new Expr\ConstFetch(new Name('null')), new Variable('a')),
),
['$a' => 'non-empty-string|null'],
['$a' => '~non-empty-string|null'],
['$a' => 'mixed~non-empty-string & ~null'],
],
[
new Expr\BinaryOp\BooleanOr(
Expand All @@ -1003,7 +1003,7 @@ public function dataCondition(): array
new Identical(new Expr\ConstFetch(new Name('null')), new Variable('a')),
),
['$a' => 'non-empty-array|null'],
['$a' => '~non-empty-array|null'],
['$a' => 'mixed~non-empty-array & ~null'],
],
[
new Expr\BinaryOp\BooleanAnd(
Expand Down Expand Up @@ -1072,7 +1072,7 @@ public function dataCondition(): array
'strlen($foo)' => '~0',
],
[
'$foo' => '~non-empty-string',
'$foo' => 'mixed~non-empty-string',
],
],
];
Expand Down
2 changes: 1 addition & 1 deletion tests/PHPStan/Analyser/data/bug-3009.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public function createRedirectRequest(string $redirectUri): ?string
{
$redirectUrlParts = parse_url($redirectUri);
if (false === is_array($redirectUrlParts) || true === array_key_exists('host', $redirectUrlParts)) {
assertType('array{scheme?: string, host?: string, port?: int, user?: string, pass?: string, path?: string, query?: string, fragment?: string}|false', $redirectUrlParts);
assertType('array{scheme?: string, host: string, port?: int, user?: string, pass?: string, path?: string, query?: string, fragment?: string}|false', $redirectUrlParts);
return null;
}

Expand Down
20 changes: 20 additions & 0 deletions tests/PHPStan/Analyser/data/bug-6696.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace Bug6696;

use function PHPStan\Testing\assertType;

class HelloWorld
{
public function sayHello(?string $s): void
{
assert($s !== '');
assertType('non-empty-string|null', $s);
}

public function sayHello2(?string $s): void
{
assert($s === null || $s !== '');
assertType('non-empty-string|null', $s);
}
}
2 changes: 0 additions & 2 deletions tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ public function testInstanceof(): void
[
'Instanceof between *NEVER* and ImpossibleInstanceOf\Foo will always evaluate to false.',
234,
$tipText,
],
[
'Instanceof between ImpossibleInstanceOf\Bar&ImpossibleInstanceOf\Foo and ImpossibleInstanceOf\Foo will always evaluate to true.',
Expand Down Expand Up @@ -227,7 +226,6 @@ public function testInstanceofWithoutAlwaysTrue(): void
[
'Instanceof between *NEVER* and ImpossibleInstanceOf\Foo will always evaluate to false.',
234,
$tipText,
],
[
'Instanceof between *NEVER* and ImpossibleInstanceOf\Bar will always evaluate to false.',
Expand Down

1 comment on commit cc6655f

@herndlm
Copy link
Contributor

@herndlm herndlm commented on cc6655f Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

NICE

the outcome is close to my temporary SubtractableType solution I had a couple hours ago ;) I stopped at the point when I had the thought of getting the scope somehow into the normalization hehe

a couple of the TypeSpecifier test cases look a bit weird maybe, but if the scope filtering outcome is fine, it should be good I guess

Please sign in to comment.