|
| 1 | +# Fix Transliteration Operator Issues in op/tr.t |
| 2 | + |
| 3 | +## Objective |
| 4 | +Fix the remaining issues in the transliteration operator (`tr///` and `y///`) to unblock tests in `t/op/tr.t`. |
| 5 | + |
| 6 | +## Current Status |
| 7 | +- **Test file:** `t/op/tr.t` |
| 8 | +- **Tests passing:** 245/318 (77%) |
| 9 | +- **Tests blocked:** 73 |
| 10 | +- **Previously:** 131/318 passing (41%) |
| 11 | +- **Improvement:** +114 tests unblocked |
| 12 | + |
| 13 | +## Issues Fixed (Session 2025-10-03) |
| 14 | + |
| 15 | +### 1. ✅ Missing compile-time error for `!~ with tr///r` |
| 16 | +**Problem:** No error when using `!~` with non-destructive transliteration (`/r` modifier) |
| 17 | +```perl |
| 18 | +$_ !~ y/a/b/r # Should error: "Using !~ with tr///r doesn't make sense" |
| 19 | +``` |
| 20 | + |
| 21 | +**Solution:** Added validation in `EmitRegex.handleNotBindRegex()` |
| 22 | +```java |
| 23 | +if (node.right instanceof OperatorNode operatorNode |
| 24 | + && (operatorNode.operator.equals("tr") || operatorNode.operator.equals("transliterate")) |
| 25 | + && operatorNode.operand instanceof ListNode listNode |
| 26 | + && listNode.elements.size() >= 3) { |
| 27 | + Node modifiersNode = listNode.elements.get(2); |
| 28 | + if (modifiersNode instanceof StringNode stringNode) { |
| 29 | + String modifiers = stringNode.value; |
| 30 | + if (modifiers.contains("r")) { |
| 31 | + throw new PerlCompilerException(node.tokenIndex, |
| 32 | + "Using !~ with tr///r doesn't make sense", |
| 33 | + emitterVisitor.ctx.errorUtil); |
| 34 | + } |
| 35 | + } |
| 36 | +} |
| 37 | +``` |
| 38 | + |
| 39 | +**Impact:** Test 129 now passes |
| 40 | + |
| 41 | +### 2. ✅ Read-only value modification with `tr///` |
| 42 | +**Problem:** `$1 =~ tr/A-Z//` threw "Modification of a read-only value" error even when only counting |
| 43 | + |
| 44 | +**Perl Behavior:** |
| 45 | +- `tr/A-Z//` on read-only = OK (counting only, no replacement) |
| 46 | +- `tr/a/b/` on read-only = ERROR (has replacement) |
| 47 | +- Empty string with replacement = ERROR |
| 48 | +- Empty string without replacement = OK |
| 49 | + |
| 50 | +**Solution:** Modified `RuntimeTransliterate.transliterate()` to only call `set()` when needed: |
| 51 | +```java |
| 52 | +// Determine if we need to call set() which will trigger read-only error if applicable |
| 53 | +// We must call set() if: |
| 54 | +// 1. The string actually changed, OR |
| 55 | +// 2. It's an empty string AND we have a replacement operation (not just counting) |
| 56 | +boolean hasReplacement = !replacementChars.isEmpty() || deleteUnmatched; |
| 57 | +boolean needsSet = !input.equals(resultString) || (input.isEmpty() && hasReplacement); |
| 58 | + |
| 59 | +if (needsSet) { |
| 60 | + originalString.set(resultString); |
| 61 | +} |
| 62 | +``` |
| 63 | + |
| 64 | +**Impact:** Tests 131-245 unblocked (+114 tests) |
| 65 | + |
| 66 | +## Remaining Issues |
| 67 | + |
| 68 | +### 1. ❌ `Internals::SvREADONLY` not working |
| 69 | +**Problem:** `Internals::SvREADONLY($var, 1)` doesn't actually make variables read-only in PerlOnJava |
| 70 | + |
| 71 | +**Test case:** |
| 72 | +```perl |
| 73 | +my $x = ""; |
| 74 | +Internals::SvREADONLY($x, 1); |
| 75 | +$x =~ tr/a/b/; # Should error but doesn't |
| 76 | +``` |
| 77 | + |
| 78 | +**Impact:** Tests 250-251 failing (expecting read-only errors) |
| 79 | + |
| 80 | +**Investigation needed:** |
| 81 | +- Find where `Internals::SvREADONLY` is implemented |
| 82 | +- Check if it sets the read-only flag correctly |
| 83 | +- Verify RuntimeScalarReadOnly is used |
| 84 | + |
| 85 | +### 2. ❌ Named sequences in `tr///` |
| 86 | +**Problem:** `\N{name}` named sequences not supported in transliteration |
| 87 | +```perl |
| 88 | +$s =~ tr/\N{LATIN SMALL LETTER A WITH ACUTE}/A/; # Errors |
| 89 | +``` |
| 90 | + |
| 91 | +**Error:** `\N{LATIN SMALL LETTER A WITH ACUTE} must not be a named sequence in transliteration operator` |
| 92 | + |
| 93 | +**Impact:** Test 256 blocks remaining tests (62 tests blocked) |
| 94 | + |
| 95 | +**Investigation needed:** |
| 96 | +- Check where `\N{}` is parsed in tr/// context |
| 97 | +- May need to expand named sequences to actual characters |
| 98 | +- Look at `expandRangesAndEscapes()` in RuntimeTransliterate |
| 99 | + |
| 100 | +### 3. ⚠️ Warning for `tr///r` in void context |
| 101 | +**Problem:** No warning for useless non-destructive transliteration in void context |
| 102 | +```perl |
| 103 | +y///r; # Should warn: "Useless use of non-destructive transliteration (tr///r)" |
| 104 | +``` |
| 105 | + |
| 106 | +**Impact:** Test 130 failing |
| 107 | + |
| 108 | +**Implementation needed:** |
| 109 | +- Add void context detection in EmitRegex.handleTransliterate() |
| 110 | +- Generate warning when `/r` modifier used in void context |
| 111 | + |
| 112 | +## Files Modified |
| 113 | +1. `/Users/fglock/projects/PerlOnJava/src/main/java/org/perlonjava/codegen/EmitRegex.java` |
| 114 | + - Added `!~ with tr///r` validation |
| 115 | + |
| 116 | +2. `/Users/fglock/projects/PerlOnJava/src/main/java/org/perlonjava/operators/RuntimeTransliterate.java` |
| 117 | + - Fixed read-only handling logic |
| 118 | + |
| 119 | +## Test Commands |
| 120 | +```bash |
| 121 | +# Run full test |
| 122 | +./jperl t/op/tr.t |
| 123 | + |
| 124 | +# Count passing tests |
| 125 | +./jperl t/op/tr.t 2>&1 | grep -c "^ok" |
| 126 | + |
| 127 | +# Check specific test |
| 128 | +./jperl t/op/tr.t 2>&1 | grep -A 3 "not ok 250" |
| 129 | + |
| 130 | +# Test read-only behavior |
| 131 | +echo 'my $x = ""; Internals::SvREADONLY($x, 1); $x =~ tr/a/b/' | ./jperl |
| 132 | + |
| 133 | +# Test named sequences |
| 134 | +echo '$s = "á"; $s =~ tr/\N{LATIN SMALL LETTER A WITH ACUTE}/A/' | ./jperl |
| 135 | +``` |
| 136 | + |
| 137 | +## Implementation Checklist |
| 138 | +- [x] Add compile-time error for `!~ with tr///r` |
| 139 | +- [x] Fix read-only value handling for counting operations |
| 140 | +- [ ] Fix `Internals::SvREADONLY` implementation |
| 141 | +- [ ] Add support for `\N{name}` in transliteration |
| 142 | +- [ ] Add void context warning for `tr///r` |
| 143 | +- [ ] Handle more complex transliteration edge cases |
| 144 | + |
| 145 | +## Priority Assessment |
| 146 | +**High Priority:** Named sequences issue (blocks 62 tests) |
| 147 | +**Medium Priority:** Internals::SvREADONLY (2 tests) |
| 148 | +**Low Priority:** Void context warning (1 test) |
| 149 | + |
| 150 | +## Commits |
| 151 | +- `ab480e0d`: Added compile-time error for !~ with tr///r |
| 152 | +- `ff614096`: Fixed tr/// on read-only values |
| 153 | + |
| 154 | +## Next Steps |
| 155 | +1. Investigate where `\N{}` sequences are parsed/expanded |
| 156 | +2. Check if similar issue exists in regex patterns |
| 157 | +3. Consider if named sequences should be expanded at parse time or runtime |
| 158 | +4. Look for existing Unicode name resolution code to leverage |
| 159 | + |
| 160 | +## Success Criteria |
| 161 | +- All 318 tests in op/tr.t passing |
| 162 | +- Proper read-only handling matching Perl behavior |
| 163 | +- Named sequence support in transliteration |
| 164 | +- Appropriate warnings generated |
0 commit comments