changelog: new lint: [split_with_space]
#13059
Conversation
Adds a new lint for checking whether the `split` method uses ASCII space as an delimeter and if it does so, recommends use of `split_whitespace`.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Centri3 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
|
|
||
| impl<'tcx> LateLintPass<'tcx> for SplitWithSpace { | ||
| fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'_>) { | ||
| if let ExprKind::MethodCall(path, _split_recv, [arg], split_ws_span) = expr.kind |
There was a problem hiding this comment.
This should check that the receiver is really a &str and not some other type with a split method that may not have split_whitespace (see this)
| split_ws_span.with_hi(split_ws_span.lo()), | ||
| "found call to `str::split` that uses the ASCII space character as an argument".to_string(), | ||
| "replace the use of `str::split` with `str::split_whitespace`".to_string(), | ||
| String::new(), |
There was a problem hiding this comment.
The current span & suggestion seems to replace nothing?
I think it would be good to use multipart_suggestion and replace arg.span with "", and split_ws_span with "split_whitespace", which would generate a diagnostic like
help: replace the use of `str::split` with `str::split_whitespace`
|
LL - some_space_delimtetered_string.split(" ")
LL + some_space_delimtetered_string.split_whitespace()
| "found call to `str::split` that uses the ASCII space character as an argument".to_string(), | ||
| "replace the use of `str::split` with `str::split_whitespace`".to_string(), | ||
| String::new(), | ||
| Applicability::MachineApplicable, |
There was a problem hiding this comment.
...It should probably not be MachineApplicable though.
Although in the majority of cases I imagine this is strictly more correct, there might be weird edge cases where people do want to split by exactly that, and rely on "a b".split(' ').count() == 3, which fails with split_whitespace, and MachineApplicable should not change behavior that the user needs
| /// Checks for calling `str::split` with the ASCII space character instead of `str::split_whitespace`. | ||
| /// | ||
| /// ### Why is this bad? | ||
| /// `split` using the ASCII space character does not handle cases where there is a different space character used. `split_whitespace` handles all whitespace characters. |
There was a problem hiding this comment.
Would be good to mention that split_whitespace also handles multiple consecutive spaces
| //@no-rustfix | ||
|
|
||
| #[deny(clippy::split_with_space)] | ||
| #[allow(unused)] |
There was a problem hiding this comment.
I think allow(unused) is already implicitly passed, so this probably isn't needed
| @@ -0,0 +1,16 @@ | |||
| //@aux-build:proc_macros.rs | |||
There was a problem hiding this comment.
This test links proc_macros.rs but never actually uses it.
It probably should, though, so this is a good idea. Can you add a test with proc macros? You can look at e.g. this file to see how to write a test that uses this.
You'll also likely need to add a !is_from_proc_macro() check to the lint code.
|
|
||
| for substr in some_space_delimtetered_string.split(" ") { | ||
| println!("{substr}"); | ||
| } |
There was a problem hiding this comment.
There should be some negative tests, e.g. splitting by something that isn't a space.
| && path.ident.name == sym!(split) | ||
| && let ExprKind::Lit(lit) = arg.kind | ||
| && (matches!(lit.node, LitKind::Char(' ')) | ||
| || matches!(lit.node, LitKind::Str(str, _) if str.as_str() == " ")) |
There was a problem hiding this comment.
There should be a check here that this isn't from a macro, (or at least, not from an external macro since the user can't change that, can use in_external_macro for this)
|
Are there some examples where However I can't think of any where |
|
☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Hey @DavidHusicka , this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation? If you have any questions, you're always welcome to ask them in this PR or on Zulip. @rustbot author Also it seems like @Centri3 is currently busy. @y21 would you mind taking over this review, since you already started? r? @y21 |
rustbot
added
S-waiting-on-author
changelog: new lint: [
split_with_space]Adds a new lint for checking whether the
splitmethod uses ASCII space as an delimiter and if it does so, recommends use ofsplit_whitespace.