On: Don't unwrap options: There are better ways (2024)

[evrimoztamur]

Talking about unwrapping: I’ve been using a rather aggressive list of clippy lints to prevent myself from getting panics, which are particularly deadly in real-time applications (like video games). unwrap/expect_used already got me 90% of the way out, but looking at the number of as conversions in my codebase, I think I have around 300+ numerical conversions (which can and do fail!)

    [lints.clippy]
    all = "deny"
    unwrap_used = "deny"
    expect_used = "deny"
    panic = "deny"
    indexing_slicing = "deny"
    unhandled_errors = "deny"
    unreachable = "deny"
    undocumented_unsafe_blocks = "deny"
    unwrap_in_result = "deny"
    ok_expect = "deny"

[mplanchard → evrimoztamur]

This is nice, but fairly miserable to deal with in in-module unit tests, IMO.

We get around it by using conditional compilation and putting the lints in our entrypoints (`main.rs` or `lib.rs`), which is done automatically for any new entrypoint in the codebase via a Make target and some awk magic.

As an example, the following forbids print and dbg statements in release builds (all output should go through logging), allows it with a warning in debug builds, and allows it unconditionally in tests:

    #![cfg_attr(not(debug_assertions), deny(clippy::dbg_macro))]
    #![cfg_attr(not(debug_assertions), deny(clippy::print_stdout))]
    #![cfg_attr(not(debug_assertions), deny(clippy::print_stderr))]
    #![cfg_attr(debug_assertions, warn(clippy::dbg_macro))]
    #![cfg_attr(debug_assertions, warn(clippy::print_stdout))]
    #![cfg_attr(debug_assertions, warn(clippy::print_stderr))]
    #![cfg_attr(test, allow(clippy::dbg_macro))]
    #![cfg_attr(test, allow(clippy::print_stdout))]
    #![cfg_attr(test, allow(clippy::print_stderr))]

AFAIK there isn't currently a way to configure per-profile lints in the top-level Cargo configs. I wish there were.

[indiv0 → mplanchard]

We just set all the lints to `warn` by default then `RUSTFLAGS="--deny warnings"` when building for release (or in CI).

[mplanchard → indiv0]

Yeah that is nice too, and that should also skip all the test code. I think all the clippy warnings on tests for unwrapping and such when working locally would bug me though. And at least the eglot LSP client tends to get bogged down when there are more than a thousand or so clippy warnings/errors, I have found.

[quadhome → evrimoztamur]

All but one of these come from the restriction[1][2] lint group.

I try to remember to look at new restriction lints with every Rust release. For example, here's what new with 1.86.0[3]; the `return_and_then` lint looks pretty nice.

n.b. no one should enable all restrictions lints— some are mutually exclusive, some are appropriate for specialised circumstances like `#[no_std]`. But I find them helpful to keep a project away from the wild parts of Rust.

P.S. `unhandled_errors` doesn't exist[4].

[1]: https://rust-lang.github.io/rust-clippy/stable/?groups=restr... [2]: https://doc.rust-lang.org/stable/clippy/lints.html#restricti... [3]: https://rust-lang.github.io/rust-clippy/stable/?groups=restr... [4]: https://rust-lang.github.io/rust-clippy/stable/index.html#/u...

[an_ko → evrimoztamur]

Does that mean your code is annotated with 300+ instances of `#[allow(clippy::unwrap_used)]` et al?

[evrimoztamur → an_ko]

It was the first time I set it up, then I went through every single instance and refactored with the appropriate choice. It wasn't as tedious as you might imagine, and again, I really don't have the option of letting my game crash.

I think the only legitimate uses are for direct indexing for tile maps etc. where I do bounds checking on two axes and know that it will map correctly. to the underlying memory (but that's `clippy::indexing_slicing`, I have 0 `clippy::unwrap_used` in my codebase now).

If you begin a new project with these lints, you'll quickly train to write idiomatic Option/Result handling code by default.

[p1necone → evrimoztamur]

yoink (although I will probably allow expect - having to provide a specific message means I'm only going to use it in cases where there's some reasonable justification)