From b7d40bc10394107afc03a126e346b5522beb0703 Mon Sep 17 00:00:00 2001 From: Andrew Pollack Date: Sun, 29 Aug 2021 18:49:09 -0700 Subject: [PATCH] Adding new linting --- CHANGELOG.md | 6 +- clippy_lints/src/if_let_some_result.rs | 76 ------------ clippy_lints/src/lib.rs | 10 +- clippy_lints/src/match_result_ok.rs | 110 ++++++++++++++++++ tests/ui/if_let_some_result.fixed | 28 ----- tests/ui/if_let_some_result.rs | 28 ----- tests/ui/match_result_ok.fixed | 63 ++++++++++ tests/ui/match_result_ok.rs | 63 ++++++++++ ...e_result.stderr => match_result_ok.stderr} | 19 ++- 9 files changed, 261 insertions(+), 142 deletions(-) delete mode 100644 clippy_lints/src/if_let_some_result.rs create mode 100644 clippy_lints/src/match_result_ok.rs delete mode 100644 tests/ui/if_let_some_result.fixed delete mode 100644 tests/ui/if_let_some_result.rs create mode 100644 tests/ui/match_result_ok.fixed create mode 100644 tests/ui/match_result_ok.rs rename tests/ui/{if_let_some_result.stderr => match_result_ok.stderr} (56%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 040c906a722..9518c511171 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ document. [74d1561...master](https://github.com/rust-lang/rust-clippy/compare/74d1561...master) +### New Lints + +* Renamed Lint: `if_let_some_result` is now called [`match_result_ok`]. + ## Rust 1.55 Current beta, release 2021-09-09 @@ -2685,7 +2689,6 @@ Released 2018-09-13 [`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op [`if_let_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_mutex [`if_let_redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_redundant_pattern_matching -[`if_let_some_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_some_result [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else [`if_then_panic`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_panic @@ -2775,6 +2778,7 @@ Released 2018-09-13 [`match_on_vec_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_on_vec_items [`match_overlapping_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_overlapping_arm [`match_ref_pats`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_ref_pats +[`match_result_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_result_ok [`match_same_arms`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_same_arms [`match_single_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding [`match_wild_err_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_wild_err_arm diff --git a/clippy_lints/src/if_let_some_result.rs b/clippy_lints/src/if_let_some_result.rs deleted file mode 100644 index adcd78ed0d4..00000000000 --- a/clippy_lints/src/if_let_some_result.rs +++ /dev/null @@ -1,76 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::higher; -use clippy_utils::method_chain_args; -use clippy_utils::source::snippet_with_applicability; -use clippy_utils::ty::is_type_diagnostic_item; -use if_chain::if_chain; -use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, PatKind, QPath}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::sym; - -declare_clippy_lint! { - /// ### What it does - ///* Checks for unnecessary `ok()` in if let. - /// - /// ### Why is this bad? - /// Calling `ok()` in if let is unnecessary, instead match - /// on `Ok(pat)` - /// - /// ### Example - /// ```ignore - /// for i in iter { - /// if let Some(value) = i.parse().ok() { - /// vec.push(value) - /// } - /// } - /// ``` - /// Could be written: - /// - /// ```ignore - /// for i in iter { - /// if let Ok(value) = i.parse() { - /// vec.push(value) - /// } - /// } - /// ``` - pub IF_LET_SOME_RESULT, - style, - "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead" -} - -declare_lint_pass!(OkIfLet => [IF_LET_SOME_RESULT]); - -impl<'tcx> LateLintPass<'tcx> for OkIfLet { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if_chain! { //begin checking variables - if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(cx, expr); - if let ExprKind::MethodCall(_, ok_span, [ref result_types_0, ..], _) = let_expr.kind; //check is expr.ok() has type Result.ok(, _) - if let PatKind::TupleStruct(QPath::Resolved(_, x), y, _) = let_pat.kind; //get operation - if method_chain_args(let_expr, &["ok"]).is_some(); //test to see if using ok() methoduse std::marker::Sized; - if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(result_types_0), sym::result_type); - if rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| s.print_path(x, false)) == "Some"; - - then { - let mut applicability = Applicability::MachineApplicable; - let some_expr_string = snippet_with_applicability(cx, y[0].span, "", &mut applicability); - let trimmed_ok = snippet_with_applicability(cx, let_expr.span.until(ok_span), "", &mut applicability); - let sugg = format!( - "if let Ok({}) = {}", - some_expr_string, - trimmed_ok.trim().trim_end_matches('.'), - ); - span_lint_and_sugg( - cx, - IF_LET_SOME_RESULT, - expr.span.with_hi(let_expr.span.hi()), - "matching on `Some` with `ok()` is redundant", - &format!("consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string), - sugg, - applicability, - ); - } - } - } -} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1d24d02511b..879f3ba68f4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -225,7 +225,6 @@ mod future_not_send; mod get_last_with_len; mod identity_op; mod if_let_mutex; -mod if_let_some_result; mod if_not_else; mod if_then_panic; mod if_then_some_else_none; @@ -264,6 +263,7 @@ mod map_clone; mod map_err_ignore; mod map_unit_fn; mod match_on_vec_items; +mod match_result_ok; mod matches; mod mem_discriminant; mod mem_forget; @@ -658,7 +658,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: get_last_with_len::GET_LAST_WITH_LEN, identity_op::IDENTITY_OP, if_let_mutex::IF_LET_MUTEX, - if_let_some_result::IF_LET_SOME_RESULT, if_not_else::IF_NOT_ELSE, if_then_panic::IF_THEN_PANIC, if_then_some_else_none::IF_THEN_SOME_ELSE_NONE, @@ -728,6 +727,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: map_unit_fn::OPTION_MAP_UNIT_FN, map_unit_fn::RESULT_MAP_UNIT_FN, match_on_vec_items::MATCH_ON_VEC_ITEMS, + match_result_ok::MATCH_RESULT_OK, matches::INFALLIBLE_DESTRUCTURING_MATCH, matches::MATCH_AS_REF, matches::MATCH_BOOL, @@ -1259,7 +1259,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(get_last_with_len::GET_LAST_WITH_LEN), LintId::of(identity_op::IDENTITY_OP), LintId::of(if_let_mutex::IF_LET_MUTEX), - LintId::of(if_let_some_result::IF_LET_SOME_RESULT), LintId::of(if_then_panic::IF_THEN_PANIC), LintId::of(indexing_slicing::OUT_OF_BOUNDS_INDEXING), LintId::of(infinite_iter::INFINITE_ITER), @@ -1303,6 +1302,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(map_clone::MAP_CLONE), LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN), + LintId::of(match_result_ok::MATCH_RESULT_OK), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(matches::MATCH_AS_REF), LintId::of(matches::MATCH_LIKE_MATCHES_MACRO), @@ -1513,7 +1513,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(functions::DOUBLE_MUST_USE), LintId::of(functions::MUST_USE_UNIT), LintId::of(functions::RESULT_UNIT_ERR), - LintId::of(if_let_some_result::IF_LET_SOME_RESULT), LintId::of(if_then_panic::IF_THEN_PANIC), LintId::of(inherent_to_string::INHERENT_TO_STRING), LintId::of(len_zero::COMPARISON_TO_EMPTY), @@ -1530,6 +1529,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(manual_map::MANUAL_MAP), LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(map_clone::MAP_CLONE), + LintId::of(match_result_ok::MATCH_RESULT_OK), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(matches::MATCH_LIKE_MATCHES_MACRO), LintId::of(matches::MATCH_OVERLAPPING_ARM), @@ -1985,7 +1985,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(missing_doc::MissingDoc::new())); store.register_late_pass(|| Box::new(missing_inline::MissingInline)); store.register_late_pass(move || Box::new(exhaustive_items::ExhaustiveItems)); - store.register_late_pass(|| Box::new(if_let_some_result::OkIfLet)); + store.register_late_pass(|| Box::new(match_result_ok::MatchResultOk)); store.register_late_pass(|| Box::new(partialeq_ne_impl::PartialEqNeImpl)); store.register_late_pass(|| Box::new(unused_io_amount::UnusedIoAmount)); let enum_variant_size_threshold = conf.enum_variant_size_threshold; diff --git a/clippy_lints/src/match_result_ok.rs b/clippy_lints/src/match_result_ok.rs new file mode 100644 index 00000000000..02b09a2dec6 --- /dev/null +++ b/clippy_lints/src/match_result_ok.rs @@ -0,0 +1,110 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::higher; +use clippy_utils::method_chain_args; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::ty::is_type_diagnostic_item; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, PatKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// Checks for unnecessary `ok()` in `while let`. + /// + /// ### Why is this bad? + /// Calling `ok()` in `while let` is unnecessary, instead match + /// on `Ok(pat)` + /// + /// ### Example + /// ```ignore + /// while let Some(value) = iter.next().ok() { + /// vec.push(value) + /// } + /// + /// if let Some(valie) = iter.next().ok() { + /// vec.push(value) + /// } + /// ``` + /// Use instead: + /// ```ignore + /// while let Ok(value) = iter.next() { + /// vec.push(value) + /// } + /// + /// if let Ok(value) = iter.next() { + /// vec.push_value) + /// } + /// ``` + pub MATCH_RESULT_OK, + style, + "usage of `ok()` in `let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead" +} + +declare_lint_pass!(MatchResultOk => [MATCH_RESULT_OK]); + +impl<'tcx> LateLintPass<'tcx> for MatchResultOk { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if_chain! { + if let Some(higher::IfLet { let_pat, let_expr, .. }) = higher::IfLet::hir(cx, expr); + if let ExprKind::MethodCall(_, ok_span, [ref result_types_0, ..], _) = let_expr.kind; //check is expr.ok() has type Result.ok(, _) + if let PatKind::TupleStruct(QPath::Resolved(_, x), y, _) = let_pat.kind; //get operation + if method_chain_args(let_expr, &["ok"]).is_some(); //test to see if using ok() methoduse std::marker::Sized; + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(result_types_0), sym::result_type); + if rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| s.print_path(x, false)) == "Some"; + + then { + + let mut applicability = Applicability::MachineApplicable; + let some_expr_string = snippet_with_applicability(cx, y[0].span, "", &mut applicability); + let trimmed_ok = snippet_with_applicability(cx, let_expr.span.until(ok_span), "", &mut applicability); + let sugg = format!( + "if let Ok({}) = {}", + some_expr_string, + trimmed_ok.trim().trim_end_matches('.'), + ); + span_lint_and_sugg( + cx, + MATCH_RESULT_OK, + expr.span.with_hi(let_expr.span.hi()), + "matching on `Some` with `ok()` is redundant", + &format!("consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string), + sugg, + applicability, + ); + } + } + + if_chain! { + if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr); + if let ExprKind::MethodCall(_, ok_span, [ref result_types_0, ..], _) = let_expr.kind; //check is expr.ok() has type Result.ok(, _) + if let PatKind::TupleStruct(QPath::Resolved(_, x), y, _) = let_pat.kind; //get operation + if method_chain_args(let_expr, &["ok"]).is_some(); //test to see if using ok() methoduse std::marker::Sized; + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(result_types_0), sym::result_type); + if rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| s.print_path(x, false)) == "Some"; + + then { + + let mut applicability = Applicability::MachineApplicable; + let some_expr_string = snippet_with_applicability(cx, y[0].span, "", &mut applicability); + let trimmed_ok = snippet_with_applicability(cx, let_expr.span.until(ok_span), "", &mut applicability); + let sugg = format!( + "while let Ok({}) = {}", + some_expr_string, + trimmed_ok.trim().trim_end_matches('.'), + ); + span_lint_and_sugg( + cx, + MATCH_RESULT_OK, + expr.span.with_hi(let_expr.span.hi()), + "matching on `Some` with `ok()` is redundant", + &format!("consider matching on `Ok({})` and removing the call to `ok` instead", some_expr_string), + sugg, + applicability, + ); + } + } + } +} diff --git a/tests/ui/if_let_some_result.fixed b/tests/ui/if_let_some_result.fixed deleted file mode 100644 index 1bddc47721e..00000000000 --- a/tests/ui/if_let_some_result.fixed +++ /dev/null @@ -1,28 +0,0 @@ -// run-rustfix - -#![warn(clippy::if_let_some_result)] -#![allow(dead_code)] - -fn str_to_int(x: &str) -> i32 { - if let Ok(y) = x.parse() { y } else { 0 } -} - -fn str_to_int_ok(x: &str) -> i32 { - if let Ok(y) = x.parse() { y } else { 0 } -} - -#[rustfmt::skip] -fn strange_some_no_else(x: &str) -> i32 { - { - if let Ok(y) = x . parse() { - return y; - }; - 0 - } -} - -fn negative() { - while let Some(1) = "".parse().ok() {} -} - -fn main() {} diff --git a/tests/ui/if_let_some_result.rs b/tests/ui/if_let_some_result.rs deleted file mode 100644 index d4a52ec9881..00000000000 --- a/tests/ui/if_let_some_result.rs +++ /dev/null @@ -1,28 +0,0 @@ -// run-rustfix - -#![warn(clippy::if_let_some_result)] -#![allow(dead_code)] - -fn str_to_int(x: &str) -> i32 { - if let Some(y) = x.parse().ok() { y } else { 0 } -} - -fn str_to_int_ok(x: &str) -> i32 { - if let Ok(y) = x.parse() { y } else { 0 } -} - -#[rustfmt::skip] -fn strange_some_no_else(x: &str) -> i32 { - { - if let Some(y) = x . parse() . ok () { - return y; - }; - 0 - } -} - -fn negative() { - while let Some(1) = "".parse().ok() {} -} - -fn main() {} diff --git a/tests/ui/match_result_ok.fixed b/tests/ui/match_result_ok.fixed new file mode 100644 index 00000000000..d4760a97567 --- /dev/null +++ b/tests/ui/match_result_ok.fixed @@ -0,0 +1,63 @@ +// run-rustfix + +#![warn(clippy::match_result_ok)] +#![allow(clippy::boxed_local)] +#![allow(dead_code)] + +// Checking `if` cases + +fn str_to_int(x: &str) -> i32 { + if let Ok(y) = x.parse() { y } else { 0 } +} + +fn str_to_int_ok(x: &str) -> i32 { + if let Ok(y) = x.parse() { y } else { 0 } +} + +#[rustfmt::skip] +fn strange_some_no_else(x: &str) -> i32 { + { + if let Ok(y) = x . parse() { + return y; + }; + 0 + } +} + +// Checking `while` cases + +struct Wat { + counter: i32, +} + +impl Wat { + fn next(&mut self) -> Result { + self.counter += 1; + if self.counter < 5 { + Ok(self.counter) + } else { + Err("Oh no") + } + } +} + +fn base_1(x: i32) { + let mut wat = Wat { counter: x }; + while let Ok(a) = wat.next() { + println!("{}", a); + } +} + +fn base_2(x: i32) { + let mut wat = Wat { counter: x }; + while let Ok(a) = wat.next() { + println!("{}", a); + } +} + +fn base_3(test_func: Box>) { + // Expected to stay as is + while let Some(_b) = test_func.ok() {} +} + +fn main() {} diff --git a/tests/ui/match_result_ok.rs b/tests/ui/match_result_ok.rs new file mode 100644 index 00000000000..0b818723d98 --- /dev/null +++ b/tests/ui/match_result_ok.rs @@ -0,0 +1,63 @@ +// run-rustfix + +#![warn(clippy::match_result_ok)] +#![allow(clippy::boxed_local)] +#![allow(dead_code)] + +// Checking `if` cases + +fn str_to_int(x: &str) -> i32 { + if let Some(y) = x.parse().ok() { y } else { 0 } +} + +fn str_to_int_ok(x: &str) -> i32 { + if let Ok(y) = x.parse() { y } else { 0 } +} + +#[rustfmt::skip] +fn strange_some_no_else(x: &str) -> i32 { + { + if let Some(y) = x . parse() . ok () { + return y; + }; + 0 + } +} + +// Checking `while` cases + +struct Wat { + counter: i32, +} + +impl Wat { + fn next(&mut self) -> Result { + self.counter += 1; + if self.counter < 5 { + Ok(self.counter) + } else { + Err("Oh no") + } + } +} + +fn base_1(x: i32) { + let mut wat = Wat { counter: x }; + while let Some(a) = wat.next().ok() { + println!("{}", a); + } +} + +fn base_2(x: i32) { + let mut wat = Wat { counter: x }; + while let Ok(a) = wat.next() { + println!("{}", a); + } +} + +fn base_3(test_func: Box>) { + // Expected to stay as is + while let Some(_b) = test_func.ok() {} +} + +fn main() {} diff --git a/tests/ui/if_let_some_result.stderr b/tests/ui/match_result_ok.stderr similarity index 56% rename from tests/ui/if_let_some_result.stderr rename to tests/ui/match_result_ok.stderr index bc3a5e7698d..cc3bc8c76ff 100644 --- a/tests/ui/if_let_some_result.stderr +++ b/tests/ui/match_result_ok.stderr @@ -1,17 +1,17 @@ error: matching on `Some` with `ok()` is redundant - --> $DIR/if_let_some_result.rs:7:5 + --> $DIR/match_result_ok.rs:10:5 | LL | if let Some(y) = x.parse().ok() { y } else { 0 } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = note: `-D clippy::if-let-some-result` implied by `-D warnings` + = note: `-D clippy::match-result-ok` implied by `-D warnings` help: consider matching on `Ok(y)` and removing the call to `ok` instead | LL | if let Ok(y) = x.parse() { y } else { 0 } | ~~~~~~~~~~~~~~~~~~~~~~~~ error: matching on `Some` with `ok()` is redundant - --> $DIR/if_let_some_result.rs:17:9 + --> $DIR/match_result_ok.rs:20:9 | LL | if let Some(y) = x . parse() . ok () { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -21,5 +21,16 @@ help: consider matching on `Ok(y)` and removing the call to `ok` instead LL | if let Ok(y) = x . parse() { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -error: aborting due to 2 previous errors +error: matching on `Some` with `ok()` is redundant + --> $DIR/match_result_ok.rs:46:5 + | +LL | while let Some(a) = wat.next().ok() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider matching on `Ok(a)` and removing the call to `ok` instead + | +LL | while let Ok(a) = wat.next() { + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 3 previous errors