diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 41cf012ac..caeb1e8b9 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -83,6 +83,12 @@ declare_clippy_lint! { "needless unit expression" } +#[derive(PartialEq, Eq, Copy, Clone)] +enum RetReplacement { + Empty, + Unit +} + declare_lint_pass!(Return => [NEEDLESS_RETURN, LET_AND_RETURN, UNUSED_UNIT]); impl Return { @@ -91,7 +97,7 @@ impl Return { if let Some(stmt) = block.stmts.last() { match stmt.node { ast::StmtKind::Expr(ref expr) | ast::StmtKind::Semi(ref expr) => { - self.check_final_expr(cx, expr, Some(stmt.span)); + self.check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty); }, _ => (), } @@ -99,13 +105,13 @@ impl Return { } // Check a the final expression in a block if it's a return. - fn check_final_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr, span: Option) { + fn check_final_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr, span: Option, replacement: RetReplacement) { match expr.node { // simple return is always "bad" - ast::ExprKind::Ret(Some(ref inner)) => { + ast::ExprKind::Ret(ref inner) => { // allow `#[cfg(a)] return a; #[cfg(b)] return b;` if !expr.attrs.iter().any(attr_is_cfg) { - self.emit_return_lint(cx, span.expect("`else return` is not possible"), inner.span); + self.emit_return_lint(cx, span.expect("`else return` is not possible"), inner.as_ref().map(|i| i.span), replacement); } }, // a whole block? check it! @@ -117,32 +123,61 @@ impl Return { // (except for unit type functions) so we don't match it ast::ExprKind::If(_, ref ifblock, Some(ref elsexpr)) => { self.check_block_return(cx, ifblock); - self.check_final_expr(cx, elsexpr, None); + self.check_final_expr(cx, elsexpr, None, RetReplacement::Empty); }, // a match expr, check all arms ast::ExprKind::Match(_, ref arms) => { for arm in arms { - self.check_final_expr(cx, &arm.body, Some(arm.body.span)); + self.check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Unit); } }, _ => (), } } - fn emit_return_lint(&mut self, cx: &EarlyContext<'_>, ret_span: Span, inner_span: Span) { - if in_external_macro(cx.sess(), inner_span) || in_macro_or_desugar(inner_span) { - return; - } - span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| { - if let Some(snippet) = snippet_opt(cx, inner_span) { - db.span_suggestion( - ret_span, - "remove `return` as shown", - snippet, - Applicability::MachineApplicable, - ); + fn emit_return_lint(&mut self, cx: &EarlyContext<'_>, ret_span: Span, inner_span: Option, replacement: RetReplacement) { + match inner_span { + Some(inner_span) => { + if in_external_macro(cx.sess(), inner_span) || in_macro_or_desugar(inner_span) { + return; + } + + span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| { + if let Some(snippet) = snippet_opt(cx, inner_span) { + db.span_suggestion( + ret_span, + "remove `return` as shown", + snippet, + Applicability::MachineApplicable, + ); + } + }) + }, + None => { + match replacement { + RetReplacement::Empty => { + span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| { + db.span_suggestion( + ret_span, + "remove `return`", + String::new(), + Applicability::MachineApplicable, + ); + }); + } + RetReplacement::Unit => { + span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| { + db.span_suggestion( + ret_span, + "replace `return` with the unit type `()`", + "()".to_string(), + Applicability::MachineApplicable, + ); + }); + } + } } - }); + } } // Check for "let x = EXPR; x" @@ -195,7 +230,7 @@ impl EarlyLintPass for Return { fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, decl: &ast::FnDecl, span: Span, _: ast::NodeId) { match kind { FnKind::ItemFn(.., block) | FnKind::Method(.., block) => self.check_block_return(cx, block), - FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span)), + FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span), RetReplacement::Empty), } if_chain! { if let ast::FunctionRetTy::Ty(ref ty) = decl.output; diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index 939233dbe..bc6072202 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -1,5 +1,9 @@ #![warn(clippy::needless_return)] +macro_rules! the_answer { + () => (42) +} + fn test_end_of_fn() -> bool { if true { // no error! @@ -36,6 +40,22 @@ fn test_closure() { let _ = || return true; } +fn test_macro_call() -> i32 { + return the_answer!(); +} + +fn test_void_fun() { + return; +} + +fn test_void_if_fun(b: bool) { + if b { + return; + } else { + return; + } +} + fn main() { let _ = test_end_of_fn(); let _ = test_no_semicolon(); diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index d7132ce49..71646f32d 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -1,5 +1,5 @@ error: unneeded return statement - --> $DIR/needless_return.rs:8:5 + --> $DIR/needless_return.rs:12:5 | LL | return true; | ^^^^^^^^^^^^ help: remove `return` as shown: `true` @@ -7,46 +7,70 @@ LL | return true; = note: `-D clippy::needless-return` implied by `-D warnings` error: unneeded return statement - --> $DIR/needless_return.rs:12:5 + --> $DIR/needless_return.rs:16:5 | LL | return true; | ^^^^^^^^^^^^ help: remove `return` as shown: `true` error: unneeded return statement - --> $DIR/needless_return.rs:17:9 + --> $DIR/needless_return.rs:21:9 | LL | return true; | ^^^^^^^^^^^^ help: remove `return` as shown: `true` error: unneeded return statement - --> $DIR/needless_return.rs:19:9 + --> $DIR/needless_return.rs:23:9 | LL | return false; | ^^^^^^^^^^^^^ help: remove `return` as shown: `false` error: unneeded return statement - --> $DIR/needless_return.rs:25:17 + --> $DIR/needless_return.rs:29:17 | LL | true => return false, | ^^^^^^^^^^^^ help: remove `return` as shown: `false` error: unneeded return statement - --> $DIR/needless_return.rs:27:13 + --> $DIR/needless_return.rs:31:13 | LL | return true; | ^^^^^^^^^^^^ help: remove `return` as shown: `true` error: unneeded return statement - --> $DIR/needless_return.rs:34:9 + --> $DIR/needless_return.rs:38:9 | LL | return true; | ^^^^^^^^^^^^ help: remove `return` as shown: `true` error: unneeded return statement - --> $DIR/needless_return.rs:36:16 + --> $DIR/needless_return.rs:40:16 | LL | let _ = || return true; | ^^^^^^^^^^^ help: remove `return` as shown: `true` -error: aborting due to 8 previous errors +error: unneeded return statement + --> $DIR/needless_return.rs:44:5 + | +LL | return the_answer!(); + | ^^^^^^^^^^^^^^^^^^^^^ + +error: unneeded return statement + --> $DIR/needless_return.rs:48:5 + | +LL | return; + | ^^^^^^^ help: remove `return` + +error: unneeded return statement + --> $DIR/needless_return.rs:53:9 + | +LL | return; + | ^^^^^^^ help: remove `return` + +error: unneeded return statement + --> $DIR/needless_return.rs:55:9 + | +LL | return; + | ^^^^^^^ help: remove `return` + +error: aborting due to 12 previous errors