diff options
| -rw-r--r-- | clippy_lints/src/returns.rs | 65 | ||||
| -rw-r--r-- | tests/ui/needless_return.fixed | 27 | ||||
| -rw-r--r-- | tests/ui/needless_return.rs | 27 | ||||
| -rw-r--r-- | tests/ui/needless_return.stderr | 189 |
4 files changed, 244 insertions, 64 deletions
diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 721c5f64a90..f7ceb415a79 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, span_lint_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; @@ -9,7 +9,7 @@ use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_middle::ty::subst::GenericArgKind; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::source_map::Span; +use rustc_span::source_map::{Span, DUMMY_SP}; declare_clippy_lint! { /// ### What it does @@ -73,8 +73,8 @@ enum RetReplacement { } impl RetReplacement { - fn sugg_help(&self) -> &'static str { - match *self { + 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", @@ -160,24 +160,30 @@ impl<'tcx> LateLintPass<'tcx> for Return { } else { RetReplacement::Empty }; - check_final_expr(cx, body.value, body.value.span, replacement); + check_final_expr(cx, body.value, vec![], replacement); }, FnKind::ItemFn(..) | FnKind::Method(..) => { - check_block_return(cx, &body.value.kind); + check_block_return(cx, &body.value.kind, vec![]); }, } } } // 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>) { +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, block_expr.span, RetReplacement::Empty); + 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) | StmtKind::Semi(expr) => { - check_final_expr(cx, expr, stmt.span, RetReplacement::Empty); + 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` + semi_spans_and_this_one.push(stmt.span.trim_start(semi_expr.span).unwrap_or(DUMMY_SP)); + check_final_expr(cx, semi_expr, semi_spans_and_this_one, RetReplacement::Empty); }, _ => (), } @@ -185,8 +191,15 @@ fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>) } } -fn check_final_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, span: Span, replacement: RetReplacement) { - match &expr.peel_drop_temps().kind { +fn check_final_expr<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + semi_spans: Vec<Span>, /* containing all the places where we would need to remove semicolons if finding an + * needless return */ + replacement: RetReplacement, +) { + 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() { @@ -194,7 +207,8 @@ fn check_final_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, span: if !borrows { emit_return_lint( cx, - span, + peeled_drop_expr.span, + semi_spans, inner.as_ref().map(|i| i.span), replacement, ); @@ -202,9 +216,9 @@ fn check_final_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, span: } }, ExprKind::If(_, then, else_clause_opt) => { - check_block_return(cx, &then.kind); + check_block_return(cx, &then.kind, semi_spans.clone()); if let Some(else_clause) = else_clause_opt { - check_block_return(cx, &else_clause.kind) + check_block_return(cx, &else_clause.kind, semi_spans); } }, // a match expr, check all arms @@ -213,17 +227,18 @@ fn check_final_expr<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, span: // (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, arm.body.span, RetReplacement::Unit); + check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Unit); } }, // if it's a whole block, check it - other_expr_kind => check_block_return(cx, &other_expr_kind), + other_expr_kind => check_block_return(cx, other_expr_kind, semi_spans), } } fn emit_return_lint( cx: &LateContext<'_>, ret_span: Span, + semi_spans: Vec<Span>, inner_span: Option<Span>, replacement: RetReplacement, ) { @@ -243,15 +258,13 @@ fn emit_return_lint( } else { replacement.sugg_help() }; - span_lint_and_then( - cx, - NEEDLESS_RETURN, - ret_span, - "unneeded `return` statement", - |diag| { - diag.span_suggestion(ret_span, sugg_help, return_replacement, applicability); - }, - ); + 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..a5e28b9b1a4 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -233,4 +233,31 @@ 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 main() {} diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index 5a86e656255..49fd49c7293 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -233,4 +233,31 @@ 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 main() {} diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index 83ff0763869..d8dba766bf4 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -2,225 +2,338 @@ 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: 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: aborting due to 37 previous errors +error: aborting due to 42 previous errors |