Skip to content

Commit

Permalink
Fix performance problem with nested BooleanOr and BooleanAnd
Browse files Browse the repository at this point in the history
  • Loading branch information
@ondrejmirtes
ondrejmirtes committed Aug 8, 2023
1 parent 1b0c6a0 commit 9adae6c
Show file tree
Hide file tree
Showing 3 changed files with 212 additions and 8 deletions.
40 changes: 32 additions & 8 deletions src/Analyser/MutatingScope.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@
class MutatingScope implements Scope
{

private const BOOLEAN_EXPRESSION_MAX_PROCESS_DEPTH = 4;

/** @var Type[] */
private array $resolvedTypes = [];

Expand Down Expand Up @@ -768,10 +770,14 @@ private function resolveType(string $exprString, Expr $node): Type
return new ConstantBooleanType(false);
}

$noopCallback = static function (): void {
};
$leftResult = $this->nodeScopeResolver->processExprNode($node->left, $this, $noopCallback, ExpressionContext::createDeep());
$rightBooleanType = $leftResult->getTruthyScope()->getType($node->right)->toBoolean();
if ($this->getBooleanExpressionDepth($node->left) <= self::BOOLEAN_EXPRESSION_MAX_PROCESS_DEPTH) {
$noopCallback = static function (): void {
};
$leftResult = $this->nodeScopeResolver->processExprNode($node->left, $this, $noopCallback, ExpressionContext::createDeep());
$rightBooleanType = $leftResult->getTruthyScope()->getType($node->right)->toBoolean();
} else {
$rightBooleanType = $this->filterByTruthyValue($node->left)->getType($node->right)->toBoolean();
}

if ($rightBooleanType->isFalse()->yes()) {
return new ConstantBooleanType(false);
Expand All @@ -796,10 +802,14 @@ private function resolveType(string $exprString, Expr $node): Type
return new ConstantBooleanType(true);
}

$noopCallback = static function (): void {
};
$leftResult = $this->nodeScopeResolver->processExprNode($node->left, $this, $noopCallback, ExpressionContext::createDeep());
$rightBooleanType = $leftResult->getFalseyScope()->getType($node->right)->toBoolean();
if ($this->getBooleanExpressionDepth($node->left) <= self::BOOLEAN_EXPRESSION_MAX_PROCESS_DEPTH) {
$noopCallback = static function (): void {
};
$leftResult = $this->nodeScopeResolver->processExprNode($node->left, $this, $noopCallback, ExpressionContext::createDeep());
$rightBooleanType = $leftResult->getFalseyScope()->getType($node->right)->toBoolean();
} else {
$rightBooleanType = $this->filterByFalseyValue($node->left)->getType($node->right)->toBoolean();
}

