[clang-Format] Fix indentation of member call after block

[ank]

Summary

before patch:

echo "test() {([]() -> {int b = 32;return 3;}).as("");});" | clang-format -style=Google

test() {
  ([]() -> {
    int b = 32;
    return 3;
  })
      .as();
});

after patch:

echo "test() {([]() -> {int b = 32;return 3;}).as("");});" | clang-format -style=Google

test() {
  ([]() -> {
    int b = 32;
    return 3;
  }).as();
});

[ank]

ping

[klimek]

unittests/Format/FormatTest.cpp

4425

What would be interesting is tests that:
a) have another value after the closing }; doesn't really make sense in this test, but usually these are in calls
f([]() { ... }, foo, bar).call(...)

b) make .as("") have paramters that go beyond the limit

c) add another chained call behind .as("").

[ank]

unittests/Format/FormatTest.cpp

4425

Hello, sorry for the late follow up on this!

Indeed an interesting thing to test, and so I did, the patterns you describe seems to give strange indentation as far back as release_36 and probably has always done so. The case I'm testing here worked in release_40 but stopped working somewhere before release_50

maybe fixing those other cases can be done in another patch

cheers,
Anders

[ank]

Is there any chance to get this change or a similar one in so we get same behaviour as in release_40, even though it does not correct all of the problems?

[klimek]

unittests/Format/FormatTest.cpp

4425

I meant: please add these tests :)

[ank]

unittests/Format/FormatTest.cpp

4425

I need to clarify, those tests will not be correctly indented by this commit, I could add those tests but then they would verify a faulty behaviour, this is how they will be indented even with this patch applied:

// Dont break if only closing statements before member call
verifyFormat("test() {\n"
             "  ([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  }).foo();\n"
             "}");
verifyFormat("test() {\n"
             "  (\n"
             "      []() -> {\n"
             "        int b = 32;\n"
             "        return 3;\n"
             "      },\n"
             "      foo, bar)\n"
             "      .foo();\n"
             "}");
verifyFormat("test() {\n"
             "  ([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  })\n"
             "      .foo()\n"
             "      .bar();\n"
             "}");
verifyFormat("test() {\n"
             "  ([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  })\n"
             "      .foo(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
             "           \"bbbbbbbbbbbbbbbbbbbbbb\");\n"
             "}");

verifyFormat("test() {\n"
             "  foo([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  }).foo();\n"
             "}");
verifyFormat("test() {\n"
             "  foo(\n"
             "      []() -> {\n"
             "        int b = 32;\n"
             "        return 3;\n"
             "      },\n"
             "      foo, bar)\n"
             "      .as();\n"
             "}");
verifyFormat("test() {\n"
             "  foo([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  })\n"
             "      .foo()\n"
             "      .bar();\n"
             "}");
verifyFormat("test() {\n"
             "  foo([]() -> {\n"
             "    int b = 32;\n"
             "    return 3;\n"
             "  })\n"
             "      .as(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
             "          \"bbbbbbbbbbbbbbbbbbbbbb\");\n"
             "}");

[klimek]

unittests/Format/FormatTest.cpp

4425

I'm not sure we agree that the behavior is "faulty" - I do believe it was an intentional change :)
This is an indent of 4 from the start of the expression that call belongs to.
For example, in example (2), having the .foo() at the end of line after a potentially complex parameter strongly de-emphasizes the parameter.
In example (3), how would you want to layout a chain of calls?

[ank]

unittests/Format/FormatTest.cpp

4425

I see! I think your argument makes perfect sense, I think example 3 should be as is, considering that it is consistent with the other behaviour :) thanks for taking the time to explain!

[klimek]

unittests/Format/FormatTest.cpp

4449–4450

One more nit: I'd use a Style with a smaller column limit for these tests, makes them a lot more readable :)

4452

Are the ones below here testing anything that's not tested above?

[ank]

unittests/Format/FormatTest.cpp

4449–4450

makes sense, I'll do that.

4452

nope I'll remove them!

[klimek]

LG.

[ank]

awesome, I do not have merge rights so help with merging this would be greatly appreciated

[ank]

Ping!

it would be awesome to get some help getting this merged since I do not have merge rights

[klimek]

Sorry that I lost track of that, but feel free to ping once / week - unfortunately the patch doesn't apply cleanly, can you rebase?

[danilaml]

lib/Format/FormatToken.h

323

