diff --git a/clippy_lints/src/invalid_ref.rs b/clippy_lints/src/invalid_ref.rs index af100f4c41d9a..8f9ccaea26d21 100644 --- a/clippy_lints/src/invalid_ref.rs +++ b/clippy_lints/src/invalid_ref.rs @@ -51,6 +51,5 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidRef { span_help_and_lint(cx, INVALID_REF, expr.span, msg, HELP); } } - return; } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 304a39cfd49fe..07eb70a33acf6 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1648,16 +1648,15 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: ); // Check if the first argument to .fold is a suitable literal - match fold_args[1].node { - hir::ExprKind::Lit(ref lit) => match lit.node { + if let hir::ExprKind::Lit(ref lit) = fold_args[1].node { + match lit.node { ast::LitKind::Bool(false) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Or, "any", true), ast::LitKind::Bool(true) => check_fold_with_op(cx, fold_args, hir::BinOpKind::And, "all", true), ast::LitKind::Int(0, _) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Add, "sum", false), ast::LitKind::Int(1, _) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Mul, "product", false), - _ => return, - }, - _ => return, - }; + _ => (), + } + } } fn lint_iter_nth<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_args: &'tcx [hir::Expr], is_mut: bool) { diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index cc0192feb5cb4..a8c45166a8b35 100644 --- a/clippy_lints/src/non_expressive_names.rs +++ b/clippy_lints/src/non_expressive_names.rs @@ -169,7 +169,9 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> { .any(|id| id.name == ident.name) { return; - } else if let Some(scope) = &mut self.0.single_char_names.last_mut() { + } + + if let Some(scope) = &mut self.0.single_char_names.last_mut() { scope.push(ident); } } diff --git a/clippy_lints/src/redundant_pattern_matching.rs b/clippy_lints/src/redundant_pattern_matching.rs index fe1a29b98833c..a178db3132300 100644 --- a/clippy_lints/src/redundant_pattern_matching.rs +++ b/clippy_lints/src/redundant_pattern_matching.rs @@ -95,8 +95,6 @@ fn find_sugg_for_if_let<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, ); }, ); - } else { - return; } } @@ -161,8 +159,6 @@ fn find_sugg_for_match<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, o }, ); } - } else { - return; } } diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 41cf012ac879f..2245b719c23bb 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,24 @@ 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 +134,60 @@ 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; + 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`", 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, + ); + }); + }, + }, } - 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, - ); - } - }); } // Check for "let x = EXPR; x" @@ -195,7 +240,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 939233dbecb3a..e9f064ea3efe3 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -1,5 +1,11 @@ #![warn(clippy::needless_return)] +macro_rules! the_answer { + () => { + 42 + }; +} + fn test_end_of_fn() -> bool { if true { // no error! @@ -36,6 +42,29 @@ 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 test_void_match(x: u32) { + match x { + 0 => (), + _ => 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 d7132ce495093..7858ecfba9787 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -1,52 +1,76 @@ error: unneeded return statement - --> $DIR/needless_return.rs:8:5 + --> $DIR/needless_return.rs:14:5 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return` as shown: `true` + | ^^^^^^^^^^^^ help: remove `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:18:5 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return` as shown: `true` + | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded return statement - --> $DIR/needless_return.rs:17:9 + --> $DIR/needless_return.rs:23:9 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return` as shown: `true` + | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded return statement - --> $DIR/needless_return.rs:19:9 + --> $DIR/needless_return.rs:25:9 | LL | return false; - | ^^^^^^^^^^^^^ help: remove `return` as shown: `false` + | ^^^^^^^^^^^^^ help: remove `return`: `false` error: unneeded return statement - --> $DIR/needless_return.rs:25:17 + --> $DIR/needless_return.rs:31:17 | LL | true => return false, - | ^^^^^^^^^^^^ help: remove `return` as shown: `false` + | ^^^^^^^^^^^^ help: remove `return`: `false` error: unneeded return statement - --> $DIR/needless_return.rs:27:13 + --> $DIR/needless_return.rs:33:13 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return` as shown: `true` + | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded return statement - --> $DIR/needless_return.rs:34:9 + --> $DIR/needless_return.rs:40:9 | LL | return true; - | ^^^^^^^^^^^^ help: remove `return` as shown: `true` + | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded return statement - --> $DIR/needless_return.rs:36:16 + --> $DIR/needless_return.rs:42:16 | LL | let _ = || return true; - | ^^^^^^^^^^^ help: remove `return` as shown: `true` + | ^^^^^^^^^^^ help: remove `return`: `true` -error: aborting due to 8 previous errors +error: unneeded return statement + --> $DIR/needless_return.rs:50:5 + | +LL | return; + | ^^^^^^^ help: remove `return` + +error: unneeded return statement + --> $DIR/needless_return.rs:55:9 + | +LL | return; + | ^^^^^^^ help: remove `return` + +error: unneeded return statement + --> $DIR/needless_return.rs:57:9 + | +LL | return; + | ^^^^^^^ help: remove `return` + +error: unneeded return statement + --> $DIR/needless_return.rs:64:14 + | +LL | _ => return, + | ^^^^^^ help: replace `return` with the unit type: `()` + +error: aborting due to 12 previous errors