Reword `needless_question_mark` diagnostics and docs (#14682)

Fixes https://github.com/rust-lang/rust-clippy/issues/14675 by making it
clearer that the constructor needs to be removed as well
Also fixes https://github.com/rust-lang/rust-clippy/issues/8392

changelog: none

5 files changed, 171 insertions, 77 deletions

diff --git a/clippy_lints/src/needless_question_mark.rs b/clippy_lints/src/needless_question_mark.rs
index 72b0a80260e..2a2160c3be2 100644
--- a/clippy_lints/src/needless_question_mark.rs
+++ b/clippy_lints/src/needless_question_mark.rs
@@ -1,6 +1,5 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::span_lint_hir_and_then;
 use clippy_utils::path_res;
-use clippy_utils::source::snippet;
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::{Block, Body, Expr, ExprKind, LangItem, MatchSource, QPath};
@@ -9,52 +8,38 @@ use rustc_session::declare_lint_pass;
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Suggests alternatives for useless applications of `?` in terminating expressions
+    /// Suggests replacing `Ok(x?)` or `Some(x?)` with `x` in return positions where the `?` operator
+    /// is not needed to convert the type of `x`.
     ///
     /// ### Why is this bad?
     /// There's no reason to use `?` to short-circuit when execution of the body will end there anyway.
     ///
     /// ### Example
     /// ```no_run
-    /// struct TO {
-    ///     magic: Option<usize>,
+    /// # use std::num::ParseIntError;
+    /// fn f(s: &str) -> Option<usize> {
+    ///     Some(s.find('x')?)
     /// }
     ///
-    /// fn f(to: TO) -> Option<usize> {
-    ///     Some(to.magic?)
+    /// fn g(s: &str) -> Result<usize, ParseIntError> {
+    ///     Ok(s.parse()?)
     /// }
-    ///
-    /// struct TR {
-    ///     magic: Result<usize, bool>,
-    /// }
-    ///
-    /// fn g(tr: Result<TR, bool>) -> Result<usize, bool> {
-    ///     tr.and_then(|t| Ok(t.magic?))
-    /// }
-    ///
     /// ```
     /// Use instead:
     /// ```no_run
-    /// struct TO {
-    ///     magic: Option<usize>,
+    /// # use std::num::ParseIntError;
+    /// fn f(s: &str) -> Option<usize> {
+    ///     s.find('x')
     /// }
     ///
-    /// fn f(to: TO) -> Option<usize> {
-    ///     to.magic
-    /// }
-    ///
-    /// struct TR {
-    ///     magic: Result<usize, bool>,
-    /// }
-    ///
-    /// fn g(tr: Result<TR, bool>) -> Result<usize, bool> {
-    ///     tr.and_then(|t| t.magic)
+    /// fn g(s: &str) -> Result<usize, ParseIntError> {
+    ///     s.parse()
     /// }
     /// ```
     #[clippy::version = "1.51.0"]
     pub NEEDLESS_QUESTION_MARK,
     complexity,
-    "Suggest `value.inner_option` instead of `Some(value.inner_option?)`. The same goes for `Result<T, E>`."
+    "using `Ok(x?)` or `Some(x?)` where `x` would be equivalent"
 }
 
 declare_lint_pass!(NeedlessQuestionMark => [NEEDLESS_QUESTION_MARK]);
