Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cf7608
fix: match stmts conversion to expr and their interaction with return
feniljain Aug 15, 2023
8843cd2
fix: formatting and make dogfood tests pass
feniljain Aug 15, 2023
00c1b1b
refactor: send None simply instead of sending full match when known
feniljain Aug 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 94 additions & 33 deletions clippy_lints/src/returns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::intravisit::FnKind;
use rustc_hir::{
Block, Body, Expr, ExprKind, FnDecl, ItemKind, LangItem, MatchSource, OwnerNode, PatKind, QPath, Stmt, StmtKind,
Block, Body, Expr, ExprKind, FnDecl, FnRetTy, ItemKind, LangItem, MatchSource, OwnerNode, PatKind, QPath, Stmt,
StmtKind,
};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::{self, GenericArgKind, Ty};
use rustc_middle::ty::{self, GenericArgKind};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::def_id::LocalDefId;
use rustc_span::source_map::Span;
Expand Down Expand Up @@ -236,7 +237,7 @@ impl<'tcx> LateLintPass<'tcx> for Return {
&mut self,
cx: &LateContext<'tcx>,
kind: FnKind<'tcx>,
_: &'tcx FnDecl<'tcx>,
fn_decl: &'tcx FnDecl<'tcx>,
body: &'tcx Body<'tcx>,
sp: Span,
_: LocalDefId,
Expand All @@ -245,6 +246,8 @@ impl<'tcx> LateLintPass<'tcx> for Return {
return;
}

let fn_return_type = fn_decl.output;

match kind {
FnKind::Closure => {
// when returning without value in closure, replace this `return`
Expand All @@ -254,24 +257,44 @@ impl<'tcx> LateLintPass<'tcx> for Return {
} else {
RetReplacement::Empty
};
check_final_expr(cx, body.value, vec![], replacement, None);
check_final_expr(cx, body.value, vec![], replacement, None, Some(fn_return_type));
},
FnKind::ItemFn(..) | FnKind::Method(..) => {
check_block_return(cx, &body.value.kind, sp, vec![]);
check_block_return(cx, &body.value.kind, sp, vec![], Some(fn_return_type));
},
}
}
}

// if `expr` is a block, check if there are needless returns in it
fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>, sp: Span, mut semi_spans: Vec<Span>) {
fn check_block_return<'tcx>(
cx: &LateContext<'tcx>,
expr_kind: &ExprKind<'tcx>,
sp: Span,
mut semi_spans: Vec<Span>,
parent_fn_return_type: Option<FnRetTy<'_>>,
) {
if let ExprKind::Block(block, _) = expr_kind {
if let Some(block_expr) = block.expr {
check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty, None);
check_final_expr(
cx,
block_expr,
semi_spans,
RetReplacement::Empty,
None,
parent_fn_return_type,
);
} else if let Some(stmt) = block.stmts.iter().last() {
match stmt.kind {
StmtKind::Expr(expr) => {
check_final_expr(cx, expr, semi_spans, RetReplacement::Empty, None);
check_final_expr(
cx,
expr,
semi_spans,
RetReplacement::Empty,
Some(stmt.kind),
parent_fn_return_type,
);
},
StmtKind::Semi(semi_expr) => {
// Remove ending semicolons and any whitespace ' ' in between.
Expand All @@ -281,7 +304,14 @@ fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>,
span_find_starting_semi(cx.sess().source_map(), semi_span.with_hi(sp.hi()));
semi_spans.push(semi_span_to_remove);
}
check_final_expr(cx, semi_expr, semi_spans, RetReplacement::Empty, None);
check_final_expr(
cx,
semi_expr,
semi_spans,
RetReplacement::Empty,
Some(stmt.kind),
parent_fn_return_type,
);
},
_ => (),
}
Expand All @@ -295,7 +325,8 @@ fn check_final_expr<'tcx>(
semi_spans: Vec<Span>, /* containing all the places where we would need to remove semicolons if finding an
* needless return */
replacement: RetReplacement<'tcx>,
match_ty_opt: Option<Ty<'_>>,
parent_stmt_kind_opt: Option<StmtKind<'_>>,
parent_fn_return_type: Option<FnRetTy<'_>>,
) {
let peeled_drop_expr = expr.peel_drop_temps();
match &peeled_drop_expr.kind {
Expand Down Expand Up @@ -324,22 +355,7 @@ fn check_final_expr<'tcx>(
RetReplacement::Expr(snippet, applicability)
}
} else {
match match_ty_opt {
Some(match_ty) => {
match match_ty.kind() {
// If the code got till here with
// tuple not getting detected before it,
// then we are sure it's going to be Unit
// type
ty::Tuple(_) => RetReplacement::Unit,
// We don't want to anything in this case
// cause we can't predict what the user would
// want here
_ => return,
}
},
None => replacement,
}
replacement
};

