Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

false positives fixes of implicit_return #4274

Merged
merged 3 commits into from
Jul 18, 2019
Merged

false positives fixes of implicit_return #4274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c067bf
false positives fixes of `implicit_return`
daxpedda Jul 15, 2019
a2b63af
Removed lintining on never type.
daxpedda Jul 16, 2019
9e5e0f8
Improved imports
daxpedda Jul 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 91 additions & 63 deletions clippy_lints/src/implicit_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
use crate::utils::{in_macro_or_desugar, is_expn_of, snippet_opt, span_lint_and_then};
use rustc::hir::{intravisit::FnKind, Body, ExprKind, FnDecl, HirId, MatchSource};
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
use rustc::{declare_lint_pass, declare_tool_lint};
use crate::utils::{
in_macro_or_desugar, match_def_path,
paths::{BEGIN_PANIC, BEGIN_PANIC_FMT},
resolve_node, snippet_opt, span_lint_and_then,
};
use if_chain::if_chain;
use rustc::{
declare_lint_pass, declare_tool_lint,
hir::{intravisit::FnKind, Body, Expr, ExprKind, FnDecl, HirId, MatchSource, StmtKind},
lint::{LateContext, LateLintPass, LintArray, LintPass},
};
use rustc_errors::Applicability;
use syntax::source_map::Span;

Expand Down Expand Up @@ -35,71 +42,92 @@ declare_clippy_lint! {

declare_lint_pass!(ImplicitReturn => [IMPLICIT_RETURN]);

impl ImplicitReturn {
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(
outer_span,
msg,
format!("return {}", snippet),
Applicability::MachineApplicable,
);
}
});
static LINT_BREAK: &str = "change `break` to `return` as shown";
static LINT_RETURN: &str = "add `return` as shown";

fn lint(cx: &LateContext<'_, '_>, outer_span: Span, inner_span: Span, msg: &str) {
let outer_span = span_to_outer_expn(outer_span);
let inner_span = span_to_outer_expn(inner_span);

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(
outer_span,
msg,
format!("return {}", snippet),
Applicability::MachineApplicable,
);
}
});
}

fn span_to_outer_expn(span: Span) -> Span {
if let Some(expr) = span.ctxt().outer_expn_info() {
span_to_outer_expn(expr.call_site)
} else {
span
}
}

fn expr_match(cx: &LateContext<'_, '_>, expr: &rustc::hir::Expr) {
match &expr.node {
// loops could be using `break` instead of `return`
ExprKind::Block(block, ..) | ExprKind::Loop(block, ..) => {
if let Some(expr) = &block.expr {
Self::expr_match(cx, expr);
}
// 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 {
// 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");
}
}
fn expr_match(cx: &LateContext<'_, '_>, expr: &Expr) {
match &expr.node {
// loops could be using `break` instead of `return`
ExprKind::Block(block, ..) | ExprKind::Loop(block, ..) => {
if let Some(expr) = &block.expr {
expr_match(cx, expr);
}
// only needed in the case of `break` with `;` at the end
else if let Some(stmt) = block.stmts.last() {
if_chain! {
if let StmtKind::Semi(expr, ..) = &stmt.node;
// 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;
then {
lint(cx, expr.span, break_expr.span, LINT_BREAK);
}
}
},
// use `return` instead of `break`
ExprKind::Break(.., break_expr) => {
if let Some(break_expr) = break_expr {
Self::lint(cx, expr.span, break_expr.span, "change `break` to `return` as shown");
}
},
ExprKind::Match(.., arms, source) => {
let check_all_arms = match source {
MatchSource::IfLetDesugar {
contains_else_clause: has_else,
} => *has_else,
_ => true,
};
}
},
// use `return` instead of `break`
ExprKind::Break(.., break_expr) => {
if let Some(break_expr) = break_expr {
lint(cx, expr.span, break_expr.span, LINT_BREAK);
}
},
ExprKind::Match(.., arms, source) => {
let check_all_arms = match source {
MatchSource::IfLetDesugar {
contains_else_clause: has_else,
} => *has_else,
_ => true,
};

if check_all_arms {
for arm in arms {
Self::expr_match(cx, &arm.body);
}
} else {
Self::expr_match(cx, &arms.first().expect("if let doesn't have a single arm").body);
if check_all_arms {
for arm in arms {
expr_match(cx, &arm.body);
}
},
// skip if it already has a return statement
ExprKind::Ret(..) => (),
// everything else is missing `return`
_ => {
// make sure it's not just an unreachable expression
if is_expn_of(expr.span, "unreachable").is_none() {
Self::lint(cx, expr.span, expr.span, "add `return` as shown")
} else {
expr_match(cx, &arms.first().expect("if let doesn't have a single arm").body);
}
},
// skip if it already has a return statement
ExprKind::Ret(..) => (),
// make sure it's not a call that panics
ExprKind::Call(expr, ..) => {
if_chain! {
if let ExprKind::Path(qpath) = &expr.node;
if let Some(path_def_id) = resolve_node(cx, qpath, expr.hir_id).opt_def_id();
if match_def_path(cx, path_def_id, &BEGIN_PANIC) ||
match_def_path(cx, path_def_id, &BEGIN_PANIC_FMT);
then { }
else {
lint(cx, expr.span, expr.span, LINT_RETURN)
}
},
}
}
},
// everything else is missing `return`
_ => lint(cx, expr.span, expr.span, LINT_RETURN),
}
}

Expand All @@ -119,7 +147,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitReturn {
// checking return type through MIR, HIR is not able to determine inferred closure return types
// make sure it's not a macro
if !mir.return_ty().is_unit() && !in_macro_or_desugar(span) {
Self::expr_match(cx, &body.value);
expr_match(cx, &body.value);
}
}
}
10 changes: 10 additions & 0 deletions tests/ui/implicit_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ fn test_end_of_fn() -> bool {
// no error!
return true;
}

