diff options
| -rw-r--r-- | clippy_lints/src/returns.rs | 42 | ||||
| -rw-r--r-- | tests/ui/needless_return_with_question_mark.fixed | 15 | ||||
| -rw-r--r-- | tests/ui/needless_return_with_question_mark.rs | 14 | ||||
| -rw-r--r-- | tests/ui/needless_return_with_question_mark.stderr | 8 |
4 files changed, 35 insertions, 44 deletions
diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 2d4e7d269fd..1047793b822 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -2,10 +2,14 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then, span_lin use clippy_utils::source::{snippet_opt, snippet_with_context}; use clippy_utils::sugg::has_enclosing_paren; use clippy_utils::visitors::{for_each_expr_with_closures, Descend}; -use clippy_utils::{fn_def_id, is_from_proc_macro, is_inside_let_else, path_to_local_id, span_find_starting_semi}; +use clippy_utils::{ + fn_def_id, is_from_proc_macro, is_inside_let_else, is_res_lang_ctor, path_res, path_to_local_id, + span_find_starting_semi, +}; use core::ops::ControlFlow; use rustc_errors::Applicability; use rustc_hir::intravisit::FnKind; +use rustc_hir::LangItem::ResultErr; use rustc_hir::{ Block, Body, Expr, ExprKind, FnDecl, HirId, ItemKind, LangItem, MatchSource, Node, OwnerNode, PatKind, QPath, Stmt, StmtKind, @@ -176,37 +180,20 @@ fn stmt_needs_never_type(cx: &LateContext<'_>, stmt_hir_id: HirId) -> bool { }) } -/// -/// The expression of the desugared `try` operator is a match over an expression with type: -/// `ControlFlow<A:Result<Infallible, E>, B:Result<_, E'>>`, with final type `B`. -/// If E and E' are the same type, then there is no error conversion happening. -/// Error conversion happens when E can be transformed into E' via a `From` or `Into` conversion. -fn desugar_expr_performs_error_conversion(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - let ty = cx.typeck_results().expr_ty(expr); - - if let ty::Adt(_, generics) = ty.kind() - && let Some(brk) = generics.first() - && let Some(cont) = generics.get(1) - && let Some(brk_type) = brk.as_type() - && let Some(cont_type) = cont.as_type() - && let ty::Adt(_, brk_generics) = brk_type.kind() - && let ty::Adt(_, cont_generics) = cont_type.kind() - && let Some(brk_err) = brk_generics.get(1) - && let Some(cont_err) = cont_generics.get(1) - && let Some(brk_err_type) = brk_err.as_type() - && let Some(cont_err_type) = cont_err.as_type() - { - return brk_err_type != cont_err_type; - } - false -} - impl<'tcx> LateLintPass<'tcx> for Return { fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { if !in_external_macro(cx.sess(), stmt.span) && let StmtKind::Semi(expr) = stmt.kind && let ExprKind::Ret(Some(ret)) = expr.kind - && let ExprKind::Match(match_expr, _, MatchSource::TryDesugar(..)) = ret.kind + // return Err(...)? desugars to a match + // over a Err(...).branch() + // which breaks down to a branch call, with the callee being + // the constructor of the Err variant + && let ExprKind::Match(maybe_cons, _, MatchSource::TryDesugar(_)) = ret.kind + && let ExprKind::Call(_, [maybe_result_err]) = maybe_cons.kind + && let ExprKind::Call(maybe_constr, _) = maybe_result_err.kind + && is_res_lang_ctor(cx, path_res(cx, maybe_constr), ResultErr) + // Ensure this is not the final stmt, otherwise removing it would cause a compile error && let OwnerNode::Item(item) = cx.tcx.hir().owner(cx.tcx.hir().get_parent_item(expr.hir_id)) && let ItemKind::Fn(_, _, body) = item.kind @@ -217,7 +204,6 @@ impl<'tcx> LateLintPass<'tcx> for Return { && final_stmt.hir_id != stmt.hir_id && !is_from_proc_macro(cx, expr) && !stmt_needs_never_type(cx, stmt.hir_id) - && !desugar_expr_performs_error_conversion(cx, match_expr) { span_lint_and_sugg( cx, diff --git a/tests/ui/needless_return_with_question_mark.fixed b/tests/ui/needless_return_with_question_mark.fixed index 2b0a835bc7f..9b7da852663 100644 --- a/tests/ui/needless_return_with_question_mark.fixed +++ b/tests/ui/needless_return_with_question_mark.fixed @@ -78,9 +78,6 @@ fn issue11616() -> Result<(), ()> { Ok(()) } -/// This is a false positive that occurs because of the way `?` is handled. -/// The `?` operator is also doing a conversion from `Result<T, E>` to `Result<T, E'>`. -/// In this case the conversion is needed, and thus the `?` operator is also needed. fn issue11982() { mod bar { pub struct Error; @@ -115,10 +112,18 @@ fn issue11982_no_conversion() { fn foo(ok: bool) -> Result<(), bar::Error> { if !ok { - bar::foo(ok).map(|_| Ok::<(), bar::Error>(()))?; - //~^ ERROR: unneeded `return` statement with `?` operator + return bar::foo(ok).map(|_| Ok::<(), bar::Error>(()))?; }; Ok(()) } +} +fn general_return() { + fn foo(ok: bool) -> Result<(), ()> { + let bar = Result::Ok(Result::<(), ()>::Ok(())); + if !ok { + return bar?; + }; + Ok(()) + } } diff --git a/tests/ui/needless_return_with_question_mark.rs b/tests/ui/needless_return_with_question_mark.rs index ecbf00254f0..68e76d2b640 100644 --- a/tests/ui/needless_return_with_question_mark.rs +++ b/tests/ui/needless_return_with_question_mark.rs @@ -78,9 +78,6 @@ fn issue11616() -> Result<(), ()> { Ok(()) } -/// This is a false positive that occurs because of the way `?` is handled. -/// The `?` operator is also doing a conversion from `Result<T, E>` to `Result<T, E'>`. -/// In this case the conversion is needed, and thus the `?` operator is also needed. fn issue11982() { mod bar { pub struct Error; @@ -116,7 +113,16 @@ fn issue11982_no_conversion() { fn foo(ok: bool) -> Result<(), bar::Error> { if !ok { return bar::foo(ok).map(|_| Ok::<(), bar::Error>(()))?; - //~^ ERROR: unneeded `return` statement with `?` operator + }; + Ok(()) + } +} + +fn general_return() { + fn foo(ok: bool) -> Result<(), ()> { + let bar = Result::Ok(Result::<(), ()>::Ok(())); + if !ok { + return bar?; }; Ok(()) } diff --git a/tests/ui/needless_return_with_question_mark.stderr b/tests/ui/needless_return_with_question_mark.stderr index 59c212bcbb3..17aa212ae8d 100644 --- a/tests/ui/needless_return_with_question_mark.stderr +++ b/tests/ui/needless_return_with_question_mark.stderr @@ -13,11 +13,5 @@ error: unneeded `return` statement with `?` operator LL | return Err(())?; | ^^^^^^^ help: remove it -error: unneeded `return` statement with `?` operator - --> $DIR/needless_return_with_question_mark.rs:118:13 - | -LL | return bar::foo(ok).map(|_| Ok::<(), bar::Error>(()))?; - | ^^^^^^^ help: remove it - -error: aborting due to 3 previous errors +error: aborting due to 2 previous errors |