fix(needless_return): do not trigger on ambiguous match arms return

[feniljain]

If we see a case where match returns something other than (), we just skip needless_return lint in that case

Should fix #10546

Relevant Zulip Discussion: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Issue.20.2310546

---

changelog: FP: [needless_return]: No longer lints match statements with incompatible branches
#10593

[rustbot]

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

[xFrednet]

Thank you for the PR :)

[xFrednet]

I might be mistaken, but I feel like this is not quite the right fix for the issue. The problem, as I understand it, is that the return value has a different type than the other match arms. From my understanding, this can only happen, if the match is at the end of the function and has a semicolon, making it a statement. This therefore also implies that the function returns the unit type (), as otherwise there would be something after the statement or the types would be compatible.

fn test_match_as_stmt() {
    let x = 9;
    match x {
        1 => 2,
        _ => return,
    }; // The semicolon makes it a statement, and allows the branches to be non-unit
    // This makes the branches incompatible
}

fn does_not_compile() {
    let x = 9;
    match x {
        1 => 2,
        _ => return,
    }
    // The first branch is incompatible with the function signature.
}

fn does_compile_because_types_are_compatible() -> u32 {
    let x = 9;
    match x {
        1 => 2,
        _ => return 3,
    }
    // Should be linted since the types are compatible
}

(Maybe there are some other edge cases with impl Trait, that might be worth testing)

Based on this, I think the proper fix would be to check the type of the match statement. If it's () the types are compatible. If not and the return has a value than they would still be compatible. It would only cause problems, if the match type is != () while the function is -> ().

Does this make sense?

[feniljain]

Hey! Thanks for the awesome in-depth review

I think the proper fix would be to check the type of the match statement

That's what I am doing currently in the function check_final_expr here:
https://github.com/feniljain/rust-clippy/blob/c12748fab3a14beae4958f13551e7e6c52298490/clippy_lints/src/returns.rs#L250-L260

😅

If not and the return has a value than they would still be compatible.

We would not reach this part of code ( one I linked just above ) if the return is not empty, in which case we just tell clippy to ignore it, at least that's what we discussed in zulip here

Do help me if I am still misunderstanding things 🙇🏻

[xFrednet]

Ahh okay, if that part is never reached, then it should be fine, that slipped my radar, during my first review. :)

[xFrednet]

Everything looks good to me, thank you for the PR :)

@bors r+

[bors]

📌 Commit 9cf57d0 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit 9cf57d0 with merge e22019d...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing e22019d to master...

[kadiwa4]

Sadly, this fix is not general enough to also solve #10390. @feniljain would you like to give that a go as well?