Skip to content

Commit 6e4c556

Browse files
pksunkarapksunkara
committed
Preserve type annotations when present
1 parent 6cf138e commit 6e4c556

7 files changed

+932
-224
lines changed

‎clippy_lints/src/methods/mod.rs

Lines changed: 9 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -276,52 +276,28 @@ declare_clippy_lint! {
276276

277277
declare_clippy_lint! {
278278
/// ### What it does
279-
/// Checks for `.unwrap()` or `.unwrap_err()` calls on `Result`s and `.unwrap()` call on `Option`s.
279+
/// Checks for `.unwrap()` related calls on `Result`s and `Option`s that are constructed.
280280
///
281281
/// ### Why is this bad?
282-
/// It is better to handle the `None` or `Err` case,
283-
/// or at least call `.expect(_)` with a more helpful message. Still, for a lot of
284-
/// quick-and-dirty code, `unwrap` is a good choice, which is why this lint is
285-
/// `Allow` by default.
286-
///
287-
/// `result.unwrap()` will let the thread panic on `Err` values.
288-
/// Normally, you want to implement more sophisticated error handling,
289-
/// and propagate errors upwards with `?` operator.
290-
///
291-
/// Even if you want to panic on errors, not all `Error`s implement good
292-
/// messages on display. Therefore, it may be beneficial to look at the places
293-
/// where they may get displayed. Activate this lint to do just that.
282+
/// It is better to write the value directly without the indirection.
294283
///
295284
/// ### Examples
296285
/// ```rust
297-
/// # let option = Some(1);
298-
/// # let result: Result<usize, ()> = Ok(1);
299-
/// option.unwrap();
300-
/// result.unwrap();
286+
/// let val1 = Some(1).unwrap();
287+
/// let val2 = Ok::<_, ()>(1).unwrap();
288+
/// let val3 = Err::<(), _>(1).unwrap_err();
301289
/// ```
302290
///
303291
/// Use instead:
304292
/// ```rust
305-
/// # let option = Some(1);
306-
/// # let result: Result<usize, ()> = Ok(1);
307-
/// option.expect("more helpful message");
308-
/// result.expect("more helpful message");
309-
/// ```
310-
///
311-
/// If [expect_used](#expect_used) is enabled, instead:
312-
/// ```rust,ignore
313-
/// # let option = Some(1);
314-
/// # let result: Result<usize, ()> = Ok(1);
315-
/// option?;
316-
///
317-
/// // or
318-
///
319-
/// result?;
293+
/// let val1 = 1;
294+
/// let val2 = 1;
295+
/// let val3 = 1;
320296
/// ```
321297
#[clippy::version = "1.69.0"]
322298
pub UNNECESSARY_LITERAL_UNWRAP,
323299
complexity,
324-
"checks for calls of `unwrap()` or `expect()` on `Some()` that cannot fail"
300+
"using `unwrap()` related calls on `Result` and `Option` constructors"
325301
}
326302

327303
declare_clippy_lint! {

‎clippy_lints/src/methods/unnecessary_literal_unwrap.rs

Lines changed: 66 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,26 @@
1-
use clippy_utils::{diagnostics::span_lint_and_then, is_res_lang_ctor, path_res};
1+
use clippy_utils::{diagnostics::span_lint_and_then, is_res_lang_ctor, last_path_segment, path_res, MaybePath};
22
use rustc_errors::Applicability;
33
use rustc_hir as hir;
44
use rustc_lint::LateContext;
55

66
use super::UNNECESSARY_LITERAL_UNWRAP;
77

8+
fn get_ty_from_args<'a>(args: Option<&'a [hir::GenericArg<'a>]>, index: usize) -> Option<&'a hir::Ty<'a>> {
9+
let args = args?;
10+
11+
if args.len() <= index {
12+
return None;
13+
}
14+
15+
match args[index] {
16+
hir::GenericArg::Type(ty) => match ty.kind {
17+
hir::TyKind::Infer => None,
18+
_ => Some(ty),
19+
},
20+
_ => None,
21+
}
22+
}
23+
824
pub(super) fn check(
925
cx: &LateContext<'_>,
1026
expr: &hir::Expr<'_>,
@@ -14,59 +30,66 @@ pub(super) fn check(
1430
) {
1531
let init = clippy_utils::expr_or_init(cx, recv);
1632

17-
let (constructor, call_args) = if let hir::ExprKind::Call(call, call_args) = init.kind {
18-
if is_res_lang_ctor(cx, path_res(cx, call), hir::LangItem::OptionSome) {
19-
("Some", call_args)
20-
} else if is_res_lang_ctor(cx, path_res(cx, call), hir::LangItem::ResultOk) {
21-
("Ok", call_args)
22-
} else if is_res_lang_ctor(cx, path_res(cx, call), hir::LangItem::ResultErr) {
23-
("Err", call_args)
33+
let (constructor, call_args, ty) = if let hir::ExprKind::Call(call, call_args) = init.kind {
34+
let Some(qpath) = call.qpath_opt() else { return };
35+
36+
let args = last_path_segment(qpath).args.map(|args| args.args);
37+
let res = cx.qpath_res(qpath, call.hir_id());
38+
39+
if is_res_lang_ctor(cx, res, hir::LangItem::OptionSome) {
40+
("Some", call_args, get_ty_from_args(args, 0))
41+
} else if is_res_lang_ctor(cx, res, hir::LangItem::ResultOk) {
42+
("Ok", call_args, get_ty_from_args(args, 0))
43+
} else if is_res_lang_ctor(cx, res, hir::LangItem::ResultErr) {
44+
("Err", call_args, get_ty_from_args(args, 1))
2445
} else {
2546
return;
2647
}
2748
} else if is_res_lang_ctor(cx, path_res(cx, init), hir::LangItem::OptionNone) {
2849
let call_args: &[hir::Expr<'_>] = &[];
29-
("None", call_args)
50+
("None", call_args, None)
3051
} else {
3152
return;
3253
};
3354

3455
let help_message = format!("used `{method}()` on `{constructor}` value");
3556
let suggestion_message = format!("remove the `{constructor}` and `{method}()`");
3657

37-
if init.span == recv.span {
38-
span_lint_and_then(cx, UNNECESSARY_LITERAL_UNWRAP, expr.span, &help_message, |diag| {
39-
let suggestions = match (constructor, method) {
40-
("None", "unwrap") => vec![(expr.span, "panic!()".to_string())],
41-
("None", "expect") => vec![
42-
(expr.span.with_hi(args[0].span.lo()), "panic!(".to_string()),
43-
(expr.span.with_lo(args[0].span.hi()), ")".to_string()),
44-
],
45-
("Ok", "unwrap_err") | ("Err", "unwrap") => vec![
46-
(
47-
recv.span.with_hi(call_args[0].span.lo()),
48-
"panic!(\"{:?}\", ".to_string(),
49-
),
50-
(expr.span.with_lo(call_args[0].span.hi()), ")".to_string()),
51-
],
52-
("Ok", "expect_err") | ("Err", "expect") => vec![
53-
(
54-
recv.span.with_hi(call_args[0].span.lo()),
55-
"panic!(\"{1}: {:?}\", ".to_string(),
56-
),
57-
(call_args[0].span.with_lo(args[0].span.lo()), ", ".to_string()),
58-
],
59-
_ => vec![
60-
(recv.span.with_hi(call_args[0].span.lo()), String::new()),
61-
(expr.span.with_lo(call_args[0].span.hi()), String::new()),
62-
],
63-
};
58+
span_lint_and_then(cx, UNNECESSARY_LITERAL_UNWRAP, expr.span, &help_message, |diag| {
59+
let suggestions = match (constructor, method, ty) {
60+
("None", "unwrap", _) => Some(vec![(expr.span, "panic!()".to_string())]),
61+
("None", "expect", _) => Some(vec![
62+
(expr.span.with_hi(args[0].span.lo()), "panic!(".to_string()),
63+
(expr.span.with_lo(args[0].span.hi()), ")".to_string()),
64+
]),
65+
(_, _, Some(_)) => None,
66+
("Ok", "unwrap_err", None) | ("Err", "unwrap", None) => Some(vec![
67+
(
68+
recv.span.with_hi(call_args[0].span.lo()),
69+
"panic!(\"{:?}\", ".to_string(),
70+
),
71+
(expr.span.with_lo(call_args[0].span.hi()), ")".to_string()),
72+
]),
73+
("Ok", "expect_err", None) | ("Err", "expect", None) => Some(vec![
74+
(
75+
recv.span.with_hi(call_args[0].span.lo()),
76+
"panic!(\"{1}: {:?}\", ".to_string(),
77+
),
78+
(call_args[0].span.with_lo(args[0].span.lo()), ", ".to_string()),
79+
]),
80+
(_, _, None) => Some(vec![
81+
(recv.span.with_hi(call_args[0].span.lo()), String::new()),
82+
(expr.span.with_lo(call_args[0].span.hi()), String::new()),
83+
]),
84+
};
6485

65-
diag.multipart_suggestion(suggestion_message, suggestions, Applicability::MachineApplicable);
66-
});
67-
} else {
68-
span_lint_and_then(cx, UNNECESSARY_LITERAL_UNWRAP, expr.span, &help_message, |diag| {
69-
diag.span_help(init.span, suggestion_message);
70-
});
71-
}
86+
match (init.span == recv.span, suggestions) {
87+
(true, Some(suggestions)) => {
88+
diag.multipart_suggestion(suggestion_message, suggestions, Applicability::MachineApplicable);
89+
},
90+
_ => {
91+
diag.span_help(init.span, suggestion_message);
92+
},
93+
}
94+
});
7295
}

‎tests/ui/unnecessary_literal_unwrap.fixed

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,49 @@
11
//@run-rustfix
22
#![warn(clippy::unnecessary_literal_unwrap)]
33
#![allow(unreachable_code)]
4-
#![allow(clippy::unnecessary_lazy_evaluations)]
4+
#![allow(
5+
clippy::unnecessary_lazy_evaluations,
6+
clippy::diverging_sub_expression,
7+
clippy::let_unit_value,
8+
clippy::no_effect
9+
)]
510

611
fn unwrap_option_some() {
712
let _val = 1;
813
let _val = 1;
14+
15+
1;
16+
1;
917
}
1018

1119
fn unwrap_option_none() {
20+
let _val = panic!();
21+
let _val = panic!("this always happens");
22+
1223
panic!();
1324
panic!("this always happens");
1425
}
1526

1627
fn unwrap_result_ok() {
1728
let _val = 1;
1829
let _val = 1;
30+
let _val = panic!("{:?}", 1);
31+
let _val = panic!("{1}: {:?}", 1, "this always happens");
32+
33+
1;
34+
1;
1935
panic!("{:?}", 1);
2036
panic!("{1}: {:?}", 1, "this always happens");
2137
}
2238

2339
fn unwrap_result_err() {
2440
let _val = 1;
2541
let _val = 1;
42+
let _val = panic!("{:?}", 1);
43+
let _val = panic!("{1}: {:?}", 1, "this always happens");
44+
45+
1;
46+
1;
2647
panic!("{:?}", 1);
2748
panic!("{1}: {:?}", 1, "this always happens");
2849
}
@@ -31,12 +52,20 @@ fn unwrap_methods_option() {
3152
let _val = 1;
3253
let _val = 1;
3354
let _val = 1;
55+
56+
1;
57+
1;
58+
1;
3459
}
3560

3661
fn unwrap_methods_result() {
3762
let _val = 1;
3863
let _val = 1;
3964
let _val = 1;
65+
66+
1;
67+
1;
68+
1;
4069
}
4170

4271
fn main() {

‎tests/ui/unnecessary_literal_unwrap.rs

Lines changed: 43 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,71 @@
11
//@run-rustfix
22
#![warn(clippy::unnecessary_literal_unwrap)]
33
#![allow(unreachable_code)]
4-
#![allow(clippy::unnecessary_lazy_evaluations)]
4+
#![allow(
5+
clippy::unnecessary_lazy_evaluations,
6+
clippy::diverging_sub_expression,
7+
clippy::let_unit_value,
8+
clippy::no_effect
9+
)]
510

611
fn unwrap_option_some() {
712
let _val = Some(1).unwrap();
813
let _val = Some(1).expect("this never happens");
14+
15+
Some(1).unwrap();
16+
Some(1).expect("this never happens");
917
}
1018

1119
fn unwrap_option_none() {
12-
None::<usize>.unwrap();
13-
None::<usize>.expect("this always happens");
20+
let _val = None::<()>.unwrap();
21+
let _val = None::<()>.expect("this always happens");
22+
23+
None::<()>.unwrap();
24+
None::<()>.expect("this always happens");
1425
}
1526

1627
fn unwrap_result_ok() {
17-
let _val = Ok::<usize, ()>(1).unwrap();
18-
let _val = Ok::<usize, ()>(1).expect("this never happens");
19-
Ok::<usize, ()>(1).unwrap_err();
20-
Ok::<usize, ()>(1).expect_err("this always happens");
28+
let _val = Ok::<_, ()>(1).unwrap();
29+
let _val = Ok::<_, ()>(1).expect("this never happens");
30+
let _val = Ok::<_, ()>(1).unwrap_err();
31+
let _val = Ok::<_, ()>(1).expect_err("this always happens");
32+
33+
Ok::<_, ()>(1).unwrap();
34+
Ok::<_, ()>(1).expect("this never happens");
35+
Ok::<_, ()>(1).unwrap_err();
36+
Ok::<_, ()>(1).expect_err("this always happens");
2137
}
2238

2339
fn unwrap_result_err() {
24-
let _val = Err::<(), usize>(1).unwrap_err();
25-
let _val = Err::<(), usize>(1).expect_err("this never happens");
26-
Err::<(), usize>(1).unwrap();
27-
Err::<(), usize>(1).expect("this always happens");
40+
let _val = Err::<(), _>(1).unwrap_err();
41+
let _val = Err::<(), _>(1).expect_err("this never happens");
42+
let _val = Err::<(), _>(1).unwrap();
43+
let _val = Err::<(), _>(1).expect("this always happens");
44+
45+
Err::<(), _>(1).unwrap_err();
46+
Err::<(), _>(1).expect_err("this never happens");
47+
Err::<(), _>(1).unwrap();
48+
Err::<(), _>(1).expect("this always happens");
2849
}
2950

3051
fn unwrap_methods_option() {
3152
let _val = Some(1).unwrap_or(2);
3253
let _val = Some(1).unwrap_or_default();
3354
let _val = Some(1).unwrap_or_else(|| 2);
55+
56+
Some(1).unwrap_or(2);
57+
Some(1).unwrap_or_default();
58+
Some(1).unwrap_or_else(|| 2);
3459
}
3560

3661
fn unwrap_methods_result() {
37-
let _val = Ok::<usize, ()>(1).unwrap_or(2);
38-
let _val = Ok::<usize, ()>(1).unwrap_or_default();
39-
let _val = Ok::<usize, ()>(1).unwrap_or_else(|_| 2);
62+
let _val = Ok::<_, ()>(1).unwrap_or(2);
63+
let _val = Ok::<_, ()>(1).unwrap_or_default();
64+
let _val = Ok::<_, ()>(1).unwrap_or_else(|_| 2);
65+
66+
Ok::<_, ()>(1).unwrap_or(2);
67+
Ok::<_, ()>(1).unwrap_or_default();
68+
Ok::<_, ()>(1).unwrap_or_else(|_| 2);
4069
}
4170

4271
fn main() {

0 commit comments

Comments
 (0)