From 8b00381f0cee7f3438d062926f536a137e8b52e2 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Fri, 23 Oct 2026 20:56:45 +0200 Subject: [PATCH 1/3] add NoLongInstanceofChainRule to forbid more than 7 instanceof in one boolean chain --- phpstan.neon | 15 +++ .../src/Rule/NoLongInstanceofChainRule.php | 93 +++++++++++++++++++ .../NoLongInstanceofChainRuleTest.php | 32 +++++++ .../Source/LongChain.php | 23 +++++ .../Source/SkipShortChain.php | 20 ++++ 5 files changed, 183 insertions(+) create mode 100644 utils/phpstan/src/Rule/NoLongInstanceofChainRule.php create mode 100644 utils/phpstan/tests/Rule/NoLongInstanceofChainRule/NoLongInstanceofChainRuleTest.php create mode 100644 utils/phpstan/tests/Rule/NoLongInstanceofChainRule/Source/LongChain.php create mode 100644 utils/phpstan/tests/Rule/NoLongInstanceofChainRule/Source/SkipShortChain.php diff --git a/phpstan.neon b/phpstan.neon index ef2ef1e13e0..385a4667b2e 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -6,6 +6,7 @@ rules: - Rector\Utils\PHPStan\Rule\RegisterRelatedPolyfillRectorRule - Rector\Utils\PHPStan\Rule\NoResolveClassReflectionInForeachRule - Rector\Utils\PHPStan\Rule\RegisterRectorRule + - Rector\Utils\PHPStan\Rule\NoLongInstanceofChainRule parameters: level: 8 @@ -77,6 +78,20 @@ parameters: offloadCollectorData: false ignoreErrors: + # long instanceof chains, to refactor into a helper/match/lookup later + - + identifier: rector.longInstanceofChain + path: src/NodeAnalyzer/ExprAnalyzer.php + count: 2 + - + identifier: rector.longInstanceofChain + path: src/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php + count: 1 + - + identifier: rector.longInstanceofChain + path: src/PhpParser/Node/NodeFactory.php + count: 1 + # called magically by parent - message: '#^Unused Rector\\PhpParser\\Printer\\BetterStandardPrinter\:\:(pInterpolatedStringPart|pStmt_FileNode)$#' diff --git a/utils/phpstan/src/Rule/NoLongInstanceofChainRule.php b/utils/phpstan/src/Rule/NoLongInstanceofChainRule.php new file mode 100644 index 00000000000..2f14c9c25fc --- /dev/null +++ b/utils/phpstan/src/Rule/NoLongInstanceofChainRule.php @@ -0,0 +1,93 @@ + + * @see \Rector\Utils\PHPStan\Tests\Rule\NoLongInstanceofChainRule\NoLongInstanceofChainRuleTest + */ +final readonly class NoLongInstanceofChainRule implements Rule +{ + private const int MAX_INSTANCEOF_COUNT = 7; + + private const string ERROR_MESSAGE = 'Boolean chain has %d instanceof checks, more than the allowed %d. Extract it to a helper method, a match(), or a keyed lookup.'; + + private readonly NodeFinder $nodeFinder; + + public function __construct() + { + $this->nodeFinder = new NodeFinder(); + } + + public function getNodeType(): string + { + return BinaryOp::class; + } + + /** + * @param BinaryOp $node + * @return list + */ + public function processNode(Node $node, Scope $scope): array + { + if (! $this->isBooleanBinaryOp($node)) { + return []; + } + + $instanceofCount = $this->countInstanceof($node); + if ($instanceofCount <= self::MAX_INSTANCEOF_COUNT) { + return []; + } + + // report a chain once: if an inner boolean operand already exceeds the limit, that inner + // node reports it instead (nodes are visited inner-first, so avoid the outer duplicate) + foreach ([$node->left, $node->right] as $operand) { + if ($operand instanceof BinaryOp + && $this->isBooleanBinaryOp($operand) + && $this->countInstanceof($operand) > self::MAX_INSTANCEOF_COUNT) { + return []; + } + } + + $errorMessage = sprintf(self::ERROR_MESSAGE, $instanceofCount, self::MAX_INSTANCEOF_COUNT); + + return [ + RuleErrorBuilder::message($errorMessage) + ->identifier('rector.longInstanceofChain') + ->line($node->getStartLine()) + ->build(), + ]; + } + + private function countInstanceof(BinaryOp $binaryOp): int + { + return count($this->nodeFinder->findInstanceOf([$binaryOp], Instanceof_::class)); + } + + private function isBooleanBinaryOp(BinaryOp $binaryOp): bool + { + return $binaryOp instanceof BooleanOr + || $binaryOp instanceof BooleanAnd + || $binaryOp instanceof LogicalOr + || $binaryOp instanceof LogicalAnd; + } +} diff --git a/utils/phpstan/tests/Rule/NoLongInstanceofChainRule/NoLongInstanceofChainRuleTest.php b/utils/phpstan/tests/Rule/NoLongInstanceofChainRule/NoLongInstanceofChainRuleTest.php new file mode 100644 index 00000000000..73423199bfd --- /dev/null +++ b/utils/phpstan/tests/Rule/NoLongInstanceofChainRule/NoLongInstanceofChainRuleTest.php @@ -0,0 +1,32 @@ + + */ +final class NoLongInstanceofChainRuleTest extends RuleTestCase +{ + public function testLongChain(): void + { + $expectedErrorMessage = 'Boolean chain has 8 instanceof checks, more than the allowed 7. Extract it to a helper method, a match(), or a keyed lookup.'; + + $this->analyse([__DIR__ . '/Source/LongChain.php'], [[$expectedErrorMessage, 13]]); + } + + public function testSkipShortChain(): void + { + $this->analyse([__DIR__ . '/Source/SkipShortChain.php'], []); + } + + protected function getRule(): Rule + { + return new NoLongInstanceofChainRule(); + } +} diff --git a/utils/phpstan/tests/Rule/NoLongInstanceofChainRule/Source/LongChain.php b/utils/phpstan/tests/Rule/NoLongInstanceofChainRule/Source/LongChain.php new file mode 100644 index 00000000000..3df56b711ac --- /dev/null +++ b/utils/phpstan/tests/Rule/NoLongInstanceofChainRule/Source/LongChain.php @@ -0,0 +1,23 @@ + Date: Fri, 23 Oct 2026 18:57:54 +0000 Subject: [PATCH 2/3] [ci-review] Rector Rectify --- utils/phpstan/src/Rule/NoLongInstanceofChainRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/phpstan/src/Rule/NoLongInstanceofChainRule.php b/utils/phpstan/src/Rule/NoLongInstanceofChainRule.php index 2f14c9c25fc..4627f23b1b8 100644 --- a/utils/phpstan/src/Rule/NoLongInstanceofChainRule.php +++ b/utils/phpstan/src/Rule/NoLongInstanceofChainRule.php @@ -31,7 +31,7 @@ private const string ERROR_MESSAGE = 'Boolean chain has %d instanceof checks, more than the allowed %d. Extract it to a helper method, a match(), or a keyed lookup.'; - private readonly NodeFinder $nodeFinder; + private NodeFinder $nodeFinder; public function __construct() { From 99d5befd197eb83eb02221d4c863cb7c224b9029 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Fri, 23 Oct 2026 18:28:02 +0200 Subject: [PATCH 3/3] add bench comparing instanceof chain vs property_exists for scope expr guard --- bench/scope_expr_dispatch_bench.php | 188 ++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 bench/scope_expr_dispatch_bench.php diff --git a/bench/scope_expr_dispatch_bench.php b/bench/scope_expr_dispatch_bench.php new file mode 100644 index 00000000000..6bcd79196c9 --- /dev/null +++ b/bench/scope_expr_dispatch_bench.php @@ -0,0 +1,188 @@ +expr for a hand-listed set of node types via a long + * instanceof chain. This compares that chain against the shorter property_exists($node, 'expr'). + * + * Two things are measured: + * 1. Equivalence - which node types each predicate matches. They are NOT the same set. + * 2. Speed - CPU time (user+sys) over a REPEAT loop; wall is reported but noisy under load. + * + * Run: + * php bench/scope_expr_dispatch_bench.php + */ + +use PhpParser\Node; +use PhpParser\Node\Expr; +use PhpParser\Node\Expr\ArrowFunction; +use PhpParser\Node\Expr\Assign; +use PhpParser\Node\Expr\AssignOp; +use PhpParser\Node\Expr\AssignRef; +use PhpParser\Node\Expr\BitwiseNot; +use PhpParser\Node\Expr\BooleanNot; +use PhpParser\Node\Expr\Cast; +use PhpParser\Node\Expr\Clone_; +use PhpParser\Node\Expr\Empty_; +use PhpParser\Node\Expr\ErrorSuppress; +use PhpParser\Node\Expr\Eval_; +use PhpParser\Node\Expr\Exit_; +use PhpParser\Node\Expr\Include_; +use PhpParser\Node\Expr\Instanceof_; +use PhpParser\Node\Expr\Print_; +use PhpParser\Node\Expr\Throw_; +use PhpParser\Node\Expr\UnaryMinus; +use PhpParser\Node\Expr\UnaryPlus; +use PhpParser\Node\Expr\Variable; +use PhpParser\Node\Expr\YieldFrom; +use PhpParser\Node\Stmt\EnumCase; +use PhpParser\Node\Stmt\Expression; +use PhpParser\Node\Stmt\Foreach_; +use PhpParser\Node\Stmt\Nop; +use PhpParser\Node\Stmt\Return_; + +require __DIR__ . '/../vendor/autoload.php'; + +const REPEAT = 500_000; + +/** + * Current production guard: the explicit instanceof chain. + */ +function matchesChain(Node $node): bool +{ + return ( + $node instanceof Expression || + $node instanceof Return_ || + $node instanceof EnumCase || + $node instanceof Cast || + $node instanceof YieldFrom || + $node instanceof UnaryMinus || + $node instanceof UnaryPlus || + $node instanceof Throw_ || + $node instanceof Empty_ || + $node instanceof BooleanNot || + $node instanceof Clone_ || + $node instanceof ErrorSuppress || + $node instanceof BitwiseNot || + $node instanceof Eval_ || + $node instanceof Print_ || + $node instanceof Exit_ || + $node instanceof Include_ || + $node instanceof Instanceof_ + ) && $node->expr instanceof Expr; +} + +/** + * Candidate guard: a single property lookup. + */ +function matchesPropertyExists(Node $node): bool +{ + return property_exists($node, 'expr') && $node->expr instanceof Expr; +} + +$v = new Variable('x'); + +// nodes that carry an $expr property (set it so the Expr guard passes) +$withExpr = [ + new Expression($v), + new Return_($v), + new EnumCase('CASE', $v), + new Cast\Int_($v), + new YieldFrom($v), + new UnaryMinus($v), + new UnaryPlus($v), + new Throw_($v), + new Empty_($v), + new BooleanNot($v), + new Clone_($v), + new ErrorSuppress($v), + new BitwiseNot($v), + new Eval_($v), + new Print_($v), + new Exit_($v), + new Include_($v, Include_::TYPE_INCLUDE), + new Instanceof_($v, new Node\Name('Foo')), + // the five extra types property_exists also matches, the chain does not: + new Assign($v, $v), + new AssignOp\Plus($v, $v), + new AssignRef($v, $v), + new ArrowFunction([ + 'expr' => $v, + ]), + new Foreach_($v, $v), +]; + +// nodes without an $expr property (both predicates must reject) +$withoutExpr = [ + new Variable('y'), + new Nop(), + new Node\Name('Bar'), + new Node\Arg($v), +]; + +$all = [...$withExpr, ...$withoutExpr]; + +// --- 1. equivalence ---------------------------------------------------------- +$divergent = []; +foreach ($all as $node) { + if (matchesChain($node) !== matchesPropertyExists($node)) { + $divergent[] = $node::class; + } +} + +echo "Equivalence check\n"; +echo "-----------------\n"; +if ($divergent === []) { + echo "identical on sample set\n\n"; +} else { + echo "DIVERGENT - property_exists matches but chain does not:\n"; + foreach ($divergent as $class) { + echo ' - ' . $class . "\n"; + } + + echo "\n"; +} + +// --- 2. speed ---------------------------------------------------------------- +function cpuSeconds(): float +{ + $usage = getrusage(); + return $usage['ru_utime.tv_sec'] + $usage['ru_utime.tv_usec'] / 1e6 + + $usage['ru_stime.tv_sec'] + $usage['ru_stime.tv_usec'] / 1e6; +} + +function run(callable $predicate, array $all): void +{ + for ($r = 0; $r < REPEAT; ++$r) { + foreach ($all as $node) { + $predicate($node); + } + } +} + +// warmup (excluded) +run(matchesChain(...), $all); +run(matchesPropertyExists(...), $all); + +$cpu0 = cpuSeconds(); +$wall0 = microtime(true); +run(matchesChain(...), $all); +$chainCpu = cpuSeconds() - $cpu0; +$chainWall = microtime(true) - $wall0; + +$cpu0 = cpuSeconds(); +$wall0 = microtime(true); +run(matchesPropertyExists(...), $all); +$peCpu = cpuSeconds() - $cpu0; +$peWall = microtime(true) - $wall0; + +$calls = REPEAT * count($all); + +printf("Speed (%s calls each)\n", number_format($calls)); +echo "---------------------\n"; +printf("instanceof chain : CPU %.3fs wall %.3fs (%.1f ns/call)\n", $chainCpu, $chainWall, $chainCpu / $calls * 1e9); +printf("property_exists : CPU %.3fs wall %.3fs (%.1f ns/call)\n", $peCpu, $peWall, $peCpu / $calls * 1e9); +printf("ratio (chain/pe) : %.2fx CPU\n", $peCpu > 0 ? $chainCpu / $peCpu : 0.0);