Disagreement with missing_panics_doc changes regarding expect

[ecton]

Summary

In #10953, missing_panics_doc was updated to include reporting on Option::expect and Result::expect. This has caused a large number of warnings across my projects, and I wanted to start a discussion over the changes.

To me, these two approaches to asserting that an option shouldn't be None are identical from a programmer's intent:

pub fn example(opt: Option<()>) {
    match opt {
        Some(_) => {}
        None => unreachable!("invalid"),
    }

    opt.expect("invalid")
}

Despite the first match statement possibly panicking, no warning is given. Yet, with the new behavior, opt.expect() now warns about possible panics. To me, my intention as a programmer between these two approaches is identical.

The net result of this change is that I'm going to be forced into an ugly pattern in my code to avoid this false positive. Could we split the warnings for panicking on expect into its own lint so that we can keep the previous behavior?

Lint Name

missing_panics_doc

Reproducer

I tried this code:

pub fn example(opt: Option<()>) {
    opt.expect("invalid")
}

I saw this happen:

warning: docs for function which may panic missing `# Panics` section
   --> src/shapes.rs:442:5
    |
442 |     pub fn test(i: Option<()>) {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: first possible panic found here
   --> src/shapes.rs:448:9
    |
448 |         i.expect("invalid");
    |         ^^^^^^^^^^^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc

I expected to see this happen: No warnings

Version

rustc 1.72.0 (5680fa18f 2023-08-23)
binary: rustc
commit-hash: 5680fa18feaa87f3ff04063800aec256c3d4b4be
commit-date: 2023-08-23
host: x86_64-unknown-linux-gnu
release: 1.72.0
LLVM version: 16.0.5

Additional Labels

No response

[PKilcommons]

I have similar reservations about this change so I figure I'd chime in.

Recently saw it show up in CI and was a bit surprised. While my line of thought on this matter may be potentially unidiomatic, I consider usages of .except() to be similar to // SAFETY comments for unsafe blocks in the body of a safe function—dev-to-dev explanation of an invariant being upheld in a way the compile cannot infer. A panic is then a result of that invariant being broken inside the function body, not as a result of the input to the function. As such this is then a bug that needs to be fixed within the body of the function, not at the callsite.

In this case, the # Panics section ends up just becoming pointless noise in the doc comment simply repeating the message from the .except(), something the caller has zero control over.

To illustrate with a contrived function:

/// Return a `String` pulled out of the byte stream.
pub fn string_from_byte_stream() -> String {
    let bytes = get_valid_utf8();
    String::from_utf8(bytes).except("`get_valid_utf8()` always returns valid UTF-8")
}

fn get_valid_utf8() -> Vec<u8> {
    /* Gets some bytes that the dev knows is always valid UTF-8 */
}

To appease the lint now:

/// Return a `String` pulled out of the byte stream.
/// # Panics
/// Panics if the byte stream is not valid UTF-8.
pub fn string_from_byte_stream() -> String {
    /* ... */ 
}

This doesn't exactly help the caller prevent or avoid a panic in anyway.

Perhaps adding a clippy.toml config variable that allows the .except() case would also be a desirable alternative to a new lint?

[oxalica]

Running into this recently. I use .expect() and/or .unwrap() interchangebly for already-checked-conditions or invariants that another library/code guarantee. So if it really panics, it's my fault or upstream library's fault. It's meaningless for downstream users to know in documentations that it may panic, because the end users cannot control or fix it at all.

cc the author of that change @KisaragiEffective

[Alexendoo]

A list of methods/macro names to ignore seems like a good config option to add. There have been several different incompatible conventions mentioned for what unwrap/expect mean so that seems like the best way forward

[ModProg]

@Alexendoo

A list of methods/macro names to ignore seems like a good config option to add.

What should the config be? How should it handle paths, e.g. std::*, core::*,

Initial idea, though one could also consider an allowlist instead:

panics_requiring_doc = ["Option::unwrap", "Option::expect", "Result::unwrap", "Result::expect", "panic!", "assert!", "assert_eq!", "assert_ne!"]

One issue is that the detection is implemented quite differently:

rust-clippy/clippy_utils/src/macros.rs

Lines 194 to 207 in 789bc73

	/// Is `def_id` of `std::panic`, `core::panic` or any inner implementation macros
	pub fn is_panic(cx: &LateContext<'_>, def_id: DefId) -> bool {
	let Some(name) = cx.tcx.get_diagnostic_name(def_id) else {
	return false;
	};
	matches!(
	name,
	sym::core_panic_macro
	| sym::std_panic_macro
	| sym::core_panic_2015_macro
	| sym::std_panic_2015_macro
	| sym::core_panic_2021_macro
	)
	}

rust-clippy/clippy_lints/src/doc.rs

Lines 843 to 846 in 789bc73

	|| matches!(
	self.cx.tcx.item_name(macro_call.def_id).as_str(),
	"assert" | "assert_eq" | "assert_ne"
	)

and

rust-clippy/clippy_lints/src/doc.rs

Lines 852 to 856 in 789bc73

	// check for `unwrap` and `expect` for both `Option` and `Result`
	if let Some(arglists) = method_chain_args(expr, &["unwrap"]).or(method_chain_args(expr, &["expect"])) {
	let receiver_ty = self.typeck_results.expr_ty(arglists[0].0).peel_refs();
	if is_type_diagnostic_item(self.cx, receiver_ty, sym::Option)
	|| is_type_diagnostic_item(self.cx, receiver_ty, sym::Result)

[Alexendoo]

Personally I would go with a list of paths to ignore as that's more common for clippy configuration, you could take inspiration from disallowed_macros/disallowed_methods

[ModProg]

Personally I would go with a list of paths to ignore as that's more common for clippy configuration, you could take inspiration from disallowed_macros/disallowed_methods

my idea was that one could add their own functions/macros to the lint, but personally I don't have a usecase for that, so just having an ignore list is fine by me

[duelafn]

My deepest wish is that I could flag individual lines/blocks as not requiring panic docs. I haven't looked at the lint code and fully expect that what I want might be difficult to impossible, but in the world of dreams, I'd really like to be able to (reusing some above examples),

/// Do something
pub fn string_from_byte_stream() -> String {
    let bytes = get_valid_utf8();
    #[expect(clippy::missing_panics_doc_ok, reason="caller can't do anything about this")]
    String::from_utf8(bytes).expect("`get_valid_utf8()` always returns valid UTF-8")
}

Time passes, function modified,

/// Do something
pub fn string_from_byte_stream(input: i64) -> String {
    let input = u64::try_from(input).expect("non-negative");// Oops, added something that needs "# Panics"

    // ... stuff

    let bytes = get_valid_utf8();
    #[expect(clippy::missing_panics_doc_ok, reason="caller can't do anything about this")]
    String::from_utf8(bytes).expect("`get_valid_utf8()` always returns valid UTF-8")
}

In my dreams the first fn wouldn't lint, but the second one would since it has an untagged panic.

[KisaragiEffective]

you can use the _unchecked variant to eliminate both warning and unnecessary control flow jump, aren't you?

[KisaragiEffective]

Aside that, can we rename this as unexplained_panic and restore old behavior to exclude the expect method?

[ModProg]

I started using a crate (https://docs.rs/intentional/latest/intentional/trait.Assert.html) that provides some helper methods that do the same as expect & unwrap so I can differentiate between cases I want to document and cases I don't.

[duelafn]

Another example not involving .expect() or .unwrap(),

pub fn do_something(input: &[i64]) {
    if handle_small_vec(input) { return; }

    #[expect(clippy::missing_panics_doc_ok, reason="small vecs handled above")]
    assert!(input.len() > 3, "small vecs handled above, this assert satisfies clippy::missing_asserts_for_indexing");
    do_something_else((input[0] - input[1]) * (input[2] - input[3]));
}

[Alexendoo]

Opened #14407 for allow/expect within functions

For assert/panic etc it would have to be on a parent expression as attributes directly on macro calls are currently ignored, which may mean an extra block has to be introduced

pub fn do_something(input: &[i64]) {
    if handle_small_vec(input) {
        return;
    }

    #[expect(clippy::missing_panics_doc_ok)]
    {
        assert!(
            input.len() > 3,
            "small vecs handled above, this assert satisfies clippy::missing_asserts_for_indexing"
        );
    }
    do_something_else((input[0] - input[1]) * (input[2] - input[3]));
}