Add unnecessary_trailing_comma lint#16530
Conversation
|
r? @Jarcho rustbot has assigned @Jarcho. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
46ad29b to
0f7b216
Compare
This comment has been minimized.
This comment has been minimized.
0f7b216 to
f3f0517
Compare
|
Lintcheck changes for 87d5992
This comment will be updated if you push new changes |
f3f0517 to
f180a19
Compare
|
Some changes occurred in clippy_lints/src/doc cc @notriddle |
clippy_lints/src/format_args.rs
Outdated
| let sm = self.cx.sess().source_map(); | ||
| let span = self.macro_call.span.source_callsite(); | ||
| if !sm.is_multiline(span) | ||
| && let span = sm.span_extend_to_prev_char_before(span.shrink_to_hi(), ')', false) |
There was a problem hiding this comment.
This means println!["hi",], which is allowed, won't lint.
There was a problem hiding this comment.
Thx, great catch, totally forgot!!
There was a problem hiding this comment.
This still doesn't work right in all cases. This test case gives an incorrect "correction":
println!["Foo(,)", ]; //~ unnecessary_trailing_commaThere was a problem hiding this comment.
@notriddle thx for catching all the edge cases! It took me a bit longer than anticipated. Seem to work now with all the new test cases
f180a19 to
c8541cc
Compare
This comment has been minimized.
This comment has been minimized.
|
The |
|
@samueltardieu this lint is essentially the missing piece for a single line macro calls that |
…g#16533) This noop PR makes the new comma-removing lint PR less noisy, allowing it focus only on the new functionality See rust-lang#16530 changelog: none
ccd5535 to
1f35186
Compare
This comment has been minimized.
This comment has been minimized.
…n, r=mati865 style: remove unneeded trailing commas Make format-like macro calls look similar to what `cargo fmt` does automatically - remove trailing commas. When removing a comma, I also inlined some variables for consistency and clarity. I'm working on a [clippy lint](rust-lang/rust-clippy#16530) to make this process automatic.
…n, r=mati865 style: remove unneeded trailing commas Make format-like macro calls look similar to what `cargo fmt` does automatically - remove trailing commas. When removing a comma, I also inlined some variables for consistency and clarity. I'm working on a [clippy lint](rust-lang/rust-clippy#16530) to make this process automatic.
…n, r=mati865 style: remove unneeded trailing commas Make format-like macro calls look similar to what `cargo fmt` does automatically - remove trailing commas. When removing a comma, I also inlined some variables for consistency and clarity. I'm working on a [clippy lint](rust-lang/rust-clippy#16530) to make this process automatic.
…n, r=mati865 style: remove unneeded trailing commas Make format-like macro calls look similar to what `cargo fmt` does automatically - remove trailing commas. When removing a comma, I also inlined some variables for consistency and clarity. I'm working on a [clippy lint](rust-lang/rust-clippy#16530) to make this process automatic.
…n, r=mati865 style: remove unneeded trailing commas Make format-like macro calls look similar to what `cargo fmt` does automatically - remove trailing commas. When removing a comma, I also inlined some variables for consistency and clarity. I'm working on a [clippy lint](rust-lang/rust-clippy#16530) to make this process automatic.
…n, r=mati865 style: remove unneeded trailing commas Make format-like macro calls look similar to what `cargo fmt` does automatically - remove trailing commas. When removing a comma, I also inlined some variables for consistency and clarity. I'm working on a [clippy lint](rust-lang/rust-clippy#16530) to make this process automatic.
…n, r=mati865 style: remove unneeded trailing commas Make format-like macro calls look similar to what `cargo fmt` does automatically - remove trailing commas. When removing a comma, I also inlined some variables for consistency and clarity. I'm working on a [clippy lint](rust-lang/rust-clippy#16530) to make this process automatic.
1f35186 to
d6d1b45
Compare
This comment has been minimized.
This comment has been minimized.
Rollup merge of #152372 - nyurik:clippy-rustc_trait_selection, r=mati865 style: remove unneeded trailing commas Make format-like macro calls look similar to what `cargo fmt` does automatically - remove trailing commas. When removing a comma, I also inlined some variables for consistency and clarity. I'm working on a [clippy lint](rust-lang/rust-clippy#16530) to make this process automatic.
clippy_lints/src/format_args.rs
Outdated
| #[allow(clippy::result_large_err, reason = "due to span_to_source()")] | ||
| fn check_trailing_comma(&self) { | ||
| let sm = self.cx.sess().source_map(); | ||
| let span = self.macro_call.span.source_callsite(); |
There was a problem hiding this comment.
Why are you calling source_callsite here? macro_call.span is already the call site of the macro.
clippy_lints/src/format_args.rs
Outdated
| if !sm.is_multiline(span) | ||
| && let Ok(removal_span) = sm.span_to_source(span, |s, start, end| { | ||
| // This fn returns a span between the last non-whitespace character | ||
| // and the closing parenthesis, but only if it contains a ',' char. | ||
| // Iterates in reverse, checking last char is a closing parenthesis, | ||
| // then looking for a comma before it, ignoring whitespace. | ||
| if let Some(text) = s.get(start..end) | ||
| && let mut chars = text.char_indices().rev() | ||
| && let Some((last_char_index, ')' | ']' | '}')) = chars.next() | ||
| { | ||
| let mut has_comma = false; | ||
| for (index, c) in chars { | ||
| if c == ',' { | ||
| has_comma = true; | ||
| } else if c.is_whitespace() { | ||
| // keep iterating | ||
| } else if has_comma { | ||
| return Ok(span | ||
| .with_lo(span.lo() + BytePos::from_usize(index + c.len_utf8())) | ||
| .with_hi(span.lo() + BytePos::from_usize(last_char_index))); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| Err(SpanSnippetError::IllFormedSpan(span)) | ||
| }) |
There was a problem hiding this comment.
This whole thing could be:
let span_data = span.data();
if let Some(src) = span_data.get_source_text(cx)
&& let Some(src) = src.strip_suffix([')', ']', '}'])
&& let Some(src) = src.trim_end_matches(|c| c.is_whitespace() && c != '\n').strip_suffix(',')
{
let removal_sp = SpanData {
lo: span_data.lo + src.len(),
hi: span_data.hi - 1,
..span_data
}.span();
}This will also remove the comma on weird formatting like:
format!("{}",
arg,);
There was a problem hiding this comment.
thank you!!! I spent over an hour trying to figure out how to optimize it, and didn't notice that strip_suffix supports a pattern!
clippy_lints/src/format_args.rs
Outdated
| self.cx, | ||
| UNNECESSARY_TRAILING_COMMA, | ||
| removal_span, | ||
| format!("unnecessary trailing comma in `{name}!` macro"), |
There was a problem hiding this comment.
Calling out the macro name doesn't seem like adds anything useful to the message.
clippy_lints/src/format_args.rs
Outdated
| /// single-line macro invocations. | ||
| /// | ||
| /// ### Why is this bad? | ||
| /// The trailing comma is redundant and removing it keeps the code cleaner. |
There was a problem hiding this comment.
I wouldn't call this cleaner, but more consistent with regular function calls.
|
Reminder, once the PR becomes ready for a review, use |
Suggest removing an unnecessary trailing comma before the closing parenthesis in single-line format-like macro invocations (e.g. println!, format!, write!). The lint currently only runs on format-like macros because it relies on format-argument parsing; arbitrary user macros are not supported to avoid incorrect suggestions. - Lint is in the `style` group (allow-by-default) - Single-line only: multi-line macro invocations are not linted - Machine-applicable fix: removes the trailing comma
d6d1b45 to
ddde8fe
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
clippy_lints/src/format_args.rs
Outdated
| #[allow(clippy::result_large_err, reason = "due to span_to_source()")] | ||
| fn check_trailing_comma(&self) { | ||
| let sm = self.cx.sess().source_map(); | ||
| let span = self.macro_call.span.source_callsite(); |
clippy_lints/src/format_args.rs
Outdated
| if !sm.is_multiline(span) | ||
| && let Ok(removal_span) = sm.span_to_source(span, |s, start, end| { | ||
| // This fn returns a span between the last non-whitespace character | ||
| // and the closing parenthesis, but only if it contains a ',' char. | ||
| // Iterates in reverse, checking last char is a closing parenthesis, | ||
| // then looking for a comma before it, ignoring whitespace. | ||
| if let Some(text) = s.get(start..end) | ||
| && let mut chars = text.char_indices().rev() | ||
| && let Some((last_char_index, ')' | ']' | '}')) = chars.next() | ||
| { | ||
| let mut has_comma = false; | ||
| for (index, c) in chars { | ||
| if c == ',' { | ||
| has_comma = true; | ||
| } else if c.is_whitespace() { | ||
| // keep iterating | ||
| } else if has_comma { | ||
| return Ok(span | ||
| .with_lo(span.lo() + BytePos::from_usize(index + c.len_utf8())) | ||
| .with_hi(span.lo() + BytePos::from_usize(last_char_index))); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| Err(SpanSnippetError::IllFormedSpan(span)) | ||
| }) |
There was a problem hiding this comment.
thank you!!! I spent over an hour trying to figure out how to optimize it, and didn't notice that strip_suffix supports a pattern!
clippy_lints/src/format_args.rs
Outdated
| self.cx, | ||
| UNNECESSARY_TRAILING_COMMA, | ||
| removal_span, | ||
| format!("unnecessary trailing comma in `{name}!` macro"), |
clippy_lints/src/format_args.rs
Outdated
| && let src = src.trim_end_matches(|c: char| c.is_whitespace() && c != '\n') | ||
| && let Some(src) = src.strip_suffix(',') | ||
| { | ||
| let src = src.trim_end_matches(|c: char| c.is_whitespace() && c != '\n'); |
There was a problem hiding this comment.
This should either take all whitespace or not lint if this hits the start of the line.
print!("{}"
, arg
,);
Either the parenthesis should be placed right after arg, or the lint shouldn't trigger. I'm fine either way, but not linting is probably better.
There was a problem hiding this comment.
All this needed was
&& let src = src.trim_end_matches(|c: char| c.is_whitespace() && c != '\n')
&& !src.end_with('\n')
This also avoids scanning the whole macro text for line breaks, which will be faster.
This matches
cargo fmtbehavior but forformat!(...)-style macro calls.To make this PR easier to review, it moves all linting of the clippy code to #16533
Suggest removing an unnecessary trailing comma before the closing parenthesis in single-line format-like macro invocations (e.g. println!, format!, write!). The lint currently only runs on format-like macros because it relies on format-argument parsing; arbitrary user macros are not supported to avoid incorrect suggestions.
stylegroup (allow-by-default)Closes #13965
changelog: [
unnecessary_trailing_comma]: new lint to remove trailing commas in a single-line format macro usage