diff --git a/CHANGELOG.md b/CHANGELOG.md index 5805511c7..26dce1f92 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 79fd76393..b6a35bb3e 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 145254780..820f4c858 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 000000000..cc8053ef5 --- /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, + ); + }, + ); + } + } +} diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index 88966b41f..664429981 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -749,7 +749,7 @@ fn print_stats(old_stats: HashMap, 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::(new_key).is_none()) + .filter(|(new_key, _)| !old_stats_deduped.contains_key::(new_key)) .for_each(|(new_key, new_value)| { println!("{new_key} 0 => {new_value}"); }); @@ -757,7 +757,7 @@ fn print_stats(old_stats: HashMap, new_stats: HashMap<&String, us // 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::(new_key).is_some()) + .filter(|(new_key, _new_val)| old_stats_deduped.contains_key::(new_key)) .for_each(|(new_key, new_val)| { let old_val = old_stats_deduped.get::(new_key).unwrap(); println!("{new_key} {old_val} => {new_val}"); @@ -766,7 +766,7 @@ fn print_stats(old_stats: HashMap, 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 index abc3a47fa..8a818c0c6 100644 --- 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 index c74775a82..9eda93a25 100644 --- 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 index 165ccfedf..54aff892b 100644 --- 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 new file mode 100644 index 000000000..178a3300c --- /dev/null +++ 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 = 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 = 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 = 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 = 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 = 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()` +} diff --git a/tests/ui/unnecessary_get_then_check.rs b/tests/ui/unnecessary_get_then_check.rs new file mode 100644 index 000000000..c197bdef4 --- /dev/null +++ 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 = 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 = 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 = 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 = 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 = 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()` +} diff --git a/tests/ui/unnecessary_get_then_check.stderr b/tests/ui/unnecessary_get_then_check.stderr new file mode 100644 index 000000000..0477c03d1 --- /dev/null +++ b/tests/ui/unnecessary_get_then_check.stderr @@ -0,0 +1,75 @@ +error: unnecessary use of `get("a").is_some()` + --> tests/ui/unnecessary_get_then_check.rs:7:15 + | +LL | let _ = s.get("a").is_some(); + | ^^^^^^^^^^^^^^^^^^ help: replace it with: `contains("a")` + | + = note: `-D clippy::unnecessary-get-then-check` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_get_then_check)]` + +error: unnecessary use of `get("a").is_none()` + --> tests/ui/unnecessary_get_then_check.rs:8:15 + | +LL | let _ = s.get("a").is_none(); + | --^^^^^^^^^^^^^^^^^^ + | | + | help: replace it with: `!s.contains("a")` + +error: unnecessary use of `get("a").is_some()` + --> tests/ui/unnecessary_get_then_check.rs:11:15 + | +LL | let _ = s.get("a").is_some(); + | ^^^^^^^^^^^^^^^^^^ help: replace it with: `contains_key("a")` + +error: unnecessary use of `get("a").is_none()` + --> tests/ui/unnecessary_get_then_check.rs:12:15 + | +LL | let _ = s.get("a").is_none(); + | --^^^^^^^^^^^^^^^^^^ + | | + | help: replace it with: `!s.contains_key("a")` + +error: unnecessary use of `get("a").is_some()` + --> tests/ui/unnecessary_get_then_check.rs:15:15 + | +LL | let _ = s.get("a").is_some(); + | ^^^^^^^^^^^^^^^^^^ help: replace it with: `contains("a")` + +error: unnecessary use of `get("a").is_none()` + --> tests/ui/unnecessary_get_then_check.rs:16:15 + | +LL | let _ = s.get("a").is_none(); + | --^^^^^^^^^^^^^^^^^^ + | | + | help: replace it with: `!s.contains("a")` + +error: unnecessary use of `get("a").is_some()` + --> tests/ui/unnecessary_get_then_check.rs:19:15 + | +LL | let _ = s.get("a").is_some(); + | ^^^^^^^^^^^^^^^^^^ help: replace it with: `contains_key("a")` + +error: unnecessary use of `get("a").is_none()` + --> tests/ui/unnecessary_get_then_check.rs:20:15 + | +LL | let _ = s.get("a").is_none(); + | --^^^^^^^^^^^^^^^^^^ + | | + | help: replace it with: `!s.contains_key("a")` + +error: unnecessary use of `get::("a").is_some()` + --> tests/ui/unnecessary_get_then_check.rs:24:15 + | +LL | let _ = s.get::("a").is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `contains::("a")` + +error: unnecessary use of `get::("a").is_none()` + --> tests/ui/unnecessary_get_then_check.rs:25:15 + | +LL | let _ = s.get::("a").is_none(); + | --^^^^^^^^^^^^^^^^^^^^^^^^^ + | | + | help: replace it with: `!s.contains::("a")` + +error: aborting due to 10 previous errors +