Skip to content

Commit 888c779

Browse files
fix(lib/common): two latent correctness bugs in stdin/radix paths (refs #22) (#24)
Two bugs flagged by clippy's correctness lints that would each cause real runtime failure given the right input -- not just style nits: 1. `read_all_lines` in lib/common/io.rs spun forever on a broken stdin. `BufRead::lines()` returns an `io::Lines` iterator that, once the underlying reader yields an error, keeps yielding the same `Err` forever. The old `filter_map(|l| l.ok())` swallowed every `Err` and asked for the next one -- so a single decoding error or closed handle made the function never return. Swapped to `map_while(Result::ok)` so the iterator terminates at the first error (clippy::lines_filter_map_ok). 2. `int_to_string_radix` (string.rs:209) and `int_to_str_radix` (types.rs:103) both did `n.abs() as u64`, which panics in debug on `i64::MIN` because `i64::MIN.abs()` overflows (the magnitude 9_223_372_036_854_775_808 doesn't fit in `i64`). Swapped to `n.unsigned_abs()`, which returns the magnitude as `u64` directly and is defined for every `i64` (clippy::cast_abs_to_unsigned). Regression tests added for the radix-conversion bug: * `test_radix_i64_min_does_not_panic` in lib/common/string.rs * `test_int_to_str_radix_i64_min_does_not_panic` in lib/common/types.rs Both formerly panicked in debug builds (the test process aborted with "attempt to negate with overflow"); both now round-trip to "-9223372036854775808". `read_all_lines` is harder to regression-test in unit tests since it reads real stdin; the behavior change is verified by the swapped iterator combinator and clippy no longer flagging that site. Part 2 of 4 from #22. Off clean origin/main; does not depend on PR1 (#23).
1 parent fe4b51c commit 888c779

3 files changed

Lines changed: 35 additions & 4 deletions

File tree

crates/my-lang/lib/common/io.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,17 @@ pub fn read_line_prompt(prompt: &str) -> String {
3636
read_line()
3737
}
3838

39-
/// Read all lines from stdin until EOF
39+
/// Read all lines from stdin until EOF.
40+
///
41+
/// Uses `map_while(Result::ok)` rather than `filter_map(Result::ok)` so a
42+
/// broken or non-UTF-8 stdin terminates the iterator at the first error
43+
/// instead of spinning forever -- `io::Lines` keeps yielding the same
44+
/// `Err` indefinitely once the underlying reader fails.
4045
pub fn read_all_lines() -> Vec<String> {
4146
io::stdin()
4247
.lock()
4348
.lines()
44-
.filter_map(|l| l.ok())
49+
.map_while(Result::ok)
4550
.collect()
4651
}
4752

crates/my-lang/lib/common/string.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,10 @@ pub fn int_to_string_radix(n: i64, radix: u32) -> Option<String> {
206206
}
207207

208208
let mut result = String::new();
209-
let mut num = n.abs() as u64;
209+
// `unsigned_abs` instead of `n.abs() as u64`: the latter panics in debug
210+
// on `i64::MIN` because `i64::MIN.abs()` overflows. `unsigned_abs`
211+
// returns the absolute value as a `u64` losslessly for every `i64`.
212+
let mut num = n.unsigned_abs();
210213

211214
while num > 0 {
212215
let digit = (num % radix as u64) as u32;
@@ -305,4 +308,14 @@ mod tests {
305308
assert_eq!(int_to_string_radix(255, 16), Some("ff".to_string()));
306309
assert_eq!(int_to_string_radix(10, 2), Some("1010".to_string()));
307310
}
311+
312+
#[test]
313+
fn test_radix_i64_min_does_not_panic() {
314+
// Regression for `n.abs() as u64` panicking on `i64::MIN` in debug
315+
// builds. After the switch to `unsigned_abs`, this must round-trip.
316+
assert_eq!(
317+
int_to_string_radix(i64::MIN, 10),
318+
Some("-9223372036854775808".to_string())
319+
);
320+
}
308321
}

crates/my-lang/lib/common/types.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,10 @@ pub fn int_to_str_radix(n: i64, radix: u32) -> Option<String> {
100100
}
101101

102102
let mut result = String::new();
103-
let mut num = n.abs() as u64;
103+
// `unsigned_abs` instead of `n.abs() as u64`: the latter panics in debug
104+
// on `i64::MIN` because `i64::MIN.abs()` overflows. `unsigned_abs`
105+
// returns the absolute value as a `u64` losslessly for every `i64`.
106+
let mut num = n.unsigned_abs();
104107

105108
while num > 0 {
106109
let digit = (num % radix as u64) as u32;
@@ -312,6 +315,16 @@ mod tests {
312315
assert_eq!(int_to_str_radix(255, 16), Some("ff".to_string()));
313316
}
314317

318+
#[test]
319+
fn test_int_to_str_radix_i64_min_does_not_panic() {
320+
// Regression for `n.abs() as u64` panicking on `i64::MIN` in debug
321+
// builds. After the switch to `unsigned_abs`, this must round-trip.
322+
assert_eq!(
323+
int_to_str_radix(i64::MIN, 10),
324+
Some("-9223372036854775808".to_string())
325+
);
326+
}
327+
315328
#[test]
316329
fn test_bool_conversions() {
317330
assert_eq!(str_to_bool("true"), Some(true));

0 commit comments

Comments
 (0)