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.

Closes: #16251
changelog: [writeln_empty_string]: fix suggestion caused error when there is a comma in the comment before 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

[samueltardieu]

I had more something like that in mind (which is globally your code), which properly removes the empty string in the general case (no comments inside the macro), but only lints without providing a suggestion when comments are present:

    if let [FormatArgsPiece::Literal(sym::LF)] = &format_args.template[..] {
        let is_writeln = name == "writeln";
        span_lint_and_then(
            cx,
            if is_writeln {
                WRITELN_EMPTY_STRING
            } else {
                PRINTLN_EMPTY_STRING
            },
            format_args.span,
            format!("empty string literal in `{name}!`"),
            |diag| {
                if span_extract_comments(cx.sess().source_map(), macro_call.span).is_empty() {
                    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());
                    if is_writeln {
                        span = expand_past_previous_comma(cx, span);
                    }
                    diag.span_suggestion(span, "remove the empty string", "", Applicability::MachineApplicable);
                }
            },
        );
    }

That requires splitting the tests between the existing files and a "_unfixable.rs" version, for when no suggestions are applied.

WDYT?

(edit at 17:35: use narrower format_args.span to place the lint message)

View changes since this review

[relaxcn]

Thank you.
I think your solution makes sense, but there’s a small point you might have overlooked.

For println_empty_string, even if there’s a comment inside the span of the macro call, the method you provided for computing the span can still correctly suggest code changes.

However, for the writeln_empty_string lint, if a comment contains a comma before the arg, it becomes difficult to compute the correct auto-fix span. That’s why, as you mentioned earlier, we don’t provide an auto-fix suggestion when a comment is present in the macro call’s span.

Please take a look at the latest changes. I’d appreciate your feedback if you have any further comments.

@rustbot ready

[samueltardieu]

You misunderstood: it is not acceptable to remove a user-provided comment. The solution I propose (although the span it lint on should be format_args.span to generate a narrower diagnostic) preserves user-provided comments in any case, while yours currently remove them in println!().

[samueltardieu]

This kind of comment, that the user wrote explicitly, must not be removed.

[relaxcn]

Thank you. I understand your point now — that makes sense.

@rustbot ready