Lint to require `// PANIC: ...` comments

[ojeda]

What it does

Similar to the // SAFETY: ... comments, we would like to require // PANIC: ... comments when certain functions/methods/... are called (e.g. unwrap() and expect() on Result and Option).

That is, we would like code to look like:

// PANIC: ...
f().unwrap();

The list of methods would be provided in the configuration, like it is done in other lints.

In other words, like undocumented_unsafe_blocks, but for certain panicking functions/methods.

In addition, there would be a dual lint to detect unnecessary comments: #15896.

Cc: @blyxyas

Advantage

The advantages are very similar to the ones we have seen from applying the // SAFETY: ... convention:

• Explicit panics get documented better, i.e. the reason they are needed must be explained.

• It gives some pause to developers when introducing an explicit panic which is generally not what they should do.

These advantages also mean review time (and thus maintainers' workload) gets reduced, since developers will more often realize a panic should not have been used before submitting the code (or, if the panic was truly needed, then the reason will help others understand why without having to ask about it).

Drawbacks

No response

Example

f().unwrap();

Should be written as:

// PANIC: ...
f().unwrap();

Comparison with existing lints

No response

Additional Context

• We want to avoid most unwrap()s and expect()s and similar panicking functions in the Linux kernel.

  For a similar use case, the C side essentially requires developers to pay attention to some warnings that a script (scripts/checkpatch.pl) generates on patches, i.e. new code. We will do that for Rust too (checkpatch.pl: introduce warning for unwrap() calls Rust-for-Linux/linux#1191), but we would like to get something even better/stricter/... for Rust.

• There is of course #[allow(..., reason = "...")] nowadays as well, but ideally we would want it as a comment since we already have other similar "tags", e.g. SAFETY, INVARIANT, CAST..., which would make it more consistent, and it would not require the "burden" of mentioning the lint name, deciding whether to allow or expect, writing the usual attribute syntax, and so on.

  Or, seen from another perspective: these // PANIC: comments are not so much about allowing a false positive of a lint, but rather about writing a required comment. That also allows to keep allows for this lint reserved for the case where the lint somehow fires but it wasn't supposed to fire.

• An idea is to require the comment on every function that has a # Panics section. However, that would have some consequences which are likely best left as an extension or a different lint:

  • It makes writing a # Panics section have a very big effect, just like marking a function unsafe, and perhaps sometimes a project wants to have the docs and yet not force everyone to write a comment on every call. So it would likely need a way to opt-out, e.g. an attribute. On the other hand, it could make people think twice about writing a panicking function.

  • It may be too many, e.g. even things like indexing could be already considered as such, since the Index trait docs has the section in the method and so on. So it would likely need a way to opt-out of things, e.g. certain methods or certain crates.

  • It would mean that what requires a // PANIC: ... comment could potentially change with the compiler version if core or other dependencies update their docs (whether or not the behavior actually changed). So what gets linted would change over time, which isn't great (unless there is a way to control it for dependencies etc.). On the other hand, it may be actually a good idea for some projects to keep track of anything new that may actually panic and so on.

• It could also be nice to be able to mark some of the items with an attribute. That way, external items (e.g. from core) can be marked in the configuration, while user items (e.g. from their own crates) can be directly marked in the code instead of having to maintain an independent list somewhere. However, this is a nice-to-have that would apply to several lints, i.e. it is likely best discussed separately.

[blyxyas]

This is currently being worked on #15861 by @hcbarker . I'll extend that issue with info from this oke. But for sake of centralizing everything, I'll close this issue.

[ojeda]

Ah, great, I didn't notice. Of course, please feel free to copy-paste and/or link this one and close it. (It has some more details/context, I took most of it from my original email; I also opened the dual lint one).

And thanks @hcbarker for working on this! :)