[clang-format] Fixed AfterEnum handling

[atirit]

Summary

Previously, the AfterEnum option of the BraceWrapping section was treated as always true, regardless of the format passed. These changes resolve this behaviour.

[HazardyKnusperkeks]

You should add tests to prove what you are doing.

[curdeius]

Please add and fix tests.

clang/lib/Format/UnwrappedLineParser.cpp

2485

Why is this removed?

[atirit]

From what I can tell the unit tests are broken. Take FormatTest.ShortEnums for example. It passes the following code:

enum
{
  A,
  B,
  C
} ShortEnum1, ShortEnum2;

And expects no changes to be made.

However, format unit tests use the BreakBeforeBraces: Attach option. This option should attach braces to the same line, as demonstrated in the docs:

BS_Attach (in configuration: Attach) Always attach braces to surrounding context.

namespace N {
enum E {
  E1,
  E2,
};
// ...
}

I'm new to this project so please correct me if I'm wrong, but this appears to indicate broken tests. I'll push a commit with fixed tests.

[atirit]

The FormatTestJS.EnumDeclarations test in fact isn't broken, but FormatTest.ShortEnums and FormatTestCSharp.CSharpKeyWordEscaping are.

EDIT: My mistake, FormatTestCSharp uses a different format than FormatTest. That test appears to be correct. Re-adding the removed lines at L2485-6 fixes this test.

[atirit]

Fixed the FormatTest.ShortEnums unit test and fixed compatibility with FormatTestCSharp.CSharpKeyWordEscaping and FormatTestJS.EnumDeclarations

[atirit]

Changed commit username and email

[MyDeveloperDay]

I think if you think you have a bug that you log it in the bug tracker and we track it with this issue.

[MyDeveloperDay]

Take the following example:

enum { A, B, C, D, E, F, G, H, I } Short;

And the following minimal .clang-format
---
ColumnLimit: 10
BreakBeforeBraces: Custom
BraceWrapping:
    AfterEnum: true

To use AfterEnum you must be using "Custom" for BreakBeforeBraces, the result is

enum
{
  A,
  B,
  C,
  D,
  E,
  F,
  G,
  H,
  I
} Short;

changer AfterEnum to false and you have

enum {
  A,
  B,
  C,
  D,
  E,
  F,
  G,
  H,
  I
} Short;

Could you explain using this example what you think is wrong and why? what do you expect to see?

[atirit]

@MyDeveloperDay I expect to see exactly that. clang-format currently does not respect AfterEnum, treating it as always true. This is why I changed UnwrappedLineParser.cpp.

The unit test is incorrect as the style used for the clang-format C(++) unit tests uses BreakBeforeBraces: Attach, which should result in AfterEnum being treated as false, as indicated by the docs I quoted a few messages back. The unit test expects AfterEnum: true behaviour from a style that should result in AfterEnum: false behaviour. This is why I changed the unit test.

[atirit]

Additionally, this bug has already been reported: https://bugs.llvm.org/show_bug.cgi?id=46927

[atirit]

@MyDeveloperDay Would you mind explaining what changes you're wanting?

[MyDeveloperDay]

I think we are missing some clarity in this bug as to what the actual problem is, I do agree the test looks wrong,

This seems to be that in "Attach" mode, then AllowShortEnumsOnASingleLine=false doesn't attach the brace.

I'm somewhat struggling to understand how the code change fixes that.. given that Style.BraceWrapping.AfterEnum should be true correct?

clang/lib/Format/UnwrappedLineParser.cpp

1305

surely this means always add a newline? but that isn't what is wanted from what I can tell.

[curdeius]

In D93938#2476186, @MyDeveloperDay wrote:

I think we are missing some clarity in this bug as to what the actual problem is, I do agree the test looks wrong,

I agree on this. I'd like to see a more exhaustive test suite for all the combinations of AfterEnum and AllowShortEnumsOnASingleLine, not only fixing the single wrong test.

This seems to be that in "Attach" mode, then AllowShortEnumsOnASingleLine=false doesn't attach the brace.

That's my understanding as well.

I'm somewhat struggling to understand how the code change fixes that.. given that Style.BraceWrapping.AfterEnum should be true correct?

In Attach mode, AfterEnum is false. Cf. expandPresets https://github.com/llvm/llvm-project/blob/47877c9079c27f19a954b660201ea47717c82fec/clang/lib/Format/Format.cpp#L752.

