Treat a regex alternation as maybe-empty when any branch is maybe-empty in isMaybeEmptyNode - #5851
Conversation
|
|
||
| // guard: an alternation whose branches are all non-empty stays non-falsy | ||
| if (preg_match('~(a(b|c))~', $s, $m)) { | ||
| assertType("array{non-falsy-string, 'b'|'c', 'b'|'c'}", $m); |
There was a problem hiding this comment.
these expectations are wrong.
| assertType("array{non-falsy-string, 'b'|'c', 'b'|'c'}", $m); | |
| assertType("array{non-falsy-string, 'ab'|'ac', 'b'|'c'}", $m); |
There was a problem hiding this comment.
I've addressed the review. Here's a summary of what I did:
The review
staabm pointed out that the test expectation for ~(a(b|c))~ was wrong: group 1 (a(b|c)) was asserted as 'b'|'c' but should be 'ab'|'ac' (the literal a prefix belongs to that capture group's value).
Investigation
I confirmed PHPStan actually inferred 'b'|'c' for group 1 — so the reviewer's suggested expectation wasn't just a typo to swap in; it exposed a real, separate bug. Probing adjacent cases pinpointed it:
(a(b|c))→'b'|'c'❌ (prefix dropped)(a(b|c)d)→'bd'|'cd'❌ (prefix dropped, suffix kept)((b|c)a)→'ba'|'ca'✓ (suffix worked)(a(b|c)(d|e))→'d'|'e'❌ (everything before the last alternation dropped)
The #alternation branch in walkGroupAst() discarded the literal prefix accumulated so far (it started fresh with $newLiterals = []), while suffix tokens after an alternation correctly appended.
Fix
In RegexGroupParser::walkGroupAst(), the #alternation branch now cross-products the incoming prefix literals with each branch's literals (and correctly falls back to a non-constant string when the prefix is non-constant, e.g. (.(b|c)) now yields non-falsy-string instead of a bogus 'b'|'c').
After the fix:
(a(b|c))→'ab'|'ac'✓ (matches the reviewer's suggestion)(a(b|c)(d|e))→'abd'|'abe'|'acd'|'ace'✓- The PR's original
(.?(.|$))→array{string, string}still holds ✓
Verification
- Updated the test expectation to
array{non-falsy-string, 'ab'|'ac', 'b'|'c'}. - Confirmed the test fails for the right reason without the source fix (
expected 'ab'|'ac', actual 'b'|'c') and passes with it. make tests(12668 tests),make phpstan(no errors), andmake cs/lint all pass.
Committed and pushed to the PR branch.
…ty in `isMaybeEmptyNode` - Add an explicit `#alternation` case to `RegexGroupParser::isMaybeEmptyNode()`: an alternation is maybe-empty if any of its branches is maybe-empty, and only non-falsy if every branch is non-falsy. - Previously alternation nodes fell through to the concatenation-style loop, which reported a node as non-empty/non-falsy as soon as a single branch was non-empty. For patterns like `.?(.|$)` this made the `(.|$)` group count as a non-falsy token inside the surrounding concatenation, so the whole match (and any enclosing capture group) was inferred as `non-falsy-string` instead of `string`, even though the `$` branch matches the empty string. - Affects both the full-match subject type and capturing-group types, since both are derived from `walkGroupAst()` / `isMaybeEmptyNode()`.
…gex group types When a capturing group concatenates a literal prefix before a nested alternation, e.g. "(a(b|c))", the prefix accumulated so far was dropped when walking the #alternation node, yielding "b"|"c" instead of "ab"|"ac". Cross-product the incoming prefix literals with each branch's literals (and give up to a non-constant string when the prefix is non-constant). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c1cfd8c to
b788f79
Compare
3 participants
Summary
preg_matchshape inference inferrednon-falsy-stringinstead ofstringfor thewhole match and enclosing capture groups of patterns that nest a captured optional
subpattern, e.g.
'(.?(.|$))'(where the outer(/)are the regex delimiters, so theactual pattern is
.?(.|$)). The(.|$)alternation can match the empty string at$,so the whole match may be empty and should be
string.Changes
src/Type/Regex/RegexGroupParser.php: add an explicit#alternationcase toisMaybeEmptyNode(). An alternation is now considered maybe-empty when any of itsbranches is maybe-empty, and non-falsy only when every branch is non-falsy.
tests/PHPStan/Analyser/nsrt/bug-14809.php: regression test covering the reportedreproducers plus a guard that an all-non-empty alternation (
(a(b|c))) still yields anon-falsy string.
Root cause
isMaybeEmptyNode()is used bywalkGroupAst()while counting the "meaningful tokens"of a concatenation to decide whether the match is non-empty / non-falsy. The method only
had concatenation semantics: it recursed over a node's children and returned "not empty"
(and propagated
isNonFalsy) as soon as a single child was non-empty. For an#alternationnode this is exactly backwards — an alternation is empty-capable whenany branch is empty-capable. So
(.|$)was classified as a non-falsy, non-empty token;inside the surrounding
.?(.|$)concatenation that single non-falsy token short-circuitedthe loop and marked the whole match (and the enclosing capture group) as
non-falsy-string.The fix gives
#alternationits own branch with the correct quantifier semantics(any-branch-empty ⇒ maybe empty; all-branches-non-falsy ⇒ non-falsy). This corrects both
the full-match subject type and the capture-group types, since both flow through
walkGroupAst().Test
tests/PHPStan/Analyser/nsrt/bug-14809.phpasserts:'(.?(.|$))'→array{string, string}(reported issue)'((.?(.|$)))'→array{string, string, string}(reported issue)'((.?(.|.?)))'→array{string, string, string}(simplified reproducer)'~(a(b|c))~'→array{non-falsy-string, 'b'|'c', 'b'|'c'}(guard: all-non-emptyalternation must remain non-falsy)
The test fails on the first three assertions before the fix and passes after. The
existing
preg_match/preg_split/preg_replaceshape tests and the fullNodeScopeResolverTestsuite continue to pass.Probed but intentionally not changed
~.(.|$)~still infers the full match asnon-falsy-string. This is a separate,pre-existing over-approximation (the
.any-char metacharacter is tokenized as a literal'.', whichgetLiteralValue()returns as a non-falsy string) — independent of thealternation maybe-empty bug fixed here — and would change many existing expectations, so
it is left out of this change.
Fixes phpstan/phpstan#14809