Skip to content

Commit edb76df

Browse files
JarchoJarcho
committed
Fix implicit_return with async functions.
Improve `implicit_return` suggestions when returning the result of a macro Check for `break` expressions inside a loop which are then implicitly returned. Allow all diverging functions in `implicit_return`, not just panic functions.
1 parent 6f2a6fe commit edb76df

File tree

7 files changed

+517
-117
lines changed

7 files changed

+517
-117
lines changed

‎clippy_lints/src/implicit_return.rs‎

Lines changed: 155 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
1-
use clippy_utils::diagnostics::span_lint_and_then;
2-
use clippy_utils::match_panic_def_id;
3-
use clippy_utils::source::snippet_opt;
4-
use if_chain::if_chain;
1+
use clippy_utils::{
2+
diagnostics::span_lint_and_sugg,
3+
expr_sig, get_async_fn_body, is_async_fn,
4+
source::{snippet_with_applicability, snippet_with_context, walk_span_to_context},
5+
visitors::visit_break_exprs,
6+
ExprFnSig,
7+
};
58
use rustc_errors::Applicability;
69
use rustc_hir::intravisit::FnKind;
7-
use rustc_hir::{Body, Expr, ExprKind, FnDecl, HirId, MatchSource, StmtKind};
8-
use rustc_lint::{LateContext, LateLintPass};
10+
use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, FnRetTy, HirId};
11+
use rustc_lint::{LateContext, LateLintPass, LintContext};
12+
use rustc_middle::lint::in_external_macro;
913
use rustc_session::{declare_lint_pass, declare_tool_lint};
10-
use rustc_span::source_map::Span;
14+
use rustc_span::{Span, SyntaxContext};
1115

