internal/textbound: Cut's straddle-backup block cannot change what Cut returns, because ToValidUTF8 already drops the incomplete rune
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
Found while adding property tests to this package in #1991, and deliberately left out of that PR: removing working code that a table test pins is a judgment call worth making on its own rather than inside a diff about something else.
Problem or observed behavior
Cut walks back from the limit to find a rune the cut would split, and shortens the limit to that rune's start:
if len(s) > limit {
start := limit
for start > 0 && !utf8.RuneStart(s[start]) {
start--
}
// Only a rune that begins before the limit and ends past it straddles
// the cut. Continuation bytes that do not complete one are already
// invalid, and the sanitizing pass below drops them either way.
if _, size := utf8.DecodeRuneInString(s[start:]); start+size > limit {
limit = start
}
s = s[:limit]
}
return strings.ToValidUTF8(s, "")
Whatever that block decides, the answer is the same. Without it, s[:limit] keeps the leading bytes of the split rune, and those bytes are an incomplete sequence — so strings.ToValidUTF8(s, "") on the last line drops exactly them. With it, s[:start] never contains them in the first place. Both paths return the same string for every input.
The block is not wrong and it is not dead in the compiler's sense — it runs, and its own comment correctly anticipates half of this ("the sanitizing pass below drops them either way"). It is that the work it does is already done by the line after it. Whether that is worth eleven lines of the file most-read function in a package about a security-adjacent bound is the decision this issue asks for.
Evidence
Differential, both spellings against each other over five million random strings at every limit from -2 to len(s)+2. The generator mixes ASCII, valid two/three/four-byte runes, and arbitrary bytes, so straddles and pre-existing invalid input are both common rather than incidental:
0 differing (s, limit) pairs over 5000000 random strings
The same conclusion from the test side: deleting the block leaves go test ./internal/textbound green, including the twenty-six table rows in textbound_test.go that pin Cut and Truncate's exact outputs and the three property checks #1991 added. Every other mutation tried against those tests reds them — strings.Clone in place of ToValidUTF8, a limit off by one, an answer shortened by a byte per call — so this is not a gap in the tests, it is the block having no observable effect to test.
Reasoning that matches the measurement: the bytes s[start:limit] are, by the block's own condition, a strict prefix of one rune's encoding. A strict prefix of a multi-byte encoding is never valid UTF-8, and ToValidUTF8 removes every invalid byte wherever it sits. So ToValidUTF8(s[:limit]) and ToValidUTF8(s[:start]) differ only in bytes ToValidUTF8 deletes.
Desired outcome
One of two, decided rather than left implicit:
- The block goes, and the rune-splitting guarantee is documented as resting on
ToValidUTF8— which is where it actually rests today, and which the package doc already half-says. - The block stays because it states the intent independently of
ToValidUTF8's behaviour, and a comment says that it is deliberately redundant rather than load-bearing, so the next reader does not conclude the guarantee depends on it.
Either way the reader ends up with a true account of which line makes "no result ever splits a rune" true. Today the code implies it is the backup walk, and it is not.
Acceptance criteria
Cut's guarantee is attributable to a specific line by reading the function, and that attribution is correct.- If the block is removed:
go test ./internal/textboundstays green with no test changed, which is the evidence that nothing observable was removed, and the property check for a straddling rune still reports reaching that case (it reports 499 of 5000 draws today). - If the block is kept: its comment says it is redundant with the sanitizing pass and why it is kept anyway, so a future reader does not add a test for behaviour it cannot produce.
Constraints and dependencies
Callers depend on the postcondition, not on how it is reached: internal/textbound's package doc explains that a broken rune fails a whole protojson response rather than shortening its own sentence, which is why every result must be valid UTF-8. Nothing in that argument names the backup walk. Truncate is Cut plus a marker and decides whether to append it from len(s) > max(limit, 0) on the original string, so it is unaffected either way.
This is the one mutation, of seven tried while writing #1991's property tests, that reds nothing — recorded here because "a mutation that changes no behaviour" is a finding about the code and not about the tests.
Open questions
- Does the block have a value the measurement cannot see — a reader understanding the intent faster from an explicit walk than from a trailing sanitizer? That is a taste question about this specific function, and it is the whole of the decision.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the Cut implementation under internal/textbound and read textbound_test.go, then run go test ./internal/textbound to confirm the current behavior. Decide whether the straddle-backup block should be removed or retained as explicitly redundant, and leave Cut's guarantee and the chosen rationale accurately documented without changing the tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100