Skip to content

Commit fd0428f

Browse files
borsbors
committed
Auto merge of rust-lang#5032 - JohnTitor:add-sugg-some-result, r=flip1995
Add suggestions for `if_let_some_result` Fixes rust-lang#4991 This approach may be fragile though... changelog: Add suggestions for `if_let_some_result`
2 parents f728bcd + c9f8d03 commit fd0428f

File tree

8 files changed

+124
-54
lines changed

8 files changed

+124
-54
lines changed

Diff for: ‎clippy_lints/src/ok_if_let.rs renamed to ‎clippy_lints/src/if_let_some_result.rs

+23-10
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
use crate::utils::{match_type, method_chain_args, paths, snippet, span_help_and_lint};
1+
use crate::utils::{match_type, method_chain_args, paths, snippet_with_applicability, span_lint_and_sugg};
22
use if_chain::if_chain;
3+
use rustc_errors::Applicability;
34
use rustc_hir::*;
45
use rustc_lint::{LateContext, LateLintPass};
56
use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -39,20 +40,32 @@ declare_lint_pass!(OkIfLet => [IF_LET_SOME_RESULT]);
3940
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OkIfLet {
4041
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
4142
if_chain! { //begin checking variables
42-
if let ExprKind::Match(ref op, ref body, ref source) = expr.kind; //test if expr is a match
43-
if let MatchSource::IfLetDesugar { .. } = *source; //test if it is an If Let
44-
if let ExprKind::MethodCall(_, _, ref result_types) = op.kind; //check is expr.ok() has type Result<T,E>.ok()
43+
if let ExprKind::Match(ref op, ref body, source) = expr.kind; //test if expr is a match
44+
if let MatchSource::IfLetDesugar { .. } = source; //test if it is an If Let
45+
if let ExprKind::MethodCall(_, ok_span, ref result_types) = op.kind; //check is expr.ok() has type Result<T,E>.ok()
4546
if let PatKind::TupleStruct(QPath::Resolved(_, ref x), ref y, _) = body[0].pat.kind; //get operation
4647
if method_chain_args(op, &["ok"]).is_some(); //test to see if using ok() methoduse std::marker::Sized;
48+
let is_result_type = match_type(cx, cx.tables.expr_ty(&result_types[0]), &paths::RESULT);
49+
if print::to_string(print::NO_ANN, |s| s.print_path(x, false)) == "Some" && is_result_type;
4750

4851
then {
49-
let is_result_type = match_type(cx, cx.tables.expr_ty(&result_types[0]), &paths::RESULT);
50-
let some_expr_string = snippet(cx, y[0].span, "");
51-
if print::to_string(print::NO_ANN, |s| s.print_path(x, false)) == "Some" && is_result_type {
52-
span_help_and_lint(cx, IF_LET_SOME_RESULT, expr.span,
52+
let mut applicability = Applicability::MachineApplicable;
53+
let some_expr_string = snippet_with_applicability(cx, y[0].span, "", &mut applicability);
54+
let trimmed_ok = snippet_with_applicability(cx, op.span.until(ok_span), "", &mut applicability);
55+
let sugg = format!(
56+
"if let Ok({}) = {}",
57+
some_expr_string,
58+
trimmed_ok.trim().trim_end_matches('.'),
59+
);
60+
span_lint_and_sugg(
61+
cx,
62+
IF_LET_SOME_RESULT,
63+
expr.span.with_hi(op.span.hi()),
5364
"Matching on `Some` with `ok()` is redundant",
54-
&format!("Consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string));
55-
}
65+
&format!("Consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string),
66+
sugg,
67+
applicability,
68+
);
5669
}
5770
}
5871
}

Diff for: ‎clippy_lints/src/lib.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,7 @@ pub mod functions;
210210
pub mod get_last_with_len;
211211
pub mod identity_conversion;
212212
pub mod identity_op;
213+
pub mod if_let_some_result;
213214
pub mod if_not_else;
214215
pub mod implicit_return;
215216
pub mod indexing_slicing;
@@ -263,7 +264,6 @@ pub mod new_without_default;
263264
pub mod no_effect;
264265
pub mod non_copy_const;
265266
pub mod non_expressive_names;
266-
pub mod ok_if_let;
267267
pub mod open_options;
268268
pub mod overflow_check_conditional;
269269
pub mod panic_unimplemented;
@@ -545,6 +545,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
545545
&get_last_with_len::GET_LAST_WITH_LEN,
546546
&identity_conversion::IDENTITY_CONVERSION,
547547
&identity_op::IDENTITY_OP,
548+
&if_let_some_result::IF_LET_SOME_RESULT,
548549
&if_not_else::IF_NOT_ELSE,
549550
&implicit_return::IMPLICIT_RETURN,
550551
&indexing_slicing::INDEXING_SLICING,
@@ -703,7 +704,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
703704
&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS,
704705
&non_expressive_names::MANY_SINGLE_CHAR_NAMES,
705706
&non_expressive_names::SIMILAR_NAMES,
706-
&ok_if_let::IF_LET_SOME_RESULT,
707707
&open_options::NONSENSICAL_OPEN_OPTIONS,
708708
&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
709709
&panic_unimplemented::PANIC,
@@ -904,7 +904,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
904904
store.register_late_pass(|| box eval_order_dependence::EvalOrderDependence);
905905
store.register_late_pass(|| box missing_doc::MissingDoc::new());
906906
store.register_late_pass(|| box missing_inline::MissingInline);
907-
store.register_late_pass(|| box ok_if_let::OkIfLet);
907+
store.register_late_pass(|| box if_let_some_result::OkIfLet);
908908
store.register_late_pass(|| box redundant_pattern_matching::RedundantPatternMatching);
909909
store.register_late_pass(|| box partialeq_ne_impl::PartialEqNeImpl);
910910
store.register_late_pass(|| box unused_io_amount::UnusedIoAmount);
@@ -1153,6 +1153,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
11531153
LintId::of(&get_last_with_len::GET_LAST_WITH_LEN),
11541154
LintId::of(&identity_conversion::IDENTITY_CONVERSION),
11551155
LintId::of(&identity_op::IDENTITY_OP),
1156+
LintId::of(&if_let_some_result::IF_LET_SOME_RESULT),
11561157
LintId::of(&indexing_slicing::OUT_OF_BOUNDS_INDEXING),
11571158
LintId::of(&infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH),
11581159
LintId::of(&infinite_iter::INFINITE_ITER),
@@ -1265,7 +1266,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12651266
LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST),
12661267
LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),
12671268
LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES),
1268-
LintId::of(&ok_if_let::IF_LET_SOME_RESULT),
12691269
LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS),
12701270
LintId::of(&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL),
12711271
LintId::of(&panic_unimplemented::PANIC_PARAMS),
@@ -1367,6 +1367,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
13671367
LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING),
13681368
LintId::of(&functions::DOUBLE_MUST_USE),
13691369
LintId::of(&functions::MUST_USE_UNIT),
1370+
LintId::of(&if_let_some_result::IF_LET_SOME_RESULT),
13701371
LintId::of(&infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH),
13711372
LintId::of(&inherent_to_string::INHERENT_TO_STRING),
13721373
LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),
@@ -1413,7 +1414,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
14131414
LintId::of(&new_without_default::NEW_WITHOUT_DEFAULT),
14141415
LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),
14151416
LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES),
1416-
LintId::of(&ok_if_let::IF_LET_SOME_RESULT),
14171417
LintId::of(&panic_unimplemented::PANIC_PARAMS),
14181418
LintId::of(&ptr::CMP_NULL),
14191419
LintId::of(&ptr::PTR_ARG),

