From c13cb54e25140afe2845eee3130b2b369d130d48 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Mon, 26 Jun 2023 00:44:17 -0500 Subject: [PATCH 1/2] New lint `needless_return_with_try` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/returns.rs | 74 +++++++++++++++++++++++- tests/ui/needless_return_with_try.fixed | 40 +++++++++++++ tests/ui/needless_return_with_try.rs | 40 +++++++++++++ tests/ui/needless_return_with_try.stderr | 10 ++++ tests/ui/try_err.fixed | 6 +- tests/ui/try_err.rs | 6 +- tests/ui/try_err.stderr | 22 +++---- 9 files changed, 184 insertions(+), 16 deletions(-) create mode 100644 tests/ui/needless_return_with_try.fixed create mode 100644 tests/ui/needless_return_with_try.rs create mode 100644 tests/ui/needless_return_with_try.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index ea8450ed5c2..47b64d27ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5093,6 +5093,7 @@ Released 2018-09-13 [`needless_raw_string_hashes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_string_hashes [`needless_raw_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_strings [`needless_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return +[`needless_return_with_try`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return_with_try [`needless_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_splitn [`needless_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_update [`neg_cmp_op_on_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_cmp_op_on_partial_ord diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7472158a81c..7676e787ee7 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -569,6 +569,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::return_self_not_must_use::RETURN_SELF_NOT_MUST_USE_INFO, crate::returns::LET_AND_RETURN_INFO, crate::returns::NEEDLESS_RETURN_INFO, + crate::returns::NEEDLESS_RETURN_WITH_TRY_INFO, crate::same_name_method::SAME_NAME_METHOD_INFO, crate::self_named_constructors::SELF_NAMED_CONSTRUCTORS_INFO, crate::semicolon_block::SEMICOLON_INSIDE_BLOCK_INFO, diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index fe797219073..58868e97e23 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then}; +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then, span_lint_hir_and_then}; use clippy_utils::source::{snippet_opt, snippet_with_context}; use clippy_utils::visitors::{for_each_expr_with_closures, Descend}; use clippy_utils::{fn_def_id, path_to_local_id, span_find_starting_semi}; @@ -6,7 +6,9 @@ use core::ops::ControlFlow; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::intravisit::FnKind; -use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, LangItem, MatchSource, PatKind, QPath, StmtKind}; +use rustc_hir::{ + Block, Body, Expr, ExprKind, FnDecl, ItemKind, LangItem, MatchSource, OwnerNode, PatKind, QPath, Stmt, StmtKind, +}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_middle::ty::subst::GenericArgKind; @@ -77,6 +79,46 @@ declare_clippy_lint! { "using a return statement like `return expr;` where an expression would suffice" } +declare_clippy_lint! { + /// ### What it does + /// Checks for return statements on `Err` paired with the `?` operator. + /// + /// ### Why is this bad? + /// The `return` is unnecessary. + /// + /// ### Example + /// ```rust,ignore + /// fn foo(x: usize) -> Result<(), Box> { + /// if x == 0 { + /// return Err(...)?; + /// } + /// Ok(()) + /// } + /// ``` + /// simplify to + /// ```rust,ignore + /// fn foo(x: usize) -> Result<(), Box> { + /// if x == 0 { + /// Err(...)?; + /// } + /// Ok(()) + /// } + /// ``` + /// if paired with `try_err`, use instead: + /// ```rust,ignore + /// fn foo(x: usize) -> Result<(), Box> { + /// if x == 0 { + /// return Err(...); + /// } + /// Ok(()) + /// } + /// ``` + #[clippy::version = "1.72.0"] + pub NEEDLESS_RETURN_WITH_TRY, + style, + "using a return statement like `return Err(expr)?;` where removing it would suffice" +} + #[derive(PartialEq, Eq)] enum RetReplacement<'tcx> { Empty, @@ -116,9 +158,35 @@ impl<'tcx> ToString for RetReplacement<'tcx> { } } -declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN]); +declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN, NEEDLESS_RETURN_WITH_TRY]); 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 + // 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 + && let block = cx.tcx.hir().body(body).value + && let ExprKind::Block(block, _) = block.kind + && let [.., final_stmt] = block.stmts + && final_stmt.hir_id != stmt.hir_id + && !is_from_proc_macro(cx, expr) + { + span_lint_and_sugg( + cx, + NEEDLESS_RETURN_WITH_TRY, + expr.span.until(ret.span), + "unneeded `return` statement with `?` operator", + "remove it", + String::new(), + Applicability::MachineApplicable, + ); + } + } + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) { // we need both a let-binding stmt and an expr if_chain! { diff --git a/tests/ui/needless_return_with_try.fixed b/tests/ui/needless_return_with_try.fixed new file mode 100644 index 00000000000..96d3e6df2d1 --- /dev/null +++ b/tests/ui/needless_return_with_try.fixed @@ -0,0 +1,40 @@ +//@run-rustfix +//@aux-build:proc_macros.rs +#![allow( + clippy::needless_return, + clippy::no_effect, + clippy::unit_arg, + clippy::useless_conversion, + unused +)] + +#[macro_use] +extern crate proc_macros; + +fn a() -> u32 { + return 0; +} + +fn b() -> Result { + return Err(0); +} + +// Do not lint +fn c() -> Option<()> { + return None?; +} + +fn main() -> Result<(), ()> { + Err(())?; + return Ok::<(), ()>(()); + Err(())?; + Ok::<(), ()>(()); + return Err(().into()); + external! { + return Err(())?; + } + with_span! { + return Err(())?; + } + Err(()) +} diff --git a/tests/ui/needless_return_with_try.rs b/tests/ui/needless_return_with_try.rs new file mode 100644 index 00000000000..039afba0602 --- /dev/null +++ b/tests/ui/needless_return_with_try.rs @@ -0,0 +1,40 @@ +//@run-rustfix +//@aux-build:proc_macros.rs +#![allow( + clippy::needless_return, + clippy::no_effect, + clippy::unit_arg, + clippy::useless_conversion, + unused +)] + +#[macro_use] +extern crate proc_macros; + +fn a() -> u32 { + return 0; +} + +fn b() -> Result { + return Err(0); +} + +// Do not lint +fn c() -> Option<()> { + return None?; +} + +fn main() -> Result<(), ()> { + return Err(())?; + return Ok::<(), ()>(()); + Err(())?; + Ok::<(), ()>(()); + return Err(().into()); + external! { + return Err(())?; + } + with_span! { + return Err(())?; + } + Err(()) +} diff --git a/tests/ui/needless_return_with_try.stderr b/tests/ui/needless_return_with_try.stderr new file mode 100644 index 00000000000..72f2bba9200 --- /dev/null +++ b/tests/ui/needless_return_with_try.stderr @@ -0,0 +1,10 @@ +error: unneeded `return` statement with `?` operator + --> $DIR/needless_return_with_try.rs:28:5 + | +LL | return Err(())?; + | ^^^^^^^ help: remove it + | + = note: `-D clippy::needless-return-with-try` implied by `-D warnings` + +error: aborting due to previous error + diff --git a/tests/ui/try_err.fixed b/tests/ui/try_err.fixed index 1816740870a..e7e2379fa4c 100644 --- a/tests/ui/try_err.fixed +++ b/tests/ui/try_err.fixed @@ -2,7 +2,11 @@ //@aux-build:proc_macros.rs:proc-macro #![deny(clippy::try_err)] -#![allow(clippy::unnecessary_wraps, clippy::needless_question_mark)] +#![allow( + clippy::unnecessary_wraps, + clippy::needless_question_mark, + clippy::needless_return_with_try +)] extern crate proc_macros; use proc_macros::{external, inline_macros}; diff --git a/tests/ui/try_err.rs b/tests/ui/try_err.rs index 0e47c4d023e..ece21887ed3 100644 --- a/tests/ui/try_err.rs +++ b/tests/ui/try_err.rs @@ -2,7 +2,11 @@ //@aux-build:proc_macros.rs:proc-macro #![deny(clippy::try_err)] -#![allow(clippy::unnecessary_wraps, clippy::needless_question_mark)] +#![allow( + clippy::unnecessary_wraps, + clippy::needless_question_mark, + clippy::needless_return_with_try +)] extern crate proc_macros; use proc_macros::{external, inline_macros}; diff --git a/tests/ui/try_err.stderr b/tests/ui/try_err.stderr index 79f7b70224a..9968b383ebb 100644 --- a/tests/ui/try_err.stderr +++ b/tests/ui/try_err.stderr @@ -1,5 +1,5 @@ error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:19:9 + --> $DIR/try_err.rs:23:9 | LL | Err(err)?; | ^^^^^^^^^ help: try: `return Err(err)` @@ -11,25 +11,25 @@ LL | #![deny(clippy::try_err)] | ^^^^^^^^^^^^^^^ error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:29:9 + --> $DIR/try_err.rs:33:9 | LL | Err(err)?; | ^^^^^^^^^ help: try: `return Err(err.into())` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:49:17 + --> $DIR/try_err.rs:53:17 | LL | Err(err)?; | ^^^^^^^^^ help: try: `return Err(err)` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:68:17 + --> $DIR/try_err.rs:72:17 | LL | Err(err)?; | ^^^^^^^^^ help: try: `return Err(err.into())` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:88:23 + --> $DIR/try_err.rs:92:23 | LL | Err(_) => Err(1)?, | ^^^^^^^ help: try: `return Err(1)` @@ -37,7 +37,7 @@ LL | Err(_) => Err(1)?, = note: this error originates in the macro `__inline_mac_fn_calling_macro` (in Nightly builds, run with -Z macro-backtrace for more info) error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:95:23 + --> $DIR/try_err.rs:99:23 | LL | Err(_) => Err(inline!(1))?, | ^^^^^^^^^^^^^^^^ help: try: `return Err(inline!(1))` @@ -45,31 +45,31 @@ LL | Err(_) => Err(inline!(1))?, = note: this error originates in the macro `__inline_mac_fn_calling_macro` (in Nightly builds, run with -Z macro-backtrace for more info) error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:122:9 + --> $DIR/try_err.rs:126:9 | LL | Err(inline!(inline!(String::from("aasdfasdfasdfa"))))?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `return Err(inline!(inline!(String::from("aasdfasdfasdfa"))))` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:129:9 + --> $DIR/try_err.rs:133:9 | LL | Err(io::ErrorKind::WriteZero)? | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `return Poll::Ready(Err(io::ErrorKind::WriteZero.into()))` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:131:9 + --> $DIR/try_err.rs:135:9 | LL | Err(io::Error::new(io::ErrorKind::InvalidInput, "error"))? | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `return Poll::Ready(Err(io::Error::new(io::ErrorKind::InvalidInput, "error")))` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:139:9 + --> $DIR/try_err.rs:143:9 | LL | Err(io::ErrorKind::NotFound)? | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `return Poll::Ready(Some(Err(io::ErrorKind::NotFound.into())))` error: returning an `Err(_)` with the `?` operator - --> $DIR/try_err.rs:148:16 + --> $DIR/try_err.rs:152:16 | LL | return Err(42)?; | ^^^^^^^^ help: try: `Err(42)` From 0d59d1d617531c1fe73cc94a4eaac7db5f23aa72 Mon Sep 17 00:00:00 2001 From: Catherine <114838443+Centri3@users.noreply.github.com> Date: Wed, 12 Jul 2023 21:48:22 -0500 Subject: [PATCH 2/2] Rename to `needless_return_with_question_mark` --- CHANGELOG.md | 2 +- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/returns.rs | 10 +++++----- ....fixed => needless_return_with_question_mark.fixed} | 2 +- ...th_try.rs => needless_return_with_question_mark.rs} | 2 +- ...tderr => needless_return_with_question_mark.stderr} | 4 ++-- tests/ui/try_err.fixed | 2 +- tests/ui/try_err.rs | 2 +- 8 files changed, 13 insertions(+), 13 deletions(-) rename tests/ui/{needless_return_with_try.fixed => needless_return_with_question_mark.fixed} (93%) rename tests/ui/{needless_return_with_try.rs => needless_return_with_question_mark.rs} (93%) rename tests/ui/{needless_return_with_try.stderr => needless_return_with_question_mark.stderr} (54%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47b64d27ad7..a0a4a541cef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5093,7 +5093,7 @@ Released 2018-09-13 [`needless_raw_string_hashes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_string_hashes [`needless_raw_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_strings [`needless_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return -[`needless_return_with_try`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return_with_try +[`needless_return_with_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return_with_question_mark [`needless_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_splitn [`needless_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_update [`neg_cmp_op_on_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_cmp_op_on_partial_ord diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7676e787ee7..a75efd9bbb6 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -569,7 +569,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::return_self_not_must_use::RETURN_SELF_NOT_MUST_USE_INFO, crate::returns::LET_AND_RETURN_INFO, crate::returns::NEEDLESS_RETURN_INFO, - crate::returns::NEEDLESS_RETURN_WITH_TRY_INFO, + crate::returns::NEEDLESS_RETURN_WITH_QUESTION_MARK_INFO, crate::same_name_method::SAME_NAME_METHOD_INFO, crate::self_named_constructors::SELF_NAMED_CONSTRUCTORS_INFO, crate::semicolon_block::SEMICOLON_INSIDE_BLOCK_INFO, diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 58868e97e23..e944da6bc81 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then, span_lint_hir_and_then}; use clippy_utils::source::{snippet_opt, snippet_with_context}; use clippy_utils::visitors::{for_each_expr_with_closures, Descend}; -use clippy_utils::{fn_def_id, path_to_local_id, span_find_starting_semi}; +use clippy_utils::{fn_def_id, is_from_proc_macro, path_to_local_id, span_find_starting_semi}; use core::ops::ControlFlow; use if_chain::if_chain; use rustc_errors::Applicability; @@ -113,8 +113,8 @@ declare_clippy_lint! { /// Ok(()) /// } /// ``` - #[clippy::version = "1.72.0"] - pub NEEDLESS_RETURN_WITH_TRY, + #[clippy::version = "1.73.0"] + pub NEEDLESS_RETURN_WITH_QUESTION_MARK, style, "using a return statement like `return Err(expr)?;` where removing it would suffice" } @@ -158,7 +158,7 @@ impl<'tcx> ToString for RetReplacement<'tcx> { } } -declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN, NEEDLESS_RETURN_WITH_TRY]); +declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN, NEEDLESS_RETURN_WITH_QUESTION_MARK]); impl<'tcx> LateLintPass<'tcx> for Return { fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { @@ -177,7 +177,7 @@ impl<'tcx> LateLintPass<'tcx> for Return { { span_lint_and_sugg( cx, - NEEDLESS_RETURN_WITH_TRY, + NEEDLESS_RETURN_WITH_QUESTION_MARK, expr.span.until(ret.span), "unneeded `return` statement with `?` operator", "remove it", diff --git a/tests/ui/needless_return_with_try.fixed b/tests/ui/needless_return_with_question_mark.fixed similarity index 93% rename from tests/ui/needless_return_with_try.fixed rename to tests/ui/needless_return_with_question_mark.fixed index 96d3e6df2d1..d6e47d07b0f 100644 --- a/tests/ui/needless_return_with_try.fixed +++ b/tests/ui/needless_return_with_question_mark.fixed @@ -1,5 +1,5 @@ //@run-rustfix -//@aux-build:proc_macros.rs +//@aux-build:proc_macros.rs:proc-macro #![allow( clippy::needless_return, clippy::no_effect, diff --git a/tests/ui/needless_return_with_try.rs b/tests/ui/needless_return_with_question_mark.rs similarity index 93% rename from tests/ui/needless_return_with_try.rs rename to tests/ui/needless_return_with_question_mark.rs index 039afba0602..4fc04d363a9 100644 --- a/tests/ui/needless_return_with_try.rs +++ b/tests/ui/needless_return_with_question_mark.rs @@ -1,5 +1,5 @@ //@run-rustfix -//@aux-build:proc_macros.rs +//@aux-build:proc_macros.rs:proc-macro #![allow( clippy::needless_return, clippy::no_effect, diff --git a/tests/ui/needless_return_with_try.stderr b/tests/ui/needless_return_with_question_mark.stderr similarity index 54% rename from tests/ui/needless_return_with_try.stderr rename to tests/ui/needless_return_with_question_mark.stderr index 72f2bba9200..e1d91638d2c 100644 --- a/tests/ui/needless_return_with_try.stderr +++ b/tests/ui/needless_return_with_question_mark.stderr @@ -1,10 +1,10 @@ error: unneeded `return` statement with `?` operator - --> $DIR/needless_return_with_try.rs:28:5 + --> $DIR/needless_return_with_question_mark.rs:28:5 | LL | return Err(())?; | ^^^^^^^ help: remove it | - = note: `-D clippy::needless-return-with-try` implied by `-D warnings` + = note: `-D clippy::needless-return-with-question-mark` implied by `-D warnings` error: aborting due to previous error diff --git a/tests/ui/try_err.fixed b/tests/ui/try_err.fixed index e7e2379fa4c..930489fab76 100644 --- a/tests/ui/try_err.fixed +++ b/tests/ui/try_err.fixed @@ -5,7 +5,7 @@ #![allow( clippy::unnecessary_wraps, clippy::needless_question_mark, - clippy::needless_return_with_try + clippy::needless_return_with_question_mark )] extern crate proc_macros; diff --git a/tests/ui/try_err.rs b/tests/ui/try_err.rs index ece21887ed3..f5baf3d8f74 100644 --- a/tests/ui/try_err.rs +++ b/tests/ui/try_err.rs @@ -5,7 +5,7 @@ #![allow( clippy::unnecessary_wraps, clippy::needless_question_mark, - clippy::needless_return_with_try + clippy::needless_return_with_question_mark )] extern crate proc_macros;