Needless return with Err and question mark

[nyurik]

What it does

I sometimes see return Err(MyError::SomeError(v))?; -- which seems redundant - it has both the return AND the question mark operator. So this can be rewritten as either

• Err(MyError::SomeError(v))?; - without the return
• return Err(MyError::SomeError(v)); - without the question operator

The second option only works if the error type is the same as in Result<..., MyError>, otherwise it only works with the question mark.

So question - does it make sense to suggest needless return in the case where it can be expressed with a question mark? Or should the question mark be suggested to be removed in the case where it can? Or neither?

Advantage

• shorter code - no confusion over two "return" operators

Drawbacks

• without return might be less readable, so perhaps only for same-result-errors and remove the question mark?

Example

use thiserror::Error;

#[derive(Error, Debug)]
enum MyTopError {
    #[error("Internal Error: {0}")]
    SubError(#[from] MySubError),
    #[error("Main Error")]
    MainError,
}

#[derive(Error, Debug)]
enum MySubError {
    #[error("Value {0} too small")]
    TooSmall(i32),
}

fn foo(v: i32) -> Result<(), MyTopError> {
    if v < 5 {
        return Err(MySubError::TooSmall(v))?;
        //    This is identical to the above, but too verbose:
        // return Err(MyTopError::SubError(MySubError::TooSmall(v)));
        //    This is identical to the above, but has no "return"
        // Err(MySubError::TooSmall(v))?;
        //    This causes an error: mismatched types
        // return Err(MySubError::TooSmall(v));
    }
    if v > 100 {
        return Err(MyTopError::MainError)?;
        // Err(MyTopError::MainError)?;        // This is identical to the above, but has no "return"
        // return Err(MyTopError::MainError);  // This is identical to the above, but has no question mark
    }
    println!("v: {v}");
    Ok(())
}

fn main() {
    let v = 3;
    let r = foo(v);
    println!("r: {:?}", r);
}

[Centri3]

The second option only works if the error type is the same as in Result<..., MyError>, otherwise it only works with the question mark.

This is because ? performs an implicit into call, adding that wrapped in the Err should always compile afaik.

I think people generally use return Err(<snip>) or bail! for eyre, anyhow, etc. Clippy already lints against <snip>? as an early return, so it seems reasonable to suggest return Err(<snip>). Thoughts?
^ Huh, seems it actually doesn't...? Could've sworn it suggested that once, but guess not
try_err. It's a restriction lint, though.

@rustbot claim

[y21]

In my experience, people usually prefer return Err(x); over Err(x)?; and I agree with this. Creating an error just to immediately try to propagate it feels a bit like misuse of ? imho... The explicit return also makes it easier to see that this is an early return

[nyurik]

Thanks @Centri3 and @y21! Timo, I agree with you that return Err(x); is more concise where possible, but it gets a bit too verbose with the nested case of return Err(MyTopError::SubError(MySubError::TooSmall(v))); -- you do want to use the magical .into() produced by the ? operator, or at least the .into(). So Clippy could help standardize those cases too, or at least educate users about the availability of shorter notations with the ?, like in this conversation I recently had, where an experienced developer could have used a hint on simplifying the expression.

So a proposal could be:

• if the error is the same type as in the Result, suggest normalization to return Err(MyError);, removing question marks, or converting from the Err(MyError)?; expressions as less readable.
• if the error is not the same type as in Result, but could be converted to it with the .into(), suggest return Err(MySubError)?; format (adding the return statement and removing the .into() call).

This is really a stylistic preference to make these patterns more uniform across the Rust language, but it is opinionated, so this is probably a pedantic thing?