fix: match stmts conversion to expr and their interaction with return

[feniljain]

Should fix #10390

Eariler I opened a PR to fix an issue in needless_return lint here: #10593. This fixed issue's mentioned particular scenario, but didn't fix other scenarios like #10390 . Thanks to @kadiwa4 's tag I noticed that particular issue, this time I tried to go back to square one and think of different scenarios and made changes to hopefully cover all cases. So this PR may also cover other different issues which could have been lingering around to be found.

Explaining changes:

According to my understanding this lint's function is:

• Try to convert match stmt to match expr if it is last stmt of function/block, and then remove returns from it

• Try to remove returns in a match expr

• For cases of match expr, we don't want to change our current analysis

• For cases of match stmt, we need to check if match's return type is equal to function's return type and only do further analysis if they match exactly, for cases showed in code in above issue:

fn func(input: u8) {
    match input {
        3 => 4,
        _ => {
            // side effect ...
            return;
        },
    };
}

match expr return type is u8 which is different from function's return type and hence we do not take any action :)

Do let me know if you still think there's some problem in some change 😅

---

changelog: [needless_return]: Fix various scenarios of match stmt/expr transformation and them apperaing with returns

[rustbot]

r? @Alexendoo

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

[Alexendoo]

This matches! is always true since the pattern is just a binding, you can't neatly compare a HIR type against a rustc_middle one

I think we can step around this though as the only case where I think it would matter is if the return type is explicitly (), we should be able to make this

Suggested change

	Some(FnRetTy::Return(_ret_ty)) => matches!(match_expr_ret_type.kind(), _ret_ty),
	Some(FnRetTy::Return(Ty { kind: TyKind::Tup([]), .. })) => match_expr_ret_type.is_unit(),

and a test

fn match_explicit_unit_return() -> () {
    match 0 {
        0 => 1,
        _ => {
            return;
        },
    };
}

If you can think of an example with a non unit type though please let me know

[feniljain]

Ah I was wondering why it didn't work, when I checked the type back then both showed TyKind, didn't realize they were from different module 🤦🏻

So for the suggestion, I think that won't work out cause of this comment:

check if function return type is equal to match return type 
if yes then we want can convert this match stmt to match expr 
otherwise dont do it

We want to compare parent function's return type to match's type, and if they are the same then we can do the conversion of removing semicolon from end, etc.

[Alexendoo]

For non unit types that wouldn't type check since the implicit return of () would not apply, e.g. the following doesn't compile

fn f() -> u32 {
    match 1 {
        0 => 1, // ERROR: mismatched types
        _ => return 2,
    };
}

[feniljain]

Hey, I am sorry I wasn't able to follow what you meant, could you please explain it in some other way 😅

[Alexendoo]

Yeah I'll give it another shot, I had trouble trying to explain it so I don't blame you 😄

To annotate the example from #10390:

fn func(input: u8) {
    match input {
        // this arm's expr has type `i32`
        3 => 4,
        
        // this arm's expr has a pre-adjustment type of `!` (never) because of
        // the return, but adjusts to `i32` to match the other arm
        _ => {
            return;
        }, 
    };
    // the match expression has type i32, which is discarded by the semicolon statement

    // if the `3 => 4` arm was taken, the function returns () implicitly here
}

The FP occurs because of the combination of the discarded result and the implicit return of ()

When a function isn't returning unit it can no longer use that implicit return because it would be a type mismatch, so every arm must diverge (return/panic/etc)

[feniljain]

this example is returning the match expression

Ah ig this was the point I was missing, here it is match expr 🤦🏻, yeah then I guess your stmt of just checking unit tuple makes sense, lemme make a fix and push it!

[feniljain]

Okay, so interestingly, we have another lint called useless-unit:

+++ <stderr output>
+error: unneeded unit return type
+  --> $DIR/needless_return.rs:336:52
+   |
+LL | fn issue10390_different_arm_return_types(input: u8) -> () {
+   |                                                    ^^^^^^ help: remove the `-> ()`
+   |
+   = note: `-D clippy::unused-unit` implied by `-D warnings`

Applying this would make it same as default case, do we still keep this part of code around then? I am not sure what is the policy of project in cases where another lint applies and hence changes output of it 😅

[Alexendoo]

Ideally we want lints to be correct when applied individually, since it doesn't add much code I'd say keep it

For the test file you can add an #![allow(clippy::unused_unit)]

[feniljain]

Oh, I think I found a case where we may need to check types, what about this:

fn issue8156(x: u8) -> u64 {
    match x {
        80 => {
            return 10;
        },
        _ => {
            return 100;
        },
    };
}

In this case, it is a match statement.

[Alexendoo]

What would it be guarding against? That example compiles fine if you remove the return/semicolons

[bors]

☔ The latest upstream changes (presumably #11627) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Hey @feniljain, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

[feniljain]

Hey @xFrednet! Thanks for the reminder, I think I have lost all context at this point, and it would be better if someone else picks this up. Thanks for everyone's time! 🙇🏻

[xFrednet]

No problem at all.

Interested parties are welcome to pick this implementation up as well :)

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review