fix(protocol): saturating_mul undercounted the percentage silently
byte_pos.saturating_mul(100) does not merely lose precision at the top of the range --- it collapses the numerator to a constant. u64::MAX times 100 saturates to u64::MAX, and u64::MAX / u64::MAX is 1, so a cursor at the very end of a maximal buffer read 1%. Wrong in the worst way available: in range, plausible, and passing every test. percent_is_always_in_range asserted only p <= 100, which Percent(1) satisfies perfectly. The sweep even included the exact (u64::MAX, u64::MAX) pair and reported success, because it never asked what the answer should be. Computes in u128 now. u64::MAX * 100 fits with room to spare, so the product is exact and the only remaining clamp is the genuine one --- a caller reporting a cursor past the end still gets 100%, never above. large_byte_counts_stay_accurate is the correctness witness the range sweep could not be. It bites: against the old arithmetic it fails with left: Percent(1) right: Percent(100) while percent_is_always_in_range keeps passing, which is the point of adding it rather than extending that one. It also pins u64::MAX/2 at 49% and u64::MAX/4 at 24%, and includes u64::MAX/100 + 1 --- the smallest position whose scaling overflows u64, and therefore the first input the old code got wrong. percent_is_always_in_range keeps its sweep and gains a note about what it does not prove, so the next reader does not mistake bounded for correct. Gates: fmt, workspace clippy -D warnings, diff --check, pmacs-protocol --lib 25/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
parent
34f7913f9d
commit
b8639130d9
|
|
@ -85,10 +85,18 @@ pub fn classify(
|
|||
if byte_len == 0 {
|
||||
return ScrollPosition::All;
|
||||
}
|
||||
// Saturating, then clamped: a caller that reports a cursor
|
||||
// past the end (a stale readout mid-edit) gets 100%, not a
|
||||
// wrapped or panicking percent.
|
||||
let pct = byte_pos.saturating_mul(100) / byte_len;
|
||||
// Widen to u128 before scaling. `saturating_mul` was wrong
|
||||
// here, not merely inelegant: it *silently undercounts*.
|
||||
// `u64::MAX * 100` saturates to `u64::MAX`, so a cursor at
|
||||
// the end of a maximal buffer divided out to 1% — a wrong
|
||||
// answer that looked safe because it stayed in range.
|
||||
//
|
||||
// `u64::MAX * 100` fits in u128 with room to spare, so the
|
||||
// product is exact and the only clamp left is the genuine
|
||||
// one below.
|
||||
let pct = u128::from(byte_pos) * 100 / u128::from(byte_len);
|
||||
// Clamped for a caller that reports a cursor past the end
|
||||
// (a stale readout mid-edit): 100%, never above.
|
||||
ScrollPosition::Percent(u8::try_from(pct.min(100)).unwrap_or(100))
|
||||
}
|
||||
}
|
||||
|
|
@ -156,6 +164,12 @@ mod tests {
|
|||
}
|
||||
|
||||
/// Percent never leaves `0..=100`, for any input.
|
||||
///
|
||||
/// **In range is not the same as correct**, which is why
|
||||
/// [`large_byte_counts_stay_accurate`] exists beside this. This
|
||||
/// sweep passed against a `saturating_mul` that silently reported
|
||||
/// 1% for a cursor at the end of a maximal buffer — a wrong answer
|
||||
/// that satisfies every assertion here.
|
||||
#[test]
|
||||
fn percent_is_always_in_range() {
|
||||
for pos in [0_u64, 1, 7, 99, 100, 1_000, u64::MAX / 2, u64::MAX] {
|
||||
|
|
@ -166,4 +180,39 @@ mod tests {
|
|||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// The percentage stays *accurate* where `u64` arithmetic would
|
||||
/// overflow, not merely bounded.
|
||||
///
|
||||
/// `byte_pos * 100` exceeds `u64::MAX` for any position above
|
||||
/// `u64::MAX / 100`. Saturating there collapses the numerator to a
|
||||
/// constant, so the quotient stops tracking the position at all:
|
||||
/// `u64::MAX / u64::MAX` is 1, and the readout said **1%** at the
|
||||
/// very end of the buffer.
|
||||
#[test]
|
||||
fn large_byte_counts_stay_accurate() {
|
||||
assert_eq!(
|
||||
classify(false, false, u64::MAX, u64::MAX),
|
||||
ScrollPosition::Percent(100),
|
||||
"the end of a maximal buffer is 100%, not 1%"
|
||||
);
|
||||
assert_eq!(
|
||||
classify(false, false, u64::MAX / 2, u64::MAX),
|
||||
ScrollPosition::Percent(49),
|
||||
"halfway through a maximal buffer, floored"
|
||||
);
|
||||
assert_eq!(
|
||||
classify(false, false, u64::MAX / 4, u64::MAX),
|
||||
ScrollPosition::Percent(24),
|
||||
"a quarter through, floored"
|
||||
);
|
||||
// The smallest position whose scaling overflows u64 — the first
|
||||
// input the old implementation got wrong.
|
||||
let first_overflowing = u64::MAX / 100 + 1;
|
||||
assert_eq!(
|
||||
classify(false, false, first_overflowing, u64::MAX),
|
||||
ScrollPosition::Percent(1),
|
||||
"correct by arithmetic here, not by saturation"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue