Repository navigation
Integrate #364–#366 with rename safety, w+ seek and gsub allocation fixes - #368
Merged
Merged
Conversation
FileSystem.Rename removed the destination file before checking that the source exists, so a failed os.rename lost the destination's contents. Check the source first, as rename(2) does. With the data no longer lost, the part of files.lua that the test harness skipped now runs unmodified.
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.
# Conflicts: # tests/Lua.Tests/IOTests.cs
akeit0
marked this pull request as ready for review
October 23, 2026 07:07
This was referenced Oct 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This integrates #364, #365 and #366 and fixes the remaining problems found during review. Failed renames preserve an existing destination (including a locked Windows source and directory-to-file attempts),
w+writes respect the seek position, and suspendedgsubcalls retain the pooled async builder.The branch preserves all three original PR heads as ancestors. Merge with a merge commit, so their commits and authorship remain in the history.
Changes added during review:
File.Moveon .NET 6/8/10 andFile.Replacefor existing destinations on .NET Standard 2.1; useDirectory.Movefor directories. Remove the delete-before-move window.WriteUpdateas append mode; test bothw+andw+b, and retain append-after-seek coverage fora+/a+b.AsyncMethodBuildertoGSub, and report Lua'sunfinished captureerror for an incomplete capture referenced by a replacement.GSubBenchmarkcases for literal/capture/position/frontier replacements and synchronous/suspended function and table-index handlers.Validation on Windows, .NET 8.0.32 (SDK 11 preview):
ExpectedFailurecategory. Includespm.lua, restoredfiles.luaassertions, Incorrect handling of^anchor ingsubwhen the match succeeds, leading to multiple replacements #322 anchor coverage, Inconsistent behavior of string.find with patterns #356 bracket-class coverage, and string.gsub ignores __index on replacement tables #359 replacement-table metatable coverage.File.Replacepath.Performance, median of 7 samples, precompiled scripts, same-thread controlled suspension, tiered compilation disabled for steady JIT code:
The builder fix removes the original #366 regression of 256 B per suspended gsub call. Literal/function paths retain their allocation counts. Frontier-at-end now performs the previously missing replacement (1040 B instead of 840 B), so that allocation change reflects corrected behavior. These are local microbenchmarks, not Unity/Mono/IL2CPP measurements. The .NET Standard replacement path uses same-filesystem
File.Replace; cross-volume replacement is not supported there.The new destination-preservation and unfinished-capture tests were run before the follow-up fixes and reproduced their failures. Both w+ seek/write cases also reproduced the append bug before its fix.
Draft pending maintainer approval to merge.
GitHub CI for head
2f6b81954edbf00a36f0f48222388628fc17f759: Linux runtime tests and repository lint both passed.