Repository navigation
Fix string.gsub replacement strings and %f; enable pm.lua - #366
Conversation
gsub built the replacement with chained StringBuilder.Replace calls, so a reference to a missing capture (%2) was silently kept, a bare or unknown %x was accepted, captured text was rescanned, and NUL characters in captured text turned into '%'. Replacement strings are now expanded in a single pass and follow Lua 5.2 (invalid capture index, invalid use of '%' in replacement string). %f never matched at the end of the subject because the current character was required to exist. Lua treats it as '\0' there. %f without a set now raises "missing '[' after '%f' in pattern". tests-lua/pm.lua passes and is enabled.
There was a problem hiding this comment.
🟡 Changes recommended
The helper insertion moves the async-builder attribute off GSub, introducing allocation overhead when asynchronous replacements suspend.
1 open issue
What changed in this PR
This PR improves Lua 5.2 compatibility in the string pattern library and enables the previously disabled pattern-matching suite.
Changes:
- Expands
string.gsubreplacement strings in a single pass, preserving captured text and validating replacement escapes. - Fixes frontier patterns at the end of a subject and reports missing frontier sets.
- Adds 17 regression cases and enables
tests-lua/pm.lua.
| File | Description |
|---|---|
| tests/Lua.Tests/LuaTests.cs | Enables the Lua pattern-matching suite. |
| tests/Lua.Tests/GSubReplacementStringTests.cs | Tests replacement strings, errors, and frontier behavior. |
| src/Lua/Standard/StringLibrary.cs | Introduces single-pass replacement expansion. |
| src/Lua/Standard/Internal/MatchState.cs | Corrects frontier boundary matching and validation. |
Review effort: Balanced
| } | ||
|
|
||
| [AsyncMethodBuilder(typeof(LightAsyncValueTaskMethodBuilder<>))] | ||
| static void AppendReplacement( |
|
Thank you for the replacement-string and frontier fixes, the focused regression tests, and enabling pm.lua! Your original commit has been merged through #368 with its authorship preserved. The single-pass replacement parser also substantially improved our capture-heavy microbenchmarks. Our follow-up restores the pooled async-method builder attribute on GSub, avoiding an extra 256 bytes per suspended call, and returns Lua's expected unfinished-capture error when a replacement references an incomplete capture. We added reusable benchmarks and regression coverage; the combined changes passed local tests and Linux CI. |
Three problems in the pattern library, found by running
tests-lua/pm.lua(currently commented out inLuaTests). The expected values in the new tests were taken from Lua 5.2.4.string.gsubreplacement stringsThe replacement was built with chained
StringBuilder.Replacecalls. As a result:string.gsub("alo", "(.)", "%2")returned a string instead of raisinginvalid capture index(pm.lua:208).%xand a trailing%were accepted instead of raisinginvalid use of '%' in replacement string.%(the NUL was used as a temporary marker).The replacement is now expanded in a single pass, like
add_sinlstrlib.c.%fat the end of the subject%f[set]never matched at the end of the subject, because the current character was required to exist. Lua uses'\0'there, sostring.gsub("function", "%f[^\1-\255]", ".")must give"function."(pm.lua:291).%fwithout a set%fnot followed by[silently failed to match. It now raisesmissing '[' after '%f' in pattern.Verification
tests-lua/pm.luais enabled inLuaTestsand passes. Onmainit fails at line 208.GSubReplacementStringTests: 8 of the 17 test cases fail onmain(with the source change reverted), and all pass with it.dotnet test tests/Lua.Tests/Lua.Tests.csproj -c Release --filter "TestCategory!=ExpectedFailure": 628 passed, 0 failed (610 onmain+ 17 new +pm.luaenabled).dotnet csharpier check .is clean.