Skip to content

Commit 6396a7a

Browse files
danieledapodanieledapo
committed
make needless_return work with void functions
1 parent 5a11ed7 commit 6396a7a

File tree

3 files changed

+108
-29
lines changed

3 files changed

+108
-29
lines changed

‎clippy_lints/src/returns.rs‎

Lines changed: 55 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,12 @@ declare_clippy_lint! {
8383
"needless unit expression"
8484
}
8585

86+
#[derive(PartialEq, Eq, Copy, Clone)]
87+
enum RetReplacement {
88+
Empty,
89+
Unit
90+
}
91+
8692
declare_lint_pass!(Return => [NEEDLESS_RETURN, LET_AND_RETURN, UNUSED_UNIT]);
8793

8894
impl Return {
@@ -91,21 +97,21 @@ impl Return {
9197
if let Some(stmt) = block.stmts.last() {
9298
match stmt.node {
9399
ast::StmtKind::Expr(ref expr) | ast::StmtKind::Semi(ref expr) => {
94-
self.check_final_expr(cx, expr, Some(stmt.span));
100+
self.check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty);
95101
},
96102
_ => (),
97103
}
98104
}
99105
}
100106

101107
// Check a the final expression in a block if it's a return.
102-
fn check_final_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr, span: Option<Span>) {
108+
fn check_final_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr, span: Option<Span>, replacement: RetReplacement) {
103109
match expr.node {
104110
// simple return is always "bad"
105-
ast::ExprKind::Ret(Some(ref inner)) => {
111+
ast::ExprKind::Ret(ref inner) => {
106112
// allow `#[cfg(a)] return a; #[cfg(b)] return b;`
107113
if !expr.attrs.iter().any(attr_is_cfg) {
108-
self.emit_return_lint(cx, span.expect("`else return` is not possible"), inner.span);
114+
self.emit_return_lint(cx, span.expect("`else return` is not possible"), inner.as_ref().map(|i| i.span), replacement);
109115
}
110116
},
111117
// a whole block? check it!
@@ -117,32 +123,61 @@ impl Return {
117123
// (except for unit type functions) so we don't match it
118124
ast::ExprKind::If(_, ref ifblock, Some(ref elsexpr)) => {
119125
self.check_block_return(cx, ifblock);
120-
self.check_final_expr(cx, elsexpr, None);
126+
self.check_final_expr(cx, elsexpr, None, RetReplacement::Empty);
121127
},
122128
// a match expr, check all arms
123129
ast::ExprKind::Match(_, ref arms) => {
124130
for arm in arms {
125-
self.check_final_expr(cx, &arm.body, Some(arm.body.span));
131+
self.check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Unit);
126132
}
127133
},
128134
_ => (),
129135
}
130136
}
131137

132-
fn emit_return_lint(&mut self, cx: &EarlyContext<'_>, ret_span: Span, inner_span: Span) {
133-
if in_external_macro(cx.sess(), inner_span) || in_macro_or_desugar(inner_span) {
134-
return;
135-
}
136-
span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
137-
if let Some(snippet) = snippet_opt(cx, inner_span) {
138-
db.span_suggestion(
139-
ret_span,
140-
"remove `return` as shown",
141-
snippet,
142-
Applicability::MachineApplicable,
143-
);
138+
fn emit_return_lint(&mut self, cx: &EarlyContext<'_>, ret_span: Span, inner_span: Option<Span>, replacement: RetReplacement) {
139+
match inner_span {
140+
Some(inner_span) => {
141+
if in_external_macro(cx.sess(), inner_span) || in_macro_or_desugar(inner_span) {
142+
return;
143+
}
144+
145+
span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
146+
if let Some(snippet) = snippet_opt(cx, inner_span) {
147+
db.span_suggestion(
148+
ret_span,
149+
"remove `return` as shown",
150+
snippet,
151+
Applicability::MachineApplicable,
152+
);
153+
}
154+
})
155+
},
156+
None => {
157+
match replacement {
158+
RetReplacement::Empty => {
159+
span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
160+
db.span_suggestion(
161+
ret_span,
162+
"remove `return`",
163+
String::new(),
164+
Applicability::MachineApplicable,
165+
);
166+
});
167+
}
168+
RetReplacement::Unit => {
169+
span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
170+
db.span_suggestion(
171+
ret_span,
172+
"replace `return` with the unit type `()`",
173+
"()".to_string(),
174+
Applicability::MachineApplicable,
175+
);
176+
});
177+
}
178+
}
144179
}
145-
});
180+
}
146181
}
147182

