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
This commit is contained in:
Manish Goregaokar
GitHub
commit
dc695f53cd
5 changed files with 171 additions and 77 deletions
|
|
@ -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,
|
||||
);
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue