Auto merge of #11031 - Centri3:needless_return, r=giraffate

New lint [`needless_return_with_try`]

Closes #10902

Rather than having a config option, this will just suggest removing the "return"; if `try_err` is used as well, then it'll be added again but without the `?`.

changelog: New lint [`needless_return_with_try`]

CHANGELOG.md

@@ -5097,6 +5097,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_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

clippy_lints/src/declared_lints.rs

@@ -576,6 +576,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_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,

clippy_lints/src/returns.rs

@@ -1,12 +1,14 @@
-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};
+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;
 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<dyn Error>> {
+    ///     if x == 0 {
+    ///         return Err(...)?;
+    ///     }
+    ///     Ok(())
+    /// }
+    /// ```
+    /// simplify to
+    /// ```rust,ignore
+    /// fn foo(x: usize) -> Result<(), Box<dyn Error>> {
+    ///     if x == 0 {
+    ///         Err(...)?;
+    ///     }
+    ///     Ok(())
+    /// }
+    /// ```
+    /// if paired with `try_err`, use instead:
+    /// ```rust,ignore
+    /// fn foo(x: usize) -> Result<(), Box<dyn Error>> {
+    ///     if x == 0 {
+    ///         return Err(...);
+    ///     }
+    ///     Ok(())
+    /// }
+    /// ```
+    #[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"
+}
+
 #[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_QUESTION_MARK]);
 
 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_QUESTION_MARK,
+                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! {

tests/ui/needless_return_with_question_mark.fixed

@@ -0,0 +1,40 @@
+//@run-rustfix
+//@aux-build:proc_macros.rs:proc-macro
+#![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<u32, u32> {
+    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(())
+}

tests/ui/needless_return_with_question_mark.rs

@@ -0,0 +1,40 @@
+//@run-rustfix
+//@aux-build:proc_macros.rs:proc-macro
+#![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<u32, u32> {
+    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(())
+}

tests/ui/needless_return_with_question_mark.stderr

@@ -0,0 +1,10 @@
+error: unneeded `return` statement with `?` operator
+  --> $DIR/needless_return_with_question_mark.rs:28:5
+   |
+LL |     return Err(())?;
+   |     ^^^^^^^ help: remove it
+   |
+   = note: `-D clippy::needless-return-with-question-mark` implied by `-D warnings`
+
+error: aborting due to previous error
+

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_question_mark
+)]
 
 extern crate proc_macros;
 use proc_macros::{external, inline_macros};

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_question_mark
+)]
 
 extern crate proc_macros;
 use proc_macros::{external, inline_macros};

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)`