|
| 1 | +# Fix Conditional Regex Pattern (?(condition)yes|no) - Unlock ~3000 Tests |
| 2 | + |
| 3 | +## Problem Statement |
| 4 | +The PerlOnJava regex preprocessor incorrectly handles conditional regex patterns `(?(condition)yes|no)`, causing 499 identical test failures across 6 regexp test files (re/regexp.t, re/regexp_noamp.t, re/regexp_notrie.t, re/regexp_qr.t, re/regexp_qr_embed.t, re/regexp_trielist.t), totaling approximately 3000 test failures. |
| 5 | + |
| 6 | +## Root Cause Analysis |
| 7 | + |
| 8 | +### The Bug |
| 9 | +The `handleConditionalPattern` method in `RegexPreprocessor.java` has an incomplete implementation that: |
| 10 | +1. Converts `(?(1)yes|no)` to `(?:yes|no)` (losing conditional semantics but allowing parsing) |
| 11 | +2. When called from `handleParentheses`, uses `return handleConditionalPattern(...)` which exits the entire function |
| 12 | +3. This breaks nested groups like `((?(1)a|b))` because the outer group's processing is cut short |
| 13 | + |
| 14 | +### Specific Issue |
| 15 | +In `RegexPreprocessor.java` line 350: |
| 16 | +```java |
| 17 | +} else if (c3 == '(') { |
| 18 | + // Handle (?(condition)yes|no) conditionals |
| 19 | + return handleConditionalPattern(s, offset, length, sb, regexFlags); |
| 20 | +``` |
| 21 | + |
| 22 | +The `return` statement exits `handleParentheses` entirely, which means: |
| 23 | +- For `((?(1)a|b))`, the outer `(` never finds its matching `)` |
| 24 | +- Results in "Unmatched (" errors |
| 25 | + |
| 26 | +## Investigation Process |
| 27 | + |
| 28 | +### Test Cases Created |
| 29 | +1. `test_regexp_issues.pl` - Tests various conditional patterns |
| 30 | +2. `test_conditional_debug.pl` - Focused debugging of nested conditionals |
| 31 | + |
| 32 | +### Key Findings |
| 33 | +- Simple `(?(1)a|b)` converts correctly to `(?:a|b)` |
| 34 | +- Nested `((?(1)a|b))` fails with "Unmatched (" error |
| 35 | +- Debug output shows `handleConditionalPattern` returns position 10, skipping the outer group's `)` |
| 36 | +
|
| 37 | +## Solution |
| 38 | +
|
| 39 | +### Immediate Fix |
| 40 | +Change line 350 from: |
| 41 | +```java |
| 42 | +return handleConditionalPattern(s, offset, length, sb, regexFlags); |
| 43 | +``` |
| 44 | +To follow the pattern used by other constructs: |
| 45 | +```java |
| 46 | +offset = handleConditionalPattern(s, offset, length, sb, regexFlags); |
| 47 | +``` |
| 48 | +
|
| 49 | +However, this alone won't work because `handleConditionalPattern` was designed to be called directly, not nested. |
| 50 | + |
| 51 | +### Complete Fix |
| 52 | +The issue is more complex because `handleConditionalPattern`: |
| 53 | +1. Processes the entire conditional pattern including its closing `)` |
| 54 | +2. Returns `pos + 1` which skips past the conditional's closing `)` |
| 55 | +3. When nested, this causes the outer group to miss its closing `)` |
| 56 | +
|
| 57 | +The fix requires careful handling of the return position to ensure nested groups work correctly. |
| 58 | +
|
| 59 | +## Implementation Notes |
| 60 | +
|
| 61 | +### Current Workaround |
| 62 | +The current implementation converts `(?(condition)yes|no)` to `(?:yes|no)`, which: |
| 63 | +- Allows the pattern to parse without errors |
| 64 | +- Loses the conditional semantics (always matches both branches) |
| 65 | +- Is marked with TODO for proper implementation |
| 66 | +
|
| 67 | +### Proper Implementation (Future) |
| 68 | +To fully support conditional regex, we would need: |
| 69 | +1. Track capture group states during matching |
| 70 | +2. Implement conditional branching logic in the regex engine |
| 71 | +3. This is a significant undertaking requiring regex engine modifications |
| 72 | +
|
| 73 | +## Testing |
| 74 | +
|
| 75 | +### Before Fix |
| 76 | +- re/regexp.t: 499 failures (1678/2177 passing) |
| 77 | +- Same 499 failures in 5 other regexp test files |
| 78 | +- Total: ~3000 test failures |
| 79 | +
|
| 80 | +### Expected After Fix |
| 81 | +- All 6 regexp test files should have 499 fewer failures |
| 82 | +- Total improvement: ~3000 tests |
| 83 | +
|
| 84 | +## File Modifications |
| 85 | +- `/Users/fglock/projects/PerlOnJava/src/main/java/org/perlonjava/regex/RegexPreprocessor.java` |
| 86 | + - Line 350: Change return to assignment |
| 87 | + - `handleConditionalPattern` method: Adjust return position handling |
| 88 | +
|
| 89 | +## Debug Commands |
| 90 | +```bash |
| 91 | +# Test simple and nested conditionals |
| 92 | +./jperl test_conditional_debug.pl |
| 93 | +
|
| 94 | +# Count failures in regexp tests |
| 95 | +./jperl t/re/regexp.t 2>&1 | grep "^not ok" | wc -l |
| 96 | +
|
| 97 | +# Run all 6 affected test files |
| 98 | +for test in regexp regexp_noamp regexp_notrie regexp_qr regexp_qr_embed regexp_trielist; do |
| 99 | + echo "Testing t/re/$test.t" |
| 100 | + ./jperl t/re/$test.t 2>&1 | grep "^not ok" | wc -l |
| 101 | +done |
| 102 | +``` |
| 103 | +
|
| 104 | +## Priority |
| 105 | +**HIGH** - This single fix will resolve ~3000 test failures, representing one of the highest-impact fixes possible in the project. |
| 106 | +
|
| 107 | +## Current Status |
| 108 | +- Root cause identified |
| 109 | +- Solution approach determined |
| 110 | +- Implementation in progress |
| 111 | +- Debug output added for verification |
| 112 | +
|
| 113 | +## Notes |
| 114 | +- The conditional regex pattern `(?(condition)yes|no)` is a Perl 5.10+ feature |
| 115 | +- Java's Pattern class doesn't natively support conditionals |
| 116 | +- Current workaround loses conditional semantics but prevents parsing errors |
| 117 | +- Full support would require significant regex engine enhancements |
0 commit comments