Said all that, it *seems* to me that the fix is correct apart from the strangely looking if (!Style.isCpp()) { change that I don't really understand. Why should C++ be handled differently in this regard? What am I missing?

[atirit]

This seems to be that in "Attach" mode, then AllowShortEnumsOnASingleLine=false doesn't attach the brace.

That is correct, but the main issue is that AfterEnum: false, which Attach mode implies, doesn't function correctly.

Said all that, it *seems* to me that the fix is correct apart from the strangely looking if (!Style.isCpp()) { change that I don't really understand. Why should C++ be handled differently in this regard? What am I missing?

That code was there before. Here is the code currently in release, including comments:

case tok::kw_enum:
  // Ignore if this is part of "template <enum ...".
  if (Previous && Previous->is(tok::less)) {
    nextToken();
    break;
  }

  // parseEnum falls through and does not yet add an unwrapped line as an
  // enum definition can start a structural element.
  if (!parseEnum())
    break;
  // This only applies for C++.
  if (!Style.isCpp()) {
    addUnwrappedLine();
    return;
  }
  break;

I modified this code in an attempt to fix this bug, but that broke styling in other languages. It turns out this section required no modification. The only changes my revisions at present introduce is the change for the unit test and the modification of the following function (ignoring the // NEW comments):

static bool ShouldBreakBeforeBrace(const FormatStyle &Style,
                                   const FormatToken &InitialToken) {
  if (InitialToken.isOneOf(tok::kw_namespace, TT_NamespaceMacro))
    return Style.BraceWrapping.AfterNamespace;
  if (InitialToken.is(tok::kw_class))
    return Style.BraceWrapping.AfterClass;
  if (InitialToken.is(tok::kw_union))
    return Style.BraceWrapping.AfterUnion;
  if (InitialToken.is(tok::kw_struct))        // NEW
    return Style.BraceWrapping.AfterStruct;   // NEW
  return false;
}

I can add some unit tests for the variations of AllowShortEnumsOnASingleLine and AfterEnum.

If there's anything I can explain better please let me know.

[atirit]

After writing a unit test, I've found that a combination of AfterEnum: true and AllowShortEnumsOnASingleLine: true doesn't function properly even in release.. My next revision will include a fix for that alongside the unit test.

[atirit]

Turns out the true/true bug goes quite deep. I've managed to resolve the first bit of it with a hack that I'm sure will warrant some criticism, but I haven't familiarised myself with this codebase enough to write a cleaner version.

The second issue I'm still resolving. I'll expand on what's going on with this bug once I push a revision.

[atirit]

OK, I'm getting a few more failed unit tests and I'm really not sure what correct formatting behaviour is anymore, so I'll just ask.

Assuming the following settings:

BreakBeforeBraces: Custom
BraceWrapping: {
  AfterEnum: true
}
AllowShortEnumsOnASingleLine: true

Which of these is correct? (FormatTest.AllmanBraceBreaking)
A (expected in test):

enum X
{
  Y = 0
}

B:

enum X { Y = 0 }

Which of these is correct? (new test)
A:

enum
{
   A,
   B
} Enum1, Enum2

B:

enum
{
   A,
   B
} Enum1,
   Enum2

C:

enum { A, B } Enum1, Enum2

true/true results in option B of the "new test" section. The strange behaviour with Enum1 and Enum2 occurs when AllowShortEnumsOnASingleLine isn't allowed to shrink a line for whatever reason, be it a comma on the last case, a comment, too long an enum, or AfterEnum: true, which for whatever reason takes precedence.

The docs don't cover corner cases like this from what I can tell. It may be worth doing so in the future so that cases like this aren't implementation-defined.

[atirit]

I think accepting a revision that includes only a fix for AfterEnum being ignored (not the corner case) and the new unit test would be the best way to go about this, since they're separate bugs. Then I can fix the corner case (and compatibility with the new unit test) in a separate differential.

However, as I've already mentioned, I'm new here, so I defer to the judgment of those more experienced.

[HazardyKnusperkeks]

In D93938#2476432, @atirit wrote:

I think accepting a revision that includes only a fix for AfterEnum being ignored (not the corner case) and the new unit test would be the best way to go about this, since they're separate bugs. Then I can fix the corner case (and compatibility with the new unit test) in a separate differential.

However, as I've already mentioned, I'm new here, so I defer to the judgment of those more experienced.

I'm also quite new, but if that are different issues they should receive their own commit (and thus review).

You should always be clear what is not working correctly and reflect that's working after your change with tests which illustrate the now working code.

[MyDeveloperDay]

You need to have these conversations by adding new unit tests that prove your point, I highly doubt I'll personally be willing to accept any revision without more unit tests than this one line change

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

1346–1380

EXPECT_FALSE(Style.BraceWrapping.AfterEnum)

1393

please add

verifyFormat("enum {\n"
                     "A,\n"
                     "B,\n"
                    "} ShortEnum",
                    Style);

[atirit]

In D93938#2476519, @MyDeveloperDay wrote:

You need to have these conversations by adding new unit tests that prove your point, I highly doubt I'll personally be willing to accept any revision without more unit tests than this one line change

And that's why I said:

I think accepting a revision that includes only a fix for AfterEnum being ignored (not the corner case) and the new unit test would be the best way to go about this.

Obviously the current revision isn't sufficient. I'll be submitting a new one shortly.

[atirit]

Added unit test

[atirit]

The first test fails due to the aforementioned corner case.

[MyDeveloperDay]

we don't commit with failing tests so lets understand why it fails.

Can you add the tests without multiple names for the enum

[atirit]

Removed multiple enum names from new test

[atirit]

AfterEnum: true currently overrides AllowShortEnumsOnASingleLine: true. If this is epxected behaviour then I'll modify the test to accomodate that, but otherwise, there's a separate issue.

[MyDeveloperDay]

I'm sorry, but now you are missing the files from the review, I think you diffing against your own commits and not commit that are in the repo. This makes the review very difficult to understand.

[atirit]

I don't understand; should every commit I've made be squashed into one and then submitted here?

[atirit]

Squashed commits

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

1347

keep this test, you should keep one with 1 name and 1 with 2 names

[atirit]

The test still is there; Git is just showing the diff strangely. I didn't modify that test at all. The corner case bug doesn't affect FormatTest.ShortEnums because that test effectively has AfterEnum: false. Should I add cases for AfterEnum: true in that test too? I had figured the new test took care of that.

[MyDeveloperDay]

LGTM