true
}

Expand Down Expand Up @@ -76,6 +77,14 @@ fn test_closure() {
let _ = || true;
}

fn test_panic() -> bool {
panic!()
}

fn test_return_macro() -> String {
format!("test {}", "test")
}

fn main() {
let _ = test_end_of_fn();
let _ = test_if_block();
Expand All @@ -86,4 +95,5 @@ fn main() {
let _ = test_loop_with_nests();
let _ = test_loop_with_if_let();
test_closure();
let _ = test_return_macro();
}
28 changes: 17 additions & 11 deletions tests/ui/implicit_return.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,64 +1,70 @@
error: missing return statement
--> $DIR/implicit_return.rs:8:5
--> $DIR/implicit_return.rs:9:5
|
LL | true
| ^^^^ help: add `return` as shown: `return true`
|
= note: `-D clippy::implicit-return` implied by `-D warnings`

error: missing return statement
--> $DIR/implicit_return.rs:14:9
--> $DIR/implicit_return.rs:15:9
|
LL | true
| ^^^^ help: add `return` as shown: `return true`

error: missing return statement
--> $DIR/implicit_return.rs:16:9
--> $DIR/implicit_return.rs:17:9
|
LL | false
| ^^^^^ help: add `return` as shown: `return false`

error: missing return statement
--> $DIR/implicit_return.rs:24:17
--> $DIR/implicit_return.rs:25:17
|
LL | true => false,
| ^^^^^ help: add `return` as shown: `return false`

error: missing return statement
--> $DIR/implicit_return.rs:25:20
--> $DIR/implicit_return.rs:26:20
|
LL | false => { true },
| ^^^^ help: add `return` as shown: `return true`

error: missing return statement
--> $DIR/implicit_return.rs:40:9
--> $DIR/implicit_return.rs:41:9
|
LL | break true;
| ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`

error: missing return statement
--> $DIR/implicit_return.rs:48:13
--> $DIR/implicit_return.rs:49:13
|
LL | break true;
| ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`

error: missing return statement
--> $DIR/implicit_return.rs:57:13
--> $DIR/implicit_return.rs:58:13
|
LL | break true;
| ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`

error: missing return statement
--> $DIR/implicit_return.rs:75:18
--> $DIR/implicit_return.rs:76:18
|
LL | let _ = || { true };
| ^^^^ help: add `return` as shown: `return true`

error: missing return statement
--> $DIR/implicit_return.rs:76:16
--> $DIR/implicit_return.rs:77:16
|
LL | let _ = || true;
| ^^^^ help: add `return` as shown: `return true`

error: aborting due to 10 previous errors
error: missing return statement
--> $DIR/implicit_return.rs:85:5
|
LL | format!("test {}", "test")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add `return` as shown: `return format!("test {}", "test")`

error: aborting due to 11 previous errors