mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
bors 2024-02-25 10:39:24 +00:00
commit 76e4864119
11 changed files with 273 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -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

1
clippy_lints/src/declared_lints.rs
View File

@ -455,6 +455,7 @@
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,

47
clippy_lints/src/methods/mod.rs
View File

@ -110,6 +110,7 @@
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 @@
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 @@ pub fn new(
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 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
},
("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 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
}
}
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);
},
_ => {},
}
}

85
clippy_lints/src/methods/unnecessary_get_then_check.rs Normal file
View File

@ -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,
);
},
);
}
}
}

6
lintcheck/src/main.rs
View File

@ -749,7 +749,7 @@ 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}");
});
@ -757,7 +757,7 @@ fn print_stats(old_stats: HashMap<String, usize>, 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::<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");

3
tests/ui/iter_filter_is_some.fixed
View File

@ -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;

3
tests/ui/iter_filter_is_some.rs
View File

@ -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;

20
tests/ui/iter_filter_is_some.stderr
View File

@ -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()`

26
tests/ui/unnecessary_get_then_check.fixed Normal file
View File

@ -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()`
}

26
tests/ui/unnecessary_get_then_check.rs Normal file
View File

@ -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()`
}

75
tests/ui/unnecessary_get_then_check.stderr Normal file
View File

@ -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::<str>("a").is_some()`
--> tests/ui/unnecessary_get_then_check.rs:24:15
|
LL | let _ = s.get::<str>("a").is_some();
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `contains::<str>("a")`
error: unnecessary use of `get::<str>("a").is_none()`
--> tests/ui/unnecessary_get_then_check.rs:25:15
|
LL | let _ = s.get::<str>("a").is_none();
| --^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| help: replace it with: `!s.contains::<str>("a")`
error: aborting due to 10 previous errors