From bfbc0835870f8bf6cde79a01dfe7de351dde14aa Mon Sep 17 00:00:00 2001 From: Pierre-Andre Gagnon Date: Tue, 2 Feb 2021 18:39:23 -0500 Subject: [PATCH 1/4] Fix for issue 6640 --- clippy_lints/src/unnecessary_wraps.rs | 92 ++++++++++++++-------- tests/ui/unnecessary_wraps.rs | 28 +++++++ tests/ui/unnecessary_wraps.stderr | 109 ++------------------------ 3 files changed, 93 insertions(+), 136 deletions(-) diff --git a/clippy_lints/src/unnecessary_wraps.rs b/clippy_lints/src/unnecessary_wraps.rs index 8ac5dd696b76..eec76ce8b038 100644 --- a/clippy_lints/src/unnecessary_wraps.rs +++ b/clippy_lints/src/unnecessary_wraps.rs @@ -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; @@ -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() { @@ -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, @@ -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())); @@ -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, + ); + }, + ); + } } } } diff --git a/tests/ui/unnecessary_wraps.rs b/tests/ui/unnecessary_wraps.rs index a4570098d716..2d6aedc97ef0 100644 --- a/tests/ui/unnecessary_wraps.rs +++ b/tests/ui/unnecessary_wraps.rs @@ -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); } diff --git a/tests/ui/unnecessary_wraps.stderr b/tests/ui/unnecessary_wraps.stderr index 410f054b8efc..3ca5a8d1702b 100644 --- a/tests/ui/unnecessary_wraps.stderr +++ b/tests/ui/unnecessary_wraps.stderr @@ -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 { -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 { -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 { -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 { -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 { -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`. From e0e51e418906916123b3bd15c175d80d3dc74bf2 Mon Sep 17 00:00:00 2001 From: Pierre-Andre Gagnon Date: Tue, 2 Feb 2021 19:14:13 -0500 Subject: [PATCH 2/4] Fixed test --- clippy_lints/src/unnecessary_wraps.rs | 2 +- tests/ui/unnecessary_wraps.rs | 1 - tests/ui/unnecessary_wraps.stderr | 121 ++++++++++++++++++++++++-- 3 files changed, 116 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/unnecessary_wraps.rs b/clippy_lints/src/unnecessary_wraps.rs index eec76ce8b038..7da42ba3934f 100644 --- a/clippy_lints/src/unnecessary_wraps.rs +++ b/clippy_lints/src/unnecessary_wraps.rs @@ -140,7 +140,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { UNNECESSARY_WRAPS, fn_decl.output.span(), "unneeded wrapped unit return type", - format!("remove the `-> {}<()>`", return_type_label).as_str(), + format!("remove the `-> {}<[...]>`", return_type_label).as_str(), String::new(), Applicability::MaybeIncorrect, ); diff --git a/tests/ui/unnecessary_wraps.rs b/tests/ui/unnecessary_wraps.rs index 2d6aedc97ef0..5aaa99bbe5a3 100644 --- a/tests/ui/unnecessary_wraps.rs +++ b/tests/ui/unnecessary_wraps.rs @@ -135,7 +135,6 @@ fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> { return Ok(()); } if a { - Ok(()); Ok(()) } else { return Ok(()); diff --git a/tests/ui/unnecessary_wraps.stderr b/tests/ui/unnecessary_wraps.stderr index 3ca5a8d1702b..f28981f9e34a 100644 --- a/tests/ui/unnecessary_wraps.stderr +++ b/tests/ui/unnecessary_wraps.stderr @@ -1,9 +1,118 @@ -error[E0282]: type annotations needed - --> $DIR/unnecessary_wraps.rs:138:9 +error: this function's return value is unnecessarily wrapped by `Option` + --> $DIR/unnecessary_wraps.rs:8:1 | -LL | Ok(()); - | ^^ cannot infer type for type parameter `E` declared on the enum `Result` +LL | / fn func1(a: bool, b: bool) -> Option { +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 { +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 { +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 { +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 { +LL | | Some(1) +LL | | } + | |_____^ + | +help: remove `Option` from the return type... + | +LL | fn func12() -> i32 { + | ^^^ +help: ...and change the returning expressions + | +LL | 1 + | + +error: unneeded wrapped unit return type + --> $DIR/unnecessary_wraps.rs:120:38 + | +LL | fn issue_6640_1(a: bool, b: bool) -> Option<()> { + | ^^^^^^^^^^ help: remove the `-> Option<[...]>` + +error: unneeded wrapped unit return type + --> $DIR/unnecessary_wraps.rs:133:38 + | +LL | fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> { + | ^^^^^^^^^^^^^^^ help: remove the `-> Result<[...]>` -error: aborting due to previous error +error: aborting due to 7 previous errors -For more information about this error, try `rustc --explain E0282`. From 6165cccf7e43cf4ca2234e84dcf7060c7d89ef45 Mon Sep 17 00:00:00 2001 From: Pierre-Andre Gagnon Date: Wed, 17 Feb 2021 16:41:50 -0500 Subject: [PATCH 3/4] Added detailled suggs for new case --- clippy_lints/src/unnecessary_wraps.rs | 100 ++++++++++++-------------- tests/ui/unnecessary_wraps.stderr | 62 +++++++++++++--- 2 files changed, 97 insertions(+), 65 deletions(-) diff --git a/clippy_lints/src/unnecessary_wraps.rs b/clippy_lints/src/unnecessary_wraps.rs index 7da42ba3934f..b097d531bc4d 100644 --- a/clippy_lints/src/unnecessary_wraps.rs +++ b/clippy_lints/src/unnecessary_wraps.rs @@ -1,13 +1,13 @@ use crate::utils::{ - 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, + 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; @@ -85,33 +85,23 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { } } - // 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, 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 + // 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! { - // 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; @@ -119,12 +109,20 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { 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())); + 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 @@ -133,42 +131,36 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { }); if can_sugg && !suggs.is_empty() { - // 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, - ); + let (lint_msg, return_type_suggestion_msg, return_type_suggestion) = 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(), + ) } 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_label).as_str(), - inner_ty.to_string(), - Applicability::MaybeIncorrect, - ); - diag.multipart_suggestion( - "...and change the returning expressions", - suggs, - Applicability::MaybeIncorrect, - ); - }, + ), + format!("remove `{}` from the return type...", return_type_label), + inner_type.to_string(), + ) + }; + + span_lint_and_then(cx, UNNECESSARY_WRAPS, span, lint_msg.as_str(), |diag| { + diag.span_suggestion( + fn_decl.output.span(), + return_type_suggestion_msg.as_str(), + return_type_suggestion, + Applicability::MaybeIncorrect, ); - } + diag.multipart_suggestion( + "...and then change the returning expressions", + suggs, + Applicability::MaybeIncorrect, + ); + }); } } } diff --git a/tests/ui/unnecessary_wraps.stderr b/tests/ui/unnecessary_wraps.stderr index f28981f9e34a..40effb894990 100644 --- a/tests/ui/unnecessary_wraps.stderr +++ b/tests/ui/unnecessary_wraps.stderr @@ -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 the returning expressions | LL | return 42; LL | } @@ -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 the returning expressions | LL | return 10; LL | } @@ -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 the returning expressions | LL | 1 | @@ -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 the returning expressions | LL | 1 | @@ -97,22 +97,62 @@ help: remove `Option` from the return type... | LL | fn func12() -> i32 { | ^^^ -help: ...and change the returning expressions +help: ...and then change the returning expressions | LL | 1 | -error: unneeded wrapped unit return type - --> $DIR/unnecessary_wraps.rs:120:38 +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: remove the `-> Option<[...]>` + | ^^^^^^^^^^ +help: ...and then change the returning expressions + | +LL | return ; +LL | } +LL | if a { +LL | Some(()); +LL | +LL | } else { + ... -error: unneeded wrapped unit return type - --> $DIR/unnecessary_wraps.rs:133:38 +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: remove the `-> Result<[...]>` + | ^^^^^^^^^^^^^^^ +help: ...and then change the returning expressions + | +LL | return ; +LL | } +LL | if a { +LL | +LL | } else { +LL | return ; + | error: aborting due to 7 previous errors From a78271b8611d7ab7709976dfb94ae36b668ac42b Mon Sep 17 00:00:00 2001 From: Pierre-Andre Gagnon Date: Thu, 18 Feb 2021 17:32:55 -0500 Subject: [PATCH 4/4] Changed fn body suggestion msg --- clippy_lints/src/unnecessary_wraps.rs | 14 ++++++-------- tests/ui/unnecessary_wraps.rs | 18 ++++++++++++++++++ tests/ui/unnecessary_wraps.stderr | 14 +++++++------- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/unnecessary_wraps.rs b/clippy_lints/src/unnecessary_wraps.rs index b097d531bc4d..607585125a4c 100644 --- a/clippy_lints/src/unnecessary_wraps.rs +++ b/clippy_lints/src/unnecessary_wraps.rs @@ -131,11 +131,12 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { }); if can_sugg && !suggs.is_empty() { - let (lint_msg, return_type_suggestion_msg, return_type_suggestion) = if inner_type.is_unit() { + 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", ) } else { ( @@ -145,21 +146,18 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps { ), 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_suggestion_msg.as_str(), - return_type_suggestion, - Applicability::MaybeIncorrect, - ); - diag.multipart_suggestion( - "...and then change the returning expressions", - suggs, + return_type_sugg_msg.as_str(), + return_type_sugg, Applicability::MaybeIncorrect, ); + diag.multipart_suggestion(body_sugg_msg, suggs, Applicability::MaybeIncorrect); }); } } diff --git a/tests/ui/unnecessary_wraps.rs b/tests/ui/unnecessary_wraps.rs index 5aaa99bbe5a3..a510263e67da 100644 --- a/tests/ui/unnecessary_wraps.rs +++ b/tests/ui/unnecessary_wraps.rs @@ -141,6 +141,24 @@ fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> { } } +// 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); diff --git a/tests/ui/unnecessary_wraps.stderr b/tests/ui/unnecessary_wraps.stderr index 40effb894990..9a861c61a467 100644 --- a/tests/ui/unnecessary_wraps.stderr +++ b/tests/ui/unnecessary_wraps.stderr @@ -15,7 +15,7 @@ help: remove `Option` from the return type... | LL | fn func1(a: bool, b: bool) -> i32 { | ^^^ -help: ...and then change the returning expressions +help: ...and then change returning expressions | LL | return 42; LL | } @@ -41,7 +41,7 @@ help: remove `Option` from the return type... | LL | fn func2(a: bool, b: bool) -> i32 { | ^^^ -help: ...and then change the returning expressions +help: ...and then change returning expressions | LL | return 10; LL | } @@ -63,7 +63,7 @@ help: remove `Option` from the return type... | LL | fn func5() -> i32 { | ^^^ -help: ...and then change the returning expressions +help: ...and then change returning expressions | LL | 1 | @@ -80,7 +80,7 @@ help: remove `Result` from the return type... | LL | fn func7() -> i32 { | ^^^ -help: ...and then change the returning expressions +help: ...and then change returning expressions | LL | 1 | @@ -97,7 +97,7 @@ help: remove `Option` from the return type... | LL | fn func12() -> i32 { | ^^^ -help: ...and then change the returning expressions +help: ...and then change returning expressions | LL | 1 | @@ -118,7 +118,7 @@ help: remove the return type... | LL | fn issue_6640_1(a: bool, b: bool) -> Option<()> { | ^^^^^^^^^^ -help: ...and then change the returning expressions +help: ...and then remove returned values | LL | return ; LL | } @@ -144,7 +144,7 @@ help: remove the return type... | LL | fn issue_6640_2(a: bool, b: bool) -> Result<(), i32> { | ^^^^^^^^^^^^^^^ -help: ...and then change the returning expressions +help: ...and then remove returned values | LL | return ; LL | }