From c12748fab3a14beae4958f13551e7e6c52298490 Mon Sep 17 00:00:00 2001 From: feniljain Date: Sat, 1 Apr 2023 16:33:17 +0530 Subject: [PATCH 1/3] fix(needless_return): do not trigger on ambiguous match arms return --- clippy_lints/src/returns.rs | 32 +++++++++++++++++++++++++------- tests/ui/needless_return.fixed | 6 +++--- tests/ui/needless_return.stderr | 12 ++++++------ 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index f0d7dd23a678..b8172b74dc01 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -9,7 +9,7 @@ use rustc_hir::intravisit::FnKind; use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, LangItem, MatchSource, PatKind, QPath, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::subst::GenericArgKind; +use rustc_middle::ty::{self, subst::GenericArgKind, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::def_id::LocalDefId; use rustc_span::source_map::Span; @@ -175,7 +175,7 @@ impl<'tcx> LateLintPass<'tcx> for Return { } else { RetReplacement::Empty }; - check_final_expr(cx, body.value, vec![], replacement); + check_final_expr(cx, body.value, vec![], replacement, None); }, FnKind::ItemFn(..) | FnKind::Method(..) => { check_block_return(cx, &body.value.kind, sp, vec![]); @@ -188,11 +188,11 @@ impl<'tcx> LateLintPass<'tcx> for Return { fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>, sp: Span, mut semi_spans: Vec) { if let ExprKind::Block(block, _) = expr_kind { if let Some(block_expr) = block.expr { - check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty); + check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty, None); } else if let Some(stmt) = block.stmts.iter().last() { match stmt.kind { StmtKind::Expr(expr) => { - check_final_expr(cx, expr, semi_spans, RetReplacement::Empty); + check_final_expr(cx, expr, semi_spans, RetReplacement::Empty, None); }, StmtKind::Semi(semi_expr) => { // Remove ending semicolons and any whitespace ' ' in between. @@ -202,7 +202,7 @@ 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); + check_final_expr(cx, semi_expr, semi_spans, RetReplacement::Empty, None); }, _ => (), } @@ -216,6 +216,7 @@ fn check_final_expr<'tcx>( semi_spans: Vec, /* containing all the places where we would need to remove semicolons if finding an * needless return */ replacement: RetReplacement<'tcx>, + match_ty_opt: Option>, ) { let peeled_drop_expr = expr.peel_drop_temps(); match &peeled_drop_expr.kind { @@ -244,7 +245,22 @@ fn check_final_expr<'tcx>( RetReplacement::Expr(snippet, applicability) } } else { - replacement + 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, + } }; if !cx.tcx.hir().attrs(expr.hir_id).is_empty() { @@ -268,8 +284,9 @@ fn check_final_expr<'tcx>( // 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.iter() { - check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Unit); + check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Empty, Some(match_ty)); } }, // if it's a whole block, check it @@ -293,6 +310,7 @@ fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, semi_spans: Vec, if ret_span.from_expansion() { return; } + let applicability = replacement.applicability().unwrap_or(Applicability::MachineApplicable); let return_replacement = replacement.to_string(); let sugg_help = replacement.sugg_help(); diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index 0f525dd294c9..38ae448618c0 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -81,7 +81,7 @@ fn test_void_if_fun(b: bool) { fn test_void_match(x: u32) { match x { 0 => (), - _ => (), + _ =>(), } } @@ -91,7 +91,7 @@ fn test_nested_match(x: u32) { 1 => { let _ = 42; }, - _ => (), + _ =>(), } } @@ -196,7 +196,7 @@ async fn async_test_void_if_fun(b: bool) { async fn async_test_void_match(x: u32) { match x { 0 => (), - _ => (), + _ =>(), } } diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index 87d0cd3e14cf..5d471a3c5214 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -118,10 +118,10 @@ LL | | return; = help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:87:14 + --> $DIR/needless_return.rs:87:13 | LL | _ => return, - | ^^^^^^ + | ^^^^^^^ | = help: replace `return` with a unit value @@ -136,10 +136,10 @@ LL | | return; = help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:98:14 + --> $DIR/needless_return.rs:98:13 | LL | _ => return, - | ^^^^^^ + | ^^^^^^^ | = help: replace `return` with a unit value @@ -296,10 +296,10 @@ LL | | return; = help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:207:14 + --> $DIR/needless_return.rs:207:13 | LL | _ => return, - | ^^^^^^ + | ^^^^^^^ | = help: replace `return` with a unit value From b499b7dc73f3571e2627a34a93e3079cab848bbc Mon Sep 17 00:00:00 2001 From: feniljain Date: Wed, 5 Apr 2023 19:50:12 +0530 Subject: [PATCH 2/3] test: add test for match as stmt no triggering needless_return --- tests/ui/needless_return.fixed | 9 +++++++++ tests/ui/needless_return.rs | 9 +++++++++ 2 files changed, 18 insertions(+) diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index 38ae448618c0..47a9f031a42c 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -307,4 +307,13 @@ mod issue10049 { } } +fn test_match_as_stmt() { + let x = 9; + match x { + 1 => 2, + 2 => return, + _ => 0, + }; +} + fn main() {} diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index a1db8375d95b..7c1feefbe32b 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -317,4 +317,13 @@ mod issue10049 { } } +fn test_match_as_stmt() { + let x = 9; + match x { + 1 => 2, + 2 => return, + _ => 0, + }; +} + fn main() {} From 9cf57d0a8f95c51b0faeda4ce5c5b4cd629f1cae Mon Sep 17 00:00:00 2001 From: feniljain Date: Wed, 5 Apr 2023 20:03:45 +0530 Subject: [PATCH 3/3] fix(needles_return): correct span selection for text replacement --- clippy_lints/src/returns.rs | 2 +- tests/ui/needless_return.fixed | 6 +++--- tests/ui/needless_return.stderr | 12 ++++++------ 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index b8172b74dc01..df126d7617eb 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -286,7 +286,7 @@ fn check_final_expr<'tcx>( ExprKind::Match(_, arms, MatchSource::Normal) => { let match_ty = cx.typeck_results().expr_ty(peeled_drop_expr); for arm in arms.iter() { - check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Empty, Some(match_ty)); + check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Unit, Some(match_ty)); } }, // if it's a whole block, check it diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index 47a9f031a42c..57c08996ce25 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -81,7 +81,7 @@ fn test_void_if_fun(b: bool) { fn test_void_match(x: u32) { match x { 0 => (), - _ =>(), + _ => (), } } @@ -91,7 +91,7 @@ fn test_nested_match(x: u32) { 1 => { let _ = 42; }, - _ =>(), + _ => (), } } @@ -196,7 +196,7 @@ async fn async_test_void_if_fun(b: bool) { async fn async_test_void_match(x: u32) { match x { 0 => (), - _ =>(), + _ => (), } } diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index 5d471a3c5214..87d0cd3e14cf 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -118,10 +118,10 @@ LL | | return; = help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:87:13 + --> $DIR/needless_return.rs:87:14 | LL | _ => return, - | ^^^^^^^ + | ^^^^^^ | = help: replace `return` with a unit value @@ -136,10 +136,10 @@ LL | | return; = help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:98:13 + --> $DIR/needless_return.rs:98:14 | LL | _ => return, - | ^^^^^^^ + | ^^^^^^ | = help: replace `return` with a unit value @@ -296,10 +296,10 @@ LL | | return; = help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:207:13 + --> $DIR/needless_return.rs:207:14 | LL | _ => return, - | ^^^^^^^ + | ^^^^^^ | = help: replace `return` with a unit value