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

Take over: New lint bytes count to len #8711

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

kyoto7250
Copy link
Contributor

take over #8375
close #8083

This PR adds new lint about considering replacing .bytes().count() with .len().

Thank you in advance.


r! @Manishearth

changelog: adds new lint [bytes_count_to_len] to consider replacing .bytes().count() with .len()

@rust-highfive
Copy link

r? @Manishearth

(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 Apr 16, 2022
BYTES_COUNT_TO_LEN,
expr.span,
"using long and hard to read `.bytes().count()`",
"consider calling `.len` instead:",
Copy link
Member

@matthiaskrgr matthiaskrgr Apr 16, 2022

Choose a reason for hiding this comment

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

"consider calling .len() instead"
(you can skip the ":" here I think, looks like it is already inserted in the warning message automatically? (see tests output)

Copy link
Contributor Author

@kyoto7250 kyoto7250 Apr 16, 2022

Choose a reason for hiding this comment

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

yes, that's right.

Thanks!

@kyoto7250 kyoto7250 marked this pull request as ready for review April 16, 2022 09:36
@giraffate
Copy link
Contributor

Could you do a rebase, not merge? We follow a no merge-commit policy: https://github.com/rust-lang/rust-clippy/blob/master/doc/basics.md#pr

clippy_lints/src/bytes_count_to_len.rs Outdated Show resolved Hide resolved
clippy_lints/src/bytes_count_to_len.rs Outdated Show resolved Hide resolved
clippy_lints/src/bytes_count_to_len.rs Outdated Show resolved Hide resolved
@kyoto7250
Copy link
Contributor Author

Could you do a rebase, not merge? We follow a no merge-commit policy: https://github.com/rust-lang/rust-clippy/blob/master/doc/basics.md#pr

Thanks, I missed the description.

I will rebase this PR, and fix it.

@kyoto7250
Copy link
Contributor Author

I have corrected my PR and would like to get another review.

Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Can this review #8375 (comment) be addressed?

@giraffate
Copy link
Contributor

r? @giraffate

@kyoto7250
Copy link
Contributor Author

kyoto7250 commented Apr 18, 2022

@giraffate

Can this review #8375 (comment) be addressed?

I have addressed this commit about this comment, but I think we need to add a test that addresses this fix.

How do I add a test about this case?
(I couldn't come up with a good test for this conditional branch 🤔)

@giraffate
Copy link
Contributor

giraffate commented Apr 19, 2022

#8711 (comment)

Although str has already ensured in

if is_type_diagnostic_item(cx, ty, sym::String) || ty.kind() == &ty::Str;
, we can add the test case for it like this:

struct Foo<'a> {
    str: &'a str,
}

impl Foo<'_> {
    fn bytes(&self) -> std::str::Bytes<'_> {
        self.str.bytes()
    }
}

fn main() {
    let f = Foo { str: "foo" };
    let _ = f.bytes().count();
}

Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Overall looks good. Could you squash commits?

cdotrus and others added 2 commits April 19, 2022 10:48
formats code with

fixes single match clippy error to replace with if let

swaps ident.name.as_str to ident.name == sym for count fn
adding test patterns

cargo dev bless

fix comment

add ;

delete :

fix suggestion code

and update stderr in tests.

use match_def_path when checking method name
@kyoto7250
Copy link
Contributor Author

@giraffate

I made two commits with c-rus commit and my commit.

@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Apr 19, 2022

📌 Commit f19387d has been approved by giraffate

@bors
Copy link
Collaborator

bors commented Apr 19, 2022

⌛ Testing commit f19387d with merge e17b97c...

@bors
Copy link
Collaborator

bors commented Apr 19, 2022

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

@bors bors merged commit e17b97c into rust-lang:master Apr 19, 2022
@bors bors mentioned this pull request Apr 19, 2022
@bors bors mentioned this pull request Apr 19, 2022
@CBSpeir CBSpeir mentioned this pull request May 2, 2024
bors added a commit that referenced this pull request May 2, 2024
Remove `dead_code` paths

The following paths are `dead_code` and can be removed:

### `clippy_utils::paths::VEC_RESIZE`
* Introduced when `vec_resize_to_zero` lint added in PR #5637
* No longer used after commit 8acc4d2
### `clippy_utils::paths::SLICE_GET`
* Introduced when `get_first` lint added in PR #8882
* No longer used after commit a8d80d5
### `clippy_utils::paths::STR_BYTES`
* Introduced when `bytes_count_to_len` lint added in PR #8711
* No longer used after commit ba6a459

When the lints were moved into the `Methods` lint pass, they switched from using paths to diagnostic items.  However, the paths were never removed.  This occurred in PR #8957.

This relates to issue #5393

changelog: none
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.

Lint suggesting to replace str::bytes().count() with str::len()
7 participants