Page MenuHomePhabricator

[clang-format] Fixed AfterEnum handling
Needs ReviewPublic

Authored by atirit on Dec 30 2020, 12:59 AM.

Details

Summary

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

Diff Detail

Unit TestsFailed

TimeTest
20 msx64 debian > Clang-Unit.Format/_/FormatTests::FormatTest.AfterEnum
Note: Google Test filter = FormatTest.AfterEnum [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
50 msx64 windows > Clang-Unit.Format/_/FormatTests_exe::FormatTest.AfterEnum
Note: Google Test filter = FormatTest.AfterEnum [==========] Running 1 test from 1 test case.

Event Timeline

atirit requested review of this revision.Dec 30 2020, 12:59 AM
atirit created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2020, 12:59 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
atirit edited the summary of this revision. (Show Details)Dec 30 2020, 1:03 AM
atirit added reviewers: klimek, krasimir.
atirit edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2020, 1:03 AM

You should add tests to prove what you are doing.

lebedev.ri retitled this revision from Fixed AfterEnum handling to [clang-format] Fixed AfterEnum handling.Dec 30 2020, 2:13 AM
curdeius requested changes to this revision.Dec 30 2020, 2:42 AM
curdeius added a subscriber: curdeius.

Please add and fix tests.

clang/lib/Format/UnwrappedLineParser.cpp
2485

Why is this removed?

This revision now requires changes to proceed.Dec 30 2020, 2:42 AM

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 added a comment.EditedDec 30 2020, 2:06 PM

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 updated this revision to Diff 314149.EditedDec 30 2020, 2:35 PM

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

atirit updated this revision to Diff 314162.Dec 30 2020, 7:11 PM

Changed commit username and email

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

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?

MyDeveloperDay requested changes to this revision.Dec 31 2020, 7:04 AM
This revision now requires changes to proceed.Dec 31 2020, 7:04 AM

@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.

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

atirit added a comment.Jan 1 2021, 1:57 PM

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

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 added a comment.EditedJan 2 2021, 6:14 AM

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 added a comment.EditedJan 2 2021, 1:51 PM

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 added a comment.EditedJan 2 2021, 5:40 PM

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 added a comment.Jan 2 2021, 8:56 PM

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 added a comment.EditedJan 3 2021, 12:55 AM

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 added a comment.Jan 3 2021, 2:42 AM

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 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.

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 added inline comments.Jan 3 2021, 6:39 AM
clang/unittests/Format/FormatTest.cpp
1346–1380

EXPECT_FALSE(Style.BraceWrapping.AfterEnum)

1393

please add

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

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 updated this revision to Diff 314296.Jan 3 2021, 11:42 AM

Added unit test

The first test fails due to the aforementioned corner case.

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 updated this revision to Diff 314299.Jan 3 2021, 12:22 PM

Removed multiple enum names from new test

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 requested changes to this revision.Jan 3 2021, 12:36 PM

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.

This revision now requires changes to proceed.Jan 3 2021, 12:36 PM

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

atirit updated this revision to Diff 314301.Jan 3 2021, 1:02 PM

Squashed commits

MyDeveloperDay added inline comments.Jan 3 2021, 1:27 PM
clang/unittests/Format/FormatTest.cpp
1347

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

atirit added a comment.Jan 3 2021, 1:31 PM

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.