Skip to content

Commit 0f70e88

Browse files
borsbors
committed
Auto merge of rust-lang#6665 - pag4k:unnecessary_wraps_bug_6640, r=camsteffen
Fix for issue 6640 *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: unnecessary_wraps will now suggest to remove unnecessary wrapped return unit type, like Option<()> fixes rust-lang#6640
2 parents 2f815ec + a78271b commit 0f70e88

File tree

3 files changed

+158
-45
lines changed

3 files changed

+158
-45
lines changed

‎clippy_lints/src/unnecessary_wraps.rs

Lines changed: 55 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
use crate::utils::{
2-
contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_then,
2+
contains_return, in_macro, match_qpath, paths, return_ty, snippet, span_lint_and_then,
33
visitors::find_all_ret_expressions,
44
};
55
use if_chain::if_chain;
66
use rustc_errors::Applicability;
77
use rustc_hir::intravisit::FnKind;
88
use rustc_hir::{Body, ExprKind, FnDecl, HirId, Impl, ItemKind, Node};
99
use rustc_lint::{LateContext, LateLintPass};
10-
use rustc_middle::ty::subst::GenericArgKind;
10+
use rustc_middle::ty;
1111
use rustc_session::{declare_lint_pass, declare_tool_lint};
1212
use rustc_span::symbol::sym;
1313
use rustc_span::Span;
@@ -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,25 +85,44 @@ 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) {
87-
("Option", &paths::OPTION_SOME)
88-
} else if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::result_type) {
89-
("Result", &paths::RESULT_OK)
88+
// Get the wrapper and inner types, if can't, abort.
89+
let (return_type_label, path, inner_type) = if let ty::Adt(adt_def, subst) = return_ty(cx, hir_id).kind() {
90+
if cx.tcx.is_diagnostic_item(sym::option_type, adt_def.did) {
91+
("Option", &paths::OPTION_SOME, subst.type_at(0))
92+
} else if cx.tcx.is_diagnostic_item(sym::result_type, adt_def.did) {
93+
("Result", &paths::RESULT_OK, subst.type_at(0))
94+
} else {
95+
return;
96+
}
9097
} else {
9198
return;
9299
};
93100

101+
// Check if all return expression respect the following condition and collect them.
94102
let mut suggs = Vec::new();
95103
let can_sugg = find_all_ret_expressions(cx, &body.value, |ret_expr| {
96104
if_chain! {
97105
if !in_macro(ret_expr.span);
106+
// Check if a function call.
98107
if let ExprKind::Call(ref func, ref args) = ret_expr.kind;
108+
// Get the Path of the function call.
99109
if let ExprKind::Path(ref qpath) = func.kind;
110+
// Check if OPTION_SOME or RESULT_OK, depending on return type.
100111
if match_qpath(qpath, path);
101112
if args.len() == 1;
113+
// Make sure the function argument does not contain a return expression.
102114
if !contains_return(&args[0]);
103115
then {
104-
suggs.push((ret_expr.span, snippet(cx, args[0].span.source_callsite(), "..").to_string()));
116+
suggs.push(
117+
(
118+
ret_expr.span,
119+
if inner_type.is_unit() {
120+
"".to_string()
121+
} else {
122+
snippet(cx, args[0].span.source_callsite(), "..").to_string()
123+
}
124+
)
125+
);
105126
true
106127
} else {
107128
false
@@ -110,39 +131,34 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
110131
});
111132

112133
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
134+
let (lint_msg, return_type_sugg_msg, return_type_sugg, body_sugg_msg) = if inner_type.is_unit() {
135+
(
136+
"this function's return value is unnecessary".to_string(),
137+
"remove the return type...".to_string(),
138+
snippet(cx, fn_decl.output.span(), "..").to_string(),
139+
"...and then remove returned values",
120140
)
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| {
132-
diag.span_suggestion(
133-
fn_decl.output.span(),
134-
format!("remove `{}` from the return type...", return_type).as_str(),
135-
inner_ty,
136-
Applicability::MaybeIncorrect,
137-
);
138-
});
139-
diag.multipart_suggestion(
140-
"...and change the returning expressions",
141-
suggs,
142-
Applicability::MaybeIncorrect,
143-
);
144-
},
145-
);
141+
} else {
142+
(
143+
format!(
144+
"this function's return value is unnecessarily wrapped by `{}`",
145+
return_type_label
146+
),
147+
format!("remove `{}` from the return type...", return_type_label),
148+
inner_type.to_string(),
149+
"...and then change returning expressions",
150+
)
151+
};
152+
153+
span_lint_and_then(cx, UNNECESSARY_WRAPS, span, lint_msg.as_str(), |diag| {
154+
diag.span_suggestion(
155+
fn_decl.output.span(),
156+
return_type_sugg_msg.as_str(),
157+
return_type_sugg,
158+
Applicability::MaybeIncorrect,
159+
);
160+
diag.multipart_suggestion(body_sugg_msg, suggs, Applicability::MaybeIncorrect);
161+
});
146162
}
147163
}
148164
}

