From 57dd25e2ffbf9c7abd63fcdf057facff7a46c342 Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Tue, 26 Dec 2023 20:34:00 +0200 Subject: [PATCH 1/3] FP: `needless_return_with_question_mark` with implicit Error Conversion Return with a question mark was triggered in situations where the `?` desuraging was performing error conversion via `Into`/`From`. The desugared `?` produces a match over an expression with type `std::ops::ControlFlow` with `B:Result` and `C:Result<_, E':Error>`, and the arms perform the conversion. The patch adds another check in the lint that checks that `E == E'`. If `E == E'`, then the `?` is indeed unnecessary. changelog: False Positive: `needless_return_with_question_mark` when implicit Error Conversion occurs. --- clippy_lints/src/returns.rs | 28 ++++++++++++++++++- .../needless_return_with_question_mark.fixed | 27 ++++++++++++++++++ .../ui/needless_return_with_question_mark.rs | 27 ++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 2293b53b42b..2d4e7d269fd 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -176,12 +176,37 @@ 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, 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(.., MatchSource::TryDesugar(_)) = ret.kind + && let ExprKind::Match(match_expr, _, MatchSource::TryDesugar(..)) = ret.kind // 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 @@ -192,6 +217,7 @@ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { && 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 0147c73a94b..8a97d8604a5 100644 --- a/tests/ui/needless_return_with_question_mark.fixed +++ b/tests/ui/needless_return_with_question_mark.fixed @@ -77,3 +77,30 @@ 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` to `Result`. +/// In this case the conversion is needed, and thus the `?` operator is also needed. +fn issue11982() { + mod bar { + pub struct Error; + pub fn foo(_: bool) -> Result<(), Error> { + Ok(()) + } + } + + pub struct Error; + + impl From for Error { + fn from(_: bar::Error) -> Self { + Error + } + } + + fn foo(ok: bool) -> Result<(), Error> { + if !ok { + return bar::foo(ok).map(|_| Ok::<(), Error>(()))?; + }; + Ok(()) + } +} diff --git a/tests/ui/needless_return_with_question_mark.rs b/tests/ui/needless_return_with_question_mark.rs index 66e1f438f8c..be15b000f5d 100644 --- a/tests/ui/needless_return_with_question_mark.rs +++ b/tests/ui/needless_return_with_question_mark.rs @@ -77,3 +77,30 @@ 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` to `Result`. +/// In this case the conversion is needed, and thus the `?` operator is also needed. +fn issue11982() { + mod bar { + pub struct Error; + pub fn foo(_: bool) -> Result<(), Error> { + Ok(()) + } + } + + pub struct Error; + + impl From for Error { + fn from(_: bar::Error) -> Self { + Error + } + } + + fn foo(ok: bool) -> Result<(), Error> { + if !ok { + return bar::foo(ok).map(|_| Ok::<(), Error>(()))?; + }; + Ok(()) + } +} From f58950de86f36ac41a7eed134529c76e4da3a552 Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Fri, 26 Jan 2024 23:48:47 +0100 Subject: [PATCH 2/3] correct lint case --- .../needless_return_with_question_mark.fixed | 18 ++++++++++++++++++ tests/ui/needless_return_with_question_mark.rs | 17 +++++++++++++++++ .../needless_return_with_question_mark.stderr | 8 +++++++- 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/tests/ui/needless_return_with_question_mark.fixed b/tests/ui/needless_return_with_question_mark.fixed index 8a97d8604a5..2b0a835bc7f 100644 --- a/tests/ui/needless_return_with_question_mark.fixed +++ b/tests/ui/needless_return_with_question_mark.fixed @@ -104,3 +104,21 @@ fn issue11982() { Ok(()) } } + +fn issue11982_no_conversion() { + mod bar { + pub struct Error; + pub fn foo(_: bool) -> Result<(), Error> { + Ok(()) + } + } + + fn foo(ok: bool) -> Result<(), bar::Error> { + if !ok { + bar::foo(ok).map(|_| Ok::<(), bar::Error>(()))?; + //~^ ERROR: unneeded `return` statement with `?` operator + }; + Ok(()) + } + +} diff --git a/tests/ui/needless_return_with_question_mark.rs b/tests/ui/needless_return_with_question_mark.rs index be15b000f5d..ecbf00254f0 100644 --- a/tests/ui/needless_return_with_question_mark.rs +++ b/tests/ui/needless_return_with_question_mark.rs @@ -104,3 +104,20 @@ fn foo(ok: bool) -> Result<(), Error> { Ok(()) } } + +fn issue11982_no_conversion() { + mod bar { + pub struct Error; + pub fn foo(_: bool) -> Result<(), Error> { + Ok(()) + } + } + + fn foo(ok: bool) -> Result<(), bar::Error> { + if !ok { + return bar::foo(ok).map(|_| Ok::<(), bar::Error>(()))?; + //~^ ERROR: unneeded `return` statement with `?` operator + }; + Ok(()) + } +} diff --git a/tests/ui/needless_return_with_question_mark.stderr b/tests/ui/needless_return_with_question_mark.stderr index 17aa212ae8d..59c212bcbb3 100644 --- a/tests/ui/needless_return_with_question_mark.stderr +++ b/tests/ui/needless_return_with_question_mark.stderr @@ -13,5 +13,11 @@ error: unneeded `return` statement with `?` operator LL | return Err(())?; | ^^^^^^^ help: remove it -error: aborting due to 2 previous errors +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 From 3aa2c279c8d1a64c8ecbf4c4e9f1b5bc8f9c4883 Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Sun, 28 Jan 2024 22:43:40 +0100 Subject: [PATCH 3/3] rewrote to match only Result::err cons --- clippy_lints/src/returns.rs | 42 +++++++------------ .../needless_return_with_question_mark.fixed | 17 +++++--- .../ui/needless_return_with_question_mark.rs | 14 +++++-- .../needless_return_with_question_mark.stderr | 8 +--- 4 files changed, 36 insertions(+), 45 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::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, 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 @@ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { && 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` to `Result`. -/// 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` to `Result`. -/// 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 @@ pub fn foo(_: bool) -> Result<(), Error> { 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