panic_in_result_fn shouldn't fire on todo!()/unimplemented!()/unreachable!()

[tgross35]

Summary

panic_in_result_fn fires on panic!, unimplemented!, todo!, and unreachable!. panic,unimplemented, and unreachable all make since since they represent "final" code. Linting on todo! doesn't really make sense in this group however, since it usually represents a work in progress - firing on todo!() code is just noisy.

Lint Name

panic_in_result_fn

Reproducer

I tried this code:

#![warn(clippy::panic_in_result_fn)]
fn foo() -> Result<(), ()> { todo!() }

I saw this happen:

[<output>](warning: used `unimplemented!()`, `unreachable!()`, `todo!()`, `panic!()` or assertion in a function that returns `Result`)
...

I expected to see this happen:

No lint

Version

rustc 1.72.0-nightly (871b59520 2023-05-31)

Additional Labels

No response

[charmitro]

IMO it is a little bit noisy, but I have to ask, is todo! any different from unimplemented!?

[tgross35]

Good point, I was only thinking about my usage of clippy during development. I guess for my use, I probably wouldn’t want clippy to flag unreachable! or unimemented! either, since both indicate that function shouldn’t be called.

todo! unreachable! and unimplemented! all have non-result use cases and have individual lints, so in my opinion it really only makes sense to fire this on panic!. But then again, it is in restriction…

[charmitro]

Good point, I was only thinking about my usage of clippy during development. I guess for my use, I probably wouldn’t want clippy to flag unreachable! or unimemented! either, since both indicate that function shouldn’t be called.

todo! unreachable! and unimplemented! all have non-result use cases and have individual lints, so in my opinion it really only makes sense to fire this on panic!. But then again, it is in restriction…

Examining the above macros, we can see that all of them will always panic. Since panic_in_result_fn job is to check whether a panic! is present in functions with Result, it seems correct to me.

[tgross35]

unreachable and unimplemented should never actually be reached though in production code, they're used to complete type checking. In other words, reaching them indicates a bug in the program (program is objectively wrong and should fail at runtime) rather than being a part of normal control flow (things you would want to propegate). For example:

#![allow(unused)]
#![warn(clippy::panic_in_result_fn )]
use std::num::ParseIntError;

#[derive(Debug)]
enum Foo {
    I(u32),
    F(f64),
    S(String)    
}

fn prechecks(foo: &Foo) {
    assert!(!matches!(foo, Foo::F(f64)), "Floats not allowed here!");
}

fn do_things(foo: &Foo) -> Result<u32, ParseIntError> {
    prechecks(foo);
    match foo {
        Foo::I(v) => Ok(*v),
        Foo::S(v) => Ok(v.parse()?)
    }
}

This won't compile since the match isn't exhaustive, so it needs an update to:

    prechecks(foo);
    match foo {
        Foo::I(v) => Ok(*v),
        Foo::S(v) => Ok(v.parse()?),
        Foo::F(_) => unreachable!("verified in prechecks")
    }

or

        Foo::F(_) => unsafe { std::hint::unreachable_unchecked() }

This is the only way to correctly complete exhaustiveness checking, and is functionally correct since that variant can't exist at this point. This lint doesn't fire for the second (as expected) but it does fire on the first - I would expect the behavior to match between the two.

They do call panic just with specific error messages, but their intended use as type/exhaustivness indicators are pretty different than an assert or panic that is used to make normal execution infallible (and the docs corraborate that usage)

(end of the day I don't mind turning this lint off, but it just seems a bit unusual that these bug indicators cause it to happen in the first place)

[Centri3]

I agree. These shouldn't be linted; unimplemented and todo represent that this function is not meant to be called, unreachable represents that this should never occur.

If somebody wants to, I believe that what it lints is panic! in particular but those macros expand to panic!. Maybe you can call root_macro_call/root_macro_call_first_node to get the name of the macro in particular? I dunno.
Oh actually, it doesn't; it checks unimplemented!, todo! etc individually. So yeah, I think this should be pretty doable.

I believe this could be a good-first-issue. Maybe I'm wrong, anybody feel free to remove it that isn't so ^^

@rustbot label +good-first-issue

[charmitro]

@panosfol wanna co-develop this as your first contribution?

[panosfol]

@panosfol wanna co-develop this as your first contribution?

@charmitro Yeah of course! Thank you!

[panosfol]

@rustbot claim