Skip to content

Commit cb237de

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 cb237de

File tree

8 files changed

+294
-164
lines changed

8 files changed

+294
-164
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5277,6 +5277,7 @@ Released 2018-09-13
52775277
[`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty
52785278
[`const_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_is_empty
52795279
[`const_static_lifetime`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_static_lifetime
5280+
[`contains_or_insert`]: https://rust-lang.github.io/rust-clippy/master/index.html#contains_or_insert
52805281
[`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator
52815282
[`crate_in_macro_def`]: https://rust-lang.github.io/rust-clippy/master/index.html#crate_in_macro_def
52825283
[`create_dir`]: https://rust-lang.github.io/rust-clippy/master/index.html#create_dir
@@ -5800,7 +5801,6 @@ Released 2018-09-13
58005801
[`semicolon_outside_block`]: https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_outside_block
58015802
[`separated_literal_suffix`]: https://rust-lang.github.io/rust-clippy/master/index.html#separated_literal_suffix
58025803
[`serde_api_misuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#serde_api_misuse
5803-
[`set_contains_or_insert`]: https://rust-lang.github.io/rust-clippy/master/index.html#set_contains_or_insert
58045804
[`shadow_reuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_reuse
58055805
[`shadow_same`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_same
58065806
[`shadow_unrelated`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_unrelated

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

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ use rustc_span::{sym, Span};
1212

1313
declare_clippy_lint! {
1414
/// ### What it does
15-
/// Checks for usage of `contains` to see if a value is not
16-
/// present on `HashSet` followed by a `insert`.
15+
/// Checks for usage of `contains` to see if a value is not present
16+
/// in a collection like `HashSet` or `BTreeSet`, followed by an `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+
}

0 commit comments

Comments
 (0)