Skip to content

Commit 68c2e4e

Browse files
ondrejmirtesclaude
andcommitted
Keep optional groups optional within a matched regex alternation branch
When building the tagged union for a single top-level alternation, the groups of the matched branch were forced non-optional, which also dropped their own optional quantifiers, e.g. `(?:-(?P<bar>\d+))?`. Such a group is now only forced out of the alternation's optionality. With PREG_UNMATCHED_AS_NULL on PHP 7.2 and 7.3, an unmatched group followed by a matched one is reported as null, not as an empty string. Only the trailing unmatched groups are omitted there. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KjXHtRyzsYF9xktijyaRBr
1 parent 4afc290 commit 68c2e4e

11 files changed

Lines changed: 227 additions & 5 deletions

‎src/Type/Php/RegexArrayShapeMatcher.php‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,12 +197,12 @@ private function matchRegex(string $regex, ?int $flags, TrinaryLogic $wasMatched
197197
foreach ($comboList as $group) {
198198
if (in_array($group->getId(), $groupCombo, true)) {
199199
$isOptionalAlternation = $group->inOptionalAlternation();
200-
$comboList = $comboList->forceGroupNonOptional($group);
200+
$comboList = $comboList->forceGroupAlternationBranchMatched($group);
201201
$beforeCurrentCombo = false;
202202
} elseif ($beforeCurrentCombo && !$group->resetsGroupCounter()) {
203203
$comboList = $comboList->forceGroupTypeAndNonOptional(
204204
$group,
205-
$this->containsUnmatchedAsNull($flags, $matchesAll) ? new NullType() : new ConstantStringType(''),
205+
$this->getUnmatchedGroupType($flags),
206206
);
207207
} elseif (
208208
$group->getAlternationId() === $onlyTopLevelAlternation->getId()
@@ -438,7 +438,7 @@ private function createGroupValueType(RegexCapturingGroup $captureGroup, Trinary
438438

439439
if (!$isLastGroup && !$this->containsUnmatchedAsNull($flags, $matchesAll) && $captureGroup->isOptional()) {
440440
$groupValueType = $this->getValueType(
441-
TypeCombinator::union($captureGroup->getType(), new ConstantStringType('')),
441+
TypeCombinator::union($captureGroup->getType(), $this->getUnmatchedGroupType($flags)),
442442
$flags,
443443
$matchesAll,
444444
);
@@ -482,6 +482,21 @@ private function containsUnmatchedAsNull(int $flags, bool $matchesAll): bool
482482
return ($flags & PREG_UNMATCHED_AS_NULL) !== 0 && (($flags & self::PREG_UNMATCHED_AS_NULL_ON_72_73) !== 0 || $this->phpVersion->supportsPregUnmatchedAsNull());
483483
}
484484

485+
/**
486+
* Type of an unmatched group followed by a matched one.
487+
*
488+
* PHP 7.2 and 7.3 already report it as null with PREG_UNMATCHED_AS_NULL,
489+
* only the trailing unmatched groups are omitted there.
490+
*/
491+
private function getUnmatchedGroupType(int $flags): Type
492+
{
493+
if (($flags & PREG_UNMATCHED_AS_NULL) !== 0) {
494+
return new NullType();
495+
}
496+
497+
return new ConstantStringType('');
498+
}
499+
485500
private function getKeyType(int|string $key): Type
486501
{
487502
if (is_string($key)) {

‎src/Type/Regex/RegexCapturingGroup.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ public function __construct(
1616
private readonly Type $type,
1717
private readonly bool $forceNonOptional = false,
1818
private readonly ?Type $forceType = null,
19+
private readonly bool $forceAlternationBranchMatched = false,
1920
)
2021
{
2122
}
@@ -36,6 +37,26 @@ public function forceNonOptional(): self
3637
$this->type,
3738
true,
3839
$this->forceType,
40+
$this->forceAlternationBranchMatched,
41+
);
42+
}
43+
44+
/**
45+
* Unlike forceNonOptional(), the group stays optional when it is optional
46+
* within its alternation branch, e.g. because of a `?` quantifier.
47+
*/
48+
public function forceAlternationBranchMatched(): self
49+
{
50+
return new self(
51+
$this->id,
52+
$this->name,
53+
$this->alternation,
54+
$this->inOptionalQuantification,
55+
$this->parent,
56+
$this->type,
57+
$this->forceNonOptional,
58+
$this->forceType,
59+
true,
3960
);
4061
}
4162

@@ -50,6 +71,7 @@ public function forceType(Type $type): self
5071
$type,
5172
$this->forceNonOptional,
5273
$this->forceType,
74+
$this->forceAlternationBranchMatched,
5375
);
5476
}
5577

@@ -64,6 +86,7 @@ public function withParent(RegexCapturingGroup|RegexNonCapturingGroup $parent):
6486
$this->type,
6587
$this->forceNonOptional,
6688
$this->forceType,
89+
$this->forceAlternationBranchMatched,
6790
);
6891
}
6992

