rust-clippy icon indicating copy to clipboard operation
rust-clippy copied to clipboard

Lint to warn when you are calling methods that can panic or send errors but don't return Result<T, U> or Option<T>

Open 1Dragoon opened this issue 3 years ago • 4 comments

What it does

Some functions in Rust's standard library will panic if they're used incorrectly, and there isn't any warning against this.

Take for example:

let foo = Vec![1, 2, 3, 4, 5];
foo.swap_remove(8);

Since the index 8 is out of bounds, the code will panic and crash the program, instead of doing the more rusty thing of returning an Option<T> so that we can handle it gracefully.

Similar thing occurs if you try to slice into a string:

let a = "ab早";
let a = &a[..3];

Another method with this problem is Vec::remove();

Since we probably can't change the API in a new edition of rust (or could we? older edition crates could have a .unwrap() inserted after to emulate the old behavior, so that no old crates break) I think a Clippy lint is the next best thing. I like how Rust basically garantees that your program won't crash unless you specifically say that it's ok to do so with .assert() or .unwrap(). Just need something like clippy to let us know when we run into those edge cases so we can avoid them (like unchecked .swap_remove() causing a panic,) so our beautiful applications can crash, making us look bad after we had just assured them that Rust solves all problems, including world hunger!

Lint Name

Warn Operation Panic Unchecked

Category

correctness, pedantic

Advantage

No unexpected application panic on older std methods that pre-dated Optionals.

Drawbacks

The lint could annoy you if you're already know about these pitfalls.

Example

Take for example:

let foo = Vec![1, 2, 3, 4, 5];
let bar = foo.swap_remove(8);
let quuz;
let foo = Vec![1, 2, 3, 4, 5];
let bar = if 8 < foo.len { Some(foo.swap_remove(8)); } else { None } // Note that this does an effective double bounds check, nothing can be done about that unless the rust devs change the way STD works.

1Dragoon avatar Dec 28 '21 05:12 1Dragoon

+1 on the concept, I think having lints on function call which "might panic" would be nice, even if it's only for the standard lib

Currently the closest I could find which helps with one of the cases is:

  • For indexing which might out-of-bound: clippy::indexing_slicing
error: indexing may panic
   --> src/main.rs:100:57
    |
100 |             rtmp::header::BasicHeader::ID0 => u32::from(buffer[1]) + 64,
    |                                                         ^^^^^^^^^
    |
    = help: consider using `.get(n)` or `.get_mut(n)` instead
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing
  • For string slicing specifically: clippy::string_slice (with OP's example)
error: indexing into a string may panic if the index is within a UTF-8 character
   --> src/main.rs:129:15
    |
129 |     let _a = &a[..3];
    |               ^^^^^^
    |

Note: clippy::string_slice is added on 1.58, so it's relatively new

taufik-rama avatar Jan 02 '22 02:01 taufik-rama

I'm kind of getting more experienced in Rust and might try my hand at fixing this on my own (I wouldn't really count on it as I'm still very amateurish, so if anybody else has any inclination towards this please go ahead.)

Before that though, is there any way to enumerate all of the possible ways that bare rust (i.e. no_std) and std will panic? Basically every method call, every semantic, etc. It would help if we could define all of the known cases and go from there. I'm personally only aware of a few.

1Dragoon avatar Mar 18 '22 17:03 1Dragoon

The panic_in_result_fn lint might be a good starting point for this implementation. Maybe it should be discussed what counts as a panic in this case. The mentioned lint also takes unimplemented!, todo! and unreachable! into account, which is reasonable for that lint but might be annoying and counterproductive for your use cases.

I'm kind of getting more experienced in Rust and might try my hand at fixing this on my own (I wouldn't really count on it as I'm still very amateurish, so if anybody else has any inclination towards this please go ahead.)

You're welcome to try your hands on it. Clippy's lint are mostly just a combination of if statements. The implementations can usually be done without understanding too much of the surrounding system. (That was actually one reason why I started contributing to Clippy^^)

Anyways, coming to your second question. There is no good way, AFAIK. We could iterate over every function or just their documentation to catch all panicking functions, but that would be an expensive operation for little benefit. That would also not account for functions which have index operations, that can panic, but are ensured not to by some previous bound checking. I'm guessing that the best and easiest way is to manually maintain a list of functions that can panic. If you have any other ideas, feel free to suggest them :)

xFrednet avatar Mar 20 '22 15:03 xFrednet

previous bound checking

It's for this (and many other reasons) why I believe this lint should be in restriction, not pedantic

Rudxain avatar Apr 23 '24 02:04 Rudxain