[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

[flip1995]

1. Rather not. We don't want overlapping lint groups. This will just make configuring Clippy harder for users. Let's add documentation of such pseudo-lint-groups to the book. We can even add snippets for the upcoming Cargo.toml lint table feature.
2. No global lints is a pretty strict rule in Clippy. As you said, there's not much benefit for this.

For this exact usecase someone already wrote an additional tool: https://github.com/Technolution/rustig However, the last commit was 5 years ago, so not sure how well it still works.

[xFrednet]

We discussed this issue in the last Clippy meeting. The conclusion was, that we don't want a lint group for lints that prevent panics, but that it would be good to add a new section to the book, that lists these lints. A user can then simply copy the names from that page, if they want to prevent panics all together.

We didn't discuss the second question, due to the reasons @flip1995 mentioned above. I think such a lint would generally be interesting and if rustig, is dead it might be worth it to update or rewrite the tool.

---

For everyone who might want to pick this issue up or if @cocodery wants to continue work on this issue:

We want to have a new section in the book, which lists lints that all address a general overarching topic. I think it would be good to avoid using the therm group to not confuse it with the normal lint groups Clippy has. Maybe lint collection or something would be a name we can use.

I would also like to have a code block, that people can copy directly, to enable all lints in a collection, so something like this:

#[warn(clippy::foo, clippy::bar, clippy::baz)]

[tigerros]

May I suggest generating the section from some sort of metadata like lint_configuration.md? I started working on it, and the basic structure is pretty repetitive (3x lint name):

# Lint collections

file description

## name of collection

```
foo, bar
```

collection description

- [`foo`](https://rust-lang.github.io/rust-clippy/stable/index.html#foo)
- [`bar`](https://rust-lang.github.io/rust-clippy/stable/index.html#bar)
...more lints...

## name of collection 2
...

Also, here's some more panicking lints:

• exit
• as_conversions
• large_futures
• large_stack_arrays
• large_stack_frames
• modulo_one
• mem_replace_with_uninit
• iterator_step_by_zero
• invalid_regex

Some of these are covered in clippy::correctness (which most people probably use), but I see no reason to exclude them. However, I would separate the lint collection by the lint's default level.

[xFrednet]

May I suggest generating the section from some sort of metadata like lint_configuration.md?

Sure, we could add a new clippy attribute to the lints, like clippy::doc_collection or something. That's how we currently add the version to the lint list. It might be overkill for now, but once it's implemented, we could use it for other collections as well :)

[flip1995]

Bikeshedding the name: I would name it "Lint Family"

[cocodery]

Clippy doesn't detect this.

Aha, that's because missing_panics_doc only check for public visible function.

[dbanty]

Hey all, just wondering what the state of this is and the intended outcome. It sounds like the decision is to just have a page in the docs which lists all the lints that prevent panics? That would be great, but wouldn't quite cover my needs. Ideally, I'd like none of my code to panic... except tests. Manually disabling every panic rule needed by tests is a pain 😅. It'd be great if there were a group (or equivalent functionality) to say #[allow(panics)] or whatever for test modules.

Maybe this was already discussed in the Zulip meeting?

[xFrednet]

Hey @dbanty, the decision we made, was to add a section to the book, like you said. The section would explain that these lints do and most likely provide a #[warn(...)] snippet that can be copied to enable all lints at easily. This is not the same as a group, but should make your use case easier.

The next step for this issue, would to add a new attribute to the lint declarations in Clippy and collect them in our metadata collection thingy

[nyurik]

I created a similar issue #12754, but I think the book-only approach does not fully solve the problem. It moves the burden to all Rust developers, rather than solving it centrally - i.e. SEP field approach -- each project developer will have to copy/paste a large list of constantly evolving lints, and maintain each project's lint list as new relevant lints are introduced. I think this is an antipattern.

Instead, I think Clippy should start thinking of lint groups in terms of "tags" rather than "categories" - allowing any lint to have more than one tag (overlapping groups). Especially now that we can have group priority parameter. Thus, we can start tagging lints as a new panicking_code group. This will also allow "group configurations", e.g. allow-unwrap-in-tests config param could be renamed to allow-panics-in-tests -- and it will apply to all lints with the panicking_code tag.

[workspace.lints.clippy]
pedantic = { level = "warn", priority = -2 }
panicking_code = { level = "error", priority = -1 }
todo = { level = "allow", priority = 0 }