148183
// Check for "let x = EXPR; x"
@@ -195,7 +230,7 @@ impl EarlyLintPass for Return {
195230
fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, decl: &ast::FnDecl, span: Span, _: ast::NodeId) {
196231
match kind {
197232
FnKind::ItemFn(.., block) | FnKind::Method(.., block) => self.check_block_return(cx, block),
198-
FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span)),
233+
FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span), RetReplacement::Empty),
199234
}
200235
if_chain! {
201236
if let ast::FunctionRetTy::Ty(ref ty) = decl.output;

‎tests/ui/needless_return.rs‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#![warn(clippy::needless_return)]
22

3+
macro_rules! the_answer {
4+
() => (42)
5+
}
6+
37
fn test_end_of_fn() -> bool {
48
if true {
59
// no error!
@@ -36,6 +40,22 @@ fn test_closure() {
3640
let _ = || return true;
3741
}
3842

43+
fn test_macro_call() -> i32 {
44+
return the_answer!();
45+
}
46+
47+
fn test_void_fun() {
48+
return;
49+
}
50+
51+
fn test_void_if_fun(b: bool) {
52+
if b {
53+
return;
54+
} else {
55+
return;
56+
}
57+
}
58+
3959
fn main() {
4060
let _ = test_end_of_fn();
4161
let _ = test_no_semicolon();

‎tests/ui/needless_return.stderr‎

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,76 @@
11
error: unneeded return statement
2-
--> $DIR/needless_return.rs:8:5
2+
--> $DIR/needless_return.rs:12:5
33
|
44
LL | return true;
55
| ^^^^^^^^^^^^ help: remove `return` as shown: `true`
66
|
77
= note: `-D clippy::needless-return` implied by `-D warnings`
88

99
error: unneeded return statement
10-
--> $DIR/needless_return.rs:12:5
10+
--> $DIR/needless_return.rs:16:5
1111
|
1212
LL | return true;
1313
| ^^^^^^^^^^^^ help: remove `return` as shown: `true`
1414

1515
error: unneeded return statement
16-
--> $DIR/needless_return.rs:17:9
16+
--> $DIR/needless_return.rs:21:9
1717
|
1818
LL | return true;
1919
| ^^^^^^^^^^^^ help: remove `return` as shown: `true`
2020

2121
error: unneeded return statement
22-
--> $DIR/needless_return.rs:19:9
22+
--> $DIR/needless_return.rs:23:9
2323
|
2424
LL | return false;
2525
| ^^^^^^^^^^^^^ help: remove `return` as shown: `false`
2626

2727
error: unneeded return statement
28-
--> $DIR/needless_return.rs:25:17
28+
--> $DIR/needless_return.rs:29:17
2929
|
3030
LL | true => return false,
3131
| ^^^^^^^^^^^^ help: remove `return` as shown: `false`
3232

3333
error: unneeded return statement
34-
--> $DIR/needless_return.rs:27:13
34+
--> $DIR/needless_return.rs:31:13
3535
|
3636
LL | return true;
3737
| ^^^^^^^^^^^^ help: remove `return` as shown: `true`
3838

3939
error: unneeded return statement
40-
--> $DIR/needless_return.rs:34:9
40+
--> $DIR/needless_return.rs:38:9
4141
|
4242
LL | return true;
4343
| ^^^^^^^^^^^^ help: remove `return` as shown: `true`
4444

4545
error: unneeded return statement
46-
--> $DIR/needless_return.rs:36:16
46+
--> $DIR/needless_return.rs:40:16
4747
|
4848
LL | let _ = || return true;
4949
| ^^^^^^^^^^^ help: remove `return` as shown: `true`
5050

51-
error: aborting due to 8 previous errors
51+
error: unneeded return statement
52+
--> $DIR/needless_return.rs:44:5
53+
|
54+
LL | return the_answer!();
55+
| ^^^^^^^^^^^^^^^^^^^^^
56+
57+
error: unneeded return statement
58+
--> $DIR/needless_return.rs:48:5
59+
|
60+
LL | return;
61+
| ^^^^^^^ help: remove `return`
62+
63+
error: unneeded return statement
64+
--> $DIR/needless_return.rs:53:9
65+
|
66+
LL | return;
67+
| ^^^^^^^ help: remove `return`
68+
69+
error: unneeded return statement
70+
--> $DIR/needless_return.rs:55:9
71+
|
72+
LL | return;
73+
| ^^^^^^^ help: remove `return`
74+
75+
error: aborting due to 12 previous errors
5276

0 commit comments

Comments
 (0)