yapf icon indicating copy to clipboard operation
yapf copied to clipboard

Bug adding large whitespace before `)` (and then remove on second format)

Open ShabbyX opened this issue 4 years ago • 4 comments

Take this change in ANGLE: https://chromium-review.googlesource.com/c/angle/angle/+/2807780 When running $ git cl format (from depot tools), it produces the following change:

src/libANGLE/renderer/metal/gen_mtl_format_table.py:
-                    !display->getFeatures().forceD24S8AsUnsupported.enabled")
+                    !display->getFeatures().forceD24S8AsUnsupported.enabled"                                                                            )

Running git cl format again will revert the above change. The addition of whitespace before ) isn't right. I'd appreciate if this could be resolved quickly as clean formatting is a requirement in ANGLE to submit changes. Alternatively, I could attempt to change the code in such a way to not trigger the bug if you can figure out what's causing it.

Note: to get ANGLE's code, follow this: https://chromium.googlesource.com/angle/angle/+/master/README.md#sources

ShabbyX avatar Apr 07 '21 20:04 ShabbyX

Apparently depot tools does some processing before handing the file to yapf, so this might be a bug in depot tools itself. For reference: crbug.com/1196806

ShabbyX avatar Apr 07 '21 20:04 ShabbyX

You can see what Chromium depot tools does here: https://source.chromium.org/chromium/chromium/tools/depot_tools/+/main:git_cl.py;l=5058?q=git_cl

It's delegating almost entirely to yapf and the only logic is to make sure we can restrict to changed lines. At least that's what it looks like from a quick glance.

ajperel avatar Apr 08 '21 14:04 ajperel

Note that the affected line is not even in the change range!

ShabbyX avatar Apr 08 '21 15:04 ShabbyX

The failing line is using line continuation which is probably what's throwing yapf off:

                fallback_condition="metalDevice.depth24Stencil8PixelFormatSupported && \
                    !display->getFeatures().forceD24S8AsUnsupported.enabled")

I worked around this and split the line by +ing two strings, so the ANGLE change should no longer reproduce the problem. You could potentially test a line like the above in a unittest.

ShabbyX avatar Apr 10 '21 02:04 ShabbyX