Skip to content

Commit e76fdbd

Browse files
authored
fix: all alternatives in lookbehind alternation now checked (#57)
1 parent ebd29a3 commit e76fdbd

8 files changed

Lines changed: 226 additions & 41 deletions

File tree

‎reggie-codegen/src/main/java/com/datadoghq/reggie/codegen/analysis/FallbackPatternDetector.java‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,6 @@ public static String needsFallback(RegexNode ast, PatternAnalyzer.MatchingStrate
5959
if (v.lookbehindBeforeUnbounded) {
6060
return "lookbehind followed by unbounded quantifier";
6161
}
62-
// Bug 4: alternation inside lookbehind
63-
if (v.alternationInLookbehind) {
64-
return "alternation inside lookbehind";
65-
}
6662
// Bug 5: lookbehind and lookahead used together (sandwich / interaction)
6763
if (v.hasLookbehind && v.hasLookahead) {
6864
return "lookbehind and lookahead combined";
@@ -100,26 +96,9 @@ private static boolean containsLookahead(RegexNode node) {
10096
return false;
10197
}
10298

103-
/** Returns true if {@code node} is or directly contains an AlternationNode. */
104-
private static boolean containsDirectAlternation(RegexNode node) {
105-
if (node instanceof AlternationNode) {
106-
return true;
107-
}
108-
if (node instanceof GroupNode) {
109-
return ((GroupNode) node).child instanceof AlternationNode;
110-
}
111-
if (node instanceof ConcatNode) {
112-
for (RegexNode c : ((ConcatNode) node).children) {
113-
if (c instanceof AlternationNode) return true;
114-
}
115-
}
116-
return false;
117-
}
118-
11999
private static final class Visitor implements RegexVisitor<Void> {
120100
boolean lookaheadInQuantifier = false;
121101
boolean lookbehindBeforeUnbounded = false;
122-
boolean alternationInLookbehind = false;
123102
boolean hasLookahead = false;
124103
boolean hasLookbehind = false;
125104

@@ -130,9 +109,6 @@ public Void visitAssertion(AssertionNode node) {
130109
}
131110
if (isLookbehind(node.type)) {
132111
hasLookbehind = true;
133-
if (containsDirectAlternation(node.subPattern)) {
134-
alternationInLookbehind = true;
135-
}
136112
}
137113
node.subPattern.accept(this);
138114
return null;

‎reggie-codegen/src/main/java/com/datadoghq/reggie/codegen/analysis/StructuralHash.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,11 @@ private static int computeDFATopologyHash(DFA dfa) {
116116
// Group action count (affects bytecode for group tracking)
117117
hash = 31 * hash + state.groupActions.size();
118118

119-
// Assertion check count (affects bytecode for assertion validation)
119+
// Assertion checks — include type to distinguish positive from negative assertions
120120
hash = 31 * hash + state.assertionChecks.size();
121+
for (var ac : state.assertionChecks) {
122+
hash = 31 * hash + ac.type.hashCode();
123+
}
121124

122125
// Include character sets - critical for case sensitivity
123126
for (var entry : state.transitions.entrySet()) {

‎reggie-codegen/src/main/java/com/datadoghq/reggie/codegen/automaton/NFA.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,9 @@ public int contentHashCode() {
299299
// Include group markers
300300
hash = 31 * hash + (state.enterGroup != null ? state.enterGroup + 1 : 0);
301301
hash = 31 * hash + (state.exitGroup != null ? state.exitGroup + 1 : 0);
302+
303+
// Assertion type distinguishes (?<=...) from (?<!...) — must not collide
304+
hash = 31 * hash + (state.assertionType != null ? state.assertionType.hashCode() : 0);
302305
}
303306

304307
return hash;

‎reggie-codegen/src/main/java/com/datadoghq/reggie/codegen/codegen/NFABytecodeGenerator.java‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3657,7 +3657,8 @@ private void generateAssertionCheck(
36573657
// Check if we can use lightweight simulation for tiny sub-NFAs
36583658
int subNFASize = countReachableStates(assertionState.assertionStartState);
36593659

3660-
if (subNFASize <= 6
3660+
if (!isLookbehind
3661+
&& subNFASize <= 6
36613662
&& tryLightweightSimulation(
36623663
mv,
36633664
assertionState,
@@ -3666,7 +3667,6 @@ && tryLightweightSimulation(
36663667
isPositive,
36673668
assertionFailed,
36683669
assertionPassed,
3669-
isLookbehind,
36703670
allocator)) {
36713671
// Lightweight simulation succeeded
36723672
mv.visitLabel(assertionPassed);
@@ -3775,6 +3775,10 @@ private String extractLiteral(NFA.NFAState start, Set<NFA.NFAState> acceptStates
37753775
while (visited.add(current)) {
37763776
// Check for epsilon transitions (skip empty transitions)
37773777
if (!current.getEpsilonTransitions().isEmpty()) {
3778+
// Multiple epsilon transitions indicate alternation — not a simple literal
3779+
if (current.getEpsilonTransitions().size() > 1) {
3780+
return null;
3781+
}
37783782
// Follow epsilon if it leads to non-assertion state
37793783
boolean foundNonAssertion = false;
37803784
for (NFA.NFAState target : current.getEpsilonTransitions()) {
@@ -3858,6 +3862,10 @@ private String extractLiteralFromState(NFA.NFAState start, Set<NFA.NFAState> acc
38583862

38593863
// Follow epsilon transitions (skip semantically-significant states)
38603864
if (!current.getEpsilonTransitions().isEmpty()) {
3865+
// Multiple epsilon transitions indicate alternation — not a simple literal
3866+
if (current.getEpsilonTransitions().size() > 1) {
3867+
return null;
3868+
}
38613869
boolean foundPlainEpsilon = false;
38623870
for (NFA.NFAState target : current.getEpsilonTransitions()) {
38633871
// Stop at assertion states and backref states: following through them would
@@ -3975,7 +3983,6 @@ private boolean tryLightweightSimulation(
39753983
boolean isPositive,
39763984
Label assertionFailed,
39773985
Label assertionPassed,
3978-
boolean isLookbehind,
39793986
LocalVariableAllocator allocator) {
39803987
// Try to detect .*[CharClass] pattern (most common in password validation)
39813988
CharSet targetCharSet =
@@ -5704,6 +5711,8 @@ private CharSet extractDotStarCharClass(NFA.NFAState start, Set<NFA.NFAState> ac
57045711
queue.add(start);
57055712
reachable.add(start);
57065713

5714+
CharSet acceptCharSet = null; // accumulates union of all accept-state charsets
5715+
57075716
// BFS to find all reachable states
57085717
while (!queue.isEmpty()) {
57095718
NFA.NFAState current = queue.poll();
@@ -5714,7 +5723,8 @@ private CharSet extractDotStarCharClass(NFA.NFAState start, Set<NFA.NFAState> ac
57145723
if (acceptStates.contains(trans.target)) {
57155724
// Check if this is a specific charset (not "any char" which includes ANY_EXCEPT_NEWLINE)
57165725
if (!trans.chars.isAnyChar()) {
5717-
return trans.chars;
5726+
acceptCharSet =
5727+
(acceptCharSet == null) ? trans.chars : acceptCharSet.union(trans.chars);
57185728
}
57195729
}
57205730

@@ -5731,7 +5741,7 @@ private CharSet extractDotStarCharClass(NFA.NFAState start, Set<NFA.NFAState> ac
57315741
}
57325742
}
57335743

5734-
return null;
5744+
return acceptCharSet;
57355745
}
57365746

57375747
/**

‎reggie-processor/src/test/java/com/datadoghq/reggie/processor/ReggieMatcherBytecodeGeneratorTest.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -464,12 +464,13 @@ void testLargeNegatedRangeCharsetStrategy() throws Exception {
464464
@Test
465465
void testFallbackPatternsRejected() {
466466
// Patterns with known engine bugs must fail at build time, not produce buggy bytecode.
467-
// Note: multiple-backref patterns (e.g. (\w+)\s+\1\s+\1) are now handled natively
468-
// and no longer rejected.
467+
// Note: multiple-backref patterns and alternation-in-lookbehind are now handled natively.
468+
// Bug 3: lookbehind before unbounded quantifier still requires fallback.
469469
assertThrows(
470470
UnsupportedOperationException.class,
471471
() ->
472-
new ReggieMatcherBytecodeGenerator("test.generated", "AltLookbehind", "(?<=a|b)c")
472+
new ReggieMatcherBytecodeGenerator(
473+
"test.generated", "LookbehindUnbounded", "(?<=\\d)[a-z]+")
473474
.generate());
474475
}
475476

‎reggie-runtime/src/test/java/com/datadoghq/reggie/runtime/FallbackVerificationTest.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,13 @@ void lookbehindUnboundedQuantifier() {
5555
assertFalse(m.find("abc"));
5656
}
5757

58-
// Bug 4: alternation inside lookbehind
58+
// Bug 4 (fixed): alternation inside lookbehind — now handled natively
5959
@Test
6060
void alternationInsideLookbehind() {
6161
ReggieMatcher m = Reggie.compile("(?<=a|b)c");
62-
assertTrue(m instanceof JavaRegexFallbackMatcher);
62+
assertFalse(m instanceof JavaRegexFallbackMatcher);
6363
assertTrue(m.find("ac"));
64-
assertTrue(m.find("bc")); // was incorrectly false before fallback
64+
assertTrue(m.find("bc"));
6565
assertFalse(m.find("xc"));
6666
}
6767

@@ -74,12 +74,12 @@ void lookbehindLookaheadSandwich() {
7474
assertFalse(m.find("value"));
7575
}
7676

77-
// Named groups via fallback matcher: hasNamedGroups() must return true
77+
// Named groups: hasNamedGroups() must return true for native engine too
7878
@Test
79-
void namedGroupFallbackHasNamedGroupsTrue() {
80-
// alternation inside lookbehind triggers fallback; named group present
79+
void namedGroupInLookbehind_nativeEngineSupportsGroupAccess() {
80+
// Bug 4 fixed: alternation inside lookbehind is now handled natively
8181
ReggieMatcher m = Reggie.compile("(?<=a|b)(?<x>c)");
82-
assertTrue(m instanceof JavaRegexFallbackMatcher);
82+
assertFalse(m instanceof JavaRegexFallbackMatcher);
8383
MatchResult r = m.findMatch("ac");
8484
assertNotNull(r);
8585
assertTrue(r.hasNamedGroups());
Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
/*
2+
* Copyright 2026-Present Datadog, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.datadoghq.reggie.runtime;
17+
18+
import static org.junit.jupiter.api.Assertions.*;
19+
20+
import com.datadoghq.reggie.Reggie;
21+
import java.util.List;
22+
import org.junit.jupiter.api.Tag;
23+
import org.junit.jupiter.api.Test;
24+
25+
/** Acceptance tests for REQ-DataDog-java-reggie-30: lookbehind alternation correctness. */
26+
public class LookbehindAlternationTest {
27+
28+
// --- Positive lookbehind alternation (?<=a|b)c ---
29+
30+
@Test
31+
@Tag("acceptance")
32+
public void positiveLookbehindAlternation_firstAlternativeMatches() {
33+
ReggieMatcher m = Reggie.compile("(?<=a|b)c");
34+
assertTrue(m.find("ac"), "'ac': 'c' preceded by 'a' must match");
35+
}
36+
37+
@Test
38+
@Tag("acceptance")
39+
public void positiveLookbehindAlternation_secondAlternativeMatches() {
40+
// Core reproduction from the bug report: only first alternative was evaluated
41+
ReggieMatcher m = Reggie.compile("(?<=a|b)c");
42+
assertTrue(
43+
m.find("bc"), "'bc': 'c' preceded by 'b' must match — was incorrectly false before fix");
44+
}
45+
46+
@Test
47+
@Tag("acceptance")
48+
public void positiveLookbehindAlternation_noMatchWhenPrecursorAbsent() {
49+
ReggieMatcher m = Reggie.compile("(?<=a|b)c");
50+
assertFalse(m.find("xc"), "'xc': 'c' not preceded by 'a' or 'b' must not match");
51+
assertFalse(m.find("xca"), "'xca': 'c' at index 1 is preceded by 'x', not 'a' or 'b'");
52+
}
53+
54+
@Test
55+
@Tag("acceptance")
56+
public void positiveLookbehindAlternation_fullReproductionCase() {
57+
ReggieMatcher m = Reggie.compile("(?<=a|b)c");
58+
assertTrue(m.find("ac"), "ac => true (first alternative)");
59+
assertTrue(m.find("bc"), "bc => true (second alternative, was false before fix)");
60+
assertFalse(m.find("xc"), "xc => false (no matching predecessor)");
61+
}
62+
63+
// --- Negative lookbehind alternation (?<!a|b)c ---
64+
65+
@Test
66+
@Tag("acceptance")
67+
public void negativeLookbehindAlternation_firstAlternativeSuppressesMatch() {
68+
ReggieMatcher m = Reggie.compile("(?<!a|b)c");
69+
assertFalse(m.find("ac"), "'ac': 'c' preceded by 'a' must NOT match negative lookbehind");
70+
}
71+
72+
@Test
73+
@Tag("acceptance")
74+
public void negativeLookbehindAlternation_secondAlternativeSuppressesMatch() {
75+
ReggieMatcher m = Reggie.compile("(?<!a|b)c");
76+
assertFalse(m.find("bc"), "'bc': 'c' preceded by 'b' must NOT match negative lookbehind");
77+
}
78+
79+
@Test
80+
@Tag("acceptance")
81+
public void negativeLookbehindAlternation_matchWhenPrecursorAbsent() {
82+
ReggieMatcher m = Reggie.compile("(?<!a|b)c");
83+
assertTrue(m.find("xc"), "'xc': 'c' not preceded by 'a' or 'b' must match negative lookbehind");
84+
}
85+
86+
@Test
87+
@Tag("acceptance")
88+
public void negativeLookbehindAlternation_fullCoverage() {
89+
ReggieMatcher m = Reggie.compile("(?<!a|b)c");
90+
assertFalse(m.find("ac"), "ac => false (first alternative blocks)");
91+
assertFalse(m.find("bc"), "bc => false (second alternative blocks)");
92+
assertTrue(m.find("xc"), "xc => true (no alternative matches, assertion passes)");
93+
}
94+
95+
// --- Multi-character equal-width lookbehind alternation (?<=ab|cd)x ---
96+
97+
@Test
98+
@Tag("acceptance")
99+
public void multiCharPositiveLookbehind_firstAlternativeMatches() {
100+
ReggieMatcher m = Reggie.compile("(?<=ab|cd)x");
101+
assertTrue(m.find("abx"), "'abx': 'x' preceded by 'ab' must match");
102+
}
103+
104+
@Test
105+
@Tag("acceptance")
106+
public void multiCharPositiveLookbehind_secondAlternativeMatches() {
107+
ReggieMatcher m = Reggie.compile("(?<=ab|cd)x");
108+
assertTrue(m.find("cdx"), "'cdx': 'x' preceded by 'cd' must match");
109+
}
110+
111+
@Test
112+
@Tag("acceptance")
113+
public void multiCharPositiveLookbehind_noMatchForWrongPrefix() {
114+
ReggieMatcher m = Reggie.compile("(?<=ab|cd)x");
115+
assertFalse(m.find("efx"), "'efx': 'x' not preceded by 'ab' or 'cd' must not match");
116+
assertFalse(m.find("acx"), "'acx': partial overlap must not match");
117+
assertFalse(m.find("xbx"), "'xbx': 'x' preceded by 'xb', not 'ab' or 'cd'");
118+
}
119+
120+
@Test
121+
@Tag("acceptance")
122+
public void multiCharPositiveLookbehind_bothAlternativesInString() {
123+
ReggieMatcher m = Reggie.compile("(?<=ab|cd)x");
124+
List<MatchResult> matches = m.findAll("abx cdx");
125+
assertEquals(2, matches.size(), "Both 'abx' and 'cdx' occurrences must be found");
126+
}
127+
128+
@Test
129+
@Tag("acceptance")
130+
public void multiCharNegativeLookbehind_bothAlternativesSuppress() {
131+
ReggieMatcher m = Reggie.compile("(?<!ab|cd)x");
132+
assertFalse(m.find("abx"), "'abx': 'x' preceded by 'ab' must not match negative lookbehind");
133+
assertFalse(m.find("cdx"), "'cdx': 'x' preceded by 'cd' must not match negative lookbehind");
134+
assertTrue(m.find("efx"), "'efx': 'x' not preceded by 'ab' or 'cd' must match");
135+
}
136+
137+
// --- Native engine usage assertions ---
138+
139+
@Test
140+
@Tag("acceptance")
141+
public void positiveLookbehindAlternation_usesNativeEngine() {
142+
ReggieMatcher m = Reggie.compile("(?<=a|b)c");
143+
assertFalse(
144+
m instanceof JavaRegexFallbackMatcher,
145+
"(?<=a|b)c must be handled by the native Reggie engine, not java.util.regex fallback");
146+
}
147+
148+
@Test
149+
@Tag("acceptance")
150+
public void negativeLookbehindAlternation_usesNativeEngine() {
151+
ReggieMatcher m = Reggie.compile("(?<!a|b)c");
152+
assertFalse(
153+
m instanceof JavaRegexFallbackMatcher,
154+
"(?<!a|b)c must be handled by the native Reggie engine, not java.util.regex fallback");
155+
}
156+
157+
@Test
158+
@Tag("acceptance")
159+
public void multiCharLookbehindAlternation_usesNativeEngine() {
160+
ReggieMatcher m = Reggie.compile("(?<=ab|cd)x");
161+
assertFalse(
162+
m instanceof JavaRegexFallbackMatcher,
163+
"(?<=ab|cd)x must be handled by the native Reggie engine, not java.util.regex fallback");
164+
}
165+
}

0 commit comments

Comments
 (0)