Skip to content

Commit 37602e9

Browse files
ondrejmirtesclaude
andcommitted
Remap conditional expressions through constant string concat-assigns
`$var .= '<single constant string>'` used to invalidate every conditional-expression holder mentioning $var, so a format string built across `if (!empty($target))` lost its correlation with $target and a later `empty($target)` re-check could not select the matching string. Appending a fixed suffix is injective, so holders whose target or condition on $var is a single constant string are remapped through the append instead; everything else still falls back to invalidation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
1 parent 5bda673 commit 37602e9

4 files changed

Lines changed: 160 additions & 1 deletion

File tree

‎src/Analyser/ExprHandler/AssignHandler.php‎

Lines changed: 117 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@
6868
use PHPStan\Node\VariableAssignNode;
6969
use PHPStan\Node\VirtualNode;
7070
use PHPStan\Php\PhpVersion;
71+
use PHPStan\Reflection\InitializerExprTypeResolver;
7172
use PHPStan\Rules\Properties\PropertyReflectionFinder;
7273
use PHPStan\ShouldNotHappenException;
7374
use PHPStan\TrinaryLogic;
@@ -107,6 +108,7 @@
107108
use function is_nan;
108109
use function is_string;
109110
use function spl_object_id;
111+
use function str_contains;
110112

