util: strip whole CSI sequences per ECMA-48 - #65379
Open
luantaraschi wants to merge 1 commit into
Open
Conversation
stripVTControlCharacters() recognises a control sequence by a hand written list of final bytes, inherited from the bundled copy of ansi-regex. The list covers neither parameter bytes such as `<` and `:`, used by mouse reports and by sub parameters, nor final bytes such as `@`, `X`, `d`, `a` and `b`. A sequence that uses one of them matches only in part, so the rest of it is left in the string rather than removed. getStringWidth() strips before measuring and readline places the cursor from that width, so the leftovers are counted as printable columns. Add an alternative built from the control sequence structure in ECMA-48 5.4: any number of parameter bytes (0x30-0x3F), any number of intermediate bytes (0x20-0x2F), then a single final byte (0x40-0x7E). Both existing alternatives are kept and the new one is only reached where they do not match, so a match can be extended but never shortened. The new alternative carries its own introducer instead of sharing the existing prefix, because the shared prefix also consumes `;` and the two competing for the same run made the match quadratic. Signed-off-by: Luan Taraschi <130802253+luantaraschi@users.noreply.github.com>
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.
stripVTControlCharacters()recognises a control sequence by a hand written list of final bytes, inherited from the bundled copy ofansi-regex:The list is incomplete in two directions. It misses parameter bytes such as
<, and it misses final bytes such as@,X,d,aandb. When a sequence uses one of them the match ends in the middle of it and the rest is left in the string. The function does not fail to strip, it strips the wrong bytes.Measured on
mainat e2b33e2:a ESC [ < 35 ; 10 ; 20 M ba35;10;20Mbaba ESC [ > 1 ; 2 c ba1;2cbaba ESC [ 3 @ ba@baba ESC [ 3 X baXbaba ESC [ 5 d badbaba ESC [ 2 SP q ba qbaba ESC [ ! p baba ESC [ 38 : 2 : 255 : 0 : 0 m ba:2:255:0:0mbaba ESC [ 4 : 3 m ba:3mbabThe first row is an SGR mouse report, which is what a terminal sends back on every click once a program has enabled mouse reporting. Rows six and seven carry intermediate bytes:
DECSCUSRsets the cursor style andDECSTRis a soft reset, both emitted by common shells and editors. The last two use:to separate sub parameters, which is how truecolour and curly underlines are written by editors that follow the newer form.This is not limited to the public API.
getStringWidth()strips before measuring andlib/internal/readline/interface.jsplaces the cursor from that width, so the leftovers are counted as printable columns:The change
ECMA-48 5.4 defines a control sequence as CSI, then any number of parameter bytes (0x30-0x3F), then any number of intermediate bytes (0x20-0x2F), then a single final byte (0x40-0x7E). This adds that as an alternative and leaves both existing alternatives in place:
ESC \or 0x9C match exactly as before;ESC 7andESC 8, which the ECMA-48 alternative does not.The new alternative is only reached where the string terminator alternative does not match, so the change can extend a match but never shorten one. I checked that rather than assuming it. Over 400000 pseudo random strings built from introducers, parameter bytes, intermediate bytes, final bytes, string terminators and ordinary text, 10640 come out different from
main, and in zero of them does the new pattern leave more behind than the current one.Two shapes I tried first, and why they are not here
Both were discarded on measurements, and both are easy to arrive at, so they seem worth recording.
Sharing the existing introducer prefix makes the match quadratic. The prefix
[[\]()#;?]*also consumes;, so it and the parameter byte class compete for the same run and every split is retried. OnESC [followed by n semicolons and no final byte that shape took 411 ms at n = 20000 and 2.6 s at n = 50000. Giving the new alternative its own introducer, with a prefix that excludes;and?, keeps it linear: 200000 semicolons take 0.9 ms onmainand 1.2 ms here.Putting the new alternative first is faster and wrong. With the control sequence alternative ahead of the string terminator one, the common
ESC [ ... mis matched immediately and the whole thing gets faster thanmain, by about 70% on the corpora below. It also changes the result for 226 of those 400000 strings, becauseESC [followed by text and a string terminator is matched today by the string terminator alternative and would then be cut short. I would rather be slower than change what those return, but if you disagree the reordering is a two line difference and I am happy to switch.Cost
Measured with the built binary inside a quiet Linux container, minimum of 41 rounds of 50 iterations, three independent runs, reported as the range across them:
Text with no escape sequences is unaffected, which is the short circuit at the top of
stripVTControlCharacters()doing its job. Text that does contain them costs one extra alternative per match. In absolute terms it is tens of microseconds per 40 to 60 KB, but it is a real regression and I would rather put the number here than have it found in review.Testing
test/parallel/test-util-stripvtcontrolcharacters.jsgains thirteen cases, one per sequence family above. I verified them in both directions on a real binary rather than by reasoning about the pattern: reverting onlylib/internal/util/inspect.js, rebuilding and running the file givesand it passes with the change restored. None of the thirteen passed before. The eleven cases already in the file are untouched.
Built with GCC 14 on Debian trixie,
v27.0.0-pre.python3 tools/test.py -J parallelgives 4719 passing and 3 failing:test-permission-drop-ffi, andtest-cluster-primary-errorandtest-cluster-primary-killon timeout. None of the three mentionsstripVTControlCharactersorgetStringWidth, and all three fail the same way on a binary built from the revertedlib/internal/util/inspect.js, so they are my container and not this change.The twelve files that do reach this regex through
getStringWidth()all pass:test-icu-stringwidth,test-util-inspect,test-util, the four readline files,test-repl-multiline,test-repl-highlight,test-util-inspect-regexp,test-runner-runand the stripping test itself.Note on #64319
#64319 is open and approved against the same regex, rewriting the OSC alternative so hyperlinks with
(in the URI strip correctly. They are mostly disjoint, that one being about the string terminator alternative and this one about control sequences, but they do meet in one place and it seems better to say so than to let it be found: #64319 also fixes the:sub parameter case, by adding[;:]to the hand written list. This change fixes it too, as a consequence of:being a parameter byte in the grammar rather than by extending the list. I checked on the built binary that the two cases #64319 adds for the OSC alternative, and the reproduction in #64313, return exactly the same string with this change applied. If #64319 lands first I will rebase on it.Disclosure: I used an AI coding assistant while working on this, as a research and drafting aid. The analysis, the choice of the ECMA-48 grammar, the two discarded shapes and every measurement above are mine and I ran them myself. I can explain any line of this change in review.