1216
declare_clippy_lint! {
1317
/// **What it does:** Checks for missing return statements at the end of a block.
@@ -39,109 +43,179 @@ declare_clippy_lint! {
3943

4044
declare_lint_pass!(ImplicitReturn => [IMPLICIT_RETURN]);
4145

42-
static LINT_BREAK: &str = "change `break` to `return` as shown";
43-
static LINT_RETURN: &str = "add `return` as shown";
44-
45-
fn lint(cx: &LateContext<'_>, outer_span: Span, inner_span: Span, msg: &str) {
46-
let outer_span = outer_span.source_callsite();
47-
let inner_span = inner_span.source_callsite();
48-
49-
span_lint_and_then(cx, IMPLICIT_RETURN, outer_span, "missing `return` statement", |diag| {
50-
if let Some(snippet) = snippet_opt(cx, inner_span) {
51-
diag.span_suggestion(
52-
outer_span,
53-
msg,
54-
format!("return {}", snippet),
55-
Applicability::MachineApplicable,
56-
);
57-
}
58-
});
46+
fn lint_return(cx: &LateContext<'_>, span: Span) {
47+
let mut app = Applicability::MachineApplicable;
48+
let snip = snippet_with_applicability(cx, span, "..", &mut app);
49+
span_lint_and_sugg(
50+
cx,
51+
IMPLICIT_RETURN,
52+
span,
53+
"missing `return` statement",
54+
"add `return` as shown",
55+
format!("return {}", snip),
56+
app,
57+
);
58+
}
59+
60+
fn lint_break(cx: &LateContext<'_>, break_span: Span, expr_span: Span) {
61+
let mut app = Applicability::MachineApplicable;
62+
let snip = snippet_with_context(cx, expr_span, break_span.ctxt(), "..", &mut app).0;
63+
span_lint_and_sugg(
64+
cx,
65+
IMPLICIT_RETURN,
66+
break_span,
67+
"missing `return` statement",
68+
"change `break` to `return` as shown",
69+
format!("return {}", snip),
70+
app,
71+
)
5972
}
6073

61-
fn expr_match(cx: &LateContext<'_>, expr: &Expr<'_>) {
74+
fn contains_implicit_return(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
6275
match expr.kind {
63-
// loops could be using `break` instead of `return`
64-
ExprKind::Block(block, ..) | ExprKind::Loop(block, ..) => {
65-
if let Some(expr) = &block.expr {
66-
expr_match(cx, expr);
67-
}
68-
// only needed in the case of `break` with `;` at the end
69-
else if let Some(stmt) = block.stmts.last() {
70-
if_chain! {
71-
if let StmtKind::Semi(expr, ..) = &stmt.kind;
72-
// make sure it's a break, otherwise we want to skip
73-
if let ExprKind::Break(.., Some(break_expr)) = &expr.kind;
74-
then {
75-
lint(cx, expr.span, break_expr.span, LINT_BREAK);
76-
}
76+
ExprKind::Block(
77+
Block {
78+
expr: Some(block_expr), ..
79+
},
80+
_,
81+
) => contains_implicit_return(cx, block_expr),
82+
83+
ExprKind::If(_, then_expr, Some(else_expr)) => {
84+
contains_implicit_return(cx, then_expr) || contains_implicit_return(cx, else_expr)
85+
},
86+
ExprKind::Match(_, arms, _) => arms.iter().any(|a| contains_implicit_return(cx, a.body)),
87+
ExprKind::Loop(block, ..) => {
88+
let mut break_found = false;
89+
visit_break_exprs(block, |_, dest, _| {
90+
if dest.target_id.ok() == Some(expr.hir_id) {
91+
break_found = true;
7792
}
78-
}
93+
});
94+
break_found
7995
},
80-
// use `return` instead of `break`
81-
ExprKind::Break(.., break_expr) => {
82-
if let Some(break_expr) = break_expr {
83-
lint(cx, expr.span, break_expr.span, LINT_BREAK);
84-
}
96+
97+
// Diverging function and method calls are fine.
98+
ExprKind::MethodCall(..)
99+
if cx
100+
.tcx
101+
.fn_sig(cx.typeck_results().type_dependent_def_id(expr.hir_id).unwrap())
102+
.skip_binder()
103+
.output()
104+
.is_never() =>
105+
{
106+
false
107+
},
108+
ExprKind::Call(func_expr, _)
109+
if expr_sig(cx.tcx, cx.typeck_results(), func_expr)
110+
.and_then(ExprFnSig::output)
111+
.map_or(false, |ty| ty.skip_binder().is_never()) =>
112+
{
113+
false
85114
},
86-
ExprKind::If(.., if_expr, else_expr) => {
87-
expr_match(cx, if_expr);
88115

89-
if let Some(else_expr) = else_expr {
90-
expr_match(cx, else_expr);
116+
// If expressions without an else clause, and blocks without a final expression can only be the final expression
117+
// if they are divergent, or return the unit type.
118+
ExprKind::If(_, _, None) | ExprKind::Block(Block { expr: None, .. }, _) | ExprKind::Ret(_) => false,
119+
120+
// everything else is missing `return`
121+
_ => true,
122+
}
123+
}
124+
125+
fn lint_implicit_returns(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, ctxt: SyntaxContext) {
126+
match expr.kind {
127+
ExprKind::Block(
128+
Block {
129+
expr: Some(block_expr), ..
130+
},
131+
_,
132+
) => {
133+
if ctxt == block_expr.span.ctxt() {
134+
lint_implicit_returns(cx, block_expr, ctxt)
135+
} else if contains_implicit_return(cx, block_expr) {
136+
lint_return(
137+
cx,
138+
walk_span_to_context(block_expr.span, ctxt).unwrap_or(block_expr.span),
139+
);
91140
}
92141
},
93-
ExprKind::Match(.., arms, source) => {
94-
let check_all_arms = match source {
95-
MatchSource::IfLetDesugar {
96-
contains_else_clause: has_else,
97-
} => has_else,
98-
_ => true,
99-
};
100-
101-
if check_all_arms {
102-
for arm in arms {
103-
expr_match(cx, &arm.body);
142+
143+
ExprKind::If(_, then_expr, Some(else_expr)) => {
144+
lint_implicit_returns(cx, then_expr, ctxt);
145+
lint_implicit_returns(cx, else_expr, ctxt);
146+
},
147+
148+
ExprKind::Match(_, arms, _) => {
149+
for arm in arms {
150+
if ctxt == arm.body.span.ctxt() {
151+
lint_implicit_returns(cx, arm.body, ctxt);
152+
} else if contains_implicit_return(cx, arm.body) {
153+
lint_return(cx, walk_span_to_context(expr.span, ctxt).unwrap_or(expr.span));
104154
}
105-
} else {
106-
expr_match(cx, &arms.first().expect("`if let` doesn't have a single arm").body);
107155
}
108156
},
109-
// skip if it already has a return statement
110-
ExprKind::Ret(..) => (),
111-
// make sure it's not a call that panics
112-
ExprKind::Call(expr, ..) => {
113-
if_chain! {
114-
if let ExprKind::Path(qpath) = &expr.kind;
115-
if let Some(path_def_id) = cx.qpath_res(qpath, expr.hir_id).opt_def_id();
116-
if match_panic_def_id(cx, path_def_id);
117-
then { }
118-
else {
119-
lint(cx, expr.span, expr.span, LINT_RETURN)
157+
ExprKind::Loop(block, ..) => {
158+
let mut add_return = false;
159+
visit_break_exprs(block, |break_expr, dest, sub_expr| {
160+
if dest.target_id.ok() == Some(expr.hir_id) {
161+
if break_expr.span.ctxt() == ctxt {
162+
lint_break(cx, break_expr.span, sub_expr.unwrap().span);
163+
} else {
164+
add_return = true;
165+
}
120166
}
167+
});
168+
if add_return {
169+
lint_return(cx, expr.span);
121170
}
122171
},
172+
173+
// Diverging function and method calls are fine.
174+
ExprKind::MethodCall(..)
175+
if cx
176+
.tcx
177+
.fn_sig(cx.typeck_results().type_dependent_def_id(expr.hir_id).unwrap())
178+
.skip_binder()
179+
.output()
180+
.is_never() => {},
181+
ExprKind::Call(func_expr, _)
182+
if expr_sig(cx.tcx, cx.typeck_results(), func_expr)
183+
.and_then(ExprFnSig::output)
184+
.map_or(false, |ty| ty.skip_binder().is_never()) => {},
185+
186+
// If expressions without an else clause, and blocks without a final expression can only be the final expression
187+
// if they are divergent, or return the unit type.
188+
ExprKind::If(_, _, None) | ExprKind::Block(Block { expr: None, .. }, _) | ExprKind::Ret(_) => (),
189+
123190
// everything else is missing `return`
124-
_ => lint(cx, expr.span, expr.span, LINT_RETURN),
191+
_ => lint_return(cx, walk_span_to_context(expr.span, ctxt).unwrap_or(expr.span)),
125192
}
126193
}
127194

128195
impl<'tcx> LateLintPass<'tcx> for ImplicitReturn {
129196
fn check_fn(
130197
&mut self,
131198
cx: &LateContext<'tcx>,
132-
_: FnKind<'tcx>,
133-
_: &'tcx FnDecl<'_>,
199+
kind: FnKind<'tcx>,
200+
decl: &'tcx FnDecl<'_>,
134201
body: &'tcx Body<'_>,
135202
span: Span,
136203
_: HirId,
137204
) {
138-
if span.from_expansion() {
205+
if (!matches!(kind, FnKind::Closure) && matches!(decl.output, FnRetTy::DefaultReturn(_)))
206+
|| span.ctxt() != body.value.span.ctxt()
207+
|| in_external_macro(cx.sess(), span)
208+
|| cx.typeck_results().expr_ty(&body.value).is_unit()
209+
{
139210
return;
140211
}
141-
let body = cx.tcx.hir().body(body.id());
142-
if cx.typeck_results().expr_ty(&body.value).is_unit() {
143-
return;
212+
213+
if is_async_fn(kind) {
214+
if let Some(expr) = get_async_fn_body(cx.tcx, body) {
215+
lint_implicit_returns(cx, expr, expr.span.ctxt());
216+
}
217+
} else {
218+
lint_implicit_returns(cx, &body.value, body.value.span.ctxt());
144219
}
145-
expr_match(cx, &body.value);
146220
}
147221
}

0 commit comments

Comments
 (0)