Skip to content

Commit bfbc083

Browse files
pag4kpag4k
committed
Fix for issue 6640
1 parent c5f3f9d commit bfbc083

File tree

3 files changed

+93
-136
lines changed

3 files changed

+93
-136
lines changed

‎clippy_lints/src/unnecessary_wraps.rs

Lines changed: 59 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::utils::{
2-
contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_then,
3-
visitors::find_all_ret_expressions,
2+
contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_sugg,
3+
span_lint_and_then, visitors::find_all_ret_expressions,
44
};
55
use if_chain::if_chain;
66
use rustc_errors::Applicability;
@@ -64,6 +64,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
6464
span: Span,
6565
hir_id: HirId,
6666
) {
67+
// Abort if public function/method or closure.
6768
match fn_kind {
6869
FnKind::ItemFn(.., visibility, _) | FnKind::Method(.., Some(visibility), _) => {
6970
if visibility.node.is_pub() {
@@ -74,6 +75,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
7475
_ => (),
7576
}
7677

78+
// Abort if the method is implementing a trait or of it a trait method.
7779
if let Some(Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(hir_id)) {
7880
if matches!(
7981
item.kind,
@@ -83,22 +85,43 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
8385
}
8486
}
8587

86-
let (return_type, path) = if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::option_type) {
88+
// Check if return type is Option or Result. If neither, abort.
89+
let return_ty = return_ty(cx, hir_id);
90+
let (return_type_label, path) = if is_type_diagnostic_item(cx, return_ty, sym::option_type) {
8791
("Option", &paths::OPTION_SOME)
88-
} else if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::result_type) {
92+
} else if is_type_diagnostic_item(cx, return_ty, sym::result_type) {
8993
("Result", &paths::RESULT_OK)
9094
} else {
9195
return;
9296
};
9397

98+
// Take the first inner type of the Option or Result. If can't, abort.
99+
let inner_ty = if_chain! {
100+
// Skip Option or Result and take the first outermost inner type.
101+
if let Some(inner_ty) = return_ty.walk().nth(1);
102+
if let GenericArgKind::Type(inner_ty) = inner_ty.unpack();
103+
then {
104+
inner_ty
105+
} else {
106+
return;
107+
}
108+
};
109+
110+
// Check if all return expression respect the following condition and collect them.
94111
let mut suggs = Vec::new();
95112
let can_sugg = find_all_ret_expressions(cx, &body.value, |ret_expr| {
96113
if_chain! {
114+
// Abort if in macro.
97115
if !in_macro(ret_expr.span);
116+
// Check if a function call.
98117
if let ExprKind::Call(ref func, ref args) = ret_expr.kind;
118+
// Get the Path of the function call.
99119
if let ExprKind::Path(ref qpath) = func.kind;
120+
// Check if OPTION_SOME or RESULT_OK, depending on return type.
100121
if match_qpath(qpath, path);
122+
// Make sure the function call has only one argument.
101123
if args.len() == 1;
124+
// Make sure the function argument does not contain a return expression.
102125
if !contains_return(&args[0]);
103126
then {
104127
suggs.push((ret_expr.span, snippet(cx, args[0].span.source_callsite(), "..").to_string()));
@@ -110,39 +133,42 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
110133
});
111134

112135
if can_sugg && !suggs.is_empty() {
113-
span_lint_and_then(
114-
cx,
115-
UNNECESSARY_WRAPS,
116-
span,
117-
format!(
118-
"this function's return value is unnecessarily wrapped by `{}`",
119-
return_type
120-
)
121-
.as_str(),
122-
|diag| {
123-
let inner_ty = return_ty(cx, hir_id)
124-
.walk()
125-
.skip(1) // skip `std::option::Option` or `std::result::Result`
126-
.take(1) // take the first outermost inner type
127-
.filter_map(|inner| match inner.unpack() {
128-
GenericArgKind::Type(inner_ty) => Some(inner_ty.to_string()),
129-
_ => None,
130-
});
131-
inner_ty.for_each(|inner_ty| {
136+
// Issue 6640: If the inner type is Unit, emit lint similar to clippy::unused_unit.
137+
if inner_ty.is_unit() {
138+
span_lint_and_sugg(
139+
cx,
140+
UNNECESSARY_WRAPS,
141+
fn_decl.output.span(),
142+
"unneeded wrapped unit return type",
143+
format!("remove the `-> {}<()>`", return_type_label).as_str(),
144+
String::new(),
145+
Applicability::MaybeIncorrect,
146+
);
147+
} else {
148+
span_lint_and_then(
149+
cx,
150+
UNNECESSARY_WRAPS,
151+
span,
152+
format!(
153+
"this function's return value is unnecessarily wrapped by `{}`",
154+
return_type_label
155+
)
156+
.as_str(),
157+
|diag| {
132158
diag.span_suggestion(
133159
fn_decl.output.span(),
134-
format!("remove `{}` from the return type...", return_type).as_str(),
135-
inner_ty,
160+
format!("remove `{}` from the return type...", return_type_label).as_str(),
161+
inner_ty.to_string(),
136162
Applicability::MaybeIncorrect,
137163
);
138-
});
139-
diag.multipart_suggestion(
140-
"...and change the returning expressions",
141-
suggs,
142-
Applicability::MaybeIncorrect,
143-
);
144-
},
145-
);
164+
diag.multipart_suggestion(
165+
"...and change the returning expressions",
166+
suggs,
167+
Applicability::MaybeIncorrect,
168+
);
169+
},
170+
);
171+
}
146172
}
147173
}
148174
}

‎tests/ui/unnecessary_wraps.rs

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,36 @@ fn issue_6384(s: &str) -> Option<&str> {
116116
})
117117
}
118118

