Skip to content

Commit c5d1ecd

Browse files
borsbors
committed
Auto merge of #4220 - d-dorazio:4182-needless-return-void-functions, r=flip1995
make needless_return work with void functions fixes #4181. changelog: make needless_return work with void functions. I don't think the failure is related to my changes, but I'm not sure 🤔
2 parents 6f82ea5 + 316a9f2 commit c5d1ecd

File tree

7 files changed

+143
-49
lines changed

7 files changed

+143
-49
lines changed

‎clippy_lints/src/invalid_ref.rs

-1
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,5 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidRef {
5151
span_help_and_lint(cx, INVALID_REF, expr.span, msg, HELP);
5252
}
5353
}
54-
return;
5554
}
5655
}

‎clippy_lints/src/methods/mod.rs

+5-6
Original file line numberDiff line numberDiff line change
@@ -1648,16 +1648,15 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args:
16481648
);
16491649

16501650
// Check if the first argument to .fold is a suitable literal
1651-
match fold_args[1].node {
1652-
hir::ExprKind::Lit(ref lit) => match lit.node {
1651+
if let hir::ExprKind::Lit(ref lit) = fold_args[1].node {
1652+
match lit.node {
16531653
ast::LitKind::Bool(false) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Or, "any", true),
16541654
ast::LitKind::Bool(true) => check_fold_with_op(cx, fold_args, hir::BinOpKind::And, "all", true),
16551655
ast::LitKind::Int(0, _) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Add, "sum", false),
16561656
ast::LitKind::Int(1, _) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Mul, "product", false),
1657-
_ => return,
1658-
},
1659-
_ => return,
1660-
};
1657+
_ => (),
1658+
}
1659+
}
16611660
}
16621661

16631662
fn lint_iter_nth<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_args: &'tcx [hir::Expr], is_mut: bool) {

‎clippy_lints/src/non_expressive_names.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,9 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> {
169169
.any(|id| id.name == ident.name)
170170
{
171171
return;
172-
} else if let Some(scope) = &mut self.0.single_char_names.last_mut() {
172+
}
173+
174+
if let Some(scope) = &mut self.0.single_char_names.last_mut() {
173175
scope.push(ident);
174176
}
175177
}

‎clippy_lints/src/redundant_pattern_matching.rs

-4
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,6 @@ fn find_sugg_for_if_let<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr,
9595
);
9696
},
9797
);
98-
} else {
99-
return;
10098
}
10199
}
102100

@@ -161,8 +159,6 @@ fn find_sugg_for_match<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, o
161159
},
162160
);
163161
}
164-
} else {
165-
return;
166162
}
167163
}
168164

‎clippy_lints/src/returns.rs

+65-20
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,32 @@ 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(
109+
&mut self,
110+
cx: &EarlyContext<'_>,
111+
expr: &ast::Expr,
112+
span: Option<Span>,
113+
replacement: RetReplacement,
114+
) {
103115
match expr.node {
104116
// simple return is always "bad"
105-
ast::ExprKind::Ret(Some(ref inner)) => {
117+
ast::ExprKind::Ret(ref inner) => {
106118
// allow `#[cfg(a)] return a; #[cfg(b)] return b;`
107119
if !expr.attrs.iter().any(attr_is_cfg) {
108-
self.emit_return_lint(cx, span.expect("`else return` is not possible"), inner.span);
120+
self.emit_return_lint(
121+
cx,
122+
span.expect("`else return` is not possible"),
123+
inner.as_ref().map(|i| i.span),
124+
replacement,
125+
);
109126
}
110127
},
111128
// a whole block? check it!
@@ -117,32 +134,60 @@ impl Return {
117134
// (except for unit type functions) so we don't match it
118135
ast::ExprKind::If(_, ref ifblock, Some(ref elsexpr)) => {
119136
self.check_block_return(cx, ifblock);
120-
self.check_final_expr(cx, elsexpr, None);
137+
self.check_final_expr(cx, elsexpr, None, RetReplacement::Empty);
121138
},
122139
// a match expr, check all arms
123140
ast::ExprKind::Match(_, ref arms) => {
124141
for arm in arms {
125-
self.check_final_expr(cx, &arm.body, Some(arm.body.span));
142+
self.check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Unit);
126143
}
127144
},
128145
_ => (),
129146
}
130147
}
131148

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;
149+
fn emit_return_lint(
150+
&mut self,
151+
cx: &EarlyContext<'_>,
152+
ret_span: Span,
153+
inner_span: Option<Span>,
154+
replacement: RetReplacement,
155+
) {
156+
match inner_span {
157+
Some(inner_span) => {
158+
if in_external_macro(cx.sess(), inner_span) || in_macro_or_desugar(inner_span) {
159+
return;
160+
}
161+
162+
span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
163+
if let Some(snippet) = snippet_opt(cx, inner_span) {
164+
db.span_suggestion(ret_span, "remove `return`", snippet, Applicability::MachineApplicable);
165+
}
166+
})
167+
},
168+
None => match replacement {
169+
RetReplacement::Empty => {
170+
span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
171+
db.span_suggestion(
172+
ret_span,
173+
"remove `return`",
174+
String::new(),
175+
Applicability::MachineApplicable,
176+
);
177+
});
178+
},
179+
RetReplacement::Unit => {
180+
span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
181+
db.span_suggestion(
182+
ret_span,
183+
"replace `return` with the unit type",
184+
"()".to_string(),
185+
Applicability::MachineApplicable,
186+
);
187+
});
188+
},
189+
},
135190
}
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-
);
144-
}
145-
});
146191
}
147192

148193
// Check for "let x = EXPR; x"
@@ -195,7 +240,7 @@ impl EarlyLintPass for Return {
195240
fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, decl: &ast::FnDecl, span: Span, _: ast::NodeId) {
196241
match kind {
197242
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)),
243+
FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span), RetReplacement::Empty),
199244
}
200245
if_chain! {
201246
if let ast::FunctionRetTy::Ty(ref ty) = decl.output;

‎tests/ui/needless_return.rs

+29
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
#![warn(clippy::needless_return)]
22

3+
macro_rules! the_answer {
4+
() => {
5+
42
6+
};
7+
}
8+
39
fn test_end_of_fn() -> bool {
410
if true {
511
// no error!
@@ -36,6 +42,29 @@ fn test_closure() {
3642
let _ = || return true;
3743
}
3844

45+
fn test_macro_call() -> i32 {
46+
return the_answer!();
47+
}
48+
49+
fn test_void_fun() {
50+
return;
51+
}
52+
53+
fn test_void_if_fun(b: bool) {
54+
if b {
55+
return;
56+
} else {
57+
return;
58+
}
59+
}
60+
61+
fn test_void_match(x: u32) {
62+
match x {
63+
0 => (),
64+
_ => return,
65+
}
66+
}
67+
3968
fn main() {
4069
let _ = test_end_of_fn();
4170
let _ = test_no_semicolon();

‎tests/ui/needless_return.stderr

+41-17
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:14:5
33
|
44
LL | return true;
5-
| ^^^^^^^^^^^^ help: remove `return` as shown: `true`
5+
| ^^^^^^^^^^^^ help: remove `return`: `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:18:5
1111
|
1212
LL | return true;
13-
| ^^^^^^^^^^^^ help: remove `return` as shown: `true`
13+
| ^^^^^^^^^^^^ help: remove `return`: `true`
1414

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

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

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

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

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

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

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

0 commit comments

Comments
 (0)