111113
/**
112114
* @implements ExprHandler<Assign|AssignRef>
@@ -138,6 +140,7 @@ public function __construct(
138140
private StaticPropertyFetchHandler $staticPropertyFetchHandler,
139141
private MethodThrowPointHelper $methodThrowPointHelper,
140142
private PropertyHookThrowPointsResolver $propertyHookThrowPointsResolver,
143+
private InitializerExprTypeResolver $initializerExprTypeResolver,
141144
)
142145
{
143146
}
@@ -1296,7 +1299,23 @@ public function applyWrite(
12961299
}
12971300

12981301
$nodeScopeResolver->callNodeCallback($nodeCallback, new VariableAssignNode($var, $assignedExpr), $scopeBeforeAssignEval, $storage);
1299-
1302+
$remappedConditionalExpressions = [];
1303+
if (
1304+
// only the concat-assign's own write remaps - an enclosing plain
1305+
// assignment of the same variable (`$s = $s .= 'x'`) sees the
1306+
// already-remapped holders and would remap them a second time
1307+
$isAssignOp
1308+
&& $assignedExpr instanceof AssignOp\Concat
1309+
&& $assignedExpr->var instanceof Variable
1310+
&& $assignedExpr->var->name === $var->name
1311+
&& $scope->getConditionalExpressions() !== []
1312+
) {
1313+
$remappedConditionalExpressions = $this->remapConditionalExpressionsThroughConcatAssign(
1314+
$scope->getConditionalExpressions(),
1315+
'$' . $var->name,
1316+
$valueResult->getType(),
1317+
);
1318+
}
13001319
$nativeType = $this->readAssignedValueType($nodeScopeResolver, $storedAssignedExprResult, $assignedExpr, $scope->doNotTreatPhpDocTypesAsCertain());
13011320
$scope = $scope->assignVariable(
13021321
$var->name,
@@ -1305,6 +1324,9 @@ public function applyWrite(
13051324
TrinaryLogic::createYes(),
13061325
[],
13071326
);
1327+
foreach ($remappedConditionalExpressions as $exprString => $holders) {
1328+
$scope = $scope->addConditionalExpressions((string) $exprString, $holders); // @phpstan-ignore cast.useless
1329+
}
13081330
foreach ($conditionalExpressions as $exprString => $holders) {
13091331
$scope = $scope->addConditionalExpressions((string) $exprString, $holders);
13101332
}
@@ -2014,6 +2036,100 @@ private function processSureNotTypesForConditionalExpressionsAfterAssign(NodeSco
20142036
return $conditionalExpressions;
20152037
}
20162038

2039+
/**
2040+
* `$var .= <single constant string>` keeps the conditional-expression
2041+
* holders about $var alive by remapping them through the append instead of
2042+
* losing them to the write's invalidation. A consequence type about $var is
2043+
* concatenated with the appended constant; a condition on $var is remapped
2044+
* only when it is itself a single constant string - appending a fixed
2045+
* suffix is injective, so the remapped condition selects exactly the states
2046+
* the original condition did. Holders mentioning $var inside a composite
2047+
* expression, with a non-Yes certainty about $var, or with a non-constant
2048+
* condition on $var are left to the regular invalidation. This keeps e.g. a
2049+
* format string built across `if (!empty($target))` correlated with
2050+
* $target while further pieces are appended.
2051+
*
2052+
* @param array<string, ConditionalExpressionHolder[]> $conditionalExpressions
2053+
* @return array<string, ConditionalExpressionHolder[]>
2054+
*/
2055+
private function remapConditionalExpressionsThroughConcatAssign(
2056+
array $conditionalExpressions,
2057+
string $varExprString,
2058+
Type $appendedType,
2059+
): array
2060+
{
2061+
$appendedConstantStrings = $appendedType->getConstantStrings();
2062+
if (count($appendedConstantStrings) !== 1 || !$appendedType->equals($appendedConstantStrings[0])) {
2063+
return [];
2064+
}
2065+
$appendedConstantString = $appendedConstantStrings[0];
2066+
2067+
$remapped = [];
2068+
foreach ($conditionalExpressions as $targetExprString => $holders) {
2069+
$targetExprString = (string) $targetExprString; // @phpstan-ignore cast.useless
2070+
$targetIsVar = $targetExprString === $varExprString;
2071+
if (!$targetIsVar && str_contains($targetExprString, $varExprString)) {
2072+
// a composite target containing the variable ($var[0], f($var), ...)
2073+
continue;
2074+
}
2075+
2076+
foreach ($holders as $holder) {
2077+
$holderTouchesVar = $targetIsVar;
2078+
$remappable = true;
2079+
$newConditions = [];
2080+
foreach ($holder->getConditionExpressionTypeHolders() as $conditionExprString => $conditionHolder) {
2081+
$conditionExprString = (string) $conditionExprString; // @phpstan-ignore cast.useless
2082+
if ($conditionExprString === $varExprString) {
2083+
$holderTouchesVar = true;
2084+
$conditionConstantStrings = $conditionHolder->getType()->getConstantStrings();
2085+
if (
2086+
!$conditionHolder->getCertainty()->yes()
2087+
|| count($conditionConstantStrings) !== 1
2088+
|| !$conditionHolder->getType()->equals($conditionConstantStrings[0])
2089+
) {
2090+
$remappable = false;
2091+
break;
2092+
}
2093+
$newConditions[$conditionExprString] = ExpressionTypeHolder::createYes(
2094+
$conditionHolder->getExpr(),
2095+
$conditionConstantStrings[0]->append($appendedConstantString),
2096+
);
2097+
continue;
2098+
}
2099+
2100+
if (str_contains($conditionExprString, $varExprString)) {
2101+
$remappable = false;
2102+
break;
2103+
}
2104+
2105+
$newConditions[$conditionExprString] = $conditionHolder;
2106+
}
2107+
if (!$remappable || !$holderTouchesVar) {
2108+
continue;
2109+
}
2110+
2111+
$typeHolder = $holder->getTypeHolder();
2112+
if ($targetIsVar) {
2113+
if (!$typeHolder->getCertainty()->yes()) {
2114+
// the append leaves the variable defined on every path -
2115+
// an undefined/maybe consequence cannot be carried over
2116+
continue;
2117+
}
2118+
$concatType = $this->initializerExprTypeResolver->resolveConcatType($typeHolder->getType(), $appendedType);
2119+
if ($concatType instanceof ErrorType) {
2120+
continue;
2121+
}
2122+
$typeHolder = ExpressionTypeHolder::createYes($typeHolder->getExpr(), $concatType);
2123+
}
2124+
2125+
$newHolder = new ConditionalExpressionHolder($newConditions, $typeHolder);
2126+
$remapped[$targetExprString][$newHolder->getKey()] = $newHolder;
2127+
}
2128+
}
2129+
2130+
return $remapped;
2131+
}
2132+
20172133
/**
20182134
* Records "if the assigned variable has $variableType, the target variable is
20192135
* certainly defined (with its branch-scope type)" holders for variables whose
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug9854;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
$url = 'testUrl';
8+
$linkText = 'testLinkText';
9+
10+
$htmlLinkStructure = '<a href="%s"';
11+
if (!empty($target)) {
12+
$htmlLinkStructure .= ' target="%s"';
13+
}
14+
$htmlLinkStructure .= ' class="link">%s</a>';
15+
16+
if (empty($target)) {
17+
assertType('\'<a href="%s" class="link">%s</a>\'', $htmlLinkStructure);
18+
return sprintf($htmlLinkStructure, $url, $linkText);
19+
}
20+
21+
assertType('\'<a href="%s" target="%s" class="link">%s</a>\'', $htmlLinkStructure);
22+
return sprintf($htmlLinkStructure, $url, $target, $linkText);

‎tests/PHPStan/Rules/Functions/PrintfParametersRuleTest.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,4 +160,9 @@ public function testBug14567(): void
160160
$this->analyse([__DIR__ . '/data/bug-14567.php'], []);
161161
}
162162

163+
public function testBug9854(): void
164+
{
165+
$this->analyse([__DIR__ . '/data/bug-9854.php'], []);
166+
}
167+
163168
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<?php declare(strict_types = 1);
2+
3+
$url = 'testUrl';
4+
$linkText = 'testLinkText';
5+
6+
$htmlLinkStructure = '<a href="%s"';
7+
if (!empty($target)) {
8+
$htmlLinkStructure .= ' target="%s"';
9+
}
10+
$htmlLinkStructure .= ' class="link">%s</a>';
11+
12+
if (empty($target)) {
13+
return sprintf($htmlLinkStructure, $url, $linkText);
14+
}
15+
16+
return sprintf($htmlLinkStructure, $url, $target, $linkText);

0 commit comments

Comments
 (0)