fix: throw instead of corrupting the chunk list on an overlapping move - #326
Merged
antfu merged 2 commits intoAug 19, 2026
Merged
Conversation
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.
Fixes #325.
The problem
Both calls are in bounds and neither throws. The second one corrupts the chunk list into a cycle, and every later
toString()orgenerateMap()spins at 100% CPU.move()looks the range up asbyStart.get(start)andbyEnd.get(end)and then splices, which assumes those two chunks are still the ends of a forward run. Aftermove(0, 2, 3)the order isc(2,3) a(0,1) b(1,2) d(3,4)…, so the second call getsfirst = b(1,2)andlast = c(2,3)withlastsitting beforefirst. The pointer rewrites then leavea(0,1).next === a(0,1), and the walk intoString()has nowhere to stop.The change
Before splicing, walk from
firsttolastand confirm every chunk on the way is still inside[start, end). If the run has been broken up, throw aMagicStringErrorinstead of building a cycle. That matches whatmove()already does for the other case it cannot honour,cannot move a selection inside itself.Only
move()reorders chunks, so the walk is skipped entirely until one has run. A first move, which is the overwhelmingly common case, costs nothing extra. I addedhasMovedChunksfor that, andclone()copies it: a clone rebuilds the chunks in their current order, so it inherits any reordering and needs the same check. Without that line a clone of a moved string still hung, which I only noticed after readingclone(), so there is a test for it.I went with throwing rather than trying to define an ordering for overlapping moves, since that is a semantics decision rather than a bug fix, and the issue offered either. Happy to look at handling the case properly instead if you would prefer that.
Tests
Two cases in the existing
moveblock, both failing onmain:clone(), covering the flag being carried over233 passing, lint clean. The existing
handles moves to same indextest, which does two moves where the second does not straddle the first, still passes and was the main thing I wanted to be sure of.