[Book]: Add new section for collections of lints, that focus on the same problem

[tigerros]

Note: The start of the issue is a discussion, how to best address the suggestion. This comment outlines what should be done. The rest can be used for context.

---

What it does

restriction-level lint that checks for code that might panic. For example, right now there's unwrap_used, or expect_used. This would unify those types of lints. Ideally, it would also be able to detect other sources of panics, like calling a function that might panic. I noticed that missing_panics_doc is not triggered when a user-defined function that panics is called, which is a problem.

Advantage

Panicking is generally discouraged, and handling errors with Results or Options is recommended.

Drawbacks

It can be overly restrictive, and likely wouldn't provide a fix in certain cases.

Example

let numbers = &[4, 7, 3];
let my_number = numbers[5];

Could be written as:

let numbers = &[4, 7, 3];
let Some(my_number) = numbers.get(5) else {
    // handle
};

[cocodery]

@rustbot claim

[xFrednet]

Hey, thank you for the issue and @cocodery for your motivation to work on this new lint.

@tigerros could you explain what the advantage of unifying these lints would be? It would make the initial #[warn(xyz)] easier, but would be less configurable than the current setup. Clippy's lints are pretty fine-grained to give everyone flexibility.

The indexing code you suggested in this issue can be detected by clippy::indexing_slicing. Just thought it was mention worthy, since it wasn't mentioned in the lint description.

Ideally, it would also be able to detect other sources of panics, like calling a function that might panic. I noticed that missing_panics_doc is not triggered when a user-defined function that panics is called, which is a problem.

Rustc only loads and compiles one crate at a time. This means that Clippy(which uses Rustc as a driver) can't inspect function bodies from other crates during linting. So, while something like this, might be cool, it's sadly not possible right now. If there are cases you ran into, I would recommend creating a pull request for the crate in question and hope that they accept it. Even if that is sadly not as strict as having a lint against it.

[tigerros]

@xFrednet The advantage is that people who don't want their code to panic wouldn't need to look for various lints to detect various panicking scenarios. I know about indexing_slicing, but not everyone does, and that's just one of a lot of other panics-detecting lints. Flexibility is something no one would need to give up, as I'm not suggesting removing any lints. This would just make life easier for those who wish to maximize the reliability of their code. Lints that are a superset of other lints do exist (e.g., unwrap_used covers unwrap_in_result, option_env_unwrap, and panicking_unwrap), so I see no reason to have such a lint for the common scenario that is preventing panics.

Rustc only loads and compiles one crate at a time. This means that Clippy(which uses Rustc as a driver) can't inspect function bodies from other crates during linting.

Wouldn't it still be possible to detect if a panicking function defined in the same crate was called? For example:

fn bar() {
    panic!();
}

#[warn(clippy::missing_panics_doc)]
fn foo() {
    bar();
}

Clippy doesn't detect this.

[xFrednet]

The advantage is that people who don't want their code to panic wouldn't need to look for various lints to detect various panicking scenarios.

Ahh, sorry for the confusion. From the linked PR and tag, I assumed that you suggested adding a new lint. AFAIK, Clippy doesn't have a set policy for adding a new lint group. We usually discuss such suggestions during our bi-weekly meetings. The next one will be at 2023-12-12 on Zulip at 17CET (+1 hour). If you like, you can nominate this issue for discussion by commenting @rustbot label +I-nominated.

If the decision is not to add a group, we can add a section to Clippy's Book, that lists all lints, which detect panics.

Wouldn't it still be possible to detect if a panicking function defined in the same crate was called?

Yes, this would be possible. Generally, we try to avoid lints that analyze on a scope that goes across multiple function bodies, due to performance and the connected complexity. I think such a lint would also need to be discussed in a meeting. Maybe it would be worth to split this off into a new issue, and add the I-nominated label there as well.

Side note (which is me speaking as the author of Marker and not as a maintainer of Clippy): For such a lint, I would probably suggest implementing it manually using a linting interface like Marker or dylint. I've seen that you're currently trying to get Marker to run. If that works, this might be an option. I know there are one or two bugs, which could block this lint rn, but once they are fixed, we could approach this there.

[tigerros]

Ahh, sorry for the confusion. From the linked PR and tag, I assumed that you suggested adding a new lint. AFAIK, Clippy doesn't have a set policy for adding a new lint group.

It could be a lint group, but that wasn't what I had in mind... I was thinking more like the various "unwrap" lints, but instead it would be for the "panic" lints. I guess that's a group, but then unwrap_used would also be a sort of "group". I don't know the policies on this.

The point is that I'm suggesting a new lint/lint group that detects panicking code.

[xFrednet]

A group would mean that it combines multiple already existing lints of Marker. I guess clippy::unwrap_used overlapps with clippy::unwrap_in_result, it still has some distinct behavior, which is why I wouldn't call it a group. Is there a specific case, you're thinking of, which is currently not caught by an existing Clippy lint (Besides the example from your comment)?

[tigerros]

I can't think of anything, but I don't have much experience or knowledge about Rust. I don't quite care if it's a group or not though.

[xFrednet]

Okay, then I'm nominating this for the next Clippy meeting. I see two questions which should be discussed:

1. Do we want to add a new lint group, which contains all lints from the restriction group, that prevent panics or are closely connected to panics. Right now these lints would probably be:

   • unwrap_used
   • expect_used
   • missing_panics_doc
   • arithmetic_side_effects
   • unreachable
   • unchecked_duration_subtraction
   • todo
   • string_slice
   • panic_in_result_fn
   • indexing_slicing
   • panic

   @tigerros Could you double-check these lints and ensure that I didn't miss anything?

2. Do we want to add a lint or expand the missing_panics_doc to detect panics called functions?

   fn bar() {
       panic!();
   }
   
   #[warn(clippy::missing_panics_doc)]
   fn foo() {
       bar();
   }

   Note: I fear that this will cost a lot of performance, for little benefit. In the example above, it would be better to deny the use of panic! completely. Such a lint could also only check local functions and not functions from other crates.

@rustbot label +I-nominated