false negative write_with_newline: format string that starts with \n

[matthiaskrgr]

Summary

If a format string starts with a \n, the lint does not seem to fire.
I had a quick glance at the lint code but I didn't see anything that would indicate that this is intentional.

Lint Name

clippy::write_with_newline

Reproducer

I tried this code:

use std::fmt::Write as _;

pub fn main() {
    let mut s = String::new();
    let x = "a";
    write!(s, "\n{}\n", x).unwrap(); // should warn
    dbg!(s);
}

would expect a warning here since we can write writeln!(s, "\n{}")

Version

clippy 0.1.62 (7b8a08cf1 2022-04-21)

[Serial-ATA]

@rustbot label +good-first-issue

[TennyZhuang]

@rustbot claim

[TennyZhuang]

Here are some positive test cases:

// These should be fine
    write!(v, "");
    write!(v, "Hello");
    writeln!(v, "Hello");
    writeln!(v, "Hello\n");
    writeln!(v, "Hello {}\n", "world");
    write!(v, "Issue\n{}", 1265);
    write!(v, "{}", 1265);
    write!(v, "\n{}", 1275);
    write!(v, "\n\n");
    write!(v, "like eof\n\n");
    write!(v, "Hello {} {}\n\n", "world", "#2");
    writeln!(v, "\ndon't\nwarn\nfor\nmultiple\nnewlines\n"); // #3126
    writeln!(v, "\nbla\n\n"); // #3126

It seems that write! with multiple \n is allowed by the rule, and it seems like a feature not a bug? I'm not sure whether to fix it.

The variable has_internal_newline produces the result.

[matthiaskrgr]

Hmm, so it looks like we bail out as soon as there's more than one \n?
I agree that it doesn't bring much value to change write!(.."\n\n") to writeln!(.."\n") but it also doesn't look like we have an explicit \n{]\n test :)
In my case the example was a bit longer originally like

 write!(
        output,
        "\nSummary of: {} ({} total)\n",
        path.display(),
        bin_cache
            .total_size()
            .file_size(file_size_opts::DECIMAL)
            .unwrap()
    ));
    )
    .unwrap();

[TennyZhuang]

Hmm, so it looks like we bail out as soon as there's more than one \n? I agree that it doesn't bring much value to change write!(.."\n\n") to writeln!(.."\n") but it also doesn't look like we have an explicit \n{]\n test :) In my case the example was a bit longer originally like

 write!(
        output,
        "\nSummary of: {} ({} total)\n",
        path.display(),
        bin_cache
            .total_size()
            .file_size(file_size_opts::DECIMAL)
            .unwrap()
    ));
    )
    .unwrap();

I’m not sure whether it’s better that only allow multiple line-breaks at the end, instead of allow an internal/begin line-break and a line-break at the end.

[TennyZhuang]

If the answer is yes, I’d like to implement it :(

[Alexendoo]

FWIW there's a pending rewrite of write.rs in #8518, I kept the behaviour of ignoring strings that contain multiple newlines, personally I think it's clearer that way

We could also ask the author of #3138 😉

[matthiaskrgr]

🤣 I don't even remember writing that... it has been some time

[km274]

Hey everyone 👋🏻, I ended up here because I was looking for an issue to work on, but it looks like #8518 addressed this. Ignoring strings with multiple newlines makes sense to me, FWIW.

I guess this issue should be closed now (?).