google / google-java-format

Reformats Java source code to comply with Google Java Style.
Other
5.61k stars 854 forks source link

(Prepare for) Handling trailing spaces in multiline strings #450

Open cgrushko opened 4 years ago

cgrushko commented 4 years ago

We ran into this issue with ktfmt, and solved it by replacing the last of the trailing spaces with a special tombstone to prevent GJF from removing all trailing whitespace.

https://github.com/facebookincubator/ktfmt/commit/a4fdd39237d2b59ae3c03cee9322ab04ed2b9222

I'm filing this issue so ktfmt can revert to using the supported method once it's available in GJF :) (assuming multiline strings in Java are going to be a thin soon-ish)

cushon commented 4 years ago

I've been looking at supporting Java language features up to 14 recently, including text blocks, but hadn't thought through this yet. It makes sense that we don't want to make non-semantics-preserving changes to multiline strings. OTOH, have you encountered cases where trailing whitespace in multi-line strings is actually desired/useful? It seems like something that might deserve a style rule.

cgrushko commented 4 years ago

Funnily, it tripped our own tests. We have since converted to regular strings to make it more explicit.

Do you suggest the formatter refuses to format such files?

On Thu, Mar 26, 2020 at 7:28 PM Liam Miller-Cushon notifications@github.com wrote:

I've been looking at supporting Java language features up to 14 recently, including text blocks, but hadn't thought through this yet. It makes sense that we don't want to make non-semantics-preserving changes to multiline strings. OTOH, have you encountered cases where trailing whitespace in multi-line strings is actually desired/useful? It seems like something that might deserve a style rule.

— You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub https://github.com/google/google-java-format/issues/450#issuecomment-604566364, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPH6YP5UK464YKSTXHYWG3RJOGB7ANCNFSM4LUB5ARQ .

cushon commented 4 years ago

Do you suggest the formatter refuses to format such files?

No, it generally tries to accept anything that's syntactically valid and produce semantically equivalent output, I was just curious. We should probably try to preserve trailing whitespace and discourage it separately (e.g. with a linter).

cgrushko commented 4 years ago

Cool; I didn't see a test for trailing spaces, though?

On Tue, Mar 31, 2020 at 2:43 PM Christian Stein notifications@github.com wrote:

Closeable due to 4ddb914 https://github.com/google/google-java-format/commit/4ddb9148c103b9c65768e519884b0c8ad5caaf39 methinks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/google-java-format/issues/450#issuecomment-606576175, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPH6YM6MBXMKQPG2SHZCVTRKHJO3ANCNFSM4LUB5ARQ .

sormuras commented 4 years ago

Cool; I didn't see a test for trailing spaces, though?

Dooh. I replied to the wrong issue and deleted my reply in the meanwhile.

anthonyvdotbe commented 4 years ago

Why isn't removing trailing white space correct, since it's removed during compilation anyway?

cgrushko commented 4 years ago

I don't trailing white space is removed from raw (multiline) strings during compilation.

On Mon, Jun 1, 2020 at 8:50 AM Anthony Vanelverdinghe < notifications@github.com> wrote:

Why isn't removing trailing white space correct, since it's removed during compilation anyway?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/google-java-format/issues/450#issuecomment-636626759, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPH6YLLYDCQ6LFN7I4HXSLRUM6QXANCNFSM4LUB5ARQ .

anthonyvdotbe commented 4 years ago

The algorithm described in the JEP has:

  1. Remove all trailing white space [...]

For cases where trailing spaces must be kept, the line can be ended with the new \s escape sequence:

\s can act as fence to prevent the stripping of trailing white space

cgrushko commented 4 years ago

Interesting! makes sense - otherwise removing trailing whitespace from a .java file requires parsing it. So yeah, looks like no need to handle it in GJF. (I don't see something similar for Kotlin, which inspired this issue; I imagine that most editors strip trailing whitespace, so in practice Kotlin programs don't have trailing whitespace)