From ad319484f1505420586a68c9d77493898579d331 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 23 Feb 2024 16:32:34 +0100 Subject: [PATCH] Add new `unnecessary_get_then_check` clippy lint --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/mod.rs | 47 ++++++++-- .../src/methods/unnecessary_get_then_check.rs | 85 +++++++++++++++++++ 4 files changed, 129 insertions(+), 5 deletions(-) create mode 100644 clippy_lints/src/methods/unnecessary_get_then_check.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 5805511c70f..26dce1f9265 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5732,6 +5732,7 @@ Released 2018-09-13 [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map [`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map [`unnecessary_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fold +[`unnecessary_get_then_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_get_then_check [`unnecessary_join`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_join [`unnecessary_lazy_evaluations`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations [`unnecessary_literal_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_literal_unwrap diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 79fd7639307..b6a35bb3e03 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -455,6 +455,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::UNNECESSARY_FILTER_MAP_INFO, crate::methods::UNNECESSARY_FIND_MAP_INFO, crate::methods::UNNECESSARY_FOLD_INFO, + crate::methods::UNNECESSARY_GET_THEN_CHECK_INFO, crate::methods::UNNECESSARY_JOIN_INFO, crate::methods::UNNECESSARY_LAZY_EVALUATIONS_INFO, crate::methods::UNNECESSARY_LITERAL_UNWRAP_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 1452547807b..820f4c85890 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -110,6 +110,7 @@ mod unit_hash; mod unnecessary_fallible_conversions; mod unnecessary_filter_map; mod unnecessary_fold; +mod unnecessary_get_then_check; mod unnecessary_iter_cloned; mod unnecessary_join; mod unnecessary_lazy_eval; @@ -4011,6 +4012,35 @@ declare_clippy_lint! { r#"creating a `CStr` through functions when `c""` literals can be used"# } +declare_clippy_lint! { + /// ### What it does + /// Checks the usage of `.get().is_some()` or `.get().is_none()` on std map types. + /// + /// ### Why is this bad? + /// It can be done in one call with `.contains()`/`.contains_keys()`. + /// + /// ### Example + /// ```no_run + /// # use std::collections::HashSet; + /// let s: HashSet = HashSet::new(); + /// if s.get("a").is_some() { + /// // code + /// } + /// ``` + /// Use instead: + /// ```no_run + /// # use std::collections::HashSet; + /// let s: HashSet = HashSet::new(); + /// if s.contains("a") { + /// // code + /// } + /// ``` + #[clippy::version = "1.78.0"] + pub UNNECESSARY_GET_THEN_CHECK, + suspicious, + "calling `.get().is_some()` or `.get().is_none()` instead of `.contains()` or `.contains_key()`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -4171,6 +4201,7 @@ impl_lint_pass!(Methods => [ OPTION_AS_REF_CLONED, UNNECESSARY_RESULT_MAP_OR_ELSE, MANUAL_C_STR_LITERALS, + UNNECESSARY_GET_THEN_CHECK, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -4587,8 +4618,8 @@ impl Methods { }, ("is_file", []) => filetype_is_file::check(cx, expr, recv), ("is_digit", [radix]) => is_digit_ascii_radix::check(cx, expr, recv, radix, &self.msrv), - ("is_none", []) => check_is_some_is_none(cx, expr, recv, false), - ("is_some", []) => check_is_some_is_none(cx, expr, recv, true), + ("is_none", []) => check_is_some_is_none(cx, expr, recv, call_span, false), + ("is_some", []) => check_is_some_is_none(cx, expr, recv, call_span, true), ("iter" | "iter_mut" | "into_iter", []) => { iter_on_single_or_empty_collections::check(cx, expr, name, recv); }, @@ -4899,9 +4930,15 @@ impl Methods { } } -fn check_is_some_is_none(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, is_some: bool) { - if let Some((name @ ("find" | "position" | "rposition"), f_recv, [arg], span, _)) = method_call(recv) { - search_is_some::check(cx, expr, name, is_some, f_recv, arg, recv, span); +fn check_is_some_is_none(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, call_span: Span, is_some: bool) { + match method_call(recv) { + Some((name @ ("find" | "position" | "rposition"), f_recv, [arg], span, _)) => { + search_is_some::check(cx, expr, name, is_some, f_recv, arg, recv, span); + }, + Some(("get", f_recv, [arg], _, _)) => { + unnecessary_get_then_check::check(cx, call_span, recv, f_recv, arg, is_some); + }, + _ => {}, } } diff --git a/clippy_lints/src/methods/unnecessary_get_then_check.rs b/clippy_lints/src/methods/unnecessary_get_then_check.rs new file mode 100644 index 00000000000..cc8053ef507 --- /dev/null +++ b/clippy_lints/src/methods/unnecessary_get_then_check.rs @@ -0,0 +1,85 @@ +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::source::snippet_opt; +use clippy_utils::ty::is_type_diagnostic_item; + +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::Ty; +use rustc_span::{sym, Span}; + +use super::UNNECESSARY_GET_THEN_CHECK; + +fn is_a_std_set_type(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + is_type_diagnostic_item(cx, ty, sym::HashSet) || is_type_diagnostic_item(cx, ty, sym::BTreeSet) +} + +fn is_a_std_map_type(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + is_type_diagnostic_item(cx, ty, sym::HashMap) || is_type_diagnostic_item(cx, ty, sym::BTreeMap) +} + +pub(super) fn check( + cx: &LateContext<'_>, + call_span: Span, + get_call: &Expr<'_>, + get_caller: &Expr<'_>, + arg: &Expr<'_>, + is_some: bool, +) { + let caller_ty = cx.typeck_results().expr_ty(get_caller); + + let is_set = is_a_std_set_type(cx, caller_ty); + let is_map = is_a_std_map_type(cx, caller_ty); + + if !is_set && !is_map { + return; + } + let ExprKind::MethodCall(path, _, _, get_call_span) = get_call.kind else { + return; + }; + let both_calls_span = get_call_span.with_hi(call_span.hi()); + if let Some(snippet) = snippet_opt(cx, both_calls_span) + && let Some(arg_snippet) = snippet_opt(cx, arg.span) + { + let generics_snippet = if let Some(generics) = path.args + && let Some(generics_snippet) = snippet_opt(cx, generics.span_ext) + { + format!("::{generics_snippet}") + } else { + String::new() + }; + let suggestion = if is_set { + format!("contains{generics_snippet}({arg_snippet})") + } else { + format!("contains_key{generics_snippet}({arg_snippet})") + }; + if is_some { + span_lint_and_sugg( + cx, + UNNECESSARY_GET_THEN_CHECK, + both_calls_span, + &format!("unnecessary use of `{snippet}`"), + "replace it with", + suggestion, + Applicability::MaybeIncorrect, + ); + } else if let Some(caller_snippet) = snippet_opt(cx, get_caller.span) { + let full_span = get_caller.span.with_hi(call_span.hi()); + + span_lint_and_then( + cx, + UNNECESSARY_GET_THEN_CHECK, + both_calls_span, + &format!("unnecessary use of `{snippet}`"), + |diag| { + diag.span_suggestion( + full_span, + "replace it with", + format!("{}{caller_snippet}.{suggestion}", if is_some { "" } else { "!" }), + Applicability::MaybeIncorrect, + ); + }, + ); + } + } +}