Unnecessary use of .ok() followed by method equivalently defined on both Option and Result

[junbl]

What it does

There is an existing lint, ok_expect, that warns against the following code:

res.ok().expect("msg")

saying that the .ok() can be removed and expect called directly. This can be generalized to other methods that can be called on both Option and Result.

The methods I could find that this could apply to include:

• expect
• unwrap, unwrap_or, unwrap_or_default, unwrap_unchecked
• iter, iter_mut, into_iter
• map_or

I'm sure I've missed some, please add any others that could be included!

The lazily evaluated _else methods could also be included with the minor modification of adding a ignored argument to the closure for the error, e.g. res.ok().unwrap_or_else(|| 4) becomes res.unwrap_or_else(|_| 4).

This could also be applied to calls to methods with multiple arguments if .ok() is called on both (e.g. .or(), .and(), .cmp()), though this is likely less common.

The reverse lint could also be implemented, e.g. unnecessary use of ok_or(_else) on an Option followed by a method above that will never construct the Err. The call to ok_or_else could have side effects, however, making the suggested code not equivalent.

Lint Name

unnecessary_ok

Category

complexity, perf

Advantage

• Removes unnecessary code, improving clarity
• Removes an unnecessary conversion (I don't know much about compiler internals---this may be a nonexistent performance change)

Drawbacks

Might not be a common enough pattern to be worth it. I've personally seen .ok().map_or.

The unwrap_* methods require that the Err variant implement Debug, which otherwise wouldn't be necessary.

Example

let value = <Result<_, i32>>::Ok(4);
value.ok().unwrap(); // or any of the above methods

Could be written as:

let value = <Result<_, i32>>::Ok(4);
value.unwrap();

[hkBst]

Another form of unnecessary ok that I found in the wild is as an unidiomatic way to ignore an error:

Example

dotenv().ok();

This could be written more idiomatically as:

let _ = dotenv();

Although one can argue that instead this should have been written:

dotenv().unwrap();

[ithinuel]

@hkBst I have also seen this in several places (embedded oriented here and there) used to ignore the result (because known to be unfallible, or not relevant).