diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 7fc6ff8a2fb..c8cebca89d5 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -279,6 +279,7 @@ store.register_lints(&[ matches::SIGNIFICANT_DROP_IN_SCRUTINEE, matches::SINGLE_MATCH, matches::SINGLE_MATCH_ELSE, + matches::TRY_ERR, matches::WILDCARD_ENUM_MATCH_ARM, matches::WILDCARD_IN_OR_PATTERNS, mem_forget::MEM_FORGET, @@ -521,7 +522,6 @@ store.register_lints(&[ transmute::USELESS_TRANSMUTE, transmute::WRONG_TRANSMUTE, transmuting_null::TRANSMUTING_NULL, - try_err::TRY_ERR, types::BORROWED_BOX, types::BOX_COLLECTION, types::LINKEDLIST, diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs index 304b595541a..942a53459fd 100644 --- a/clippy_lints/src/lib.register_restriction.rs +++ b/clippy_lints/src/lib.register_restriction.rs @@ -30,6 +30,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(literal_representation::DECIMAL_LITERAL_REPRESENTATION), LintId::of(map_err_ignore::MAP_ERR_IGNORE), LintId::of(matches::REST_PAT_IN_FULLY_BOUND_STRUCTS), + LintId::of(matches::TRY_ERR), LintId::of(matches::WILDCARD_ENUM_MATCH_ARM), LintId::of(mem_forget::MEM_FORGET), LintId::of(methods::CLONE_ON_REF_PTR), @@ -67,7 +68,6 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(strings::STRING_SLICE), LintId::of(strings::STRING_TO_STRING), LintId::of(strings::STR_TO_STRING), - LintId::of(try_err::TRY_ERR), LintId::of(types::RC_BUFFER), LintId::of(types::RC_MUTEX), LintId::of(undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 742409d758b..d9941d75f78 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -385,7 +385,6 @@ mod trailing_empty_array; mod trait_bounds; mod transmute; mod transmuting_null; -mod try_err; mod types; mod undocumented_unsafe_blocks; mod unicode; @@ -700,7 +699,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ); store.register_late_pass(move || Box::new(pass_by_ref_or_value)); store.register_late_pass(|| Box::new(ref_option_ref::RefOptionRef)); - store.register_late_pass(|| Box::new(try_err::TryErr)); store.register_late_pass(|| Box::new(bytecount::ByteCount)); store.register_late_pass(|| Box::new(infinite_iter::InfiniteIter)); store.register_late_pass(|| Box::new(inline_fn_without_body::InlineFnWithoutBody)); diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 3c0dc8041d8..fee7471f17d 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -27,6 +27,7 @@ mod redundant_pattern_match; mod rest_pat_in_fully_bound_struct; mod significant_drop_in_scrutinee; mod single_match; +mod try_err; mod wild_in_or_pats; declare_clippy_lint! { @@ -825,6 +826,41 @@ declare_clippy_lint! { "warns when a temporary of a type with a drop with a significant side-effect might have a surprising lifetime" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usages of `Err(x)?`. + /// + /// ### Why is this bad? + /// The `?` operator is designed to allow calls that + /// can fail to be easily chained. For example, `foo()?.bar()` or + /// `foo(bar()?)`. Because `Err(x)?` can't be used that way (it will + /// always return), it is more clear to write `return Err(x)`. + /// + /// ### Example + /// ```rust + /// fn foo(fail: bool) -> Result { + /// if fail { + /// Err("failed")?; + /// } + /// Ok(0) + /// } + /// ``` + /// Could be written: + /// + /// ```rust + /// fn foo(fail: bool) -> Result { + /// if fail { + /// return Err("failed".into()); + /// } + /// Ok(0) + /// } + /// ``` + #[clippy::version = "1.38.0"] + pub TRY_ERR, + restriction, + "return errors explicitly rather than hiding them behind a `?`" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -864,6 +900,7 @@ impl_lint_pass!(Matches => [ MATCH_ON_VEC_ITEMS, MATCH_STR_CASE_MISMATCH, SIGNIFICANT_DROP_IN_SCRUTINEE, + TRY_ERR, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -888,6 +925,10 @@ impl<'tcx> LateLintPass<'tcx> for Matches { wild_in_or_pats::check(cx, arms); } + if source == MatchSource::TryDesugar { + try_err::check(cx, expr, ex); + } + if !from_expansion && !contains_cfg_arm(cx, expr, ex, arms) { if source == MatchSource::Normal { if !(meets_msrv(self.msrv, msrvs::MATCHES_MACRO) diff --git a/clippy_lints/src/matches/try_err.rs b/clippy_lints/src/matches/try_err.rs new file mode 100644 index 00000000000..0491a0679f3 --- /dev/null +++ b/clippy_lints/src/matches/try_err.rs @@ -0,0 +1,145 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{get_parent_expr, is_lang_ctor, match_def_path, paths}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::LangItem::ResultErr; +use rustc_hir::{Expr, ExprKind, LangItem, MatchSource, QPath}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, Ty}; +use rustc_span::{hygiene, sym}; + +use super::TRY_ERR; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, scrutinee: &'tcx Expr<'_>) { + // Looks for a structure like this: + // match ::std::ops::Try::into_result(Err(5)) { + // ::std::result::Result::Err(err) => + // #[allow(unreachable_code)] + // return ::std::ops::Try::from_error(::std::convert::From::from(err)), + // ::std::result::Result::Ok(val) => + // #[allow(unreachable_code)] + // val, + // }; + if_chain! { + if let ExprKind::Call(match_fun, try_args) = scrutinee.kind; + if let ExprKind::Path(ref match_fun_path) = match_fun.kind; + if matches!(match_fun_path, QPath::LangItem(LangItem::TryTraitBranch, ..)); + if let Some(try_arg) = try_args.get(0); + if let ExprKind::Call(err_fun, err_args) = try_arg.kind; + if let Some(err_arg) = err_args.get(0); + if let ExprKind::Path(ref err_fun_path) = err_fun.kind; + if is_lang_ctor(cx, err_fun_path, ResultErr); + if let Some(return_ty) = find_return_type(cx, &expr.kind); + then { + let prefix; + let suffix; + let err_ty; + + if let Some(ty) = result_error_type(cx, return_ty) { + prefix = "Err("; + suffix = ")"; + err_ty = ty; + } else if let Some(ty) = poll_result_error_type(cx, return_ty) { + prefix = "Poll::Ready(Err("; + suffix = "))"; + err_ty = ty; + } else if let Some(ty) = poll_option_result_error_type(cx, return_ty) { + prefix = "Poll::Ready(Some(Err("; + suffix = ")))"; + err_ty = ty; + } else { + return; + }; + + let expr_err_ty = cx.typeck_results().expr_ty(err_arg); + let span = hygiene::walk_chain(err_arg.span, try_arg.span.ctxt()); + let mut applicability = Applicability::MachineApplicable; + let origin_snippet = snippet_with_applicability(cx, span, "_", &mut applicability); + let ret_prefix = if get_parent_expr(cx, expr).map_or(false, |e| matches!(e.kind, ExprKind::Ret(_))) { + "" // already returns + } else { + "return " + }; + let suggestion = if err_ty == expr_err_ty { + format!("{}{}{}{}", ret_prefix, prefix, origin_snippet, suffix) + } else { + format!("{}{}{}.into(){}", ret_prefix, prefix, origin_snippet, suffix) + }; + + span_lint_and_sugg( + cx, + TRY_ERR, + expr.span, + "returning an `Err(_)` with the `?` operator", + "try this", + suggestion, + applicability, + ); + } + } +} + +/// Finds function return type by examining return expressions in match arms. +fn find_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option> { + if let ExprKind::Match(_, arms, MatchSource::TryDesugar) = expr { + for arm in arms.iter() { + if let ExprKind::Ret(Some(ret)) = arm.body.kind { + return Some(cx.typeck_results().expr_ty(ret)); + } + } + } + None +} + +/// Extracts the error type from Result. +fn result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + if_chain! { + if let ty::Adt(_, subst) = ty.kind(); + if is_type_diagnostic_item(cx, ty, sym::Result); + then { + Some(subst.type_at(1)) + } else { + None + } + } +} + +/// Extracts the error type from Poll>. +fn poll_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + if_chain! { + if let ty::Adt(def, subst) = ty.kind(); + if match_def_path(cx, def.did(), &paths::POLL); + let ready_ty = subst.type_at(0); + + if let ty::Adt(ready_def, ready_subst) = ready_ty.kind(); + if cx.tcx.is_diagnostic_item(sym::Result, ready_def.did()); + then { + Some(ready_subst.type_at(1)) + } else { + None + } + } +} + +/// Extracts the error type from Poll>>. +fn poll_option_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + if_chain! { + if let ty::Adt(def, subst) = ty.kind(); + if match_def_path(cx, def.did(), &paths::POLL); + let ready_ty = subst.type_at(0); + + if let ty::Adt(ready_def, ready_subst) = ready_ty.kind(); + if cx.tcx.is_diagnostic_item(sym::Option, ready_def.did()); + let some_ty = ready_subst.type_at(0); + + if let ty::Adt(some_def, some_subst) = some_ty.kind(); + if cx.tcx.is_diagnostic_item(sym::Result, some_def.did()); + then { + Some(some_subst.type_at(1)) + } else { + None + } + } +} diff --git a/clippy_lints/src/try_err.rs b/clippy_lints/src/try_err.rs deleted file mode 100644 index e108f7be12e..00000000000 --- a/clippy_lints/src/try_err.rs +++ /dev/null @@ -1,186 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_applicability; -use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{get_parent_expr, is_lang_ctor, match_def_path, paths}; -use if_chain::if_chain; -use rustc_errors::Applicability; -use rustc_hir::LangItem::ResultErr; -use rustc_hir::{Expr, ExprKind, LangItem, MatchSource, QPath}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::{self, Ty}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::{hygiene, sym}; - -declare_clippy_lint! { - /// ### What it does - /// Checks for usages of `Err(x)?`. - /// - /// ### Why is this bad? - /// The `?` operator is designed to allow calls that - /// can fail to be easily chained. For example, `foo()?.bar()` or - /// `foo(bar()?)`. Because `Err(x)?` can't be used that way (it will - /// always return), it is more clear to write `return Err(x)`. - /// - /// ### Example - /// ```rust - /// fn foo(fail: bool) -> Result { - /// if fail { - /// Err("failed")?; - /// } - /// Ok(0) - /// } - /// ``` - /// Could be written: - /// - /// ```rust - /// fn foo(fail: bool) -> Result { - /// if fail { - /// return Err("failed".into()); - /// } - /// Ok(0) - /// } - /// ``` - #[clippy::version = "1.38.0"] - pub TRY_ERR, - restriction, - "return errors explicitly rather than hiding them behind a `?`" -} - -declare_lint_pass!(TryErr => [TRY_ERR]); - -impl<'tcx> LateLintPass<'tcx> for TryErr { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - // Looks for a structure like this: - // match ::std::ops::Try::into_result(Err(5)) { - // ::std::result::Result::Err(err) => - // #[allow(unreachable_code)] - // return ::std::ops::Try::from_error(::std::convert::From::from(err)), - // ::std::result::Result::Ok(val) => - // #[allow(unreachable_code)] - // val, - // }; - if_chain! { - if !in_external_macro(cx.tcx.sess, expr.span); - if let ExprKind::Match(match_arg, _, MatchSource::TryDesugar) = expr.kind; - if let ExprKind::Call(match_fun, try_args) = match_arg.kind; - if let ExprKind::Path(ref match_fun_path) = match_fun.kind; - if matches!(match_fun_path, QPath::LangItem(LangItem::TryTraitBranch, ..)); - if let Some(try_arg) = try_args.get(0); - if let ExprKind::Call(err_fun, err_args) = try_arg.kind; - if let Some(err_arg) = err_args.get(0); - if let ExprKind::Path(ref err_fun_path) = err_fun.kind; - if is_lang_ctor(cx, err_fun_path, ResultErr); - if let Some(return_ty) = find_return_type(cx, &expr.kind); - then { - let prefix; - let suffix; - let err_ty; - - if let Some(ty) = result_error_type(cx, return_ty) { - prefix = "Err("; - suffix = ")"; - err_ty = ty; - } else if let Some(ty) = poll_result_error_type(cx, return_ty) { - prefix = "Poll::Ready(Err("; - suffix = "))"; - err_ty = ty; - } else if let Some(ty) = poll_option_result_error_type(cx, return_ty) { - prefix = "Poll::Ready(Some(Err("; - suffix = ")))"; - err_ty = ty; - } else { - return; - }; - - let expr_err_ty = cx.typeck_results().expr_ty(err_arg); - let span = hygiene::walk_chain(err_arg.span, try_arg.span.ctxt()); - let mut applicability = Applicability::MachineApplicable; - let origin_snippet = snippet_with_applicability(cx, span, "_", &mut applicability); - let ret_prefix = if get_parent_expr(cx, expr).map_or(false, |e| matches!(e.kind, ExprKind::Ret(_))) { - "" // already returns - } else { - "return " - }; - let suggestion = if err_ty == expr_err_ty { - format!("{}{}{}{}", ret_prefix, prefix, origin_snippet, suffix) - } else { - format!("{}{}{}.into(){}", ret_prefix, prefix, origin_snippet, suffix) - }; - - span_lint_and_sugg( - cx, - TRY_ERR, - expr.span, - "returning an `Err(_)` with the `?` operator", - "try this", - suggestion, - applicability, - ); - } - } - } -} - -/// Finds function return type by examining return expressions in match arms. -fn find_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option> { - if let ExprKind::Match(_, arms, MatchSource::TryDesugar) = expr { - for arm in arms.iter() { - if let ExprKind::Ret(Some(ret)) = arm.body.kind { - return Some(cx.typeck_results().expr_ty(ret)); - } - } - } - None -} - -/// Extracts the error type from Result. -fn result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { - if_chain! { - if let ty::Adt(_, subst) = ty.kind(); - if is_type_diagnostic_item(cx, ty, sym::Result); - then { - Some(subst.type_at(1)) - } else { - None - } - } -} - -/// Extracts the error type from Poll>. -fn poll_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { - if_chain! { - if let ty::Adt(def, subst) = ty.kind(); - if match_def_path(cx, def.did(), &paths::POLL); - let ready_ty = subst.type_at(0); - - if let ty::Adt(ready_def, ready_subst) = ready_ty.kind(); - if cx.tcx.is_diagnostic_item(sym::Result, ready_def.did()); - then { - Some(ready_subst.type_at(1)) - } else { - None - } - } -} - -/// Extracts the error type from Poll>>. -fn poll_option_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { - if_chain! { - if let ty::Adt(def, subst) = ty.kind(); - if match_def_path(cx, def.did(), &paths::POLL); - let ready_ty = subst.type_at(0); - - if let ty::Adt(ready_def, ready_subst) = ready_ty.kind(); - if cx.tcx.is_diagnostic_item(sym::Option, ready_def.did()); - let some_ty = ready_subst.type_at(0); - - if let ty::Adt(some_def, some_subst) = some_ty.kind(); - if cx.tcx.is_diagnostic_item(sym::Result, some_def.did()); - then { - Some(some_subst.type_at(1)) - } else { - None - } - } -}