fix: case-insensitive search highlights the wrong bytes - #1157
Conversation
searchWord.FindAll lowercased the target and returned byte offsets into that copy, but callers index the original string. Lowercasing changes the byte length of some runes, so every match after one of them was shifted and the highlight landed mid-rune. allStringIndex is untouched: the remap is done in FindAll, its only case-insensitive caller.
|
Thanks for fixing the byte offset issue. One concern is the additional cost of supporting Unicode lowercasing, especially since this search path is used frequently. Rather than extending case-insensitive search to cover Unicode characters, would it make sense to avoid strings.ToLower here and only convert ASCII A-Z to a-z? That would preserve the byte length and avoid the offset mapping table, while also keeping the change simpler and faster. I’m not sure whether Unicode case-insensitive matching is an intended feature, so I’d like to hear your thoughts before deciding. |
|
Good question, and I checked rather than guessed. Unicode folding is not something this PR adds: So it would be a behaviour change for accented, Cyrillic and Greek text, not just a speedup. On cost, If you would rather keep it strictly ASCII I am happy to change it, but I would suggest doing that as its own decision since it removes existing behaviour. Your call. |
|
I do not use these characters very often, so Cyrillic and Greek letters were not included intentionally. They are included because It is difficult for me to judge how much this behavior affects users who work with these characters. If this causes problems for anyone, please leave a comment with your use case or an example. |
|
Understood, and that settles it from my side: I will leave the folding as it is. Worth noting the scope of this PR either way. It only touches So if you later decide to restrict folding to ASCII, that is an independent change and this one does not get in its way. |
|
I have created a reference implementation in #1159. I would like to wait and see if anyone finds value in this specification. |
What's broken
Case-insensitive search highlights the wrong bytes when the line contains a rune whose lowercase form has a different byte length.
searchWord.FindAlllowercases the target and returns offsets into that copy, butsearchHighlightandsearchXPosindex the original string.İis 2 bytes and lowercases to 1, so later matches shift left.Ⱥis 2 and lowercases to 3, so they shift right. 23 runes shrink and 2 grow.Repro
File containing
İİ error here, searching forerror:The reverse-video run starts one byte early and splits the word.
The fix
toLowerWithOffsetslowercases and returns a table mapping each byte of the result back to the original, andFindAllremaps the indexes through it.I did not touch
allStringIndex, since it also serves the column-delimiter path. The remap lives inFindAll, the only case-insensitive caller.ASCII lines take a fast path that returns a nil table and the original single
strings.ToLower, so the common case allocates nothing extra.Verification
Two subtests, one per direction, on a shrinking and a growing rune. Skipping the remap fails both with the matched substring shown, not a build error.
I checked
toLowerWithOffsetsagainststrings.ToLowerfor every valid rune, alone and in context, and on invalid UTF-8, and asserted the offset table is monotonic and ends atlen(s). That probe was a scratch test and is not in the diff.Output above is from real binaries under a pty.
go test ./...passes,go vetandgofmtclean.