Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BTreeSet detection to the set_contains_or_insert lint #13053

Merged
merged 1 commit into from
Jul 27, 2024

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Jul 5, 2024

  • Detect BTreeSet::contains + BTreeSet::insert usage in the same way as with the HashSet.
    CC: @lochetti @bitfield

changelog: [set_contains_or_insert]: Handle BTreeSet in addition to HashSet

@nyurik nyurik force-pushed the rename-set_contains_or_insert branch from c3a561b to f82f3de Compare July 5, 2024 17:19
@nyurik nyurik changed the title Rename set_contains_or_insert to contains_or_insert Support BTreeSet in set_contains_or_insert and rename to contains_or_insert Jul 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2024

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 5, 2024
@nyurik
Copy link
Contributor Author

nyurik commented Jul 5, 2024

Continuing from #12873 (comment)

The name of this lint could either focus on set aspect -- so no renaming, or a more generic collection type. The map variant could be implemented under a separate name (although it is highly similar to the current set variant):

let mut map = HashMap::new();
if map.contains_key(&"a") {
    map.insert("a", 1);
}

The issue here is that the lint would need to detect if the key has the same value in both calls, i.e. refer to the same const or variable, and that variable hasn't been modified between the calls...

@llogiq
Copy link
Contributor

llogiq commented Jul 5, 2024

I would have expected the old name to be registered as renamed, but I haven't looked into our machinery for that for a while, so I may be wrong. Otherwise this looks good.

@llogiq
Copy link
Contributor

llogiq commented Jul 6, 2024

We have the SpanlessEq::eq_expr_value method to check for detecting whether two exprs are the same.

@nyurik nyurik force-pushed the rename-set_contains_or_insert branch from f3aa8df to f4a9bb3 Compare July 7, 2024 15:46
@nyurik nyurik changed the title Support BTreeSet in set_contains_or_insert and rename to contains_or_insert Add BTreeSet, rename set_contains_or_insert to contains_or_insert Jul 7, 2024
@nyurik
Copy link
Contributor Author

nyurik commented Jul 7, 2024

@llogiq I just did a bit of a cleanup, but I am not sure where SpanlessEq::eq_expr_value could be applied.

@nyurik
Copy link
Contributor Author

nyurik commented Jul 7, 2024

Oh, I think you meant to use SpanlessEq::eq_expr_value for HashMap implementation. Let's do that in a separate PR, as that may require a more substantial logic change and longer review.

@nyurik nyurik force-pushed the rename-set_contains_or_insert branch from f4a9bb3 to 014800a Compare July 7, 2024 16:01
@llogiq
Copy link
Contributor

llogiq commented Jul 7, 2024

No I meant for comparing two exprs.

@nyurik
Copy link
Contributor Author

nyurik commented Jul 7, 2024

@llogiq in that case I don't follow - could you comment in the review tab - which lines you think should be changed? thx!

@nyurik nyurik force-pushed the rename-set_contains_or_insert branch from 014800a to cb237de Compare July 9, 2024 17:58
@@ -113,7 +118,7 @@ fn find_insert_calls<'tcx>(
expr: &'tcx Expr<'_>,
) -> Option<OpExpr<'tcx>> {
for_each_expr(cx, expr, |e| {
if let Some(insert_expr) = try_parse_op_call(cx, e, sym!(insert))
if let Some((insert_expr, _)) = try_parse_op_call(cx, e, sym!(insert))
&& SpanlessEq::new(cx).eq_expr(contains_expr.receiver, insert_expr.receiver)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should use eq_expr_value to avoid linting on e.g. my_vec.pop().contains(key).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@llogiq i must still be missing something. I did not change any comparison longic, but instead simply ignore one extra param. If you think this lint should be modified to use eq_expr_value somewhere where my PR is not touching, lets perhaps do it in a separate PR? Especially if you think there is a false positive in it that we should add to a test.

In the mean time, I reverted the lint rename - the cut off day has passed i think, so it would be better to keep the lint as is, and perhaps introduce another lint that focuses on map style interfaces (e.g. if !hashmap.contains(key) { hashmap.add(key, value) })

@nyurik nyurik force-pushed the rename-set_contains_or_insert branch from cb237de to 7019504 Compare July 27, 2024 02:44
* Detect `BTreeSet::contains` + `BTreeSet::insert` usage in the same way as with the `HashSet`.
@nyurik nyurik force-pushed the rename-set_contains_or_insert branch from 7019504 to 9964b4e Compare July 27, 2024 02:52
@nyurik nyurik changed the title Add BTreeSet, rename set_contains_or_insert to contains_or_insert Add BTreeSet detection to the set_contains_or_insert lint Jul 27, 2024
@llogiq
Copy link
Contributor

llogiq commented Jul 27, 2024

Ok, let's try this. We can add more PRs with changes later.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 27, 2024

📌 Commit 9964b4e has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 27, 2024

⌛ Testing commit 9964b4e with merge 92768ee...

@bors
Copy link
Contributor

bors commented Jul 27, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 92768ee to master...

@bors bors merged commit 92768ee into rust-lang:master Jul 27, 2024
8 checks passed
@nyurik nyurik deleted the rename-set_contains_or_insert branch July 27, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants