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: chars_enumerate_for_byte_indices #13435

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

y21
Copy link
Member

@y21 y21 commented Sep 21, 2024

Closes #10202.

This adds a new lint that checks for uses of the .chars().enumerate() position in a context where a byte index is required and suggests changing it to use .char_indices() instead.

I'm planning to extend this lint to also detect uses of the position in iterator chains, e.g. s.chars().enumerate().for_each(|(i, _)| s.split_at(i));, but that's for another time


changelog: new lint: chars_enumerate_for_byte_indices

@rustbot
Copy link
Collaborator

rustbot commented Sep 21, 2024

r? @Centri3

rustbot has assigned @Centri3.
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 Sep 21, 2024
Copy link
Member Author

@y21 y21 left a comment

Choose a reason for hiding this comment

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

Some notes for reviewers

Comment on lines +44 to +45
&& cx.typeck_results().expr_ty_adjusted(recv).peel_refs().is_str()
&& chars_segment.ident.name.as_str() == "chars"
Copy link
Member Author

Choose a reason for hiding this comment

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

Latest nightly has str::chars as a diagnostic item but our pinned nightly isn't there yet. Might be able to use that if the next sync happens soon enough

Comment on lines +22 to +25
// can't use #[expect] here because the .fixed file will still have the attribute and create an
// unfulfilled expectation, but make sure lint level attributes work on the use expression:
#[allow(clippy::chars_enumerate_for_byte_indices)]
let _ = prim[..idx];
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a fun one, I wonder if that's something that could be fixed in uitest, like removing #[expect] attributes in the .fixed file 🤔

/// ```
#[clippy::version = "1.83.0"]
pub CHARS_ENUMERATE_FOR_BYTE_INDICES,
correctness,
Copy link
Member Author

@y21 y21 Sep 21, 2024

Choose a reason for hiding this comment

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

The pattern is technically fine if you know what your strings are (like the description mentions) so it's not always 'outright wrong' like the usual correctness lints, but the fix is also really simple and always applicable so 🤷‍♂️

@Boshen
Copy link

Boshen commented Sep 26, 2024

Thank you for working on this.

The linter we are working on (oxlint) has encountered dozens of crashes because of this, and there were no ways to forbid such usages.

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
4 participants