From 693c5fefd2fdcf6b07a3375285d4f08e84be28b6 Mon Sep 17 00:00:00 2001 From: Guillaume Sainthillier Date: Wed, 23 Sep 2026 15:08:03 +0200 Subject: [PATCH] [CodeQuality] Skip short-circuit dependent values on RepeatedAndNotEqualToNotInArrayRector and RepeatedOrEqualToInArrayRector A repeated null check where later operands rely on earlier ones: null !== $locale && null !== $translations && null !== $translations->getTranslation($locale) became !in_array(null, [$locale, $translations, $translations->getTranslation($locale)], true) The array literal evaluates every item up front, while && stops at the first failing compare, so getTranslation() now runs on a null $translations, or with a null $locale it does not accept. The || variant in RepeatedOrEqualToInArrayRector had the same issue. Both rules build the array in InArrayFromRepeatedCompareFactory, which now only accepts values that are safe to evaluate eagerly: plain variables, scalars, constants, class constants and constant arrays. Calls, property and array fetches keep the chain as is. --- .../skip_short_circuit_dependent_call.php.inc | 23 ++++++++++++ .../Fixture/yoda_null_on_variables.php.inc | 35 +++++++++++++++++++ .../skip_short_circuit_dependent_call.php.inc | 23 ++++++++++++ .../Fixture/yoda_null_on_variables.php.inc | 35 +++++++++++++++++++ .../InArrayFromRepeatedCompareFactory.php | 28 +++++++++++++-- 5 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 rules-tests/CodeQuality/Rector/BooleanAnd/RepeatedAndNotEqualToNotInArrayRector/Fixture/skip_short_circuit_dependent_call.php.inc create mode 100644 rules-tests/CodeQuality/Rector/BooleanAnd/RepeatedAndNotEqualToNotInArrayRector/Fixture/yoda_null_on_variables.php.inc create mode 100644 rules-tests/CodeQuality/Rector/BooleanOr/RepeatedOrEqualToInArrayRector/Fixture/skip_short_circuit_dependent_call.php.inc create mode 100644 rules-tests/CodeQuality/Rector/BooleanOr/RepeatedOrEqualToInArrayRector/Fixture/yoda_null_on_variables.php.inc diff --git a/rules-tests/CodeQuality/Rector/BooleanAnd/RepeatedAndNotEqualToNotInArrayRector/Fixture/skip_short_circuit_dependent_call.php.inc b/rules-tests/CodeQuality/Rector/BooleanAnd/RepeatedAndNotEqualToNotInArrayRector/Fixture/skip_short_circuit_dependent_call.php.inc new file mode 100644 index 00000000000..20ba892c646 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/BooleanAnd/RepeatedAndNotEqualToNotInArrayRector/Fixture/skip_short_circuit_dependent_call.php.inc @@ -0,0 +1,23 @@ +getTranslation($locale)) { + return true; + } + + return false; + } +} + +final class SomeTranslations +{ + public function getTranslation(string $locale): ?string + { + return null; + } +} diff --git a/rules-tests/CodeQuality/Rector/BooleanAnd/RepeatedAndNotEqualToNotInArrayRector/Fixture/yoda_null_on_variables.php.inc b/rules-tests/CodeQuality/Rector/BooleanAnd/RepeatedAndNotEqualToNotInArrayRector/Fixture/yoda_null_on_variables.php.inc new file mode 100644 index 00000000000..a8fb145c168 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/BooleanAnd/RepeatedAndNotEqualToNotInArrayRector/Fixture/yoda_null_on_variables.php.inc @@ -0,0 +1,35 @@ + +----- + diff --git a/rules-tests/CodeQuality/Rector/BooleanOr/RepeatedOrEqualToInArrayRector/Fixture/skip_short_circuit_dependent_call.php.inc b/rules-tests/CodeQuality/Rector/BooleanOr/RepeatedOrEqualToInArrayRector/Fixture/skip_short_circuit_dependent_call.php.inc new file mode 100644 index 00000000000..1efd4a14ccc --- /dev/null +++ b/rules-tests/CodeQuality/Rector/BooleanOr/RepeatedOrEqualToInArrayRector/Fixture/skip_short_circuit_dependent_call.php.inc @@ -0,0 +1,23 @@ +getTranslation($locale)) { + return true; + } + + return false; + } +} + +final class SomeTranslations +{ + public function getTranslation(string $locale): ?string + { + return null; + } +} diff --git a/rules-tests/CodeQuality/Rector/BooleanOr/RepeatedOrEqualToInArrayRector/Fixture/yoda_null_on_variables.php.inc b/rules-tests/CodeQuality/Rector/BooleanOr/RepeatedOrEqualToInArrayRector/Fixture/yoda_null_on_variables.php.inc new file mode 100644 index 00000000000..ecdaf244961 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/BooleanOr/RepeatedOrEqualToInArrayRector/Fixture/yoda_null_on_variables.php.inc @@ -0,0 +1,35 @@ + +----- + diff --git a/rules/CodeQuality/NodeFactory/InArrayFromRepeatedCompareFactory.php b/rules/CodeQuality/NodeFactory/InArrayFromRepeatedCompareFactory.php index 75d99c556d8..8c275d22a26 100644 --- a/rules/CodeQuality/NodeFactory/InArrayFromRepeatedCompareFactory.php +++ b/rules/CodeQuality/NodeFactory/InArrayFromRepeatedCompareFactory.php @@ -5,7 +5,10 @@ namespace Rector\CodeQuality\NodeFactory; use PhpParser\Node\Arg; +use PhpParser\Node\Expr; +use PhpParser\Node\Expr\Variable; use Rector\CodeQuality\ValueObject\ComparedExprAndValueExpr; +use Rector\NodeAnalyzer\ExprAnalyzer; use Rector\PhpParser\Comparing\NodeComparator; use Rector\PhpParser\Node\NodeFactory; @@ -13,14 +16,15 @@ { public function __construct( private NodeComparator $nodeComparator, - private NodeFactory $nodeFactory + private NodeFactory $nodeFactory, + private ExprAnalyzer $exprAnalyzer ) { } /** * Builds the "$value, [...]" args of an in_array() call from a repeated compare chain, * once all compared expressions are confirmed equal. Returns null when the chain is too - * short or the compared expressions differ. + * short, the compared expressions differ, or a value expression is not safe to evaluate eagerly. * * @param ComparedExprAndValueExpr[] $comparedExprAndValueExprs * @return Arg[]|null @@ -33,7 +37,15 @@ public function createInArrayArgs(array $comparedExprAndValueExprs): ?array $valueExprs = []; foreach ($comparedExprAndValueExprs as $comparedExprAndValueExpr) { - $valueExprs[] = $comparedExprAndValueExpr->getValueExpr(); + $valueExpr = $comparedExprAndValueExpr->getValueExpr(); + + // the array literal evaluates every item up front, while the &&/|| chain stops early, + // e.g. null !== $a && null !== $a->get() would call get() on null + if (! $this->isSafeToEvaluateEagerly($valueExpr)) { + return null; + } + + $valueExprs[] = $valueExpr; } /** @var ComparedExprAndValueExpr $firstComparedExprAndValue */ @@ -53,4 +65,14 @@ public function createInArrayArgs(array $comparedExprAndValueExprs): ?array return $this->nodeFactory->createArgs([$firstComparedExprAndValue->getComparedExpr(), $array]); } + + private function isSafeToEvaluateEagerly(Expr $expr): bool + { + // reading a plain variable has no side effect; $$name could evaluate a call + if ($expr instanceof Variable) { + return is_string($expr->name); + } + + return ! $this->exprAnalyzer->isDynamicExpr($expr); + } }