False positive with unit_arg when returning Ok(function)

[peterjoel]

Lint name: unit_arg

I tried this code:

fn do_stuff() {
    println!("stuff was done");
}

fn foo() -> Result<(), String> {
    Ok(do_stuff())
}

I expected to see this happen: No error

Instead, this happened: It triggered the lint

In this case, I think the lint is overly strict and the code above is arguably better than the accepted fix:

fn foo() -> Result<(), String> {
    do_stuff();
    Ok(())
}

Meta

• cargo clippy -V: clippy 0.0.212 (04488af 2020-08-24)
• rustc -Vv:

rustc 1.46.0 (04488afe3 2020-08-24)
binary: rustc
commit-hash: 04488afe34512aa4c33566eb16d8c912a3ae04f9
commit-date: 2020-08-24
host: x86_64-unknown-linux-gnu
release: 1.46.0
LLVM version: 10.0

[peterjoel]

If do_stuff also returns a Result<()> then this does not trigger the lint:

fn foo() -> Result<(), String> {
    Ok(do_stuff()?)
}

[bengsparks]

<(), String> {
Ok(do_stuff()?)

If it's any consolation, this triggers the needless question mark lint, #6507

[peterjoel]

@bengsparks To be fair, that lint is valid in this case. I was struggling come up with simple examples, rather than pasting blocks of my real code here!

[camsteffen]

I'm not so sure this is a false positive. Ok(do_stuff()) looks like you are returning something from do_stuff but you really aren't. Less code isn't always better.

[de-vri-es]

Maybe a more compelling example:

thing.with_foo(|foo| Ok(foo.find(key)?.set_value(11)))

This pattern also triggers the lint. If we were to follow the suggestion, it would have to be:

thing.with_foo(|foo| {
    foo.find(key)?.set_value(11);
    Ok(())
})

Not the end of the world, but I would argue that the first fits just fine in idiomatic Rust.

[chrisduerr]

I do fundamentally disagree with this lint, considering it negatively impacts the ability to write concise code in iterators and match arms.

But I'd also like to point out that the description itself is somewhat confusing for this scenario.

You have an enum variant here and a function and the clippy warning clearly states that it has an issue with a function. But I'm not sure most people would immediately connect the "function" reference to the Ok enum variant constructor. The suggestion somewhat helps with this but I still think the documentation on the issue isn't conductive to understanding quickly what is wrong.

[ia0]

This is also problematic in match branches:

fn return_result(input: Stuff) -> Result<(), Error> {
    match input {
        Stuff::Weird => Err(build_error()),
        Stuff::Good { cake } => Ok(eat(cake)),
    }
}
fn eat(cake: Cake) { ... }

Having to write the following feels wrong (4 lines instead of 1, hindering readability by scattering code):

        Stuff::Good { cake } => {
            eat(cake);
            Ok(())
        }

[Shnatsel]

I've just ran into it again, with this code:

match orientation {
    Orientation::NoTransforms => Ok(()),
    Orientation::Rotate90 => Ok(*image = image.rotate90()),
    Orientation::Rotate180 => Ok(image.rotate180_in_place()),
    Orientation::Rotate270 => Ok(*image = image.rotate270()),
    Orientation::FlipHorizontal => Ok(image.fliph_in_place()),
    Orientation::FlipVertical => Ok(image.flipv_in_place()),
}

which Clippy wants me to turn into

match orientation {
    Orientation::NoTransforms => Ok(()),
    Orientation::Rotate90 => {
        *image = image.rotate90();
        Ok(())
    },
    Orientation::Rotate180 => {
        image.rotate180_in_place();
        Ok(())
    },
    Orientation::Rotate270 => {
        *image = image.rotate270();
        Ok(())
    },
    Orientation::FlipHorizontal => {
        image.fliph_in_place();
        Ok(())
    },
    Orientation::FlipVertical => {
        image.flipv_in_place();
        Ok(())
    },
}

Which is quite obviously unhelpful. The function signature already specifies it returns a unit type; all this accomplishes is create a lot of visual noise.

[de-vri-es]

I don't think the Ok(...) pattern is bad, but you could also write your example as:

match orientation {
    Orientation::NoTransforms => (),
    Orientation::Rotate90 => *image = image.rotate90(),
    Orientation::Rotate180 => image.rotate180_in_place(),
    Orientation::Rotate270 => *image = image.rotate270(),
    Orientation::FlipHorizontal => image.fliph_in_place(),
    Orientation::FlipVertical => image.flipv_in_place(),
}
Ok(())

[ia0]

I personally allow this lint at crate level in all my crates. I didn't see yet a single case where it's useful.
Regarding the example above, I very often write:

Ok(match expr {
    Pat => good()?,
    Pat => return Err(bad()),
})

So assuming the match value to be the ok side and using control flow for the err side.

[Shnatsel]

I looked up the original motivation of the lint. The documentation on the lint states:

What it does
Checks for passing a unit value as an argument to a function without using a unit literal (()).

I am not passing a unit value as an argument to a function. I am returning it from a function.

Why is this bad?
This is likely the result of an accidental semicolon.

The function is explicitly annotated as returning a unit type. There is no way it can be a result of an accidental semicolon.

[de-vri-es]

I am not passing a unit value as an argument to a function. I am returning it from a function.

You're passing it to Ok() before returning it though.

Why is this bad?
This is likely the result of an accidental semicolon.

The function is explicitly annotated as returning a unit type. There is no way it can be a result of an accidental semicolon.

I do agree, an accidental missed semicolon would normally result in a compilation error. I don't see how Rust would let you accidentally pass a unit value to a function that should have gotten something else.

I guess maybe there could be some generic code where you might end up doing the wrong thing by accident. But I can't really think of a convincing example myself.