@@ -111,10 +96,10 @@ fn check(cx: &LateContext<'_>, expr: &Expr<'_>) {
     if let ExprKind::Call(path, [arg]) = expr.kind
         && let Res::Def(DefKind::Ctor(..), ctor_id) = path_res(cx, path)
         && let Some(variant_id) = cx.tcx.opt_parent(ctor_id)
-        && let sugg_remove = if cx.tcx.lang_items().option_some_variant() == Some(variant_id) {
-            "Some()"
+        && let variant = if cx.tcx.lang_items().option_some_variant() == Some(variant_id) {
+            "Some"
         } else if cx.tcx.lang_items().result_ok_variant() == Some(variant_id) {
-            "Ok()"
+            "Ok"
         } else {
             return;
         }
@@ -126,14 +111,25 @@ fn check(cx: &LateContext<'_>, expr: &Expr<'_>) {
         && let inner_ty = cx.typeck_results().expr_ty(inner_expr)
         && expr_ty == inner_ty
     {
-        span_lint_and_sugg(
+        span_lint_hir_and_then(
             cx,
             NEEDLESS_QUESTION_MARK,
+            expr.hir_id,
             expr.span,
-            "question mark operator is useless here",
-            format!("try removing question mark and `{sugg_remove}`"),
-            format!("{}", snippet(cx, inner_expr.span, r#""...""#)),
-            Applicability::MachineApplicable,
+            format!("enclosing `{variant}` and `?` operator are unneeded"),
+            |diag| {
+                diag.multipart_suggestion(
+                    format!("remove the enclosing `{variant}` and `?` operator"),
+                    vec![
+                        (expr.span.until(inner_expr.span), String::new()),
+                        (
+                            inner_expr.span.shrink_to_hi().to(expr.span.shrink_to_hi()),
+                            String::new(),
+                        ),
+                    ],
+                    Applicability::MachineApplicable,
+                );
+            },
         );
     }
 }
diff --git a/tests/ui/needless_question_mark.stderr b/tests/ui/needless_question_mark.stderr
index 55da4f28976..8516cee48e6 100644
--- a/tests/ui/needless_question_mark.stderr
+++ b/tests/ui/needless_question_mark.stderr
@@ -1,100 +1,188 @@
-error: question mark operator is useless here
+error: enclosing `Some` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:20:12
    |
 LL |     return Some(to.magic?);
-   |            ^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `to.magic`
+   |            ^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::needless-question-mark` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_question_mark)]`
+help: remove the enclosing `Some` and `?` operator
+   |
+LL -     return Some(to.magic?);
+LL +     return to.magic;
+   |
 
-error: question mark operator is useless here
+error: enclosing `Some` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:29:12
    |
 LL |     return Some(to.magic?)
-   |            ^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `to.magic`
+   |            ^^^^^^^^^^^^^^^
+   |
+help: remove the enclosing `Some` and `?` operator
+   |
+LL -     return Some(to.magic?)
+LL +     return to.magic
+   |
 
-error: question mark operator is useless here
+error: enclosing `Some` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:35:5
    |
 LL |     Some(to.magic?)
-   |     ^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `to.magic`
+   |     ^^^^^^^^^^^^^^^
+   |
+help: remove the enclosing `Some` and `?` operator
+   |
+LL -     Some(to.magic?)
+LL +     to.magic
+   |
 
-error: question mark operator is useless here
+error: enclosing `Some` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:41:21
    |
 LL |     to.and_then(|t| Some(t.magic?))
-   |                     ^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `t.magic`
+   |                     ^^^^^^^^^^^^^^
+   |
+help: remove the enclosing `Some` and `?` operator
+   |
+LL -     to.and_then(|t| Some(t.magic?))
+LL +     to.and_then(|t| t.magic)
+   |
 
-error: question mark operator is useless here
+error: enclosing `Some` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:51:9
    |
 LL |         Some(t.magic?)
-   |         ^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `t.magic`
+   |         ^^^^^^^^^^^^^^
+   |
+help: remove the enclosing `Some` and `?` operator
+   |
+LL -         Some(t.magic?)
+LL +         t.magic
+   |
 
-error: question mark operator is useless here
+error: enclosing `Ok` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:57:12
    |
 LL |     return Ok(tr.magic?);
-   |            ^^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `tr.magic`
+   |            ^^^^^^^^^^^^^
+   |
+help: remove the enclosing `Ok` and `?` operator
+   |
+LL -     return Ok(tr.magic?);
+LL +     return tr.magic;
+   |
 
-error: question mark operator is useless here
+error: enclosing `Ok` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:65:12
    |
 LL |     return Ok(tr.magic?)
-   |            ^^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `tr.magic`
+   |            ^^^^^^^^^^^^^
+   |
+help: remove the enclosing `Ok` and `?` operator
+   |
+LL -     return Ok(tr.magic?)
+LL +     return tr.magic
+   |
 
-error: question mark operator is useless here
+error: enclosing `Ok` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:70:5
    |
 LL |     Ok(tr.magic?)
-   |     ^^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `tr.magic`
+   |     ^^^^^^^^^^^^^
+   |
+help: remove the enclosing `Ok` and `?` operator
+   |
+LL -     Ok(tr.magic?)
+LL +     tr.magic
+   |
 
-error: question mark operator is useless here
+error: enclosing `Ok` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:75:21
    |
 LL |     tr.and_then(|t| Ok(t.magic?))
-   |                     ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `t.magic`
+   |                     ^^^^^^^^^^^^
+   |
+help: remove the enclosing `Ok` and `?` operator
+   |
+LL -     tr.and_then(|t| Ok(t.magic?))
+LL +     tr.and_then(|t| t.magic)
+   |
 
-error: question mark operator is useless here
+error: enclosing `Ok` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:84:9
    |
 LL |         Ok(t.magic?)
-   |         ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `t.magic`
+   |         ^^^^^^^^^^^^
+   |
+help: remove the enclosing `Ok` and `?` operator
+   |
+LL -         Ok(t.magic?)
+LL +         t.magic
+   |
 
-error: question mark operator is useless here
+error: enclosing `Ok` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:92:16
    |
 LL |         return Ok(t.magic?);
-   |                ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `t.magic`
+   |                ^^^^^^^^^^^^
+   |
+help: remove the enclosing `Ok` and `?` operator
+   |
+LL -         return Ok(t.magic?);
+LL +         return t.magic;
+   |
 
-error: question mark operator is useless here
+error: enclosing `Some` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:128:27
    |
 LL |         || -> Option<_> { Some(Some($expr)?) }()
-   |                           ^^^^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `Some($expr)`
+   |                           ^^^^^^^^^^^^^^^^^^
 ...
 LL |     let _x = some_and_qmark_in_macro!(x?);
    |              ---------------------------- in this macro invocation
    |
    = note: this error originates in the macro `some_and_qmark_in_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
+help: remove the enclosing `Some` and `?` operator
+   |
+LL -         || -> Option<_> { Some(Some($expr)?) }()
+LL +         || -> Option<_> { Some($expr) }()
+   |
 
-error: question mark operator is useless here
+error: enclosing `Some` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:140:5
    |
 LL |     Some(to.magic?)
-   |     ^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `to.magic`
+   |     ^^^^^^^^^^^^^^^
+   |
+help: remove the enclosing `Some` and `?` operator
+   |
+LL -     Some(to.magic?)
+LL +     to.magic
+   |
 
-error: question mark operator is useless here
+error: enclosing `Ok` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:149:5
    |
 LL |     Ok(s.magic?)
-   |     ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `s.magic`
+   |     ^^^^^^^^^^^^
+   |
+help: remove the enclosing `Ok` and `?` operator
+   |
+LL -     Ok(s.magic?)
+LL +     s.magic
+   |
 
-error: question mark operator is useless here
+error: enclosing `Some` and `?` operator are unneeded
   --> tests/ui/needless_question_mark.rs:154:7
    |
 LL |     { Some(a?) }
-   |       ^^^^^^^^ help: try removing question mark and `Some()`: `a`
+   |       ^^^^^^^^
+   |
+help: remove the enclosing `Some` and `?` operator
+   |
+LL -     { Some(a?) }
+LL +     { a }
+   |
 
 error: aborting due to 15 previous errors
 
diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed
index 6bd071d07f5..507bc2b29d8 100644
--- a/tests/ui/question_mark.fixed
+++ b/tests/ui/question_mark.fixed
@@ -301,6 +301,11 @@ fn pattern() -> Result<(), PatternedError> {
     res
 }
 
+fn expect_expr(a: Option<usize>) -> Option<usize> {
+    #[expect(clippy::needless_question_mark)]
+    Some(a?)
+}
+
 fn main() {}
 
 // `?` is not the same as `return None;` if inside of a try block
diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs
index dd093c9bf48..64b51b849ed 100644
--- a/tests/ui/question_mark.rs
+++ b/tests/ui/question_mark.rs
@@ -371,6 +371,11 @@ fn pattern() -> Result<(), PatternedError> {
     res
 }
 
+fn expect_expr(a: Option<usize>) -> Option<usize> {
+    #[expect(clippy::needless_question_mark)]
+    Some(a?)
+}
+
 fn main() {}
 
 // `?` is not the same as `return None;` if inside of a try block
diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr
index 8fe04b895ce..d8ce4420aee 100644
--- a/tests/ui/question_mark.stderr
+++ b/tests/ui/question_mark.stderr
@@ -198,7 +198,7 @@ LL | |     }
    | |_____^ help: replace it with: `func_returning_result()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:390:13
+  --> tests/ui/question_mark.rs:395:13
    |
 LL | /             if a.is_none() {
 LL | |
@@ -208,7 +208,7 @@ LL | |             }
    | |_____________^ help: replace it with: `a?;`
 
 error: this `let...else` may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:451:5
+  --> tests/ui/question_mark.rs:456:5
    |
 LL | /     let Some(v) = bar.foo.owned.clone() else {
 LL | |         return None;
@@ -216,7 +216,7 @@ LL | |     };
    | |______^ help: replace it with: `let v = bar.foo.owned.clone()?;`
 
 error: this `let...else` may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:466:5
+  --> tests/ui/question_mark.rs:471:5
    |
 LL | /     let Some(ref x) = foo.opt_x else {
 LL | |         return None;
@@ -224,7 +224,7 @@ LL | |     };
    | |______^ help: replace it with: `let x = foo.opt_x.as_ref()?;`
 
 error: this `let...else` may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:476:5
+  --> tests/ui/question_mark.rs:481:5
    |
 LL | /     let Some(ref mut x) = foo.opt_x else {
 LL | |         return None;
@@ -232,7 +232,7 @@ LL | |     };
    | |______^ help: replace it with: `let x = foo.opt_x.as_mut()?;`
 
 error: this `let...else` may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:487:5
+  --> tests/ui/question_mark.rs:492:5
    |
 LL | /     let Some(ref x @ ref y) = foo.opt_x else {
 LL | |         return None;
@@ -240,7 +240,7 @@ LL | |     };
    | |______^ help: replace it with: `let x @ y = foo.opt_x.as_ref()?;`
 
 error: this `let...else` may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:491:5
+  --> tests/ui/question_mark.rs:496:5
    |
 LL | /     let Some(ref x @ WrapperStructWithString(_)) = bar else {
 LL | |         return None;
@@ -248,7 +248,7 @@ LL | |     };
    | |______^ help: replace it with: `let x @ &WrapperStructWithString(_) = bar.as_ref()?;`
 
 error: this `let...else` may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:495:5
+  --> tests/ui/question_mark.rs:500:5
    |
 LL | /     let Some(ref mut x @ WrapperStructWithString(_)) = bar else {
 LL | |         return None;
@@ -256,7 +256,7 @@ LL | |     };
    | |______^ help: replace it with: `let x @ &mut WrapperStructWithString(_) = bar.as_mut()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:517:5
+  --> tests/ui/question_mark.rs:522:5
    |
 LL | /     if arg.is_none() {
 LL | |
@@ -265,7 +265,7 @@ LL | |     }
    | |_____^ help: replace it with: `arg?;`
 
 error: this `match` expression can be replaced with `?`
-  --> tests/ui/question_mark.rs:521:15
+  --> tests/ui/question_mark.rs:526:15
    |
 LL |       let val = match arg {
    |  _______________^
@@ -276,7 +276,7 @@ LL | |     };
    | |_____^ help: try instead: `arg?`
 
 error: this `let...else` may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:531:5
+  --> tests/ui/question_mark.rs:536:5
    |
 LL | /     let Some(a) = *a else {
 LL | |         return None;