if !cx.tcx.hir().attrs(expr.hir_id).is_empty() {
Expand All @@ -353,23 +369,68 @@ fn check_final_expr<'tcx>(
emit_return_lint(cx, ret_span, semi_spans, &replacement);
},
ExprKind::If(_, then, else_clause_opt) => {
check_block_return(cx, &then.kind, peeled_drop_expr.span, semi_spans.clone());
check_block_return(
cx,
&then.kind,
peeled_drop_expr.span,
semi_spans.clone(),
parent_fn_return_type,
);
if let Some(else_clause) = else_clause_opt {
check_block_return(cx, &else_clause.kind, peeled_drop_expr.span, semi_spans);
check_block_return(
cx,
&else_clause.kind,
peeled_drop_expr.span,
semi_spans,
parent_fn_return_type,
);
}
},
// a match expr, check all arms
// an if/if let expr, check both exprs
// note, if without else is going to be a type checking error anyways
// (except for unit type functions) so we don't match it
ExprKind::Match(_, arms, MatchSource::Normal) => {
let match_ty = cx.typeck_results().expr_ty(peeled_drop_expr);
for arm in *arms {
check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Unit, Some(match_ty));
ExprKind::Match(match_expr, arms, MatchSource::Normal) => {
// need some special checks for cases where we would try to convert
// match statement to match expression
let check_arms = if parent_stmt_kind_opt.is_some_and(|x| matches!(x, StmtKind::Semi(_))) {
let match_expr_ret_type = cx.typeck_results().expr_ty(match_expr);

// 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
match parent_fn_return_type {
Some(FnRetTy::DefaultReturn(_)) if let ty::Tuple(tuple) = match_expr_ret_type.kind() =>
tuple.len() == 0,
Some(FnRetTy::Return(_ret_ty)) => matches!(match_expr_ret_type.kind(), _ret_ty),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
    };
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_ => false,
}
} else {
// we always want to check arms in case match is already an expression
true
};

if check_arms {
arms.iter().for_each(|arm| {
check_final_expr(
cx,
arm.body,
semi_spans.clone(),
RetReplacement::Unit,
None,
parent_fn_return_type,
);
});
}
},
// if it's a whole block, check it
other_expr_kind => check_block_return(cx, other_expr_kind, peeled_drop_expr.span, semi_spans),
other_expr_kind => check_block_return(
cx,
other_expr_kind,
peeled_drop_expr.span,
semi_spans,
parent_fn_return_type,
),
}
}

Expand Down
19 changes: 18 additions & 1 deletion tests/ui/needless_return.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ mod issue10049 {
}
}

fn test_match_as_stmt() {
fn issue10546() {
let x = 9;
match x {
1 => 2,
Expand All @@ -316,4 +316,21 @@ fn test_match_as_stmt() {
};
}

fn issue10390_same_arm_return_typs_diff_func_ret_typ(input: u8) {
match input {
3 => 4.1,
_ => 5.1,
};
}

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

fn main() {}
19 changes: 18 additions & 1 deletion tests/ui/needless_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ mod issue10049 {
}
}

fn test_match_as_stmt() {
fn issue10546() {
let x = 9;
match x {
1 => 2,
Expand All @@ -326,4 +326,21 @@ fn test_match_as_stmt() {
};
}

fn issue10390_same_arm_return_typs_diff_func_ret_typ(input: u8) {
match input {
3 => 4.1,
_ => 5.1,
};
}

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

fn main() {}