diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 91553240e3c..f758f4cff8b 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint_hir_and_then; +use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then}; use clippy_utils::source::{snippet_opt, snippet_with_context}; use clippy_utils::{fn_def_id, path_to_local_id}; use if_chain::if_chain; @@ -72,6 +72,27 @@ enum RetReplacement { Unit, } +impl RetReplacement { + fn sugg_help(self) -> &'static str { + match self { + Self::Empty => "remove `return`", + Self::Block => "replace `return` with an empty block", + Self::Unit => "replace `return` with a unit value", + } + } +} + +impl ToString for RetReplacement { + fn to_string(&self) -> String { + match *self { + Self::Empty => "", + Self::Block => "{}", + Self::Unit => "()", + } + .to_string() + } +} + declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN]); impl<'tcx> LateLintPass<'tcx> for Return { @@ -139,26 +160,35 @@ impl<'tcx> LateLintPass<'tcx> for Return { } else { RetReplacement::Empty }; - check_final_expr(cx, body.value, Some(body.value.span), replacement); + check_final_expr(cx, body.value, vec![], replacement); }, FnKind::ItemFn(..) | FnKind::Method(..) => { - if let ExprKind::Block(block, _) = body.value.kind { - check_block_return(cx, block); - } + check_block_return(cx, &body.value.kind, vec![]); }, } } } -fn check_block_return<'tcx>(cx: &LateContext<'tcx>, block: &Block<'tcx>) { - if let Some(expr) = block.expr { - check_final_expr(cx, expr, Some(expr.span), RetReplacement::Empty); - } else if let Some(stmt) = block.stmts.iter().last() { - match stmt.kind { - StmtKind::Expr(expr) | StmtKind::Semi(expr) => { - check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty); - }, - _ => (), +// 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>, semi_spans: Vec<Span>) { + if let ExprKind::Block(block, _) = expr_kind { + if let Some(block_expr) = block.expr { + check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty); + } else if let Some(stmt) = block.stmts.iter().last() { + match stmt.kind { + StmtKind::Expr(expr) => { + check_final_expr(cx, expr, semi_spans, RetReplacement::Empty); + }, + StmtKind::Semi(semi_expr) => { + let mut semi_spans_and_this_one = semi_spans; + // we only want the span containing the semicolon so we can remove it later. From `entry.rs:382` + if let Some(semicolon_span) = stmt.span.trim_start(semi_expr.span) { + semi_spans_and_this_one.push(semicolon_span); + check_final_expr(cx, semi_expr, semi_spans_and_this_one, RetReplacement::Empty); + } + }, + _ => (), + } } } } @@ -166,10 +196,12 @@ fn check_block_return<'tcx>(cx: &LateContext<'tcx>, block: &Block<'tcx>) { fn check_final_expr<'tcx>( cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, - span: Option<Span>, + semi_spans: Vec<Span>, /* containing all the places where we would need to remove semicolons if finding an + * needless return */ replacement: RetReplacement, ) { - match expr.kind { + let peeled_drop_expr = expr.peel_drop_temps(); + match &peeled_drop_expr.kind { // simple return is always "bad" ExprKind::Ret(ref inner) => { if cx.tcx.hir().attrs(expr.hir_id).is_empty() { @@ -177,24 +209,18 @@ fn check_final_expr<'tcx>( if !borrows { emit_return_lint( cx, - inner.map_or(expr.hir_id, |inner| inner.hir_id), - span.expect("`else return` is not possible"), + peeled_drop_expr.span, + semi_spans, inner.as_ref().map(|i| i.span), replacement, ); } } }, - // a whole block? check it! - ExprKind::Block(block, _) => { - check_block_return(cx, block); - }, ExprKind::If(_, then, else_clause_opt) => { - if let ExprKind::Block(ifblock, _) = then.kind { - check_block_return(cx, ifblock); - } + check_block_return(cx, &then.kind, semi_spans.clone()); if let Some(else_clause) = else_clause_opt { - check_final_expr(cx, else_clause, None, RetReplacement::Empty); + check_block_return(cx, &else_clause.kind, semi_spans); } }, // a match expr, check all arms @@ -203,93 +229,44 @@ fn check_final_expr<'tcx>( // (except for unit type functions) so we don't match it ExprKind::Match(_, arms, MatchSource::Normal) => { for arm in arms.iter() { - check_final_expr(cx, arm.body, Some(arm.body.span), RetReplacement::Unit); + check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Unit); } }, - ExprKind::DropTemps(expr) => check_final_expr(cx, expr, None, RetReplacement::Empty), - _ => (), + // if it's a whole block, check it + other_expr_kind => check_block_return(cx, other_expr_kind, semi_spans), } } fn emit_return_lint( cx: &LateContext<'_>, - emission_place: HirId, ret_span: Span, + semi_spans: Vec<Span>, inner_span: Option<Span>, replacement: RetReplacement, ) { if ret_span.from_expansion() { return; } - match inner_span { - Some(inner_span) => { - let mut applicability = Applicability::MachineApplicable; - span_lint_hir_and_then( - cx, - NEEDLESS_RETURN, - emission_place, - ret_span, - "unneeded `return` statement", - |diag| { - let (snippet, _) = snippet_with_context(cx, inner_span, ret_span.ctxt(), "..", &mut applicability); - diag.span_suggestion(ret_span, "remove `return`", snippet, applicability); - }, - ); + let mut applicability = Applicability::MachineApplicable; + let return_replacement = inner_span.map_or_else( + || replacement.to_string(), + |inner_span| { + let (snippet, _) = snippet_with_context(cx, inner_span, ret_span.ctxt(), "..", &mut applicability); + snippet.to_string() }, - None => match replacement { - RetReplacement::Empty => { - span_lint_hir_and_then( - cx, - NEEDLESS_RETURN, - emission_place, - ret_span, - "unneeded `return` statement", - |diag| { - diag.span_suggestion( - ret_span, - "remove `return`", - String::new(), - Applicability::MachineApplicable, - ); - }, - ); - }, - RetReplacement::Block => { - span_lint_hir_and_then( - cx, - NEEDLESS_RETURN, - emission_place, - ret_span, - "unneeded `return` statement", - |diag| { - diag.span_suggestion( - ret_span, - "replace `return` with an empty block", - "{}".to_string(), - Applicability::MachineApplicable, - ); - }, - ); - }, - RetReplacement::Unit => { - span_lint_hir_and_then( - cx, - NEEDLESS_RETURN, - emission_place, - ret_span, - "unneeded `return` statement", - |diag| { - diag.span_suggestion( - ret_span, - "replace `return` with a unit value", - "()".to_string(), - Applicability::MachineApplicable, - ); - }, - ); - }, - }, - } + ); + let sugg_help = if inner_span.is_some() { + "remove `return`" + } else { + replacement.sugg_help() + }; + span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded `return` statement", |diag| { + diag.span_suggestion_hidden(ret_span, sugg_help, return_replacement, applicability); + // for each parent statement, we need to remove the semicolon + for semi_stmt_span in semi_spans { + diag.tool_only_span_suggestion(semi_stmt_span, "remove this semicolon", "", applicability); + } + }); } fn last_statement_borrows<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index 87c8fc03b3c..112a2f57b41 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -233,4 +233,41 @@ fn issue_9361() -> i32 { return 1 + 2; } +fn issue8336(x: i32) -> bool { + if x > 0 { + println!("something"); + true + } else { + false + } +} + +fn issue8156(x: u8) -> u64 { + match x { + 80 => { + 10 + }, + _ => { + 100 + }, + } +} + +// Ideally the compiler should throw `unused_braces` in this case +fn issue9192() -> i32 { + { + 0 + } +} + +fn issue9503(x: usize) -> isize { + unsafe { + if x > 12 { + *(x as *const isize) + } else { + !*(x as *const isize) + } + } +} + fn main() {} diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index 5a86e656255..22aa2e11fad 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -233,4 +233,41 @@ fn issue_9361() -> i32 { return 1 + 2; } +fn issue8336(x: i32) -> bool { + if x > 0 { + println!("something"); + return true; + } else { + return false; + }; +} + +fn issue8156(x: u8) -> u64 { + match x { + 80 => { + return 10; + }, + _ => { + return 100; + }, + }; +} + +// Ideally the compiler should throw `unused_braces` in this case +fn issue9192() -> i32 { + { + return 0; + }; +} + +fn issue9503(x: usize) -> isize { + unsafe { + if x > 12 { + return *(x as *const isize); + } else { + return !*(x as *const isize); + }; + }; +} + fn main() {} diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index 83ff0763869..45090dbe206 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -2,225 +2,354 @@ error: unneeded `return` statement --> $DIR/needless_return.rs:27:5 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ | = note: `-D clippy::needless-return` implied by `-D warnings` + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:31:5 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:36:9 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:38:9 | LL | return false; - | ^^^^^^^^^^^^^ help: remove `return`: `false` + | ^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:44:17 | LL | true => return false, - | ^^^^^^^^^^^^ help: remove `return`: `false` + | ^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:46:13 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:53:9 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:55:16 | LL | let _ = || return true; - | ^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:59:5 | LL | return the_answer!(); - | ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `the_answer!()` + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:63:5 | LL | return; - | ^^^^^^^ help: remove `return` + | ^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:68:9 | LL | return; - | ^^^^^^^ help: remove `return` + | ^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:70:9 | LL | return; - | ^^^^^^^ help: remove `return` + | ^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:77:14 | LL | _ => return, - | ^^^^^^ help: replace `return` with a unit value: `()` + | ^^^^^^ + | + = help: replace `return` with a unit value error: unneeded `return` statement --> $DIR/needless_return.rs:86:13 | LL | return; - | ^^^^^^^ help: remove `return` + | ^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:88:14 | LL | _ => return, - | ^^^^^^ help: replace `return` with a unit value: `()` + | ^^^^^^ + | + = help: replace `return` with a unit value error: unneeded `return` statement --> $DIR/needless_return.rs:101:9 | LL | return String::from("test"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::from("test")` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:103:9 | LL | return String::new(); - | ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::new()` + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:125:32 | LL | bar.unwrap_or_else(|_| return) - | ^^^^^^ help: replace `return` with an empty block: `{}` + | ^^^^^^ + | + = help: replace `return` with an empty block error: unneeded `return` statement --> $DIR/needless_return.rs:130:13 | LL | return; - | ^^^^^^^ help: remove `return` + | ^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:132:20 | LL | let _ = || return; - | ^^^^^^ help: replace `return` with an empty block: `{}` + | ^^^^^^ + | + = help: replace `return` with an empty block error: unneeded `return` statement --> $DIR/needless_return.rs:138:32 | LL | res.unwrap_or_else(|_| return Foo) - | ^^^^^^^^^^ help: remove `return`: `Foo` + | ^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:147:5 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:151:5 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:156:9 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:158:9 | LL | return false; - | ^^^^^^^^^^^^^ help: remove `return`: `false` + | ^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:164:17 | LL | true => return false, - | ^^^^^^^^^^^^ help: remove `return`: `false` + | ^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:166:13 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:173:9 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:175:16 | LL | let _ = || return true; - | ^^^^^^^^^^^ help: remove `return`: `true` + | ^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:179:5 | LL | return the_answer!(); - | ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `the_answer!()` + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:183:5 | LL | return; - | ^^^^^^^ help: remove `return` + | ^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:188:9 | LL | return; - | ^^^^^^^ help: remove `return` + | ^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:190:9 | LL | return; - | ^^^^^^^ help: remove `return` + | ^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:197:14 | LL | _ => return, - | ^^^^^^ help: replace `return` with a unit value: `()` + | ^^^^^^ + | + = help: replace `return` with a unit value error: unneeded `return` statement --> $DIR/needless_return.rs:210:9 | LL | return String::from("test"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::from("test")` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:212:9 | LL | return String::new(); - | ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::new()` + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` error: unneeded `return` statement --> $DIR/needless_return.rs:228:5 | LL | return format!("Hello {}", "world!"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `format!("Hello {}", "world!")` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` -error: aborting due to 37 previous errors +error: unneeded `return` statement + --> $DIR/needless_return.rs:239:9 + | +LL | return true; + | ^^^^^^^^^^^ + | + = help: remove `return` + +error: unneeded `return` statement + --> $DIR/needless_return.rs:241:9 + | +LL | return false; + | ^^^^^^^^^^^^ + | + = help: remove `return` + +error: unneeded `return` statement + --> $DIR/needless_return.rs:248:13 + | +LL | return 10; + | ^^^^^^^^^ + | + = help: remove `return` + +error: unneeded `return` statement + --> $DIR/needless_return.rs:251:13 + | +LL | return 100; + | ^^^^^^^^^^ + | + = help: remove `return` + +error: unneeded `return` statement + --> $DIR/needless_return.rs:259:9 + | +LL | return 0; + | ^^^^^^^^ + | + = help: remove `return` + +error: unneeded `return` statement + --> $DIR/needless_return.rs:266:13 + | +LL | return *(x as *const isize); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` + +error: unneeded `return` statement + --> $DIR/needless_return.rs:268:13 + | +LL | return !*(x as *const isize); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` + +error: aborting due to 44 previous errors