Skip to content

Commit 57c93e4

Browse files
phpstan-botclaude
andcommitted
Require the union with the other branch's type to equal the consequent before rescuing a one-sided conditional holder
- MutatingScope::preserveVacuousConditionalExpressions() kept a conditional holder present in only one branch when the other branch's state was a subtype of the holder's consequent. Without bleeding edge, array shapes are unsealed in isSuperTypeOf(), so array{}|array{a?: T} counted as a supertype of array{a?: T, b: T}. Applying that holder later dropped the 'b' key, which caused false positives such as "Offset ... on left side of ?? does not exist". - Now also require TypeCombinator::union(consequent, other) to equal the consequent, so the other branch's state adds nothing to it. - Ported the same check to turbo-ext/src/MutatingScope.cpp. - Probed the other isSuperTypeOf() uses in the conditional-holder machinery (vacuous-guard check, ScopeOps guard matching, StatementsHandler loop holders). They compare guards rather than check that a consequent still holds, so they don't have this problem. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent ba954c3 commit 57c93e4

6 files changed

Lines changed: 193 additions & 0 deletions

File tree

‎src/Analyser/MutatingScope.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4557,6 +4557,14 @@ private function preserveVacuousConditionalExpressions(
45574557
continue;
45584558
}
45594559

4560+
// Array shapes are unsealed in isSuperTypeOf(): array{a?: int} is a supertype
4561+
// of array{a?: int, b: int}, yet applying the holder would drop the 'b' key
4562+
// the other branch tracks. The consequent only holds on both paths when the
4563+
// other branch's state adds nothing to it.
4564+
if (!TypeCombinator::union($typeHolder->getType(), $otherTargetHolder->getType())->equals($typeHolder->getType())) {
4565+
continue;
4566+
}
4567+
45604568
$currentConditionalExpressions[$exprString][$key] = $holder;
45614569
}
45624570
}
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Analyser;
4+
5+
use PHPStan\Testing\TypeInferenceTestCase;
6+
use PHPUnit\Framework\Attributes\DataProvider;
7+
8+
class Bug15534Test extends TypeInferenceTestCase
9+
{
10+
11+
public static function dataFileAsserts(): iterable
12+
{
13+
yield from self::gatherAssertTypes(__DIR__ . '/data/bug-15534.php');
14+
}
15+
16+
/**
17+
* @param mixed ...$args
18+
*/
19+
#[DataProvider('dataFileAsserts')]
20+
public function testFileAsserts(
21+
string $assertType,
22+
string $file,
23+
...$args,
24+
): void
25+
{
26+
$this->assertFileAsserts($assertType, $file, ...$args);
27+
}
28+
29+
}
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace Bug15534Types;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
function isEnabled(): bool { return (bool) rand(0, 1); }
8+
function otherEnabled(): bool { return (bool) rand(0, 1); }
9+
function getSavedPost(string $key): ?string { return rand(0, 1) === 1 ? $key : null; }
10+
11+
function doFoo(): void
12+
{
13+
$post = [];
14+
if (getSavedPost('field0')) {
15+
$post['field0'] = getSavedPost('field0');
16+
}
17+
if (getSavedPost('field1')) {
18+
$post['field1'] = getSavedPost('field1');
19+
}
20+
if (otherEnabled()) {
21+
if (getSavedPost('other0')) {
22+
$post['other0'] = 'o';
23+
}
24+
}
25+
26+
if (isEnabled()) {
27+
if (getSavedPost('c')) {
28+
$post['company_address'] = 'c';
29+
}
30+
}
31+
32+
if (!otherEnabled()) {
33+
assertType("array{}|array{field0?: non-falsy-string, field1?: non-falsy-string, other0?: 'o', company_address?: 'c'}", $post);
34+
}
35+
if (isEnabled()) {
36+
assertType("array{}|array{field0?: non-falsy-string, field1?: non-falsy-string, other0?: 'o', company_address?: 'c'}", $post);
37+
}
38+
}
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Rules\Variables;
4+
5+
use Override;
6+
use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper;
7+
use PHPStan\Rules\IssetCheck;
8+
use PHPStan\Rules\Properties\PropertyDescriptor;
9+
use PHPStan\Rules\Rule;
10+
use PHPStan\Testing\RuleTestCase;
11+
12+
/**
13+
* Array shapes are unsealed without bleeding edge.
14+
*
15+
* @extends RuleTestCase<NullCoalesceRule>
16+
*/
17+
class NullCoalesceRuleNoBleedingEdgeTest extends RuleTestCase
18+
{
19+
20+
protected function getRule(): Rule
21+
{
22+
return new NullCoalesceRule(
23+
new IssetCheck(
24+
new PropertyDescriptor(),
25+
true,
26+
$this->shouldTreatPhpDocTypesAsCertain(),
27+
),
28+
self::getContainer()->getByType(ConstantConditionInTraitHelper::class),
29+
true,
30+
);
31+
}
32+
33+
#[Override]
34+
public static function getAdditionalConfigFiles(): array
35+
{
36+
return [];
37+
}
38+
39+
public function testBug15534(): void
40+
{
41+
$this->analyse([__DIR__ . '/data/bug-15534.php'], []);
42+
}
43+
44+
}
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace Bug15534;
4+
5+
function isEnabled(): bool
6+
{
7+
return (bool) ($GLOBALS['enabled'] ?? false);
8+
}
9+
10+
function isVisible(): bool
11+
{
12+
return (bool) ($GLOBALS['visible'] ?? false);
13+
}
14+
15+
function otherEnabled(): bool
16+
{
17+
return (bool) ($GLOBALS['other'] ?? false);
18+
}
19+
20+
function getSavedPost(string $key): ?string
21+
{
22+
if (!isset($_SESSION[$key]) || !is_string($_SESSION[$key])) {
23+
return null;
24+
}
25+
26+
return (string) $_SESSION[$key];
27+
}
28+
29+
function translate(string $key): string
30+
{
31+
return $key;
32+
}
33+
34+
$post = [];
35+
36+
if (getSavedPost('field0')) {
37+
$post['field0'] = getSavedPost('field0');
38+
}
39+
if (getSavedPost('field1')) {
40+
$post['field1'] = getSavedPost('field1');
41+
}
42+
43+
if (otherEnabled()) {
44+
if (getSavedPost('other0')) {
45+
$post['other0'] = getSavedPost('other0');
46+
}
47+
}
48+
49+
if (isEnabled()) {
50+
if (getSavedPost('company_address')) {
51+
$post['company_address'] = getSavedPost('company_address');
52+
}
53+
}
54+
55+
if (isVisible() && !isEnabled()) {
56+
echo 'other';
57+
}
58+
59+
if (isVisible() && !otherEnabled()) {
60+
echo translate(isEnabled() ? 'enabled' : 'disabled');
61+
}
62+
63+
if (isEnabled()) {
64+
echo $post['company_address'] ?? '';
65+
}
66+

‎turbo-ext/src/MutatingScope.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8565,6 +8565,14 @@ class MutatingScope
85658565
bool yes;
85668566
if (UNEXPECTED(!isSuperTypeOfYes(Z_OBJ_P(typeHolderType.raw()), otherTargetType.raw(), yes))) return zv::Val();
85678567
if (!yes) continue;
8568+
/* array shapes are unsealed in isSuperTypeOf() - the consequent
8569+
* only holds on both paths when the other branch's state adds
8570+
* nothing to it */
8571+
zv::Val united = unionTypes(typeHolderType.raw(), otherTargetType.raw());
8572+
if (UNEXPECTED(united.isUndef())) return zv::Val();
8573+
bool unchanged;
8574+
if (UNEXPECTED(!typeEquals(united.raw(), typeHolderType.raw(), unchanged))) return zv::Val();
8575+
if (!unchanged) continue;
85688576
if (UNEXPECTED(!appendConditionalHolder(result, exprKey, exprIndex, key, keyIndex, holder))) return zv::Val();
85698577
}
85708578
}

0 commit comments

Comments
 (0)