Skip to content

Commit ee5899e

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 2573778 commit ee5899e

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
@@ -67,6 +67,7 @@
6767
use PHPStan\Node\VariableAssignNode;
6868
use PHPStan\Node\VirtualNode;
6969
use PHPStan\Php\PhpVersion;
70+
use PHPStan\Reflection\InitializerExprTypeResolver;
7071
use PHPStan\Rules\Properties\PropertyReflectionFinder;
7172
use PHPStan\ShouldNotHappenException;
7273
use PHPStan\TrinaryLogic;
@@ -103,6 +104,7 @@
103104
use function is_int;
104105
use function is_string;
105106
use function spl_object_id;
107+
use function str_contains;
106108

107109
/**
108110
* @implements ExprHandler<Assign|AssignRef>
@@ -130,6 +132,7 @@ public function __construct(
130132
private StaticPropertyFetchHandler $staticPropertyFetchHandler,
131133
private MethodThrowPointHelper $methodThrowPointHelper,
132134
private PropertyHookThrowPointsResolver $propertyHookThrowPointsResolver,
135+
private InitializerExprTypeResolver $initializerExprTypeResolver,
133136
)
134137
{
135138
}
@@ -1269,7 +1272,23 @@ public function applyWrite(
12691272
}
12701273

12711274
$nodeScopeResolver->callNodeCallback($nodeCallback, new VariableAssignNode($var, $assignedExpr), $scopeBeforeAssignEval, $storage);
1272-
1275+
$remappedConditionalExpressions = [];
1276+
if (
1277+
// only the concat-assign's own write remaps - an enclosing plain
1278+
// assignment of the same variable (`$s = $s .= 'x'`) sees the
1279+
// already-remapped holders and would remap them a second time
1280+
$isAssignOp
1281+
&& $assignedExpr instanceof AssignOp\Concat
1282+
&& $assignedExpr->var instanceof Variable
1283+
&& $assignedExpr->var->name === $var->name
1284+
&& $scope->getConditionalExpressions() !== []
1285+
) {
1286+
$remappedConditionalExpressions = $this->remapConditionalExpressionsThroughConcatAssign(
1287+
$scope->getConditionalExpressions(),
1288+
'$' . $var->name,
1289+
$valueResult->getType(),
1290+
);
1291+
}
12731292
$nativeType = $this->readAssignedValueType($nodeScopeResolver, $storedAssignedExprResult, $assignedExpr, $scope->doNotTreatPhpDocTypesAsCertain());
12741293
$scope = $scope->assignVariable(
12751294
$var->name,
@@ -1278,6 +1297,9 @@ public function applyWrite(
12781297
TrinaryLogic::createYes(),
12791298
[],
12801299
);
1300+
foreach ($remappedConditionalExpressions as $exprString => $holders) {
1301+
$scope = $scope->addConditionalExpressions((string) $exprString, $holders); // @phpstan-ignore cast.useless
1302+
}
12811303
foreach ($conditionalExpressions as $exprString => $holders) {
12821304
$scope = $scope->addConditionalExpressions((string) $exprString, $holders);
12831305
}
@@ -1987,6 +2009,100 @@ private function processSureNotTypesForConditionalExpressionsAfterAssign(NodeSco
19872009
return $conditionalExpressions;
19882010
}
19892011

2012+
/**
2013+
* `$var .= <single constant string>` keeps the conditional-expression
2014+
* holders about $var alive by remapping them through the append instead of
2015+
* losing them to the write's invalidation. A consequence type about $var is
2016+
* concatenated with the appended constant; a condition on $var is remapped
2017+
* only when it is itself a single constant string - appending a fixed
2018+
* suffix is injective, so the remapped condition selects exactly the states
2019+
* the original condition did. Holders mentioning $var inside a composite
2020+
* expression, with a non-Yes certainty about $var, or with a non-constant
2021+
* condition on $var are left to the regular invalidation. This keeps e.g. a
2022+
* format string built across `if (!empty($target))` correlated with
2023+
* $target while further pieces are appended.
2024+
*
2025+
* @param array<string, ConditionalExpressionHolder[]> $conditionalExpressions
2026+
* @return array<string, ConditionalExpressionHolder[]>
2027+
*/
2028+
private function remapConditionalExpressionsThroughConcatAssign(
2029+
array $conditionalExpressions,
2030+
string $varExprString,
2031+
Type $appendedType,
2032+
): array
2033+
{
2034+
$appendedConstantStrings = $appendedType->getConstantStrings();
2035+
if (count($appendedConstantStrings) !== 1 || !$appendedType->equals($appendedConstantStrings[0])) {
2036+
return [];
2037+
}
2038+
$appendedConstantString = $appendedConstantStrings[0];
2039+
2040+
$remapped = [];
2041+
foreach ($conditionalExpressions as $targetExprString => $holders) {
2042+
$targetExprString = (string) $targetExprString; // @phpstan-ignore cast.useless
2043+
$targetIsVar = $targetExprString === $varExprString;
2044+
if (!$targetIsVar && str_contains($targetExprString, $varExprString)) {
2045+
// a composite target containing the variable ($var[0], f($var), ...)
2046+
continue;
2047+
}
2048+
2049+
foreach ($holders as $holder) {
2050+
$holderTouchesVar = $targetIsVar;
2051+
$remappable = true;
2052+
$newConditions = [];
2053+
foreach ($holder->getConditionExpressionTypeHolders() as $conditionExprString => $conditionHolder) {
2054+
$conditionExprString = (string) $conditionExprString; // @phpstan-ignore cast.useless
2055+
if ($conditionExprString === $varExprString) {
2056+
$holderTouchesVar = true;
2057+
$conditionConstantStrings = $conditionHolder->getType()->getConstantStrings();
2058+
if (
2059+
!$conditionHolder->getCertainty()->yes()
2060+
|| count($conditionConstantStrings) !== 1
2061+
|| !$conditionHolder->getType()->equals($conditionConstantStrings[0])
2062+
) {
2063+
$remappable = false;
2064+
break;
2065+
}
2066+
$newConditions[$conditionExprString] = ExpressionTypeHolder::createYes(
2067+
$conditionHolder->getExpr(),
2068+
$conditionConstantStrings[0]->append($appendedConstantString),
2069+
);
2070+
continue;
2071+
}
2072+
2073+
if (str_contains($conditionExprString, $varExprString)) {
2074+
$remappable = false;
2075+
break;
2076+
}
2077+
2078+
$newConditions[$conditionExprString] = $conditionHolder;
2079+
}
2080+
if (!$remappable || !$holderTouchesVar) {
2081+
continue;
2082+
}
2083+
2084+
$typeHolder = $holder->getTypeHolder();
2085+
if ($targetIsVar) {
2086+
if (!$typeHolder->getCertainty()->yes()) {
2087+
// the append leaves the variable defined on every path -
2088+
// an undefined/maybe consequence cannot be carried over
2089+
continue;
2090+
}
2091+
$concatType = $this->initializerExprTypeResolver->resolveConcatType($typeHolder->getType(), $appendedType);
2092+
if ($concatType instanceof ErrorType) {
2093+
continue;
2094+
}
2095+
$typeHolder = ExpressionTypeHolder::createYes($typeHolder->getExpr(), $concatType);
2096+
}
2097+
2098+
$newHolder = new ConditionalExpressionHolder($newConditions, $typeHolder);
2099+
$remapped[$targetExprString][$newHolder->getKey()] = $newHolder;
2100+
}
2101+
}
2102+
2103+
return $remapped;
2104+
}
2105+
19902106
/**
19912107
* Records "if the assigned variable has $variableType, the target variable is
19922108
* 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)