Skip to content

Commit

Permalink
Auto merge of rust-lang#6665 - pag4k:unnecessary_wraps_bug_6640, r=ca…
Browse files Browse the repository at this point in the history 
…msteffen

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
  • Loading branch information
@bors
bors committed Feb 18, 2021
2 parents 2f815ec + a78271b commit 0f70e88
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 45 deletions.
94 changes: 55 additions & 39 deletions clippy_lints/src/unnecessary_wraps.rs
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use crate::utils::{
contains_return, in_macro, is_type_diagnostic_item, match_qpath, paths, return_ty, snippet, span_lint_and_then,
contains_return, in_macro, match_qpath, paths, return_ty, snippet, span_lint_and_then,
visitors::find_all_ret_expressions,
};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::intravisit::FnKind;
use rustc_hir::{Body, ExprKind, FnDecl, HirId, Impl, ItemKind, Node};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::symbol::sym;
use rustc_span::Span;
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,25 +85,44 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
}
}

let (return_type, path) = if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::option_type) {
("Option", &paths::OPTION_SOME)
} else if is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym::result_type) {
("Result", &paths::RESULT_OK)
// Get the wrapper and inner types, if can't, abort.
let (return_type_label, path, inner_type) = if let ty::Adt(adt_def, subst) = return_ty(cx, hir_id).kind() {
if cx.tcx.is_diagnostic_item(sym::option_type, adt_def.did) {
("Option", &paths::OPTION_SOME, subst.type_at(0))
} else if cx.tcx.is_diagnostic_item(sym::result_type, adt_def.did) {
("Result", &paths::RESULT_OK, subst.type_at(0))
} else {
return;
}
} 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! {
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);
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()));
suggs.push(
(
ret_expr.span,
if inner_type.is_unit() {
"".to_string()
} else {
snippet(cx, args[0].span.source_callsite(), "..").to_string()
}
)
);
true
} else {
false
Expand All @@ -110,39 +131,34 @@ 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
let (lint_msg, return_type_sugg_msg, return_type_sugg, body_sugg_msg) = if inner_type.is_unit() {
(
"this function's return value is unnecessary".to_string(),
"remove the return type...".to_string(),
snippet(cx, fn_decl.output.span(), "..").to_string(),
"...and then remove returned values",
)
.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| {
diag.span_suggestion(
fn_decl.output.span(),
format!("remove `{}` from the return type...", return_type).as_str(),
inner_ty,
Applicability::MaybeIncorrect,
);
});
diag.multipart_suggestion(
"...and change the returning expressions",
suggs,
Applicability::MaybeIncorrect,
);
},
);
} else {
(
format!(
"this function's return value is unnecessarily wrapped by `{}`",
return_type_label
),
format!("remove `{}` from the return type...", return_type_label),
inner_type.to_string(),
"...and then change returning expressions",
)
};

span_lint_and_then(cx, UNNECESSARY_WRAPS, span, lint_msg.as_str(), |diag| {
diag.span_suggestion(
fn_decl.output.span(),
return_type_sugg_msg.as_str(),
return_type_sugg,
Applicability::MaybeIncorrect,
);
diag.multipart_suggestion(body_sugg_msg, suggs, Applicability::MaybeIncorrect);
});
}
}
}
45 changes: 45 additions & 0 deletions tests/ui/unnecessary_wraps.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,53 @@ 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(())
} else {
return Ok(());
}
}

// should not be linted
fn issue_6640_3() -> Option<()> {
if true {
Some(())
} else {
None
}
}

// should not be linted
fn issue_6640_4() -> Result<(), ()> {
if true {
Ok(())
} else {
Err(())
}
}

fn main() {
// method calls are not linted
func1(true, true);
func2(true, true);
issue_6640_1(true, true);
issue_6640_2(true, true);
}
64 changes: 58 additions & 6 deletions tests/ui/unnecessary_wraps.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ help: remove `Option` from the return type...
|
LL | fn func1(a: bool, b: bool) -> i32 {
| ^^^
help: ...and change the returning expressions
help: ...and then change returning expressions
|
LL | return 42;
LL | }
Expand All @@ -41,7 +41,7 @@ help: remove `Option` from the return type...
|
LL | fn func2(a: bool, b: bool) -> i32 {
| ^^^
help: ...and change the returning expressions
help: ...and then change returning expressions
|
LL | return 10;
LL | }
Expand All @@ -63,7 +63,7 @@ help: remove `Option` from the return type...
|
LL | fn func5() -> i32 {
| ^^^
help: ...and change the returning expressions
help: ...and then change returning expressions
|
LL | 1
|
Expand All @@ -80,7 +80,7 @@ help: remove `Result` from the return type...
|
LL | fn func7() -> i32 {
| ^^^
help: ...and change the returning expressions
help: ...and then change returning expressions
|
LL | 1
|
Expand All @@ -97,10 +97,62 @@ help: remove `Option` from the return type...
|
LL | fn func12() -> i32 {
| ^^^
help: ...and change the returning expressions
help: ...and then change returning expressions
|
LL | 1
|

error: aborting due to 5 previous errors
error: this function's return value is unnecessary
--> $DIR/unnecessary_wraps.rs:120:1
|
LL | / fn issue_6640_1(a: bool, b: bool) -> Option<()> {
LL | | if a && b {
LL | | return Some(());
LL | | }
... |
LL | | }
LL | | }
| |_^
|
help: remove the return type...
|
LL | fn issue_6640_1(a: bool, b: bool) -> Option<()> {
| ^^^^^^^^^^
help: ...and then remove returned values
|
LL | return ;
LL | }
LL | if a {
LL | Some(());
LL |
LL | } else {
...

error: this function's return value is unnecessary
--> $DIR/unnecessary_wraps.rs:133:1
|
LL | / fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
LL | | if a && b {
LL | | return Ok(());
LL | | }
... |
LL | | }
LL | | }
| |_^
|
help: remove the return type...
|
LL | fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> {
| ^^^^^^^^^^^^^^^
help: ...and then remove returned values
|
LL | return ;
LL | }
LL | if a {
LL |
LL | } else {
LL | return ;
|

error: aborting due to 7 previous errors

0 comments on commit 0f70e88

Please sign in to comment.