Looks like all other ifs in this file have space before (.

[ank]

fix space in if (
and rebase

[ank]

Ping!

it would be awesome to get some help getting this merged since I do not have merge rights

[ank]

ping

[ank]

ping @klimek

[ilya-biryukov]

Landed the patch as r342363.

lib/Format/ContinuationIndenter.cpp

if (startsSegmentOfBuilderTypeCall(Current) &&
       (State.Stack.back().CallContinuation != 0 ||
        State.Stack.back().BreakBeforeParameter) &&
       // JavaScript is treated different here as there is a frequent pattern:
       //   SomeFunction(function() {
       //     ...
       //   }.bind(...));
       // FIXME: We should find a more generic solution to this problem.
       !(State.Column <= NewLineColumn &&
-        Style.Language == FormatStyle::LK_JavaScript))
+        Style.Language == FormatStyle::LK_JavaScript) &&
+      !(Previous.closesScopeAfterBlock() &&
+        State.Column <= NewLineColumn))
     return true;
 
   // If the template declaration spans multiple lines, force wrap before the
   // function/class declaration
   if (Previous.ClosesTemplateDeclaration &&
       State.Stack.back().BreakBeforeParameter && Current.CanBreakBefore)
     return true;
 

lib/Format/FormatToken.h

template <typename A, typename B> bool isOneOf(A K1, B K2) const {
     return is(K1) || is(K2);
   }
   template <typename A, typename B, typename... Ts>
   bool isOneOf(A K1, B K2, Ts... Ks) const {
     return is(K1) || isOneOf(K2, Ks...);
   }
   template <typename T> bool isNot(T Kind) const { return !is(Kind); }
 
+  bool closesScopeAfterBlock() const {
+    if(BlockKind == BK_Block)
+      return true;
+    if(closesScope())
+      return Previous->closesScopeAfterBlock();
+    return false;
+  }
+
   /// \c true if this token starts a sequence with the given tokens in order,
   /// following the ``Next`` pointers, ignoring comments.
   template <typename A, typename... Ts>
   bool startsSequence(A K1, Ts... Tokens) const {
     return startsSequenceInternal(K1, Tokens...);
   }
 
   /// \c true if this token ends a sequence with the given tokens in order,

unittests/Format/FormatTest.cpp

verifyFormat("aaaaaaaaaaaaaaaaaa.aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaa)\n"
                getLLVMStyleWithColumns(60));
   verifyFormat("aaaaaaaaaaaaaaaaaa\n"
                "    .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaa)\n"
                "    .aaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaa);",
                getLLVMStyleWithColumns(59));
   verifyFormat("aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)\n"
                "    .aaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
+
+  // Dont break if only closing statements before member call
+  verifyFormat("test() {\n"
+               "  ([]() -> {\n"
+               "    int b = 32;\n"
+               "    return 3;\n"
+               "  }).foo();\n"
+               "}");
+  verifyFormat("test() {\n"
+               "  (\n"
+               "      []() -> {\n"
+               "        int b = 32;\n"
+               "        return 3;\n"
+               "      },\n"
+               "      foo, bar)\n"
+               "      .foo();\n"
+               "}");
+  verifyFormat("test() {\n"
+               "  ([]() -> {\n"
+               "    int b = 32;\n"
+               "    return 3;\n"
+               "  })\n"
+               "      .foo()\n"
+               "      .bar();\n"
+               "}");
+  verifyFormat("test() {\n"
+               "  ([]() -> {\n"
+               "    int b = 32;\n"
+               "    return 3;\n"
+               "  })\n"
+               "      .foo(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
+               "           \"bbbbbbbbbbbbbbbbbbbbbb\");\n"
+               "}");
+
+  verifyFormat("test() {\n"
+               "  foo([]() -> {\n"
+               "    int b = 32;\n"
+               "    return 3;\n"
+               "  }).foo();\n"
+               "}");
+  verifyFormat("test() {\n"
+               "  foo(\n"
+               "      []() -> {\n"
+               "        int b = 32;\n"
+               "        return 3;\n"
+               "      },\n"
+               "      foo, bar)\n"
+               "      .as();\n"
+               "}");
+  verifyFormat("test() {\n"
+               "  foo([]() -> {\n"
+               "    int b = 32;\n"
+               "    return 3;\n"
+               "  })\n"
+               "      .foo()\n"
+               "      .bar();\n"
+               "}");
+  verifyFormat("test() {\n"
+               "  foo([]() -> {\n"
+               "    int b = 32;\n"
+               "    return 3;\n"
+               "  })\n"
+               "      .as(\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
+               "          \"bbbbbbbbbbbbbbbbbbbbbb\");\n"
+               "}");
 }
 
 TEST_F(FormatTest, BreaksAccordingToOperatorPrecedence) {
   verifyFormat(
       "if (aaaaaaaaaaaaaaaaaaaaaaaaa ||\n"
       "    bbbbbbbbbbbbbbbbbbbbbbbbb && ccccccccccccccccccccccccc) {\n}");
   verifyFormat(
       "if (aaaaaaaaaaaaaaaaaaaaaaaaa or\n"