Add a case to `lint_search_is_some` to handle searching strings #6119

rsulli55 · 2020-10-06T04:52:59Z

This adds a lint which recommends using contains() instead of find() followed by is_some() on strings as suggested in #6010.

This was added as an additional case to

rust-clippy/clippy_lints/src/methods/mod.rs

Line 3037 in 5af88e3

fn lint_search_is_some<'tcx>(

I would really appreciate any comments/suggestions for my code!

changelog: Added case to lint_search_is_some to handle searching strings

rust-highfive · 2020-10-06T04:53:02Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

clippy_lints/src/find_is_some_on_strs.rs

tests/ui/find_is_some_on_strs.stderr

clippy_lints/src/find_is_some_on_strs.rs

pickfire · 2020-10-06T15:50:18Z

I followed the tutorial and called cargo dev new_lint to make this lint, but after looking more through the code base, I am not sure if this instead should be place in clippy_lints/src/methods/mod.rs. Specfically,

I think this should go into src/methods/mod.rs but I don't have much experience contributing to clippy myself, better wait for reviewer.

flip1995 · 2020-10-09T12:41:50Z

Yeah, you're right, this should go into the methods module. IMO it doesn't even have to be a new lint, but a special case in the SEARCH_IS_SOME lint for &str and String. The

rust-clippy/clippy_lints/src/methods/mod.rs

Line 3037 in 5af88e3

fn lint_search_is_some<'tcx>(

function seems to me to be the perfect place for this.

rsulli55 · 2020-10-10T02:33:44Z

Thanks for the comment! I will move it to lint_search_is_some()

Edit: So, I have done that but unfortunately, the commits are not so nice now (one of them is mostly just deleting files). Sorry about that, I am still pretty new to the PR workflow.

flip1995

So, I have done that but unfortunately, the commits are not so nice now (one of them is mostly just deleting files). Sorry about that, I am still pretty new to the PR workflow.

Don't worry about this for now, we can deal with that right before merging.

impl LGTM overall. Just a NIT and splitting up the tests left to do.

clippy_lints/src/methods/mod.rs

tests/ui/methods.rs

flip1995

Thanks! Only a NIT left.

clippy_lints/src/methods/mod.rs

flip1995 · 2020-10-29T12:59:21Z

@rsulli55 You have to update the reference files. After that, we can merge this.

rsulli55 · 2020-10-30T03:42:40Z

@flip1995 Thanks. I have done that, however I was having trouble rebasing my local branch on the upstream branch so I squashed some of my earlier commits. Is that okay? I have been holding off pushing my local branch because I wasn't sure if that would cause problems.

bors · 2020-10-30T22:20:13Z

