Logically identical code. One compiles, one does not #116572
Description
I tried to compile this code playground link:
use std::time::{Duration, Instant};
use std::thread;
fn time_bounded_retry<T, E>(
max_time: Duration,
sleep_for: Duration,
f: impl Fn() -> Result<T, E>,
) -> Result<T, E> {
let start = Instant::now();
while let result = f() {
match result {
Ok(_) => return result,
Err(_) => {
if start.elapsed() > max_time {
return result;
}
thread::sleep(sleep_for);
}
}
}
// unreachable!()
}However, it produces an error. If you uncomment the unreachable!(), it will work. Here's the error message:
|
191 | while let result = f() {
| ^^^^^^^^^^^^^^^^^^^^^^ this might have zero elements to iterate on
192 | match result {
193 | Ok(_) => return result,
| ------------- if the loop doesn't execute, this value would never get returned
...
196 | return result;
| ------------- if the loop doesn't execute, this value would never get returned
= help: return a value for the case when the loop has zero elements to iterate on, or consider changing the return type to account for that possibility
help: try adding an expression at the end of the block
This code is functionally identical to the while let code and can compile without an unreachable!():
fn time_bounded_retry<T, E>(
max_time: Duration,
sleep_for: Duration,
f: impl Fn() -> Result<T, E>,
) -> Result<T, E> {
let start = Instant::now();
loop {
let result = f();
match result {
Ok(_) => return result,
Err(_) => {
if start.elapsed() > max_time {
return result;
}
thread::sleep(sleep_for);
}
}
}
}The code in the while loop is irrefutable, so it is functionally the same as the loop case. Even if it's not a good practice to use an irrefutable statement in a while loop (and clippy will catch this) I think it should still compile.
My main reason for wanting it to compile is that while iterating, I wrote the while let version first, and because I could not get it to compile without the unreachable!() statement mistakenly believed that the compiler would not allow something like the second result. As a result of that mistaken impression ended up with this code:
fn time_bounded_retry<T, E>(
max_time: Duration,
sleep_for: Duration,
f: impl Fn() -> Result<T, E>,
) -> Result<T, E> {
let start = Instant::now();
let mut result;
loop {
result = f();
if result.is_ok() || start.elapsed() > max_time {
break;
}
thread::sleep(sleep_for);
}
result
}Which is not ideal (needs a temp mutable variable). A co-worker without the above learned helplessness unreachable!() experience wrote this code below, which I believe is a much better way to implement the logic as it does not require a temp mutable variable:
fn time_bounded_retry<T, E>(
max_time: Duration,
sleep_for: Duration,
f: impl Fn() -> Result<T, E>,
) -> Result<T, E> {
let start = Instant::now();
loop {
result = f();
if result.is_ok() || start.elapsed() > max_time {
return result;
}
thread::sleep(sleep_for);
}
}So effectively, I'm advocating for while let var = //... to be treated the same as loop { let var = //..., not because I think it's a good idea, but because having one error and not the other gives false feedback to the coder.
Meta
rustc --version --verbose:
$ rustc --version --verbose
rustc 1.73.0 (cc66ad468 2023-10-03)
binary: rustc
commit-hash: cc66ad468955717ab92600c770da8c1601a4ff33
commit-date: 2023-10-03
host: x86_64-apple-darwin
release: 1.73.0
LLVM version: 17.0.2
Activity