Skip to content

Commit

Permalink
Fix for issue 6640
Browse files Browse the repository at this point in the history
  • Loading branch information
@pag4k
pag4k committed Feb 2, 2021
1 parent c5f3f9d commit bfbc083
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 136 deletions.
92 changes: 59 additions & 33 deletions clippy_lints/src/unnecessary_wraps.rs
View file
@@ -1,6 +1,6 @@
use crate::utils::{
contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_then,
visitors::find_all_ret_expressions,
contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_sugg,
span_lint_and_then, visitors::find_all_ret_expressions,
};
use if_chain::if_chain;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -64,6 +64,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
span: Span,
hir_id: HirId,
) {
// Abort if public function/method or closure.
match fn_kind {
FnKind::ItemFn(.., visibility, _) | FnKind::Method(.., Some(visibility), _) => {
if visibility.node.is_pub() {
Expand All @@ -74,6 +75,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
_ => (),
}

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

let (return_type, path) = if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::option_type) {
// Check if return type is Option or Result. If neither, abort.
let return_ty = return_ty(cx, hir_id);
let (return_type_label, path) = if is_type_diagnostic_item(cx, return_ty, sym::option_type) {
("Option", &paths::OPTION_SOME)
} else if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::result_type) {
} else if is_type_diagnostic_item(cx, return_ty, sym::result_type) {
("Result", &paths::RESULT_OK)
} else {
return;
};

// Take the first inner type of the Option or Result. If can't, abort.
let inner_ty = if_chain! {
// Skip Option or Result and take the first outermost inner type.
if let Some(inner_ty) = return_ty.walk().nth(1);
if let GenericArgKind::Type(inner_ty) = inner_ty.unpack();
then {
inner_ty
} else {
return;
}
};

// Check if all return expression respect the following condition and collect them.
let mut suggs = Vec::new();
let can_sugg = find_all_ret_expressions(cx, &body.value, |ret_expr| {
if_chain! {
// Abort if in macro.
if !in_macro(ret_expr.span);
// Check if a function call.
if let ExprKind::Call(ref func, ref args) = ret_expr.kind;
// Get the Path of the function call.
if let ExprKind::Path(ref qpath) = func.kind;
// Check if OPTION_SOME or RESULT_OK, depending on return type.
if match_qpath(qpath, path);
// Make sure the function call has only one argument.
if args.len() == 1;
// Make sure the function argument does not contain a return expression.
if !contains_return(&args[0]);
then {
suggs.push((ret_expr.span, snippet(cx, args[0].span.source_callsite(), "..").to_string()));
Expand All @@ -110,39 +133,42 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
});

if can_sugg && !suggs.is_empty() {
span_lint_and_then(
cx,
UNNECESSARY_WRAPS,
span,
format!(
"this function's return value is unnecessarily wrapped by `{}`",
return_type
)
.as_str(),
|diag| {
let inner_ty = return_ty(cx, hir_id)
.walk()
.skip(1) // skip `std::option::Option` or `std::result::Result`
.take(1) // take the first outermost inner type
.filter_map(|inner| match inner.unpack() {
GenericArgKind::Type(inner_ty) => Some(inner_ty.to_string()),
_ => None,
});
inner_ty.for_each(|inner_ty| {
// Issue 6640: If the inner type is Unit, emit lint similar to clippy::unused_unit.
if inner_ty.is_unit() {
span_lint_and_sugg(
cx,
UNNECESSARY_WRAPS,
fn_decl.output.span(),
"unneeded wrapped unit return type",
format!("remove the `-> {}<()>`", return_type_label).as_str(),
String::new(),
Applicability::MaybeIncorrect,
);
} else {
span_lint_and_then(
cx,
UNNECESSARY_WRAPS,
span,
format!(
"this function's return value is unnecessarily wrapped by `{}`",
return_type_label
)
.as_str(),
|diag| {
diag.span_suggestion(
fn_decl.output.span(),
format!("remove `{}` from the return type...", return_type).as_str(),
inner_ty,
format!("remove `{}` from the return type...", return_type_label).as_str(),
inner_ty.to_string(),
Applicability::MaybeIncorrect,
);
});
diag.multipart_suggestion(
"...and change the returning expressions",
suggs,
Applicability::MaybeIncorrect,
);
},
);
diag.multipart_suggestion(
"...and change the returning expressions",
suggs,
Applicability::MaybeIncorrect,
);
},
);
}
}
}
}
28 changes: 28 additions & 0 deletions tests/ui/unnecessary_wraps.rs
View file
Expand Up @@ -116,8 +116,36 @@ fn issue_6384(s: &str) -> Option<&str> {
})
}

// should be linted
fn issue_6640_1(a: bool, b: bool) -> Option<()> {
if a && b {
return Some(());
}
if a {
Some(());
Some(())
} else {
return Some(());
}
}

// should be linted
fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
if a && b {
return Ok(());
}
if a {
Ok(());
Ok(())
} else {
return Ok(());
}
}

fn main() {
// method calls are not linted
func1(true, true);
func2(true, true);
issue_6640_1(true, true);
issue_6640_2(true, true);
}
109 changes: 6 additions & 103 deletions tests/ui/unnecessary_wraps.stderr
View file
@@ -1,106 +1,9 @@
error: this function's return value is unnecessarily wrapped by `Option`
--> $DIR/unnecessary_wraps.rs:8:1
|
LL | / fn func1(a: bool, b: bool) -> Option<i32> {
LL | | if a && b {
LL | | return Some(42);
LL | | }
... |
LL | | }
LL | | }
| |_^
|
= note: `-D clippy::unnecessary-wraps` implied by `-D warnings`
help: remove `Option` from the return type...
|
LL | fn func1(a: bool, b: bool) -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | return 42;
LL | }
LL | if a {
LL | Some(-1);
LL | 2
LL | } else {
...

error: this function's return value is unnecessarily wrapped by `Option`
--> $DIR/unnecessary_wraps.rs:21:1
|
LL | / fn func2(a: bool, b: bool) -> Option<i32> {
LL | | if a && b {
LL | | return Some(10);
LL | | }
... |
LL | | }
LL | | }
| |_^
|
help: remove `Option` from the return type...
|
LL | fn func2(a: bool, b: bool) -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | return 10;
LL | }
LL | if a {
LL | 20
LL | } else {
LL | 30
|

error: this function's return value is unnecessarily wrapped by `Option`
--> $DIR/unnecessary_wraps.rs:51:1
|
LL | / fn func5() -> Option<i32> {
LL | | Some(1)
LL | | }
| |_^
|
help: remove `Option` from the return type...
|
LL | fn func5() -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | 1
|

error: this function's return value is unnecessarily wrapped by `Result`
--> $DIR/unnecessary_wraps.rs:61:1
|
LL | / fn func7() -> Result<i32, ()> {
LL | | Ok(1)
LL | | }
| |_^
|
help: remove `Result` from the return type...
|
LL | fn func7() -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | 1
|

error: this function's return value is unnecessarily wrapped by `Option`
--> $DIR/unnecessary_wraps.rs:93:5
|
LL | / fn func12() -> Option<i32> {
LL | | Some(1)
LL | | }
| |_____^
|
help: remove `Option` from the return type...
|
LL | fn func12() -> i32 {
| ^^^
help: ...and change the returning expressions
|
LL | 1
error[E0282]: type annotations needed
--> $DIR/unnecessary_wraps.rs:138:9
|
LL | Ok(());
| ^^ cannot infer type for type parameter `E` declared on the enum `Result`

error: aborting due to 5 previous errors
error: aborting due to previous error

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

0 comments on commit bfbc083

Please sign in to comment.