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

Check when from_utf8 is called from sliced byte array from string #6134

Merged
merged 1 commit into from
Nov 7, 2020

Conversation

patrickelectric
Copy link
Contributor


Please keep the line below
changelog: Fix #5487: Add linter to check when from_utf8 is called from sliced byte array from string.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @phansch (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 8, 2020
@patrickelectric patrickelectric changed the title Add string_from_utf8_as_bytes linter Check when from_utf8 is called from sliced byte array from string Oct 8, 2020
@patrickelectric patrickelectric force-pushed the as_utf8 branch 4 times, most recently from 4177a96 to 0336d71 Compare October 8, 2020 13:46
@bors
Copy link
Contributor

bors commented Oct 8, 2020

☔ The latest upstream changes (presumably #6117) 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

@patrickelectric
Copy link
Contributor Author

ping @phansch

1 similar comment
@patrickelectric
Copy link
Contributor Author

ping @phansch

@patrickelectric patrickelectric force-pushed the as_utf8 branch 2 times, most recently from d58c1e7 to 5da2b18 Compare October 24, 2020 19:22
@phansch
Copy link
Member

phansch commented Oct 30, 2020

Hey @llogiq, would you mind taking over the review of this new lint? (I'm kind of doing an open source break for a bit with all the things going on right now and removed myself from the review rotation)

@llogiq
Copy link
Contributor

llogiq commented Oct 30, 2020

Sure thing. I'll see if I can find the time to catch up over the weekend.

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

This looks mostly good, I only left a few small nitpicky comments. Please let me know if you want to tackle the suggestion and if so, whether you need any help.

Comment on lines 219 to 220
if method_names.len() == 1;
if method_names[0] == sym!(as_bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be if &[sym!(as_bytes)] == method_names;.

Copy link
Contributor Author

@patrickelectric patrickelectric Oct 31, 2020

Choose a reason for hiding this comment

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

This is not possible since method_names is a vector and the other side is a fixed size array &[something, 1], I may be wrong, but I was unable to find a way to accomplish such thing.
Any tips ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm...perhaps &method_names or even &method_names[..]? Sorry, I don't have the time to check.

Copy link
Member

Choose a reason for hiding this comment

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

If the intention is to use a slice pattern, it should be an if let

clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
@patrickelectric patrickelectric force-pushed the as_utf8 branch 2 times, most recently from d5a104c to f2d32b3 Compare October 31, 2020 19:56
@patrickelectric
Copy link
Contributor Author

Is it possible to kindly add the hacktoberfest-accepted tag or accept ?
From the hacktoberfest page:

Pull requests in repositories with the ‘hacktoberfest’ topic will also need to be merged, approved by a maintainer, or labeled as ‘hacktoberfest-accepted’ in order to qualify. The deadline for completions, merging, labeling, and approving is November 1.

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Another thing i found that may improve the lint, but no blocker.

clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
clippy_lints/src/strings.rs Outdated Show resolved Hide resolved
@phansch phansch assigned llogiq and unassigned phansch Nov 4, 2020
@llogiq
Copy link
Contributor

llogiq commented Nov 4, 2020

Hey, @patrickelectric I think we can merge this if you change the lint group to complexity, we can do the rest of the changes as a follow-up. What do you think?

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
@patrickelectric
Copy link
Contributor Author

Hi @llogiq thank you for your review and catch up, I believe that everything requested was done, let me know if otherwise.

@llogiq
Copy link
Contributor

llogiq commented Nov 7, 2020

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Nov 7, 2020

📌 Commit bc27d14 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Nov 7, 2020

⌛ Testing commit bc27d14 with merge 92ba075...

@bors
Copy link
Contributor

bors commented Nov 7, 2020

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

@bors bors merged commit 92ba075 into rust-lang:master Nov 7, 2020
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.

Suggest &string[6..11] for str::from_utf8(string.as_bytes()[6..11]).unwrap()
7 participants