This is an archive of the discontinued LLVM Phabricator instance.

clang-format: fix conflict between FormatStyle::BWACS_MultiLine and BeforeCatch/BeforeElse
ClosedPublic

Authored by pastey on Dec 27 2019, 8:40 AM.

Details

Summary

Hi,

Found a bug introduced with BraceWrappingFlags AfterControlStatement MultiLine. This feature conflicts with the existing BeforeCatch and BeforeElse flags.

For example, our team uses BeforeElse.

if (foo ||
    bar) {
  doSomething();
}
else {
  doSomethingElse();
}

If we enable MultiLine (which we'd really love to do) we expect it to work like this:

if (foo ||
    bar)
{
  doSomething();
}
else {
  doSomethingElse();
}

What we actually get is:

if (foo ||
    bar)
{
  doSomething();
}
else
{
  doSomethingElse();
}

I added two tests that show this issue. One for if/else and one for try/catch (which is affected in the same way as if/else).

One more test with ColumnLimit set to 40 is to check that a suggested fix doesn't break existing cases. This test is the copy of an existing test from the same case, but it removes line wrap from the stage.

I suggest the fix, but I'm new to this code, so maybe you could suggest something better.

As far as I understood, the problem is caused by the following code:

CompoundStatementIndenter(UnwrappedLineParser *Parser,
                            const FormatStyle &Style, unsigned &LineLevel)
      : CompoundStatementIndenter(Parser, LineLevel,
                                  Style.BraceWrapping.AfterControlStatement,  // <----- here
                                  Style.BraceWrapping.IndentBraces) {}
  CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &LineLevel,
                            bool WrapBrace, bool IndentBrace)
      : LineLevel(LineLevel), OldLineLevel(LineLevel) {
    if (WrapBrace)							      // <----- and here
      Parser->addUnwrappedLine();
    if (IndentBrace)
      ++LineLevel;
  }

Style.BraceWrapping.AfterControlStatement used to be boolean, now it's an enum. MultiLine and Always both turn into true, thus MultiLine leads to a call of addUnwrappedLine().
I tried to place Style.BraceWrapping.AfterControlStatement == FormatStyle::BWACS_Always right in the CompoundStatementIndenter constructor, but that breaks MultiLine.

As I said, I'm new to this code, so maybe my fix is not at the right place, so I would be glad if we find a better fix.

Diff Detail

Event Timeline

pastey created this revision.Dec 27 2019, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2019, 8:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pastey edited the summary of this revision. (Show Details)Dec 27 2019, 8:42 AM
Bouska added a subscriber: Bouska.Dec 27 2019, 12:28 PM

Your code still breaks MultiLine

[ RUN      ] FormatTest.MultiLineControlStatements
/home/pablomg/dev/llvm-project/clang/unittests/Format/FormatTest.cpp:1562: Failure
      Expected: "try {\n" "  foo();\n" "} catch (\n" "    Exception &bar)\n" "{\n" "  baz();\n" "}"
      Which is: "try {\n  foo();\n} catch (\n    Exception &bar)\n{\n  baz();\n}"
To be equal to: format("try{foo();}catch(Exception&bar){baz();}", Style)
      Which is: "try {\n  foo();\n} catch (Exception\n             &bar) {\n  baz();\n}"
With diff:
@@ -1,7 +1,6 @@
 try {
   foo();
-} catch (
-    Exception &bar)
-{
+} catch (Exception
+             &bar) {
   baz();
 }

[  FAILED  ] FormatTest.MultiLineControlStatements (28 ms)
Bouska requested changes to this revision.Dec 27 2019, 3:28 PM

So, you are trying to fix the issue at the wrong place. Contrary from what we should expect from a UnwrappedLine, BWACS_Multiline works by always wrapping the brace in UnwrappedLineParser, and afterwards in UnwrappedLineFormatter merging it back to the control statement if it's not in multi-line. The bug is on the control statement @ line 308 on UnwrappedLineFormatter.cpp. That's the block that merge the brace back to its control statement if it is not a multiline and it's executed only with a line with a else or a catch that begins with right curly brace.

This revision now requires changes to proceed.Dec 27 2019, 3:28 PM
pastey updated this revision to Diff 235466.Dec 28 2019, 2:05 AM

Bouska, thank you! Don't know how I missed that failing test. Never commit on friday :)

However, seeing this test result in a browser window gave me that "a-ha". I understood that I had wrong perception of what MultiLine feature is all about. As I wrote before, I would expect it to format code like this:

if (foo ||
    bar)
{
  doSomething();
}
else {
  doSomethingElse();
}

In my head it didn't have anything to do with ColumnLimit – it was all about more than one line in 'if' condition expression.

After I understood that MultiLine is bound to ColumnLimit, I could understand what was going on @ line 308 on UnwrappedLineFormatter.cpp.

Also found that UnwrappedLine actually wraps every line, which is ... confusing.

Uploaded the new diff. Removed my previous "fix" from UnwrappedLineParser.cpp. Added special case for else/catch in UnwrappedLineFormatter.cpp. Tests pass.

I'm not sure creating a special case for else/catch is the best idea. I would seem better to change the control statement @ line 309 to add it the case where we break before else or catch (add a new || test in the middle condition). I'm really new to this code too, so I don't know if it's better to check that previous line as a right curly brace and that BeforeElse/Catch is true and that first token of the line is else or catch or if just checking that the first token of the line is else or catch (or a mix in between). I'll let a more senior reviewer advise on this.

Thanks for your feedback. Let's see what other reviewers say.

I think generally speaking this looks ok, I'm ok with the extra clause, as long as it's clear what it's doing

clang/lib/Format/UnwrappedLineFormatter.cpp
329 ↗(On Diff #235466)

nit: maybe leave a comment explaining the need for the extra clause

pastey updated this revision to Diff 235621.Dec 30 2019, 10:52 AM
pastey marked an inline comment as done.

@MyDeveloperDay, thanks for looking into this. Added a comment

Bouska accepted this revision.Dec 30 2019, 5:22 PM
This revision is now accepted and ready to land.Dec 30 2019, 5:22 PM
pastey added a comment.Jan 4 2020, 2:12 AM

Guys, sorry for a dumb question – so the fix can be pushed now? If yes, then there's one issue – I don't think I have write access to push the fix – may I ask one of you to push it?

ERROR: Permission to llvm/llvm-project.git denied to pastey12.

This revision was automatically updated to reflect the committed changes.