Question about `if let ok` vs `is_ok()`

[kinxiel]

Have a quick question about syntax.

Currently my code goes something like this:

if server_response.is_ok() {
  // do something
}

However, cargo clippy recommends:

if let Ok(..) = server_response {
  // do something
}

What is the difference and why is cargo clippy suggesting that the below is more idiomatic?

Thanks

[danjl1100]

The difference is the first calls a method is_ok(&self) -> bool which only takes a reference to self. Reading this first one, I'd expect the value of the result to be used later.

The second moves the variable, and explicitly discards inner contents of the Ok variant.
In most cases, this would be the final usage of the original result binding, since results are most often not Copy.

My guess is that clippy noticed you weren't using the result binding after the if statement, so it wanted you to show it was being moved, with the contents of Ok being discarded.

[quinedot]

Are you sure it didn't suggest things the opposite way around? (Clippy is under Tools, top-right.)

If so, can you share the output?

Here's the reasoning (and associated problems) for the lint that fires in the playground. Personally I don't think it's worth being warn-by-default.

[steffahn]

Clippy does usually not only recommend something but also describes the problem. I. e. it describes the pattern that was lined against in an diagnostic message sich as e.g. the following

warning: called `unwrap` on `x` after checking its variant with `is_ok`
 --> src/main.rs:4:9
  |
3 |     if x.is_ok() {
  |     ------------ help: try: `if let Ok(..) = x`
4 |         x.unwrap();
  |         ^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap
  = note: `#[warn(clippy::unnecessary_unwrap)]` on by default

e. g. in this case the pattern would be to unwrap() a value in a if ….is_ok() block, and that Clippy warning even includes a link to a website with even more information on that lint.

If this information is not sufficient, of course feel free to ask here, as you did, but in that case you should probably include the information what lint you saw, and perhaps even concrete questions indicating what part of the existing explanations that come with the lint is not sufficient.

I can only guess that the kind of warning shown above is actually the one you received, so feel free to correct this assumption of you got something different instead. The example the website linked above fixes is

Example

if option.is_some() {
    do_something_with(option.unwrap())
}

Could be written:

if let Some(value) = option {
    do_something_with(value)
}

In case the benefits of the second version are not obvious: It's less verbose, and there's less possibility to make a mistake and create a panic, and it avoids the option being checked twice, once for the if and once for the unwrap, so there's even - in principle - a performance benefit, though perhaps compiler optimizations could alleviate this last aspect.

[kinxiel]

I think you are probably right. I mainly used the is_ok() as control flow, to make sure I get some value out of the result. After that I had to do multiple things with side-effects, like rendering stuff in the UI.

Thanks

[kinxiel]

Thanks for the tip. I think it is similar to what steffahn mentioned below.

[kinxiel]

Thanks.

The code goes something like this:

if server_response.is_ok() {
  let server_response_value = server_response.unwrap().clone();

// do some other stuff
}

Clippy suggests:

help: try: `if let Ok(..) = server_response`

the link goes to
https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap
which is the same as your example.

Ok kind of got it now. Thanks for the help.

[steffahn]

Note that the main thing Clippy warns about, as indicated by the location marked with the ^^^^^^^^^^-style markers, is the use of unwrap (in a context where it could have easily been avoided). Without the unwrap() call, the lint wouldn't trigger.

Focusing more on the unwrap instead of the code as a whole, another view on this lint is as follows: One of the main use case of unwrap in Rust is to assert, in a context where that's hard or impossible for the compiler to “know”, that a Option or Result value is Some/Ok.^[1] Since in this case it's not hard to figure out that the Result is Ok, due to the presence or the enclosing is_ok, to the point that a small refactor into if let makes the unwrap go away entirely, you've missed the purpose of unwrap described above.

In Rust, we quite often like compiler checks better than run-time checks or human checks.^[2] If you wrote code that isn't supposed to panic, you would ideally add a comment on every unwrap explaining why it will never fail. With that mindset, replacing a unwrap with code refactored in a way that makes the compiler, not potential human reviewer, figure out why the Result is Ok in this case would save the need to explain your reasoning and/or the need for a potential human reviewer to check for the possibility of panics.

---

1. (Other use case exist, too, like "handling" unrecoverable errors, propagating a panic, or as a simple solution to not handle edge cases whole while prototyping.) ↩︎

2. E. g. the reason people prefer to use ordinary compile-time checked references over &RefCell<T> based access is somewhat similar. ↩︎

[H2CO3]

Briefly, the point is: you are first testing .is_ok() and then you are separately unwrapping. That's bad for many reasons, including:

• it's redundant;
• it's dangerous – what if you later change the code, which causes the test and the unwrap to become out of sync? Eg, if you remove the test or invert it, the unwrap will sometimes or always panic.

If you test using pattern matching and get the value out in the same step, then such errors are impossible. This is why you should use the type system instead of relying on "I know it's right".

[kinxiel]

Thank you for the insight. Never thought of it that way, makes perfect sense.