Detect needless use of let else with trailing semicolon

[CryZe]

What it does

Detects uses of let else that could be handled through the ? operator.

Advantage

• The ? operator is meant for error handling, use it when possible

Drawbacks

No response

Example

let Some(x) = y else { return None; };

Could be written as:

let x = y?;

This could also be extended to stuff like:

let Some(x) = y else { return Err(err); };

Could be written as:

let x = y.ok_or(err)?;

or

let x = y.ok_or_else(|| err)?;

[y21]

This lint already exists (question_mark), but it only seems to emit a warning for else { return None }; (note the lack of a semicolon for the return), so this seems more like a false negative issue with the question_mark lint.

warning: this `let...else` may be rewritten with the `?` operator
 --> src/main.rs:3:5
  |
3 |     let Some(x) = y else { return None };
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `let x = y?;`

[CryZe]

Oh yeah I guess rustfmt placing the semicolon there caused clippy not to trigger the lint then. Good to know that it already exists.

[torfsen]

Quoting from #11994:

This fixes the false negative mentioned in #11993: the lint only used to check for return expressions, and not a statement containing a return expression (doesn't close the issue tho since there's still a useful suggestion that we could make, which is to suggest .ok_or()?/.ok_or_else()? for else { return Err(..) })

[torfsen]

@rustbot claim

[torfsen]

While working on this I noticed that the question mark lint's behavior w.r.t. to macros is not what I would have expected. Here's an example:

#![allow(dead_code)]

fn check_option() -> Option<u8> {
    let x = Some(0);
    
    // `if let` completely inside a macro
    macro_rules! lint_inside_macro {
        ($x:ident) => {
            // This currently gets fixed as `let _ = $x?;`
            let _ = if let Some(x) = $x { x } else { return None };
        };
    }

    lint_inside_macro!(x);

    // `if let` against `Ok` across a macro boundary.
    macro_rules! some {
        ($x:ident) => {
            Some($x)
        };
    }
    // Currently gets fixed as `x?;`
    if let some!(x) = x {
        x
    } else {
        return None;
    };
    
    // `is_none` across a macro boundary
    macro_rules! is_none {
        ($x:ident) => {
            $x.is_none()
        };
    }
    // This currently gets "fixed" to `$x?;` (including the `$`)
    if is_none!(x) {
        return None;
    }
    
    None
}


fn check_result() -> Result<u8, u8> {
    let x = Ok(0);
    
    // `if let` completely inside a macro
    macro_rules! lint_inside_macro {
        ($x:ident) => {
            // This currently gets fixed as `let _ = $x?;`
            let _ = if let Ok(x) = $x { x } else { return $x };
        };
    }

    lint_inside_macro!(x);

    // `if let` against `Ok` across a macro boundary.
    macro_rules! ok {
        ($x:ident) => {
            Ok($x)
        };
    }
    // Currently gets fixed as `x?;`
    if let ok!(x) = x {
        x
    } else {
        return x;
    };
    
    // `if let` against `Err` across a macro boundary
    macro_rules! err {
        ($x:ident) => {
            Err($x)
        };
    }
    // Currently gets fixed as `x?;`
    if let err!(x) = x {
        return Err(x);
    };
    
    // `is_err` across a macro boundary
    macro_rules! is_err {
        ($x:ident) => {
            $x.is_err()
        };
    }
    // This currently gets "fixed" to `$x?;` (including the `$`)
    if is_err!(x) {
        return x;
    }
    
    Ok(0)
}

fn main() { }

(playground)

So the question mark lint triggers both if the code is completely within a macro (which I personally think is fine) but also if the code spans a macro boundary -- that latter case surprised me, I would have expected that the macro is considered to be a black box and hence that there wouldn't be a lint. The Clippy book recommends to not lint in expansions in the general case. I'm not sure if the behavior is different if the macro is external, because I couldn't find a good way to test that (is there one, by the way?).

Is the current behavior as it should be?

(There's also the issue with the last example in the code, where the fixed code is invalid. I'll fix that, too.)

[flip1995]

In the ok/err/is_err (and analogue for Option) cases, the lint doesn't really lint the macro, but the if expression as a whole. Clippy doesn't really care that the pattern that is matched again comes from a macro. And I think this is correct behavior or at least not problematic.

The only issue I see here is the wrong suggestion you pointed out.

[torfsen]

Thanks for your input, @flip1995. While I'm not sure I agree with the behavior, it's good to know that it's certainly not a definite bug. In any case, I won't change the lint's behavior in that respect within this ticket, aside from fixing the problematic suggestion.

[flip1995]

The reason why I don't think that this behavior is a bug is that the examples you posted are pretty constructed and I don't really expect similar things in real world could, i.e. macros evaluating to an Ok($ident) pattern.

[torfsen]

Unfortunately I currently do not have the time to follow this through and I'm therefore going to unassign myself.