Generate semicolon following Loop

[carsonRadtke]

Fixes #5377

semicolon_for_stmt determines whether or not a statement should end with a semicolon. In the case of the issue, it was determined that Semi statements containing a Loop expression should never end with a semicolon. This is not the case as the repro demonstrates.

I changed the behavior of semicolon_for_stmt to always end in a semicolon.

Note: While expressions and ForLoop expressions also get the semicolon dropped when found in a Semi. The snippet that reproduces for Loop had no issue with these other types, so I left the behavior as-is.

[ytmimi]

Thanks for the first time contribution to rustfmt 🎉

Haven't had a chance to review this yet, but out of curiosity, why is tests/target/issue-5377 a binary file?

[carsonRadtke]

why is tests/target/issue-5377 a binary file?

That was an oversight, it has been fixed. Thanks for pointing it out.

[ytmimi]

Will rustfmt add/remove semicolons if we start with the following input?

fn must_end_with_semi() {
    loop {
        break false;
    }
}

fn cannot_end_with_semi() -> bool {
    loop {
        break false
    };
}

Or is this PR just trying to ensure that if a semicolon exists we don't remove it?

[carsonRadtke]

After my PR, rustfmt will not make any changes to the provided input.

Or is this PR just trying to ensure that if a semicolon exists we don't remove it?

Yes, regardless of whether the user provided ExprKind::Loop or StmtKind::Semi, keep it the same *Kind because otherwise, as can be seen in the issue, rustfmt could introduce compilation errors.

[calebcartwright]

Apologies as I've still not had a chance to look at this, but did want to note that any change we make still has to be compliant with the relevant prescriptions codified into the style guide (e.g. https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/statements.md#expressions-in-statement-position, https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/expressions.md#control-flow-expressions) and also have tests that incorporate relevant config options, like trailing_semicolon

[oxalica]

Why is this closed? I think correctness is more important than the styling. The current behavior breaks semantic of loop { break value; };, although this kind of code effectively does a drop(value).