Auto merge of #12339 - GuillaumeGomez:add-unnecessary_get_then_check, r=llogiq

bors · bors · commit 76e48641199d · 2024-02-25T10:39:24.000Z
Add new `unnecessary_get_then_check` lint

No issue linked to this as far as I can see. It's a lint I discovered that could be added when I worked on another lint.

r? `@llogiq`

changelog: Add new `unnecessary_get_then_check` lint
diff --git 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
@@ -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
@@ -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<String> = HashSet::new();
+    /// if s.get("a").is_some() {
+    ///     // code
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # use std::collections::HashSet;
+    /// let s: HashSet<String> = 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
@@ -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,
+                    );
+                },
+            );
+        }
+    }
+}
diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs
@@ -749,15 +749,15 @@ fn print_stats(old_stats: HashMap<String, usize>, new_stats: HashMap<&String, us
     // list all new counts  (key is in new stats but not in old stats)
     new_stats_deduped
         .iter()
-        .filter(|(new_key, _)| old_stats_deduped.get::<str>(new_key).is_none())
+        .filter(|(new_key, _)| !old_stats_deduped.contains_key::<str>(new_key))
         .for_each(|(new_key, new_value)| {
             println!("{new_key} 0 => {new_value}");
         });
 
     // list all changed counts (key is in both maps but value differs)
     new_stats_deduped
         .iter()
-        .filter(|(new_key, _new_val)| old_stats_deduped.get::<str>(new_key).is_some())
+        .filter(|(new_key, _new_val)| old_stats_deduped.contains_key::<str>(new_key))
         .for_each(|(new_key, new_val)| {
             let old_val = old_stats_deduped.get::<str>(new_key).unwrap();
             println!("{new_key} {old_val} => {new_val}");
@@ -766,7 +766,7 @@ fn print_stats(old_stats: HashMap<String, usize>, new_stats: HashMap<&String, us
     // list all gone counts (key is in old status but not in new stats)
     old_stats_deduped
         .iter()
-        .filter(|(old_key, _)| new_stats_deduped.get::<&String>(old_key).is_none())
+        .filter(|(old_key, _)| !new_stats_deduped.contains_key::<&String>(old_key))
         .filter(|(old_key, _)| lint_filter.is_empty() || lint_filter.contains(old_key))
         .for_each(|(old_key, old_value)| {
             println!("{old_key} {old_value} => 0");
diff --git a/tests/ui/iter_filter_is_some.fixed b/tests/ui/iter_filter_is_some.fixed
@@ -4,7 +4,8 @@
     clippy::result_filter_map,
     clippy::needless_borrow,
     clippy::option_filter_map,
-    clippy::redundant_closure
+    clippy::redundant_closure,
+    clippy::unnecessary_get_then_check
 )]
 
 use std::collections::HashMap;
diff --git a/tests/ui/iter_filter_is_some.rs b/tests/ui/iter_filter_is_some.rs
@@ -4,7 +4,8 @@
     clippy::result_filter_map,
     clippy::needless_borrow,
     clippy::option_filter_map,
-    clippy::redundant_closure
+    clippy::redundant_closure,
+    clippy::unnecessary_get_then_check
 )]
 
 use std::collections::HashMap;
diff --git a/tests/ui/iter_filter_is_some.stderr b/tests/ui/iter_filter_is_some.stderr
@@ -1,5 +1,5 @@
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:14:58
+  --> tests/ui/iter_filter_is_some.rs:15:58
    |
 LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(Option::is_some);
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
@@ -8,55 +8,55 @@ LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(Option::is_
    = help: to override `-D warnings` add `#[allow(clippy::iter_filter_is_some)]`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:16:58
+  --> tests/ui/iter_filter_is_some.rs:17:58
    |
 LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(|a| a.is_some());
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:19:58
+  --> tests/ui/iter_filter_is_some.rs:20:58
    |
 LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(|o| { o.is_some() });
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:26:14
+  --> tests/ui/iter_filter_is_some.rs:27:14
    |
 LL |             .filter(std::option::Option::is_some);
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:31:14
+  --> tests/ui/iter_filter_is_some.rs:32:14
    |
 LL |             .filter(|a| std::option::Option::is_some(a));
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:34:58
+  --> tests/ui/iter_filter_is_some.rs:35:58
    |
 LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(|a| { std::option::Option::is_some(a) });
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:39:58
+  --> tests/ui/iter_filter_is_some.rs:40:58
    |
 LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(|&a| a.is_some());
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:43:58
+  --> tests/ui/iter_filter_is_some.rs:44:58
    |
 LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(|&o| { o.is_some() });
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:48:58
+  --> tests/ui/iter_filter_is_some.rs:49:58
    |
 LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(|ref a| a.is_some());
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `is_some` on iterator over `Option`
-  --> tests/ui/iter_filter_is_some.rs:52:58
+  --> tests/ui/iter_filter_is_some.rs:53:58
    |
 LL |         let _ = vec![Some(1), None, Some(3)].into_iter().filter(|ref o| { o.is_some() });
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
diff --git a/tests/ui/unnecessary_get_then_check.fixed b/tests/ui/unnecessary_get_then_check.fixed
@@ -0,0 +1,26 @@
+#![warn(clippy::unnecessary_get_then_check)]
+
+use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+
+fn main() {
+    let s: HashSet<String> = HashSet::new();
+    let _ = s.contains("a"); //~ ERROR: unnecessary use of `get("a").is_some()`
+    let _ = !s.contains("a"); //~ ERROR: unnecessary use of `get("a").is_none()`
+
+    let s: HashMap<String, ()> = HashMap::new();
+    let _ = s.contains_key("a"); //~ ERROR: unnecessary use of `get("a").is_some()`
+    let _ = !s.contains_key("a"); //~ ERROR: unnecessary use of `get("a").is_none()`
+
+    let s: BTreeSet<String> = BTreeSet::new();
+    let _ = s.contains("a"); //~ ERROR: unnecessary use of `get("a").is_some()`
+    let _ = !s.contains("a"); //~ ERROR: unnecessary use of `get("a").is_none()`
+
+    let s: BTreeMap<String, ()> = BTreeMap::new();
+    let _ = s.contains_key("a"); //~ ERROR: unnecessary use of `get("a").is_some()`
+    let _ = !s.contains_key("a"); //~ ERROR: unnecessary use of `get("a").is_none()`
+
+    // Import to check that the generic annotations are kept!
+    let s: HashSet<String> = HashSet::new();
+    let _ = s.contains::<str>("a"); //~ ERROR: unnecessary use of `get::<str>("a").is_some()`
+    let _ = !s.contains::<str>("a"); //~ ERROR: unnecessary use of `get::<str>("a").is_none()`
+}
diff --git a/tests/ui/unnecessary_get_then_check.rs b/tests/ui/unnecessary_get_then_check.rs
@@ -0,0 +1,26 @@
+#![warn(clippy::unnecessary_get_then_check)]
+
+use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+
+fn main() {
+    let s: HashSet<String> = HashSet::new();
+    let _ = s.get("a").is_some(); //~ ERROR: unnecessary use of `get("a").is_some()`
+    let _ = s.get("a").is_none(); //~ ERROR: unnecessary use of `get("a").is_none()`
+
+    let s: HashMap<String, ()> = HashMap::new();
+    let _ = s.get("a").is_some(); //~ ERROR: unnecessary use of `get("a").is_some()`
+    let _ = s.get("a").is_none(); //~ ERROR: unnecessary use of `get("a").is_none()`
+
+    let s: BTreeSet<String> = BTreeSet::new();
+    let _ = s.get("a").is_some(); //~ ERROR: unnecessary use of `get("a").is_some()`
+    let _ = s.get("a").is_none(); //~ ERROR: unnecessary use of `get("a").is_none()`
+
+    let s: BTreeMap<String, ()> = BTreeMap::new();
+    let _ = s.get("a").is_some(); //~ ERROR: unnecessary use of `get("a").is_some()`
+    let _ = s.get("a").is_none(); //~ ERROR: unnecessary use of `get("a").is_none()`
+
+    // Import to check that the generic annotations are kept!
+    let s: HashSet<String> = HashSet::new();
+    let _ = s.get::<str>("a").is_some(); //~ ERROR: unnecessary use of `get::<str>("a").is_some()`
+    let _ = s.get::<str>("a").is_none(); //~ ERROR: unnecessary use of `get::<str>("a").is_none()`
+}
diff --git a/tests/ui/unnecessary_get_then_check.stderr b/tests/ui/unnecessary_get_then_check.stderr