if ($rightBooleanType->isTrue()->yes()) {
return new ConstantBooleanType(true);
Expand Down Expand Up @@ -4707,6 +4717,20 @@ private function compareVariableTypeHolders(array $variableTypeHolders, array $o
return true;
}

private function getBooleanExpressionDepth(Expr $expr, int $depth = 0): int
{
while (
$expr instanceof BinaryOp\BooleanOr
|| $expr instanceof BinaryOp\LogicalOr
|| $expr instanceof BinaryOp\BooleanAnd
|| $expr instanceof BinaryOp\LogicalAnd
) {
return $this->getBooleanExpressionDepth($expr->left, $depth + 1);
}

return $depth;
}

/** @api */
public function canAccessProperty(PropertyReflection $propertyReflection): bool
{
Expand Down
6 changes: 6 additions & 0 deletions tests/PHPStan/Analyser/AnalyserIntegrationTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,12 @@ public function testBug9428(): void
$this->assertNoErrors($errors);
}

public function testBug9690(): void
{
$errors = $this->runAnalyse(__DIR__ . '/data/bug-9690.php');
$this->assertNoErrors($errors);

This comment has been minimized.

Sign in to view

Copy link
@mad-briller

mad-briller Aug 8, 2023

Contributor

it would be nice if there was a way of asserting performance characteristics of code
}

/**
* @param string[]|null $allAnalysedFiles
* @return Error[]
Expand Down
174 changes: 174 additions & 0 deletions tests/PHPStan/Analyser/data/bug-9690.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
<?php

namespace Bug9690;

class par {
public static function isId(string $id): bool {
return (bool) rand(0,1);
}
};

class a1 extends par {};
class a2 extends par {};
class a3 extends par {};
class a4 extends par {};
class a5 extends par {};
class a6 extends par {};
class a7 extends par {};
class a8 extends par {};
class a9 extends par {};
class a10 extends par {};
class a11 extends par {};
class a12 extends par {};
class a13 extends par {};
class a14 extends par {};
class a15 extends par {};
class a16 extends par {};
class a17 extends par {};
class a18 extends par {};
class a19 extends par {};
class a20 extends par {};
class a21 extends par {};
class a22 extends par {};
class a23 extends par {};
class a24 extends par {};
class a25 extends par {};
class a26 extends par {};
class a27 extends par {};
class a28 extends par {};
class a29 extends par {};
class a30 extends par {};
class a31 extends par {};
class a32 extends par {};
class a33 extends par {};
class a34 extends par {};
class a35 extends par {};
class a36 extends par {};
class a37 extends par {};
class a38 extends par {};
class a39 extends par {};
class a40 extends par {};
class a41 extends par {};
class a42 extends par {};
class a43 extends par {};
class a44 extends par {};
class a45 extends par {};
class a46 extends par {};
class a47 extends par {};
class a48 extends par {};
class a49 extends par {};
class a50 extends par {};

class test {
public static function isId(string $id): bool {
return (
a1::isId($id)
|| a2::isId($id)
|| a3::isId($id)
|| a4::isId($id)
|| a5::isId($id)
|| a6::isId($id)
|| a7::isId($id)
|| a8::isId($id)
|| a9::isId($id)
|| a10::isId($id)
|| a11::isId($id)
|| a12::isId($id)
|| a13::isId($id)
|| a14::isId($id)
|| a15::isId($id)
|| a16::isId($id)
|| a17::isId($id)
|| a18::isId($id)
|| a19::isId($id)
|| a20::isId($id)
|| a21::isId($id)
|| a22::isId($id)
|| a23::isId($id)
|| a24::isId($id)
|| a25::isId($id)
|| a26::isId($id)
|| a27::isId($id)
|| a28::isId($id)
|| a29::isId($id)
|| a30::isId($id)
|| a31::isId($id)
|| a32::isId($id)
|| a33::isId($id)
|| a34::isId($id)
|| a35::isId($id)
|| a36::isId($id)
|| a37::isId($id)
|| a38::isId($id)
|| a39::isId($id)
|| a40::isId($id)
|| a41::isId($id)
|| a42::isId($id)
|| a43::isId($id)
|| a44::isId($id)
|| a45::isId($id)
|| a46::isId($id)
|| a47::isId($id)
|| a48::isId($id)
|| a49::isId($id)
|| a50::isId($id)
);
}
}

class test2 {
public static function isId(string $id): bool {
return (
a1::isId($id)
&& a2::isId($id)
&& a3::isId($id)
&& a4::isId($id)
&& a5::isId($id)
&& a6::isId($id)
&& a7::isId($id)
&& a8::isId($id)
&& a9::isId($id)
&& a10::isId($id)
&& a11::isId($id)
&& a12::isId($id)
&& a13::isId($id)
&& a14::isId($id)
&& a15::isId($id)
&& a16::isId($id)
&& a17::isId($id)
&& a18::isId($id)
&& a19::isId($id)
&& a20::isId($id)
&& a21::isId($id)
&& a22::isId($id)
&& a23::isId($id)
&& a24::isId($id)
&& a25::isId($id)
&& a26::isId($id)
&& a27::isId($id)
&& a28::isId($id)
&& a29::isId($id)
&& a30::isId($id)
&& a31::isId($id)
&& a32::isId($id)
&& a33::isId($id)
&& a34::isId($id)
&& a35::isId($id)
&& a36::isId($id)
&& a37::isId($id)
&& a38::isId($id)
&& a39::isId($id)
&& a40::isId($id)
&& a41::isId($id)
&& a42::isId($id)
&& a43::isId($id)
&& a44::isId($id)
&& a45::isId($id)
&& a46::isId($id)
&& a47::isId($id)
&& a48::isId($id)
&& a49::isId($id)
&& a50::isId($id)
);
}
}

0 comments on commit 9adae6c

Please sign in to comment.