☔ The latest upstream changes (presumably #6197) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

rsulli55 · 2020-10-31T19:49:00Z

clippy_lints/src/methods/mod.rs

@@ -3073,7 +3073,7 @@ fn lint_search_is_some<'tcx>(
            then {
                let msg = "called `is_some()` after calling `find()` \
                    on a string. This is more succinctly expressed by calling \
-                    `contains()`";
+                    `contains()`.";


@flip1995 I added back in the period after contains() to match the other cases of search_is_some (they all end with a period). Let me know if you do not want that and I will remove it and re-run update-all-references.

By the way, thank you for you patience with me! Sorry if my questions are all basic things.

Oh they're also in other places? Can you remove them there too, please? Rust error/hint/suggestion messages are always without punctuation at the end and start lower-case.

You could also remove the second part of this message here completely and replace the "try this" below with use `contains()` instead.

@flip1995 Ok, I think they should be correctly updated. I also cleaned up the other lints in search_is_some that suggested using any().

flip1995

Two more NITs left. This should be the last.

tests/ui/search_is_some.rs

clippy_lints/src/methods/mod.rs

instead of `find()` follows by `is_some()` on strings Update clippy_lints/src/find_is_some_on_strs.rs Co-authored-by: Takayuki Nakata <f.seasons017@gmail.com> Update clippy_lints/src/methods/mod.rs Co-authored-by: Philipp Krones <hello@philkrones.com>

`search_is_some.rs` and `search_is_some_fixable.rs`

Co-authored-by: Philipp Krones <hello@philkrones.com>

`update-all-references.sh`

Co-authored-by: Philipp Krones <hello@philkrones.com>

flip1995 · 2020-11-16T08:34:51Z

@bors r+

Thanks!

bors · 2020-11-16T08:34:52Z

📌 Commit 5c1c50e has been approved by flip1995

bors · 2020-11-16T08:45:14Z

⌛ Testing commit 5c1c50e with merge ad4f829...

bors · 2020-11-16T09:07:20Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing ad4f829 to master...

rust-highfive assigned flip1995 Oct 6, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 6, 2020

pickfire reviewed Oct 6, 2020

View reviewed changes

clippy_lints/src/find_is_some_on_strs.rs Outdated Show resolved Hide resolved

pickfire reviewed Oct 6, 2020

View reviewed changes

clippy_lints/src/find_is_some_on_strs.rs Outdated Show resolved Hide resolved

pickfire reviewed Oct 6, 2020

View reviewed changes

clippy_lints/src/find_is_some_on_strs.rs Outdated Show resolved Hide resolved

pickfire reviewed Oct 6, 2020

View reviewed changes

tests/ui/find_is_some_on_strs.stderr Outdated Show resolved Hide resolved

giraffate reviewed Oct 6, 2020

View reviewed changes

clippy_lints/src/find_is_some_on_strs.rs Outdated Show resolved Hide resolved

clippy_lints/src/find_is_some_on_strs.rs Outdated Show resolved Hide resolved

rsulli55 force-pushed the find_is_some_on_strs branch from cafb428 to a51e0ea Compare October 9, 2020 06:08

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 9, 2020

rsulli55 force-pushed the find_is_some_on_strs branch from a51e0ea to b72e769 Compare October 10, 2020 04:42

rsulli55 changed the title ~~WIP Add lint: find_is_some_on_strs~~ Add lint: find_is_some_on_strs Oct 10, 2020

rsulli55 changed the title ~~Add lint: find_is_some_on_strs~~ Add a case to lint_search_is_some to handle searching strings Oct 10, 2020

flip1995 reviewed Oct 10, 2020

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

tests/ui/methods.rs Outdated Show resolved Hide resolved

rsulli55 force-pushed the find_is_some_on_strs branch from a46b437 to df9e647 Compare October 17, 2020 21:29

flip1995 approved these changes Oct 25, 2020

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

rsulli55 force-pushed the find_is_some_on_strs branch from b2c71f5 to 31cb5ce Compare October 31, 2020 19:44

rsulli55 commented Oct 31, 2020

View reviewed changes

rsulli55 force-pushed the find_is_some_on_strs branch from 31cb5ce to 2a575a6 Compare November 7, 2020 07:20

flip1995 approved these changes Nov 8, 2020

View reviewed changes

tests/ui/search_is_some.rs Outdated Show resolved Hide resolved

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

rsulli55 added 3 commits November 10, 2020 23:18

Moved the tests for lint search_is_some to new files

431fcbc

`search_is_some.rs` and `search_is_some_fixable.rs`

Ran tests/ui/update-all-references.sh" and cargo dev fmt`

55dc822

rsulli55 and others added 6 commits November 10, 2020 23:18

Remove to_string on msg

e9612f3

Co-authored-by: Philipp Krones <hello@philkrones.com>

Remove borrow

fb74b48

Co-authored-by: Philipp Krones <hello@philkrones.com>

Added period back to lint search_is_some and ran

ee1b959

`update-all-references.sh`

Cleaned up message and suggestion for lint_search_is_some

fd30313

Update clippy_lints/src/methods/mod.rs

56d252c

Co-authored-by: Philipp Krones <hello@philkrones.com>

Change variable named foo and rerun update-all-references

5c1c50e

rsulli55 force-pushed the find_is_some_on_strs branch from af3b1f4 to 5c1c50e Compare November 11, 2020 06:49

bors merged commit ad4f829 into rust-lang:master Nov 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a case to `lint_search_is_some` to handle searching strings #6119

Add a case to `lint_search_is_some` to handle searching strings #6119

rsulli55 commented Oct 6, 2020 •

edited

Loading

rust-highfive commented Oct 6, 2020

pickfire commented Oct 6, 2020

flip1995 commented Oct 9, 2020 •

edited

Loading

rsulli55 commented Oct 10, 2020 •

edited

Loading

flip1995 left a comment

flip1995 left a comment

flip1995 commented Oct 29, 2020

rsulli55 commented Oct 30, 2020

bors commented Oct 30, 2020

rsulli55 Oct 31, 2020

flip1995 Nov 3, 2020

flip1995 Nov 3, 2020

rsulli55 Nov 7, 2020

flip1995 left a comment

flip1995 commented Nov 16, 2020

bors commented Nov 16, 2020

bors commented Nov 16, 2020

bors commented Nov 16, 2020

Add a case to lint_search_is_some to handle searching strings #6119

Add a case to lint_search_is_some to handle searching strings #6119

Conversation

rsulli55 commented Oct 6, 2020 • edited Loading

rust-highfive commented Oct 6, 2020

pickfire commented Oct 6, 2020

flip1995 commented Oct 9, 2020 • edited Loading

rsulli55 commented Oct 10, 2020 • edited Loading

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Oct 29, 2020

rsulli55 commented Oct 30, 2020

bors commented Oct 30, 2020

rsulli55 Oct 31, 2020

Choose a reason for hiding this comment

flip1995 Nov 3, 2020

Choose a reason for hiding this comment

flip1995 Nov 3, 2020

Choose a reason for hiding this comment

rsulli55 Nov 7, 2020

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Nov 16, 2020

bors commented Nov 16, 2020

bors commented Nov 16, 2020

bors commented Nov 16, 2020

Add a case to `lint_search_is_some` to handle searching strings #6119

Add a case to `lint_search_is_some` to handle searching strings #6119

rsulli55 commented Oct 6, 2020 •

edited

Loading

flip1995 commented Oct 9, 2020 •

edited

Loading

rsulli55 commented Oct 10, 2020 •

edited

Loading