rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.28k stars 1.52k forks source link

indexing does not trigger missing_panics_doc #13229

Open antonilol opened 1 month ago

antonilol commented 1 month ago

Summary

missing_panics_doc does not trigger on indexing, which could panic. .get(...).unwrap() is equivalent and does get caught.

Lint Name

missing_panics_doc

Reproducer

I tried this code:

#![warn(clippy::missing_panics_doc)]

pub fn a(sl: &[u8]) {
    let _ = sl[2];
}

pub fn b(sl: &[u8]) {
    let _ = sl.get(2).unwrap();
}

I expected to see this happen: warnings on both a and b

Instead, this happened: warning only on b

Version

rustc 1.80.0 (051478957 2024-07-21)
binary: rustc
commit-hash: 051478957371ee0084a7c0913941d2a8c4757bb9
commit-date: 2024-07-21
host: x86_64-unknown-linux-gnu
release: 1.80.0
LLVM version: 18.1.7
kyoto7250 commented 1 month ago

I have an interest for this issue.

https://github.com/rust-lang/rust-clippy/blob/5ead90f13ae3e03f0c346aea3198f18298960d83/clippy_lints/src/doc/mod.rs#L866-L874

Currently, we are looking at Option and Result types, so this behavior appears to be the case.

Is it OK to add a new pattern? I'm concerned that this rule will grow big, as I feel there are multiple other operations that can panic.

Jarcho commented 1 month ago

This is intentional. Linting on index expressions will result in a lot of false positives. Most of the time indexing is used in a way that either can't panic, or the function's implementation has a bug if it does. Since neither of those cases should lint, and we have no way to discern them from cases which should, we err on the side of false negatives for these.

If anyone can come up with a decent heuristic for this that would be considered.

antonilol commented 1 month ago

I thought this lint would search for places that could panic and missed this one, but it is just a list of methods and macros. unwrap_err() will not trigger it and rewriting res.unwrap() to Result::unwrap(res) will also cause it to not trigger anymore.

Should this recursively search for functions that can panic and have some filter to exclude common operations that can panic but are 'allowed' (like slicing)?

Jarcho commented 1 month ago

Intra-function lints are generally not a thing we do in clippy as it's easy to make them add a fair bit to the runtime. In this case scanning the entire call graph for panics will result in an incredible false-positive rate. It's easy to call a function which can panic, but will never under the circumstances it's called in.