@@ -101,11 +124,32 @@ public function isOptional(): bool
101124
return false;
102125
}
103126

127+
if ($this->forceAlternationBranchMatched) {
128+
return $this->isOptionalWithinAlternationBranch();
129+
}
130+
104131
return $this->inAlternation()
105132
|| $this->inOptionalQuantification
106133
|| $this->parent !== null && $this->parent->isOptional();
107134
}
108135

136+
private function isOptionalWithinAlternationBranch(): bool
137+
{
138+
if ($this->inOptionalQuantification) {
139+
return true;
140+
}
141+
142+
$parent = $this->parent;
143+
while ($parent !== null && $parent->getAlternationId() === $this->getAlternationId()) {
144+
if ($parent->inOptionalQuantification()) {
145+
return true;
146+
}
147+
$parent = $parent->getParent();
148+
}
149+
150+
return false;
151+
}
152+
109153
public function inOptionalQuantification(): bool
110154
{
111155
return $this->inOptionalQuantification;

‎src/Type/Regex/RegexGroupList.php‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,18 @@ public function forceGroupTypeAndNonOptional(RegexCapturingGroup $group, Type $t
4848
return $this->cloneAndReParentList($group, $type);
4949
}
5050

51-
private function cloneAndReParentList(RegexCapturingGroup $target, ?Type $type = null): self
51+
public function forceGroupAlternationBranchMatched(RegexCapturingGroup $group): self
52+
{
53+
return $this->cloneAndReParentList($group, null, true);
54+
}
55+
56+
private function cloneAndReParentList(RegexCapturingGroup $target, ?Type $type = null, bool $alternationBranchOnly = false): self
5257
{
5358
$groups = [];
5459
$forcedGroup = null;
5560
foreach ($this->groups as $i => $group) {
5661
if ($group->getId() === $target->getId()) {
57-
$forcedGroup = $group->forceNonOptional();
62+
$forcedGroup = $alternationBranchOnly ? $group->forceAlternationBranchMatched() : $group->forceNonOptional();
5863
if ($type !== null) {
5964
$forcedGroup = $forcedGroup->forceType($type);
6065
}

‎src/Type/Regex/RegexNonCapturingGroup.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,11 @@ public function isOptional(): bool
3636
|| ($this->parent !== null && $this->parent->isOptional());
3737
}
3838

39+
public function inOptionalQuantification(): bool
40+
{
41+
return $this->inOptionalQuantification;
42+
}
43+
3944
public function isTopLevel(): bool
4045
{
4146
return $this->parent === null
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
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 Bug15297Php73Test extends TypeInferenceTestCase
9+
{
10+
11+
public static function dataFileAsserts(): iterable
12+
{
13+
yield from self::gatherAssertTypes(__DIR__ . '/data/bug-15297-php73.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+
public static function getAdditionalConfigFiles(): array
30+
{
31+
return [
32+
__DIR__ . '/nodeScopeResolverPhp73.neon',
33+
];
34+
}
35+
36+
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug15297Php73;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
$pattern = "/^(?:
8+
(?P<foo1>foo[a-z]+)
9+
(?:-(?P<bar1>[0-9]+))?
10+
|
11+
(?P<foo2>foo[0-9]+)
12+
(?:-(?P<bar2>[a-z]+))?
13+
)$/ix";
14+
if (preg_match($pattern, "foobar", $matches, \PREG_UNMATCHED_AS_NULL)) {
15+
assertType("array{0: non-falsy-string, foo1: non-falsy-string, 1: non-falsy-string, bar1?: numeric-string, 2?: numeric-string}|array{0: non-falsy-string, foo1: null, 1: null, bar1: null, 2: null, foo2: non-falsy-string, 3: non-falsy-string, bar2?: non-empty-string, 4?: non-empty-string}", $matches);
16+
$foo = $matches["foo1"] ?? $matches["foo2"] ?? "";
17+
$bar = $matches["bar1"] ?? $matches["bar2"] ?? "";
18+
}
19+
20+
function (string $s): void {
21+
if (preg_match('/(a)?(b)?(c)/', $s, $matches, \PREG_UNMATCHED_AS_NULL)) {
22+
assertType("array{non-falsy-string, 'a'|null, 'b'|null, 'c'}", $matches);
23+
}
24+
if (preg_match('/(a)(b)?(c)?/', $s, $matches, \PREG_UNMATCHED_AS_NULL)) {
25+
assertType("list{0: non-falsy-string, 1: 'a', 2?: 'b'|null, 3?: 'c'}", $matches);
26+
}
27+
};
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
parameters:
2+
phpVersion: 70300 # PHP 7.3
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug15297;
4+
5+
use function PHPStan\Testing\assertType;
6+
7+
function doFoo(string $s): void
8+
{
9+
$pattern = "/^(?:
10+
(?P<foo1>foo[a-z]+)
11+
(?:-(?P<bar1>[0-9]+))?
12+
|
13+
(?P<foo2>foo[0-9]+)
14+
(?:-(?P<bar2>[a-z]+))?
15+
)$/ix";
16+
if (preg_match($pattern, $s, $matches)) {
17+
assertType("array{0: non-falsy-string, foo1: '', 1: '', bar1: '', 2: '', foo2: non-falsy-string, 3: non-falsy-string, bar2?: non-empty-string, 4?: non-empty-string}|array{0: non-falsy-string, foo1: non-falsy-string, 1: non-falsy-string, bar1?: numeric-string, 2?: numeric-string}", $matches);
18+
}
19+
}
20+
21+
function doBar(string $s): void
22+
{
23+
if (preg_match('/^(?:(?:(a))?(b)|(c))$/', $s, $matches)) {
24+
assertType("array{non-falsy-string, '', '', 'c'}|list{0: non-falsy-string, 1?: ''|'a', 2: 'b'}", $matches);
25+
}
26+
}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Rules\Variables;
4+
5+
use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper;
6+
use PHPStan\Rules\IssetCheck;
7+
use PHPStan\Rules\Properties\PropertyDescriptor;
8+
use PHPStan\Rules\Rule;
9+
use PHPStan\Testing\RuleTestCase;
10+
11+
/**
12+
* @extends RuleTestCase<NullCoalesceRule>
13+
*/
14+
class NullCoalesceRulePhp73Test extends RuleTestCase
15+
{
16+
17+
protected function getRule(): Rule
18+
{
19+
return new NullCoalesceRule(
20+
new IssetCheck(
21+
new PropertyDescriptor(),
22+
true,
23+
$this->shouldTreatPhpDocTypesAsCertain(),
24+
),
25+
self::getContainer()->getByType(ConstantConditionInTraitHelper::class),
26+
true,
27+
);
28+
}
29+
30+
public function testBug15297(): void
31+
{
32+
$this->analyse([__DIR__ . '/data/bug-15297.php'], [
33+
[
34+
'Offset \'foo2\' on array{0: non-falsy-string, foo1: null, 1: null, bar1: null, 2: null, foo2: non-falsy-string, 3: non-falsy-string, bar2?: non-empty-string, ...} on left side of ?? always exists and is not nullable.',
35+
11,
36+
],
37+
]);
38+
}
39+
40+
public static function getAdditionalConfigFiles(): array
41+
{
42+
return [
43+
__DIR__ . '/data/null-coalesce-php73.neon',
44+
];
45+
}
46+
47+
}
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php declare(strict_types = 1);
2+
3+
$pattern = "/^(?:
4+
(?P<foo1>foo[a-z]+)
5+
(?:-(?P<bar1>[0-9]+))?
6+
|
7+
(?P<foo2>foo[0-9]+)
8+
(?:-(?P<bar2>[a-z]+))?
9+
)$/ix";
10+
if (preg_match($pattern, "foobar", $matches, \PREG_UNMATCHED_AS_NULL)) {
11+
$foo = $matches["foo1"] ?? $matches["foo2"] ?? "";
12+
$bar = $matches["bar1"] ?? $matches["bar2"] ?? "";
13+
}

0 commit comments

Comments
 (0)