Skip to content

Commit dc695f5

Browse files
ManishearthManishearth
authored
Reword needless_question_mark diagnostics and docs (#14682)
Fixes #14675 by making it clearer that the constructor needs to be removed as well Also fixes #8392 changelog: none
2 parents aa27ae3 + 7c5312b commit dc695f5

File tree

5 files changed

+171
-77
lines changed

5 files changed

+171
-77
lines changed

‎clippy_lints/src/needless_question_mark.rs

+33-37
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
1+
use clippy_utils::diagnostics::span_lint_hir_and_then;
22
use clippy_utils::path_res;
3-
use clippy_utils::source::snippet;
43
use rustc_errors::Applicability;
54
use rustc_hir::def::{DefKind, Res};
65
use rustc_hir::{Block, Body, Expr, ExprKind, LangItem, MatchSource, QPath};
@@ -9,52 +8,38 @@ use rustc_session::declare_lint_pass;
98

109
declare_clippy_lint! {
1110
/// ### What it does
12-
/// Suggests alternatives for useless applications of `?` in terminating expressions
11+
/// Suggests replacing `Ok(x?)` or `Some(x?)` with `x` in return positions where the `?` operator
12+
/// is not needed to convert the type of `x`.
1313
///
1414
/// ### Why is this bad?
1515
/// There's no reason to use `?` to short-circuit when execution of the body will end there anyway.
1616
///
1717
/// ### Example
1818
/// ```no_run
19-
/// struct TO {
20-
/// magic: Option<usize>,
19+
/// # use std::num::ParseIntError;
20+
/// fn f(s: &str) -> Option<usize> {
21+
/// Some(s.find('x')?)
2122
/// }
2223
///
23-
/// fn f(to: TO) -> Option<usize> {
24-
/// Some(to.magic?)
24+
/// fn g(s: &str) -> Result<usize, ParseIntError> {
25+
/// Ok(s.parse()?)
2526
/// }
26-
///
27-
/// struct TR {
28-
/// magic: Result<usize, bool>,
29-
/// }
30-
///
31-
/// fn g(tr: Result<TR, bool>) -> Result<usize, bool> {
32-
/// tr.and_then(|t| Ok(t.magic?))
33-
/// }
34-
///
3527
/// ```
3628
/// Use instead:
3729
/// ```no_run
38-
/// struct TO {
39-
/// magic: Option<usize>,
30+
/// # use std::num::ParseIntError;
31+
/// fn f(s: &str) -> Option<usize> {
32+
/// s.find('x')
4033
/// }
4134
///
42-
/// fn f(to: TO) -> Option<usize> {
43-
/// to.magic
44-
/// }
45-
///
46-
/// struct TR {
47-
/// magic: Result<usize, bool>,
48-
/// }
49-
///
50-
/// fn g(tr: Result<TR, bool>) -> Result<usize, bool> {
51-
/// tr.and_then(|t| t.magic)
35+
/// fn g(s: &str) -> Result<usize, ParseIntError> {
36+
/// s.parse()
5237
/// }
5338
/// ```
5439
#[clippy::version = "1.51.0"]
5540
pub NEEDLESS_QUESTION_MARK,
5641
complexity,
57-
"Suggest `value.inner_option` instead of `Some(value.inner_option?)`. The same goes for `Result<T, E>`."
42+
"using `Ok(x?)` or `Some(x?)` where `x` would be equivalent"
5843
}
5944

6045
declare_lint_pass!(NeedlessQuestionMark => [NEEDLESS_QUESTION_MARK]);
@@ -111,10 +96,10 @@ fn check(cx: &LateContext<'_>, expr: &Expr<'_>) {
11196
if let ExprKind::Call(path, [arg]) = expr.kind
11297
&& let Res::Def(DefKind::Ctor(..), ctor_id) = path_res(cx, path)
11398
&& let Some(variant_id) = cx.tcx.opt_parent(ctor_id)
114-
&& let sugg_remove = if cx.tcx.lang_items().option_some_variant() == Some(variant_id) {
115-
"Some()"
99+
&& let variant = if cx.tcx.lang_items().option_some_variant() == Some(variant_id) {
100+
"Some"
116101
} else if cx.tcx.lang_items().result_ok_variant() == Some(variant_id) {
117-
"Ok()"
102+
"Ok"
118103
} else {
119104
return;
120105
}
@@ -126,14 +111,25 @@ fn check(cx: &LateContext<'_>, expr: &Expr<'_>) {
126111
&& let inner_ty = cx.typeck_results().expr_ty(inner_expr)
127112
&& expr_ty == inner_ty
128113
{
129-
span_lint_and_sugg(
114+
span_lint_hir_and_then(
130115
cx,
131116
NEEDLESS_QUESTION_MARK,
117+
expr.hir_id,
132118
expr.span,
133-
"question mark operator is useless here",
134-
format!("try removing question mark and `{sugg_remove}`"),
135-
format!("{}", snippet(cx, inner_expr.span, r#""...""#)),
136-
Applicability::MachineApplicable,
119+
format!("enclosing `{variant}` and `?` operator are unneeded"),
120+
|diag| {
121+
diag.multipart_suggestion(
122+
format!("remove the enclosing `{variant}` and `?` operator"),
123+
vec![
124+
(expr.span.until(inner_expr.span), String::new()),
125+
(
126+
inner_expr.span.shrink_to_hi().to(expr.span.shrink_to_hi()),
127+
String::new(),
128+
),
129+
],
130+
Applicability::MachineApplicable,
131+
);
132+
},
137133
);
138134
}
139135
}

‎tests/ui/needless_question_mark.stderr

+118-30
Original file line numberDiff line numberDiff line change
@@ -1,100 +1,188 @@
1-
error: question mark operator is useless here
1+
error: enclosing `Some` and `?` operator are unneeded
22
--> tests/ui/needless_question_mark.rs:20:12
33
|
44
LL | return Some(to.magic?);
5-
| ^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `to.magic`
5+
| ^^^^^^^^^^^^^^^
66
|
77
= note: `-D clippy::needless-question-mark` implied by `-D warnings`
88
= help: to override `-D warnings` add `#[allow(clippy::needless_question_mark)]`
9+
help: remove the enclosing `Some` and `?` operator
10+
|
11+
LL - return Some(to.magic?);
12+
LL + return to.magic;
13+
|
914

10-
error: question mark operator is useless here
15+
error: enclosing `Some` and `?` operator are unneeded
1116
--> tests/ui/needless_question_mark.rs:29:12
1217
|
1318
LL | return Some(to.magic?)
14-
| ^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `to.magic`
19+
| ^^^^^^^^^^^^^^^
20+
|
21+
help: remove the enclosing `Some` and `?` operator
22+
|
23+
LL - return Some(to.magic?)
24+
LL + return to.magic
25+
|
1526

16-
error: question mark operator is useless here
27+
error: enclosing `Some` and `?` operator are unneeded
1728
--> tests/ui/needless_question_mark.rs:35:5
1829
|
1930
LL | Some(to.magic?)
20-
| ^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `to.magic`
31+
| ^^^^^^^^^^^^^^^
32+
|
33+
help: remove the enclosing `Some` and `?` operator
34+
|
35+
LL - Some(to.magic?)
36+
LL + to.magic
37+
|
2138

22-
error: question mark operator is useless here
39+
error: enclosing `Some` and `?` operator are unneeded
2340
--> tests/ui/needless_question_mark.rs:41:21
2441
|
2542
LL | to.and_then(|t| Some(t.magic?))
26-
| ^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `t.magic`
43+
| ^^^^^^^^^^^^^^
44+
|
45+
help: remove the enclosing `Some` and `?` operator
46+
|
47+
LL - to.and_then(|t| Some(t.magic?))
48+
LL + to.and_then(|t| t.magic)
49+
|
2750

28-
error: question mark operator is useless here
51+
error: enclosing `Some` and `?` operator are unneeded
2952
--> tests/ui/needless_question_mark.rs:51:9
3053
|
3154
LL | Some(t.magic?)
32-
| ^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `t.magic`
55+
| ^^^^^^^^^^^^^^
56+
|
57+
help: remove the enclosing `Some` and `?` operator
58+
|
59+
LL - Some(t.magic?)
60+
LL + t.magic
61+
|
3362

34-
error: question mark operator is useless here
63+
error: enclosing `Ok` and `?` operator are unneeded
3564
--> tests/ui/needless_question_mark.rs:57:12
3665
|
3766
LL | return Ok(tr.magic?);
38-
| ^^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `tr.magic`
67+
| ^^^^^^^^^^^^^
68+
|
69+
help: remove the enclosing `Ok` and `?` operator
70+
|
71+
LL - return Ok(tr.magic?);
72+
LL + return tr.magic;
73+
|
3974

40-
error: question mark operator is useless here
75+
error: enclosing `Ok` and `?` operator are unneeded
4176
--> tests/ui/needless_question_mark.rs:65:12
4277
|
4378
LL | return Ok(tr.magic?)
44-
| ^^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `tr.magic`
79+
| ^^^^^^^^^^^^^
80+
|
81+
help: remove the enclosing `Ok` and `?` operator
82+
|
83+
LL - return Ok(tr.magic?)
84+
LL + return tr.magic
85+
|
4586

46-
error: question mark operator is useless here
87+
error: enclosing `Ok` and `?` operator are unneeded
4788
--> tests/ui/needless_question_mark.rs:70:5
4889
|
4990
LL | Ok(tr.magic?)
50-
| ^^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `tr.magic`
91+
| ^^^^^^^^^^^^^
92+
|
93+
help: remove the enclosing `Ok` and `?` operator
94+
|
95+
LL - Ok(tr.magic?)
96+
LL + tr.magic
97+
|
5198

52-
error: question mark operator is useless here
99+
error: enclosing `Ok` and `?` operator are unneeded
53100
--> tests/ui/needless_question_mark.rs:75:21
54101
|
55102
LL | tr.and_then(|t| Ok(t.magic?))
56-
| ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `t.magic`
103+
| ^^^^^^^^^^^^
104+
|
105+
help: remove the enclosing `Ok` and `?` operator
106+
|
107+
LL - tr.and_then(|t| Ok(t.magic?))
108+
LL + tr.and_then(|t| t.magic)
109+
|
57110

58-
error: question mark operator is useless here
111+
error: enclosing `Ok` and `?` operator are unneeded
59112
--> tests/ui/needless_question_mark.rs:84:9
60113
|
61114
LL | Ok(t.magic?)
62-
| ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `t.magic`
115+
| ^^^^^^^^^^^^
116+
|
117+
help: remove the enclosing `Ok` and `?` operator
118+
|
119+
LL - Ok(t.magic?)
120+
LL + t.magic
121+
|
63122

64-
error: question mark operator is useless here
123+
error: enclosing `Ok` and `?` operator are unneeded
65124
--> tests/ui/needless_question_mark.rs:92:16
66125
|
67126
LL | return Ok(t.magic?);
68-
| ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `t.magic`
127+
| ^^^^^^^^^^^^
128+
|
129+
help: remove the enclosing `Ok` and `?` operator
130+
|
131+
LL - return Ok(t.magic?);
132+
LL + return t.magic;
133+
|
69134

70-
error: question mark operator is useless here
135+
error: enclosing `Some` and `?` operator are unneeded
71136
--> tests/ui/needless_question_mark.rs:128:27
72137
|
73138
LL | || -> Option<_> { Some(Some($expr)?) }()
74-
| ^^^^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `Some($expr)`
139+
| ^^^^^^^^^^^^^^^^^^
75140
...
76141
LL | let _x = some_and_qmark_in_macro!(x?);
77142
| ---------------------------- in this macro invocation
78143
|
79144
= note: this error originates in the macro `some_and_qmark_in_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
145+
help: remove the enclosing `Some` and `?` operator
146+
|
147+
LL - || -> Option<_> { Some(Some($expr)?) }()
148+
LL + || -> Option<_> { Some($expr) }()
149+
|
80150

81-
error: question mark operator is useless here
151+
error: enclosing `Some` and `?` operator are unneeded
82152
--> tests/ui/needless_question_mark.rs:140:5
83153
|
84154
LL | Some(to.magic?)
85-
| ^^^^^^^^^^^^^^^ help: try removing question mark and `Some()`: `to.magic`
155+
| ^^^^^^^^^^^^^^^
156+
|
157+
help: remove the enclosing `Some` and `?` operator
158+
|
159+
LL - Some(to.magic?)
160+
LL + to.magic
161+
|
86162

87-
error: question mark operator is useless here
163+
error: enclosing `Ok` and `?` operator are unneeded
88164
--> tests/ui/needless_question_mark.rs:149:5
89165
|
90166
LL | Ok(s.magic?)
91-
| ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `s.magic`
167+
| ^^^^^^^^^^^^
168+
|
169+
help: remove the enclosing `Ok` and `?` operator
170+
|
171+
LL - Ok(s.magic?)
172+
LL + s.magic
173+
|
92174

93-
error: question mark operator is useless here
175+
error: enclosing `Some` and `?` operator are unneeded
94176
--> tests/ui/needless_question_mark.rs:154:7
95177
|
96178
LL | { Some(a?) }
97-
| ^^^^^^^^ help: try removing question mark and `Some()`: `a`
179+
| ^^^^^^^^
180+
|
181+
help: remove the enclosing `Some` and `?` operator
182+
|
183+
LL - { Some(a?) }
184+
LL + { a }
185+
|
98186

99187
error: aborting due to 15 previous errors
100188

‎tests/ui/question_mark.fixed

+5
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,11 @@ fn pattern() -> Result<(), PatternedError> {
301301
res
302302
}
303303

304+
fn expect_expr(a: Option<usize>) -> Option<usize> {
305+
#[expect(clippy::needless_question_mark)]
306+
Some(a?)
307+
}
308+
304309
fn main() {}
305310

306311
// `?` is not the same as `return None;` if inside of a try block

‎tests/ui/question_mark.rs

+5
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,11 @@ fn pattern() -> Result<(), PatternedError> {
371371
res
372372
}
373373

374+
fn expect_expr(a: Option<usize>) -> Option<usize> {
375+
#[expect(clippy::needless_question_mark)]
376+
Some(a?)
377+
}
378+
374379
fn main() {}
375380

376381
// `?` is not the same as `return None;` if inside of a try block

0 commit comments

Comments
 (0)