Fix println_empty_string suggestion caused error

[relaxcn]

Closes: #16167

changelog: [println_empty_string]: fix suggestion caused error when there is a comma after arg.

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
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

[samueltardieu]

This still fails if there are spaces, newlines, or comments before the comma. The fix should be more robust.

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[relaxcn]

@rustbot ready

[samueltardieu]

This is not robust: if there is no comma at all in the file, a span is built which extends to the next file in the source buffer which is wrong even though we discard it later.

Also, the following code will suggest a replacement which now does not compile, while it did before:

println!("" /* Let's break the Clippy lint, <= because of this comma */);

Why not:

• Ensure that there is no comment inside the macro call and don't provide a suggestion in this case.
• Compute the span in the closure of span_lint_and_then(), and only when we give a suggestion.
• Use something simpler but more robust to compute the span, for example:

        let closing_paren = cx.sess().source_map().span_extend_to_prev_char_before(macro_call.span.shrink_to_hi(), ')', false);
        let mut span = format_args.span.with_hi(closing_paren.lo());

(with the existing comma adjustment in the writeln!() case).

Tests should be added for comments in println!(), but also for writeln!() as right now it would fail the same way if a comment containing a comma is present just before the empty string.

It looks like it would be a more complete and solid fix.

What do you think?

View changes since this review

[relaxcn]

Ensure that there is no comment inside the macro call and don't provide a suggestion in this case.

It's reasonable. But how about using rustc_lexer to skip comment, whitespace to find the following span of ,?

Or we just skip linting the situation you mentioned above.

[relaxcn]

@rustbot ready

[samueltardieu]

This is a lot of complex code for practically no gain. What about my proposal to just not have an autofix if there are comments inside the macro call? You can still lint, you just have to replace the suggestion by a help message saying "remove the empty string" and let the user deal with it.

View changes since this review

[samueltardieu]

Ensure that there is no comment inside the macro call and don't provide a suggestion in this case.

It's reasonable. But how about using rustc_lexer to skip comment, whitespace to find the following span of ,?

Or we just skip linting the situation you mentioned above.

Note that I'm not proposing not to lint, but to not provide a suggestion (as in "autofix") in this case.

[rustbot]

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.

[relaxcn]

This is a lot of complex code for practically no gain. What about my proposal to just not have an autofix if there are comments inside the macro call? You can still lint, you just have to replace the suggestion by a help message saying "remove the empty string" and let the user deal with it.

Thank you. I think using simpler but more robust way to compute span is good as you mentioned before.

So I adopted your suggested code.

However, I still think that using rustc_lexer is a more perfect solution.

At least for writeln_empty_string line, there must be a comma in the macro. We can't use a way that determines whether to use MachineApplicable by checking for the presence of commas in the macro, so we also need to come up with another "compromise" at that time.

Compared to carefully handling strings, operating on tokens is more reassuring.

Just my two cents :)

@rustbot ready