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

New lint manual_split_once #7565

Merged
merged 3 commits into from
Aug 17, 2021
Merged

New lint manual_split_once #7565

merged 3 commits into from
Aug 17, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Aug 14, 2021

This is a WIP because it still needs to recognize more patterns. Currently handles:

s.splitn(2, ' ').next();
s.splitn(2, ' ').nth(0)
s.splitn(2, ' ').nth(1);
s.splitn(2, ' ').skip(0).next();
s.splitn(2, ' ').skip(1).next();
s.splitn(2, ' ').next_tuple(); // from itertools

// as well as `unwrap()` and `?` forms

Still to do:

let mut iter = s.splitn(2, ' ');
(iter.next().unwrap(), iter.next()?)

let mut iter = s.splitn(2, ' ');
let key = iter.next().unwrap();
let value = iter.next()?;

Suggestions on other patterns to check for would be useful. I've done a search on github for uses of splitn. Still have yet to actually look through the results.

There's also the question of whether the lint shouold trigger on all uses of splitn with two values, or only on recognized usages. The former could have false positives where it couldn't be replaced, but I'm not sure how common that would be.

changelog: Add lint manual_split_once

@rust-highfive
Copy link

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 14, 2021
@Jarcho Jarcho force-pushed the manual_split_once branch 3 times, most recently from 51024fb to 5a1f452 Compare August 15, 2021 05:09
@llogiq
Copy link
Contributor

llogiq commented Aug 15, 2021

This already looks quite comprehensive! Great job! I think it'd be ready to merge and we can extend it with a later PR if you're OK with that.

My suggestion is informed by

  • The already high code quality 👍
  • The rather low risk of false positives
  • The helpful suggestions as showcased by the tests

@Jarcho
Copy link
Contributor Author

Jarcho commented Aug 15, 2021

Just updated the docs as they mentioned something that currently isn't checked for. I see nothing wrong with merging it as is. It will probably be a week before I get to finishing the rest of the checks.

@Jarcho Jarcho force-pushed the manual_split_once branch 3 times, most recently from 11d28ee to 8a9c512 Compare August 16, 2021 00:35
@llogiq llogiq changed the title WIP manual_split_once New lint manual_split_once Aug 16, 2021
@llogiq
Copy link
Contributor

llogiq commented Aug 16, 2021

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Aug 16, 2021

📌 Commit 8a9c512 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Aug 16, 2021

⌛ Testing commit 8a9c512 with merge 0c8f375...

bors added a commit that referenced this pull request Aug 16, 2021
New lint `manual_split_once`

This is a WIP because it still needs to recognize more patterns. Currently handles:

```rust
s.splitn(2, ' ').next();
s.splitn(2, ' ').nth(0)
s.splitn(2, ' ').nth(1);
s.splitn(2, ' ').skip(0).next();
s.splitn(2, ' ').skip(1).next();
s.splitn(2, ' ').next_tuple(); // from itertools

// as well as `unwrap()` and `?` forms
```

Still to do:

```rust
let mut iter = s.splitn(2, ' ');
(iter.next().unwrap(), iter.next()?)

let mut iter = s.splitn(2, ' ');
let key = iter.next().unwrap();
let value = iter.next()?;
```

Suggestions on other patterns to check for would be useful. I've done a search on github for uses of `splitn`. Still have yet to actually look through the results.

There's also the question of whether the lint shouold trigger on all uses of `splitn` with two values, or only on recognized usages. The former could have false positives where it couldn't be replaced, but I'm not sure how common that would be.

changelog: Add lint `manual_split_once`
@bors
Copy link
Contributor

bors commented Aug 16, 2021

💔 Test failed - checks-action_test

@xFrednet
Copy link
Member

It seems like #7566 which was merged shortly before this, again uses the LocalUsedVisitor visitor

@Jarcho Jarcho force-pushed the manual_split_once branch from 8a9c512 to aab3267 Compare August 16, 2021 13:35
@llogiq
Copy link
Contributor

llogiq commented Aug 17, 2021

Let's try that again – @bors r+

@bors
Copy link
Contributor

bors commented Aug 17, 2021

📌 Commit aab3267 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Aug 17, 2021

⌛ Testing commit aab3267 with merge 1fc1aee...

@bors
Copy link
Contributor

bors commented Aug 17, 2021

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

@bors bors merged commit 1fc1aee into rust-lang:master Aug 17, 2021
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.

5 participants