‎tests/ui/unnecessary_wraps.rs

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,53 @@ 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+
} else {
140+
return Ok(());
141+
}
142+
}
143+
144+
// should not be linted
145+
fn issue_6640_3() -> Option<()> {
146+
if true {
147+
Some(())
148+
} else {
149+
None
150+
}
151+
}
152+
153+
// should not be linted
154+
fn issue_6640_4() -> Result<(), ()> {
155+
if true {
156+
Ok(())
157+
} else {
158+
Err(())
159+
}
160+
}
161+
119162
fn main() {
120163
// method calls are not linted
121164
func1(true, true);
122165
func2(true, true);
166+
issue_6640_1(true, true);
167+
issue_6640_2(true, true);
123168
}

‎tests/ui/unnecessary_wraps.stderr

Lines changed: 58 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ help: remove `Option` from the return type...
1515
|
1616
LL | fn func1(a: bool, b: bool) -> i32 {
1717
| ^^^
18-
help: ...and change the returning expressions
18+
help: ...and then change returning expressions
1919
|
2020
LL | return 42;
2121
LL | }
@@ -41,7 +41,7 @@ help: remove `Option` from the return type...
4141
|
4242
LL | fn func2(a: bool, b: bool) -> i32 {
4343
| ^^^
44-
help: ...and change the returning expressions
44+
help: ...and then change returning expressions
4545
|
4646
LL | return 10;
4747
LL | }
@@ -63,7 +63,7 @@ help: remove `Option` from the return type...
6363
|
6464
LL | fn func5() -> i32 {
6565
| ^^^
66-
help: ...and change the returning expressions
66+
help: ...and then change returning expressions
6767
|
6868
LL | 1
6969
|
@@ -80,7 +80,7 @@ help: remove `Result` from the return type...
8080
|
8181
LL | fn func7() -> i32 {
8282
| ^^^
83-
help: ...and change the returning expressions
83+
help: ...and then change returning expressions
8484
|
8585
LL | 1
8686
|
@@ -97,10 +97,62 @@ help: remove `Option` from the return type...
9797
|
9898
LL | fn func12() -> i32 {
9999
| ^^^
100-
help: ...and change the returning expressions
100+
help: ...and then change returning expressions
101101
|
102102
LL | 1
103103
|
104104

105-
error: aborting due to 5 previous errors
105+
error: this function's return value is unnecessary
106+
--> $DIR/unnecessary_wraps.rs:120:1
107+
|
108+
LL | / fn issue_6640_1(a: bool, b: bool) -> Option<()> {
109+
LL | | if a && b {
110+
LL | | return Some(());
111+
LL | | }
112+
... |
113+
LL | | }
114+
LL | | }
115+
| |_^
116+
|
117+
help: remove the return type...
118+
|
119+
LL | fn issue_6640_1(a: bool, b: bool) -> Option<()> {
120+
| ^^^^^^^^^^
121+
help: ...and then remove returned values
122+
|
123+
LL | return ;
124+
LL | }
125+
LL | if a {
126+
LL | Some(());
127+
LL |
128+
LL | } else {
129+
...
130+
131+
error: this function's return value is unnecessary
132+
--> $DIR/unnecessary_wraps.rs:133:1
133+
|
134+
LL | / fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
135+
LL | | if a && b {
136+
LL | | return Ok(());
137+
LL | | }
138+
... |
139+
LL | | }
140+
LL | | }
141+
| |_^
142+
|
143+
help: remove the return type...
144+
|
145+
LL | fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
146+
| ^^^^^^^^^^^^^^^
147+
help: ...and then remove returned values
148+
|
149+
LL | return ;
150+
LL | }
151+
LL | if a {
152+
LL |
153+
LL | } else {
154+
LL | return ;
155+
|
156+
157+
error: aborting due to 7 previous errors
106158

0 commit comments

Comments
 (0)