Diff for: ‎src/lintlist/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -705,7 +705,7 @@ pub const ALL_LINTS: [Lint; 347] = [
705705
group: "style",
706706
desc: "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead",
707707
deprecation: None,
708-
module: "ok_if_let",
708+
module: "if_let_some_result",
709709
},
710710
Lint {
711711
name: "if_not_else",

Diff for: ‎tests/ui/if_let_some_result.fixed

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::if_let_some_result)]
4+
5+
fn str_to_int(x: &str) -> i32 {
6+
if let Ok(y) = x.parse() {
7+
y
8+
} else {
9+
0
10+
}
11+
}
12+
13+
fn str_to_int_ok(x: &str) -> i32 {
14+
if let Ok(y) = x.parse() {
15+
y
16+
} else {
17+
0
18+
}
19+
}
20+
21+
#[rustfmt::skip]
22+
fn strange_some_no_else(x: &str) -> i32 {
23+
{
24+
if let Ok(y) = x . parse() {
25+
return y;
26+
};
27+
0
28+
}
29+
}
30+
31+
fn main() {
32+
let _ = str_to_int("1");
33+
let _ = str_to_int_ok("2");
34+
let _ = strange_some_no_else("3");
35+
}

Diff for: ‎tests/ui/if_let_some_result.rs

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// run-rustfix
2+
3+
#![warn(clippy::if_let_some_result)]
4+
5+
fn str_to_int(x: &str) -> i32 {
6+
if let Some(y) = x.parse().ok() {
7+
y
8+
} else {
9+
0
10+
}
11+
}
12+
13+
fn str_to_int_ok(x: &str) -> i32 {
14+
if let Ok(y) = x.parse() {
15+
y
16+
} else {
17+
0
18+
}
19+
}
20+
21+
#[rustfmt::skip]
22+
fn strange_some_no_else(x: &str) -> i32 {
23+
{
24+
if let Some(y) = x . parse() . ok () {
25+
return y;
26+
};
27+
0
28+
}
29+
}
30+
31+
fn main() {
32+
let _ = str_to_int("1");
33+
let _ = str_to_int_ok("2");
34+
let _ = strange_some_no_else("3");
35+
}

Diff for: ‎tests/ui/if_let_some_result.stderr

+25
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
error: Matching on `Some` with `ok()` is redundant
2+
--> $DIR/if_let_some_result.rs:6:5
3+
|
4+
LL | if let Some(y) = x.parse().ok() {
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::if-let-some-result` implied by `-D warnings`
8+
help: Consider matching on `Ok(y)` and removing the call to `ok` instead
9+
|
10+
LL | if let Ok(y) = x.parse() {
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^
12+
13+
error: Matching on `Some` with `ok()` is redundant
14+
--> $DIR/if_let_some_result.rs:24:9
15+
|
16+
LL | if let Some(y) = x . parse() . ok () {
17+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
18+
|
19+
help: Consider matching on `Ok(y)` and removing the call to `ok` instead
20+
|
21+
LL | if let Ok(y) = x . parse() {
22+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
23+
24+
error: aborting due to 2 previous errors
25+

Diff for: ‎tests/ui/ok_if_let.rs

-23
This file was deleted.

Diff for: ‎tests/ui/ok_if_let.stderr

-15
This file was deleted.

0 commit comments

Comments
 (0)