Skip to content

Commit f4a9bb3

Browse files
nyuriknyurik
committed
Add BTreeSet, rename set_contains_or_insert to contains_or_insert
* Detect `BTreeSet::contains` + `BTreeSet::insert` usage in the same way as with the `HashSet`. * Make the name of the lint more generic.
1 parent 94a000b commit f4a9bb3

File tree

8 files changed

+614
-244
lines changed

8 files changed

+614
-244
lines changed

‎CHANGELOG.md‎

Lines changed: 322 additions & 82 deletions
Large diffs are not rendered by default.

‎clippy_lints/src/set_contains_or_insert.rs‎ renamed to ‎clippy_lints/src/contains_or_insert.rs‎

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use rustc_span::{sym, Span};
1313
declare_clippy_lint! {
1414
/// ### What it does
1515
/// Checks for usage of `contains` to see if a value is not
16-
/// present on `HashSet` followed by a `insert`.
16+
/// present on `HashSet` or a `BTreeSet` followed by a `insert`.
1717
///
1818
/// ### Why is this bad?
1919
/// Using just `insert` and checking the returned `bool` is more efficient.
@@ -43,29 +43,29 @@ declare_clippy_lint! {
4343
/// }
4444
/// ```
4545
#[clippy::version = "1.80.0"]
46-
pub SET_CONTAINS_OR_INSERT,
46+
pub CONTAINS_OR_INSERT,
4747
nursery,
48-
"call to `HashSet::contains` followed by `HashSet::insert`"
48+
"call to `<collection>::contains` followed by `<collection>::insert`"
4949
}
5050

51-
declare_lint_pass!(HashsetInsertAfterContains => [SET_CONTAINS_OR_INSERT]);
51+
declare_lint_pass!(ContainsOrInsert => [CONTAINS_OR_INSERT]);
5252

53-
impl<'tcx> LateLintPass<'tcx> for HashsetInsertAfterContains {
53+
impl<'tcx> LateLintPass<'tcx> for ContainsOrInsert {
5454
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
5555
if !expr.span.from_expansion()
5656
&& let Some(higher::If {
5757
cond: cond_expr,
5858
then: then_expr,
5959
..
6060
}) = higher::If::hir(expr)
61-
&& let Some(contains_expr) = try_parse_op_call(cx, cond_expr, sym!(contains))//try_parse_contains(cx, cond_expr)
61+
&& let Some((contains_expr, sym)) = try_parse_op_call(cx, cond_expr, sym!(contains))//try_parse_contains(cx, cond_expr)
6262
&& let Some(insert_expr) = find_insert_calls(cx, &contains_expr, then_expr)
6363
{
6464
span_lint(
6565
cx,
66-
SET_CONTAINS_OR_INSERT,
66+
CONTAINS_OR_INSERT,
6767
vec![contains_expr.span, insert_expr.span],
68-
"usage of `HashSet::insert` after `HashSet::contains`",
68+
format!("usage of `{sym}::insert` after `{sym}::contains`"),
6969
);
7070
}
7171
}
@@ -77,7 +77,11 @@ struct OpExpr<'tcx> {
7777
span: Span,
7878
}
7979

80-
fn try_parse_op_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, symbol: Symbol) -> Option<OpExpr<'tcx>> {
80+
fn try_parse_op_call<'tcx>(
81+
cx: &LateContext<'tcx>,
82+
expr: &'tcx Expr<'_>,
83+
symbol: Symbol,
84+
) -> Option<(OpExpr<'tcx>, Symbol)> {
8185
let expr = peel_hir_expr_while(expr, |e| {
8286
if let ExprKind::Unary(UnOp::Not, e) = e.kind {
8387
Some(e)
@@ -97,11 +101,12 @@ fn try_parse_op_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, symbol:
97101
});
98102
let receiver = receiver.peel_borrows();
99103
let receiver_ty = cx.typeck_results().expr_ty(receiver).peel_refs();
100-
if value.span.eq_ctxt(expr.span)
101-
&& is_type_diagnostic_item(cx, receiver_ty, sym::HashSet)
102-
&& path.ident.name == symbol
103-
{
104-
return Some(OpExpr { receiver, value, span });
104+
if value.span.eq_ctxt(expr.span) && path.ident.name == symbol {
105+
for sym in &[sym::HashSet, sym::BTreeSet] {
106+
if is_type_diagnostic_item(cx, receiver_ty, *sym) {
107+
return Some((OpExpr { receiver, value, span }, *sym));
108+
}
109+
}
105110
}
106111
}
107112
None
@@ -113,7 +118,7 @@ fn find_insert_calls<'tcx>(
113118
expr: &'tcx Expr<'_>,
114119
) -> Option<OpExpr<'tcx>> {
115120
for_each_expr(cx, expr, |e| {
116-
if let Some(insert_expr) = try_parse_op_call(cx, e, sym!(insert))
121+
if let Some((insert_expr, _)) = try_parse_op_call(cx, e, sym!(insert))
117122
&& SpanlessEq::new(cx).eq_expr(contains_expr.receiver, insert_expr.receiver)
118123
&& SpanlessEq::new(cx).eq_expr(contains_expr.value, insert_expr.value)
119124
{

‎clippy_lints/src/declared_lints.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
111111
crate::collapsible_if::COLLAPSIBLE_IF_INFO,
112112
crate::collection_is_never_read::COLLECTION_IS_NEVER_READ_INFO,
113113
crate::comparison_chain::COMPARISON_CHAIN_INFO,
114+
crate::contains_or_insert::CONTAINS_OR_INSERT_INFO,
114115
crate::copies::BRANCHES_SHARING_CODE_INFO,
115116
crate::copies::IFS_SAME_COND_INFO,
116117
crate::copies::IF_SAME_THEN_ELSE_INFO,
@@ -647,7 +648,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
647648
crate::semicolon_block::SEMICOLON_OUTSIDE_BLOCK_INFO,
648649
crate::semicolon_if_nothing_returned::SEMICOLON_IF_NOTHING_RETURNED_INFO,
649650
crate::serde_api::SERDE_API_MISUSE_INFO,
650-
crate::set_contains_or_insert::SET_CONTAINS_OR_INSERT_INFO,
651651
crate::shadow::SHADOW_REUSE_INFO,
652652
crate::shadow::SHADOW_SAME_INFO,
653653
crate::shadow::SHADOW_UNRELATED_INFO,

‎clippy_lints/src/lib.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ mod cognitive_complexity;
9898
mod collapsible_if;
9999
mod collection_is_never_read;
100100
mod comparison_chain;
101+
mod contains_or_insert;
101102
mod copies;
102103
mod copy_iterator;
103104
mod crate_in_macro_def;
@@ -320,7 +321,6 @@ mod self_named_constructors;
320321
mod semicolon_block;
321322
mod semicolon_if_nothing_returned;
322323
mod serde_api;
323-
mod set_contains_or_insert;
324324
mod shadow;
325325
mod significant_drop_tightening;
326326
mod single_call_fn;
@@ -1175,7 +1175,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
11751175
});
11761176
store.register_late_pass(move |_| Box::new(string_patterns::StringPatterns::new(msrv())));
11771177
store.register_early_pass(|| Box::new(field_scoped_visibility_modifiers::FieldScopedVisibilityModifiers));
1178-
store.register_late_pass(|_| Box::new(set_contains_or_insert::HashsetInsertAfterContains));
1178+
store.register_late_pass(|_| Box::new(contains_or_insert::ContainsOrInsert));
11791179
store.register_early_pass(|| Box::new(byte_char_slices::ByteCharSlice));
11801180
store.register_early_pass(|| Box::new(cfg_not_test::CfgNotTest));
11811181
// add lints here, do not remove this comment, it's used in `new_lint`

‎tests/ui/contains_or_insert.rs‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
#![allow(unused)]
2+
#![allow(clippy::nonminimal_bool)]
3+
#![allow(clippy::needless_borrow)]
4+
#![warn(clippy::contains_or_insert)]
5+
6+
use std::collections::{BTreeSet, HashSet};
7+
8+
fn should_warn_hashset() {
9+
let mut set = HashSet::new();
10+
let value = 5;
11+
12+
if !set.contains(&value) {
13+
set.insert(value);
14+
println!("Just a comment");
15+
}
16+
17+
if set.contains(&value) {
18+
set.insert(value);
19+
println!("Just a comment");
20+
}
21+
22+
if !set.contains(&value) {
23+
set.insert(value);
24+
}
25+
26+
if !!set.contains(&value) {
27+
set.insert(value);
28+
println!("Just a comment");
29+
}
30+
31+
if (&set).contains(&value) {
32+
set.insert(value);
33+
}
34+
35+
let borrow_value = &6;
36+
if !set.contains(borrow_value) {
37+
set.insert(*borrow_value);
38+
}
39+
40+
let borrow_set = &mut set;
41+
if !borrow_set.contains(&value) {
42+
borrow_set.insert(value);
43+
}
44+
}
45+
46+
fn should_not_warn_hashset() {
47+
let mut set = HashSet::new();
48+
let value = 5;
49+
let another_value = 6;
50+
51+
if !set.contains(&value) {
52+
set.insert(another_value);
53+
}
54+
55+
if !set.contains(&value) {
56+
println!("Just a comment");
57+
}
58+
59+
if simply_true() {
60+
set.insert(value);
61+
}
62+
63+
if !set.contains(&value) {
64+
set.replace(value); //it is not insert
65+
println!("Just a comment");
66+
}
67+
68+
if set.contains(&value) {
69+
println!("value is already in set");
70+
} else {
71+
set.insert(value);
72+
}
73+
}
74+
75+
fn should_warn_btreeset() {
76+
let mut set = BTreeSet::new();
77+
let value = 5;
78+
79+
if !set.contains(&value) {
80+
set.insert(value);
81+
println!("Just a comment");
82+
}
83+
84+
if set.contains(&value) {
85+
set.insert(value);
86+
println!("Just a comment");
87+
}
88+
89+
if !set.contains(&value) {
90+
set.insert(value);
91+
}
92+
93+
if !!set.contains(&value) {
94+
set.insert(value);
95+
println!("Just a comment");
96+
}
97+
98+
if (&set).contains(&value) {
99+
set.insert(value);
100+
}
101+
102+
let borrow_value = &6;
103+
if !set.contains(borrow_value) {
104+
set.insert(*borrow_value);
105+
}
106+
107+
let borrow_set = &mut set;
108+
if !borrow_set.contains(&value) {
109+
borrow_set.insert(value);
110+
}
111+
}
112+
113+
fn should_not_warn_btreeset() {
114+
let mut set = BTreeSet::new();
115+
let value = 5;
116+
let another_value = 6;
117+
118+
if !set.contains(&value) {
119+
set.insert(another_value);
120+
}
121+
122+
if !set.contains(&value) {
123+
println!("Just a comment");
124+
}
125+
126+
if simply_true() {
127+
set.insert(value);
128+
}
129+
130+
if !set.contains(&value) {
131+
set.replace(value); //it is not insert
132+
println!("Just a comment");
133+
}
134+
135+
if set.contains(&value) {
136+
println!("value is already in set");
137+
} else {
138+
set.insert(value);
139+
}
140+
}
141+
142+
fn simply_true() -> bool {
143+
true
144+
}
145+
146+
// This is placed last in order to be able to add new tests without changing line numbers
147+
fn main() {
148+
should_warn_hashset();
149+
should_warn_btreeset();
150+
should_not_warn_hashset();
151+
should_not_warn_btreeset();
152+
}

‎tests/ui/contains_or_insert.stderr‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
error: usage of `HashSet::insert` after `HashSet::contains`
2+
--> tests/ui/contains_or_insert.rs:12:13
3+
|
4+
LL | if !set.contains(&value) {
5+
| ^^^^^^^^^^^^^^^^
6+
LL | set.insert(value);
7+
| ^^^^^^^^^^^^^
8+
|
9+
= note: `-D clippy::contains-or-insert` implied by `-D warnings`
10+
= help: to override `-D warnings` add `#[allow(clippy::contains_or_insert)]`
11+
12+
error: usage of `HashSet::insert` after `HashSet::contains`
13+
--> tests/ui/contains_or_insert.rs:17:12
14+
|
15+
LL | if set.contains(&value) {
16+
| ^^^^^^^^^^^^^^^^
17+
LL | set.insert(value);
18+
| ^^^^^^^^^^^^^
19+
20+
error: usage of `HashSet::insert` after `HashSet::contains`
21+
--> tests/ui/contains_or_insert.rs:22:13
22+
|
23+
LL | if !set.contains(&value) {
24+
| ^^^^^^^^^^^^^^^^
25+
LL | set.insert(value);
26+
| ^^^^^^^^^^^^^
27+
28+
error: usage of `HashSet::insert` after `HashSet::contains`
29+
--> tests/ui/contains_or_insert.rs:26:14
30+
|
31+
LL | if !!set.contains(&value) {
32+
| ^^^^^^^^^^^^^^^^
33+
LL | set.insert(value);
34+
| ^^^^^^^^^^^^^
35+
36+
error: usage of `HashSet::insert` after `HashSet::contains`
37+
--> tests/ui/contains_or_insert.rs:31:15
38+
|
39+
LL | if (&set).contains(&value) {
40+
| ^^^^^^^^^^^^^^^^
41+
LL | set.insert(value);
42+
| ^^^^^^^^^^^^^
43+
44+
error: usage of `HashSet::insert` after `HashSet::contains`
45+
--> tests/ui/contains_or_insert.rs:36:13
46+
|
47+
LL | if !set.contains(borrow_value) {
48+
| ^^^^^^^^^^^^^^^^^^^^^^
49+
LL | set.insert(*borrow_value);
50+
| ^^^^^^^^^^^^^^^^^^^^^
51+
52+
error: usage of `HashSet::insert` after `HashSet::contains`
53+
--> tests/ui/contains_or_insert.rs:41:20
54+
|
55+
LL | if !borrow_set.contains(&value) {
56+
| ^^^^^^^^^^^^^^^^
57+
LL | borrow_set.insert(value);
58+
| ^^^^^^^^^^^^^
59+
60+
error: usage of `BTreeSet::insert` after `BTreeSet::contains`
61+
--> tests/ui/contains_or_insert.rs:79:13
62+
|
63+
LL | if !set.contains(&value) {
64+
| ^^^^^^^^^^^^^^^^
65+
LL | set.insert(value);
66+
| ^^^^^^^^^^^^^
67+
68+
error: usage of `BTreeSet::insert` after `BTreeSet::contains`
69+
--> tests/ui/contains_or_insert.rs:84:12
70+
|
71+
LL | if set.contains(&value) {
72+
| ^^^^^^^^^^^^^^^^
73+
LL | set.insert(value);
74+
| ^^^^^^^^^^^^^
75+
76+
error: usage of `BTreeSet::insert` after `BTreeSet::contains`
77+
--> tests/ui/contains_or_insert.rs:89:13
78+
|
79+
LL | if !set.contains(&value) {
80+
| ^^^^^^^^^^^^^^^^
81+
LL | set.insert(value);
82+
| ^^^^^^^^^^^^^
83+
84+
error: usage of `BTreeSet::insert` after `BTreeSet::contains`
85+
--> tests/ui/contains_or_insert.rs:93:14
86+
|
87+
LL | if !!set.contains(&value) {
88+
| ^^^^^^^^^^^^^^^^
89+
LL | set.insert(value);
90+
| ^^^^^^^^^^^^^
91+
92+
error: usage of `BTreeSet::insert` after `BTreeSet::contains`
93+
--> tests/ui/contains_or_insert.rs:98:15
94+
|
95+
LL | if (&set).contains(&value) {
96+
| ^^^^^^^^^^^^^^^^
97+
LL | set.insert(value);
98+
| ^^^^^^^^^^^^^
99+
100+
error: usage of `BTreeSet::insert` after `BTreeSet::contains`
101+
--> tests/ui/contains_or_insert.rs:103:13
102+
|
103+
LL | if !set.contains(borrow_value) {
104+
| ^^^^^^^^^^^^^^^^^^^^^^
105+
LL | set.insert(*borrow_value);
106+
| ^^^^^^^^^^^^^^^^^^^^^
107+
108+
error: usage of `BTreeSet::insert` after `BTreeSet::contains`
109+
--> tests/ui/contains_or_insert.rs:108:20
110+
|
111+
LL | if !borrow_set.contains(&value) {
112+
| ^^^^^^^^^^^^^^^^
113+
LL | borrow_set.insert(value);
114+
| ^^^^^^^^^^^^^
115+
116+
error: aborting due to 14 previous errors
117+

0 commit comments

Comments
 (0)