From 35058287ce7cadd20eebf3fc77a34ca59c8a5eac Mon Sep 17 00:00:00 2001 From: daxpedda Date: Sun, 16 Dec 2018 15:42:02 +0100 Subject: [PATCH 1/2] Fix `implicit_return` false positives. --- clippy_lints/src/implicit_return.rs | 42 ++++++++++++++--------------- tests/ui/implicit_return.rs | 23 ++++++++++++++++ tests/ui/implicit_return.stderr | 22 +++++++++++---- 3 files changed, 61 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/implicit_return.rs b/clippy_lints/src/implicit_return.rs index 75c66d22647..96022db56aa 100644 --- a/clippy_lints/src/implicit_return.rs +++ b/clippy_lints/src/implicit_return.rs @@ -45,6 +45,19 @@ declare_clippy_lint! { pub struct Pass; impl Pass { + fn lint(cx: &LateContext<'_, '_>, outer_span: syntax_pos::Span, inner_span: syntax_pos::Span, msg: &str) { + span_lint_and_then(cx, IMPLICIT_RETURN, outer_span, "missing return statement", |db| { + if let Some(snippet) = snippet_opt(cx, inner_span) { + db.span_suggestion_with_applicability( + outer_span, + msg, + format!("return {}", snippet), + Applicability::MachineApplicable, + ); + } + }); + } + fn expr_match(cx: &LateContext<'_, '_>, expr: &rustc::hir::Expr) { match &expr.node { // loops could be using `break` instead of `return` @@ -55,23 +68,19 @@ impl Pass { // only needed in the case of `break` with `;` at the end else if let Some(stmt) = block.stmts.last() { if let rustc::hir::StmtKind::Semi(expr, ..) = &stmt.node { - Self::expr_match(cx, expr); + // make sure it's a break, otherwise we want to skip + if let ExprKind::Break(.., break_expr) = &expr.node { + if let Some(break_expr) = break_expr { + Self::lint(cx, expr.span, break_expr.span, "change `break` to `return` as shown"); + } + } } } }, // use `return` instead of `break` ExprKind::Break(.., break_expr) => { if let Some(break_expr) = break_expr { - span_lint_and_then(cx, IMPLICIT_RETURN, expr.span, "missing return statement", |db| { - if let Some(snippet) = snippet_opt(cx, break_expr.span) { - db.span_suggestion_with_applicability( - expr.span, - "change `break` to `return` as shown", - format!("return {}", snippet), - Applicability::MachineApplicable, - ); - } - }); + Self::lint(cx, expr.span, break_expr.span, "change `break` to `return` as shown"); } }, ExprKind::If(.., if_expr, else_expr) => { @@ -89,16 +98,7 @@ impl Pass { // skip if it already has a return statement ExprKind::Ret(..) => (), // everything else is missing `return` - _ => span_lint_and_then(cx, IMPLICIT_RETURN, expr.span, "missing return statement", |db| { - if let Some(snippet) = snippet_opt(cx, expr.span) { - db.span_suggestion_with_applicability( - expr.span, - "add `return` as shown", - format!("return {}", snippet), - Applicability::MachineApplicable, - ); - } - }), + _ => Self::lint(cx, expr.span, expr.span, "add `return` as shown"), } } } diff --git a/tests/ui/implicit_return.rs b/tests/ui/implicit_return.rs index 6188835e555..61cb35e1209 100644 --- a/tests/ui/implicit_return.rs +++ b/tests/ui/implicit_return.rs @@ -42,6 +42,27 @@ fn test_loop() -> bool { } } +#[allow(clippy::never_loop)] +fn test_loop_with_block() -> bool { + loop { + { + break true; + } + } +} + +#[allow(clippy::never_loop)] +fn test_loop_with_nests() -> bool { + loop { + if true { + let _ = true; + } + else { + break true; + } + } +} + fn test_closure() { #[rustfmt::skip] let _ = || { true }; @@ -53,5 +74,7 @@ fn main() { let _ = test_if_block(); let _ = test_match(true); let _ = test_loop(); + let _ = test_loop_with_block(); + let _ = test_loop_with_nests(); test_closure(); } diff --git a/tests/ui/implicit_return.stderr b/tests/ui/implicit_return.stderr index b2feec3f57a..6d0761554cd 100644 --- a/tests/ui/implicit_return.stderr +++ b/tests/ui/implicit_return.stderr @@ -37,16 +37,28 @@ error: missing return statement | ^^^^^^^^^^ help: change `break` to `return` as shown: `return true` error: missing return statement - --> $DIR/implicit_return.rs:47:18 + --> $DIR/implicit_return.rs:49:13 | -47 | let _ = || { true }; +49 | break true; + | ^^^^^^^^^^ help: change `break` to `return` as shown: `return true` + +error: missing return statement + --> $DIR/implicit_return.rs:61:13 + | +61 | break true; + | ^^^^^^^^^^ help: change `break` to `return` as shown: `return true` + +error: missing return statement + --> $DIR/implicit_return.rs:68:18 + | +68 | let _ = || { true }; | ^^^^ help: add `return` as shown: `return true` error: missing return statement - --> $DIR/implicit_return.rs:48:16 + --> $DIR/implicit_return.rs:69:16 | -48 | let _ = || true; +69 | let _ = || true; | ^^^^ help: add `return` as shown: `return true` -error: aborting due to 8 previous errors +error: aborting due to 10 previous errors From 6870638c3fb66c2abb20633bf40cc09ccc760047 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Sun, 16 Dec 2018 22:20:05 +0100 Subject: [PATCH 2/2] Fix an endless loop in the tests. --- tests/ui/implicit_return.rs | 4 ++-- tests/ui/implicit_return.stderr | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ui/implicit_return.rs b/tests/ui/implicit_return.rs index 61cb35e1209..9fb30135231 100644 --- a/tests/ui/implicit_return.rs +++ b/tests/ui/implicit_return.rs @@ -55,10 +55,10 @@ fn test_loop_with_block() -> bool { fn test_loop_with_nests() -> bool { loop { if true { - let _ = true; + break true; } else { - break true; + let _ = true; } } } diff --git a/tests/ui/implicit_return.stderr b/tests/ui/implicit_return.stderr index 6d0761554cd..b3562b67034 100644 --- a/tests/ui/implicit_return.stderr +++ b/tests/ui/implicit_return.stderr @@ -43,9 +43,9 @@ error: missing return statement | ^^^^^^^^^^ help: change `break` to `return` as shown: `return true` error: missing return statement - --> $DIR/implicit_return.rs:61:13 + --> $DIR/implicit_return.rs:58:13 | -61 | break true; +58 | break true; | ^^^^^^^^^^ help: change `break` to `return` as shown: `return true` error: missing return statement