119+
// should be linted
120+
fn issue_6640_1(a: bool, b: bool) -> Option<()> {
121+
if a && b {
122+
return Some(());
123+
}
124+
if a {
125+
Some(());
126+
Some(())
127+
} else {
128+
return Some(());
129+
}
130+
}
131+
132+
// should be linted
133+
fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
134+
if a && b {
135+
return Ok(());
136+
}
137+
if a {
138+
Ok(());
139+
Ok(())
140+
} else {
141+
return Ok(());
142+
}
143+
}
144+
119145
fn main() {
120146
// method calls are not linted
121147
func1(true, true);
122148
func2(true, true);
149+
issue_6640_1(true, true);
150+
issue_6640_2(true, true);
123151
}

‎tests/ui/unnecessary_wraps.stderr

Lines changed: 6 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1,106 +1,9 @@
1-
error: this function's return value is unnecessarily wrapped by `Option`
2-
--> $DIR/unnecessary_wraps.rs:8:1
3-
|
4-
LL | / fn func1(a: bool, b: bool) -> Option<i32> {
5-
LL | | if a && b {
6-
LL | | return Some(42);
7-
LL | | }
8-
... |
9-
LL | | }
10-
LL | | }
11-
| |_^
12-
|
13-
= note: `-D clippy::unnecessary-wraps` implied by `-D warnings`
14-
help: remove `Option` from the return type...
15-
|
16-
LL | fn func1(a: bool, b: bool) -> i32 {
17-
| ^^^
18-
help: ...and change the returning expressions
19-
|
20-
LL | return 42;
21-
LL | }
22-
LL | if a {
23-
LL | Some(-1);
24-
LL | 2
25-
LL | } else {
26-
...
27-
28-
error: this function's return value is unnecessarily wrapped by `Option`
29-
--> $DIR/unnecessary_wraps.rs:21:1
30-
|
31-
LL | / fn func2(a: bool, b: bool) -> Option<i32> {
32-
LL | | if a && b {
33-
LL | | return Some(10);
34-
LL | | }
35-
... |
36-
LL | | }
37-
LL | | }
38-
| |_^
39-
|
40-
help: remove `Option` from the return type...
41-
|
42-
LL | fn func2(a: bool, b: bool) -> i32 {
43-
| ^^^
44-
help: ...and change the returning expressions
45-
|
46-
LL | return 10;
47-
LL | }
48-
LL | if a {
49-
LL | 20
50-
LL | } else {
51-
LL | 30
52-
|
53-
54-
error: this function's return value is unnecessarily wrapped by `Option`
55-
--> $DIR/unnecessary_wraps.rs:51:1
56-
|
57-
LL | / fn func5() -> Option<i32> {
58-
LL | | Some(1)
59-
LL | | }
60-
| |_^
61-
|
62-
help: remove `Option` from the return type...
63-
|
64-
LL | fn func5() -> i32 {
65-
| ^^^
66-
help: ...and change the returning expressions
67-
|
68-
LL | 1
69-
|
70-
71-
error: this function's return value is unnecessarily wrapped by `Result`
72-
--> $DIR/unnecessary_wraps.rs:61:1
73-
|
74-
LL | / fn func7() -> Result<i32, ()> {
75-
LL | | Ok(1)
76-
LL | | }
77-
| |_^
78-
|
79-
help: remove `Result` from the return type...
80-
|
81-
LL | fn func7() -> i32 {
82-
| ^^^
83-
help: ...and change the returning expressions
84-
|
85-
LL | 1
86-
|
87-
88-
error: this function's return value is unnecessarily wrapped by `Option`
89-
--> $DIR/unnecessary_wraps.rs:93:5
90-
|
91-
LL | / fn func12() -> Option<i32> {
92-
LL | | Some(1)
93-
LL | | }
94-
| |_____^
95-
|
96-
help: remove `Option` from the return type...
97-
|
98-
LL | fn func12() -> i32 {
99-
| ^^^
100-
help: ...and change the returning expressions
101-
|
102-
LL | 1
1+
error[E0282]: type annotations needed
2+
--> $DIR/unnecessary_wraps.rs:138:9
1033
|
4+
LL | Ok(());
5+
| ^^ cannot infer type for type parameter `E` declared on the enum `Result`
1046

105-
error: aborting due to 5 previous errors
7+
error: aborting due to previous error
1068

9+
For more information about this error, try `rustc --explain E0282`.

0 commit comments

Comments
 (0)