clang-format: [JS] wrap params with trailing commas.

[mprobst]

Summary

JavaScript allows parameter lists to include trailing commas:

myFunction(param1, param2,);

For symmetry with other parenthesized lists ([...], {...}), clang-format
should wrap parenthesized lists one-per-line if they contain a trailing
comma:

myFunction(
    param1,
    param2,
);

This is particularly useful in function declarations or calls with many
arguments, e.g. commonly in constructors.

[mprobst]

Daniel - the tests here are currently failing, and I don't really understand why - it seems the BreakBeforeParameter is not taking effect?

[mprobst]

• get wrapping to work, but not quite

[djasper]

lib/Format/ContinuationIndenter.cpp

858 ↗

(On Diff #98386)

Seems weird to have a newline here but not before l. 866.

871 ↗

(On Diff #98386)

I think you'll have to use getPreviousNonComment() here. While generally we don't do this everywhere, this is a place where people are somewhat likely to put a comment.

[mprobst]

• closer to working code

[mprobst]

• fixed indentation of trailing close paren
• test cases

[mprobst]

• fix various failing tests

[djasper]

lib/Format/ContinuationIndenter.cpp

1043 ↗

(On Diff #98963)

Please make this specific to JavaScript for now. C++ doesn't allow trailing commas here and a trailing comma is more likely to represent "not done yet". In those cases, we don't want to reformat the whole call.

The part above (changing the indentation of a ")" wrapped to a new line is fine to be used for all languages).

unittests/Format/FormatTest.cpp

5505 ↗

(On Diff #98963)

Don't add this line break? I suspect your clang-format isn't set to LLVM style?

9645 ↗

(On Diff #98963)

What caused this change?

[mprobst]

• only trigger wrapping in js mode, revert appropriate tests

[mprobst]

unittests/Format/FormatTest.cpp

5505 ↗

(On Diff #98963)

Yeah, something is amiss. Not sure what yet, but formatted manually for the time being.

9645 ↗

(On Diff #98963)

The trailing comma triggers a break, and then the code in TokenAnnotator causes the wrapping logic (on trailing comma and trailing comment).

Only triggering the wrapping in JS mode, this now just unindents the closing paren, which I think is sensible.

[mprobst]

fix formatting a bit

cfe/trunk/lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) {
   if (Current.isOneOf(tok::r_brace, tok::r_square) && State.Stack.size() > 1) {
     if (Current.closesBlockOrBlockTypeList(Style))
       return State.Stack[State.Stack.size() - 2].NestedBlockIndent;
     if (Current.MatchingParen &&
         Current.MatchingParen->BlockKind == BK_BracedInit)
       return State.Stack[State.Stack.size() - 2].LastSpace;
     return State.FirstIndent;
   }
+  if (Current.is(tok::r_paren) && State.Stack.size() > 1)
+    return State.Stack[State.Stack.size() - 2].LastSpace;
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
     return State.Stack[State.Stack.size() - 2].LastSpace;
   if (Current.is(tok::identifier) && Current.Next &&
       Current.Next->is(TT_DictLiteral))
     return State.Stack.back().Indent;
   if (NextNonComment->is(TT_ObjCStringLiteral) &&
       State.StartOfStringLiteral != 0)
     return State.StartOfStringLiteral - 1;
if (Current.is(TT_TemplateString)) {
       unsigned Column = Current.IsMultiline
                             ? Current.LastLineColumnWidth
                             : State.Column + Current.ColumnWidth;
       NewIndent = Column;
       LastSpace = Column;
       NestedBlockIndent = Column;
     }
 
+    bool EndsInComma =
+        Current.MatchingParen &&
+        Current.MatchingParen->getPreviousNonComment() &&
+        Current.MatchingParen->getPreviousNonComment()->is(tok::comma);
+
     AvoidBinPacking =
+        (Style.Language == FormatStyle::LK_JavaScript && EndsInComma) ||
         (State.Line->MustBeDeclaration && !Style.BinPackParameters) ||
         (!State.Line->MustBeDeclaration && !Style.BinPackArguments) ||
         (Style.ExperimentalAutoDetectBinPacking &&
          (Current.PackingKind == PPK_OnePerLine ||
           (!BinPackInconclusiveFunctions &&
            Current.PackingKind == PPK_Inconclusive)));
+
     if (Current.is(TT_ObjCMethodExpr) && Current.MatchingParen) {
       if (Style.ColumnLimit) {
         // If this '[' opens an ObjC call, determine whether all parameters fit
         // into one line and put one per line if they don't.
         if (getLengthToMatchingParen(Current) + State.Column >
             getColumnLimit(State))
           BreakBeforeParameter = true;
       } else {
         // For ColumnLimit = 0, we have to figure out whether there is or has to
         // be a line break within this call.
         for (const FormatToken *Tok = &Current;
              Tok && Tok != Current.MatchingParen; Tok = Tok->Next) {
           if (Tok->MustBreakBefore ||
               (Tok->CanBreakBefore && Tok->NewlinesBefore > 0)) {
             BreakBeforeParameter = true;
             break;
           }
         }
       }
     }
+
+    if (Style.Language == FormatStyle::LK_JavaScript && EndsInComma)
+      BreakBeforeParameter = true;
   }
   // Generally inherit NoLineBreak from the current scope to nested scope.
   // However, don't do this for non-empty nested blocks, dict literals and
   // array literals as these follow different indentation rules.
   bool NoLineBreak =
       Current.Children.empty() &&
       !Current.isOneOf(TT_DictLiteral, TT_ArrayInitializerLSquare) &&
       (State.Stack.back().NoLineBreak ||

cfe/trunk/lib/Format/TokenAnnotator.cpp

if (Right.is(tok::plus) && Left.is(tok::string_literal) && Right.Next &&
       return true;
   } else if (Style.Language == FormatStyle::LK_Cpp ||
              Style.Language == FormatStyle::LK_ObjC ||
              Style.Language == FormatStyle::LK_Proto) {
     if (Left.isStringLiteral() && Right.isStringLiteral())
       return true;
   }
 
-  // If the last token before a '}' is a comma or a trailing comment, the
-  // intention is to insert a line break after it in order to make shuffling
-  // around entries easier.
+  // If the last token before a '}', ']', or ')' is a comma or a trailing
+  // comment, the intention is to insert a line break after it in order to make
+  // shuffling around entries easier.
   const FormatToken *BeforeClosingBrace = nullptr;
-  if (Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) &&
+  if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
+       (Style.Language == FormatStyle::LK_JavaScript &&
+        Left.is(tok::l_paren))) &&
       Left.BlockKind != BK_Block && Left.MatchingParen)
     BeforeClosingBrace = Left.MatchingParen->Previous;
   else if (Right.MatchingParen &&
-           Right.MatchingParen->isOneOf(tok::l_brace,
-                                        TT_ArrayInitializerLSquare))
+           (Right.MatchingParen->isOneOf(tok::l_brace,
+                                         TT_ArrayInitializerLSquare) ||
+            (Style.Language == FormatStyle::LK_JavaScript &&
+             Right.MatchingParen->is(tok::l_paren))))
     BeforeClosingBrace = &Left;
   if (BeforeClosingBrace && (BeforeClosingBrace->is(tok::comma) ||
                              BeforeClosingBrace->isTrailingComment()))
     return true;
 
   if (Right.is(tok::comment))
     return Left.BlockKind != BK_BracedInit &&
            Left.isNot(TT_CtorInitializerColon) &&

cfe/trunk/unittests/Format/FormatTest.cpp

verifyFormat("DEBUG({\n"
                "         aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;\n"
                "});\n"
                "#if a\n"
                "#else\n"
                "#endif");
 
   verifyIncompleteFormat("void f(\n"
                          "#if A\n"
-                         "    );\n"
+                         ");\n"
                          "#else\n"
                          "#endif");
 }
 
 TEST_F(FormatTest, GraciouslyHandleIncorrectPreprocessorConditions) {
   verifyFormat("#endif\n"
                "#if B");
 }
verifyFormat(
       "    new aaaaaaaaaaaaa<aaaaaaaaaa, aaaaaaaaaaa,\n"
       "                      aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
       "                      aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa>(\n"
       "        bbbbbbbbbbbbbbbbbbbbbbbb);",
       getLLVMStyleWithColumns(72));
   EXPECT_EQ("static_cast<A< //\n"
             "    B> *>(\n"
             "\n"
-            "    );",
+            ");",
             format("static_cast<A<//\n"
                    "    B>*>(\n"
                    "\n"
                    "    );"));
   verifyFormat("int aaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "    const typename aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaa);");
 
   FormatStyle AlwaysBreak = getLLVMStyle();
TEST_F(FormatTest, BreaksStringLiteralsWithin_TMacro) {
   //                   getLLVMStyleWithColumns(20)));
   EXPECT_EQ(
       "_T ( \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\" )",
       format("  _T ( \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\" )", Style));
   EXPECT_EQ("f(\n"
             "#if !TEST\n"
             "    _T(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\")\n"
             "#endif\n"
-            "    );",
+            ");",
             format("f(\n"
                    "#if !TEST\n"
                    "_T(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\")\n"
                    "#endif\n"
                    ");"));
   EXPECT_EQ("f(\n"
             "\n"
             "    _T(\"XXXXXXXXXXXXXXXXXXXXXXXXXXXXXn\"));",
verifyFormat("double &operator[](int i) { return 0; }\n"
                "int i;");
   verifyFormat("std::unique_ptr<int[]> foo() {}");
   verifyFormat("int i = a[a][a]->f();");
   verifyFormat("int i = (*b)[a]->f();");
 
   // Other corner cases.
   verifyFormat("void f() {\n"
                "  bar([]() {} // Did not respect SpacesBeforeTrailingComments\n"
-               "      );\n"
+               "  );\n"
                "}");
 
   // Lambdas created through weird macros.
   verifyFormat("void f() {\n"
                "  MACRO((const AA &a) { return 1; });\n"
                "  MACRO((AA &a) { return 1; });\n"
                "}");
 

cfe/trunk/unittests/Format/FormatTestJS.cpp

verifyFormat("class X {\n"
                "  }\n"
                "}");
   verifyFormat("function initialize() {\n"
                "  // Comment.\n"
                "  return async.then();\n"
                "}\n");
 }
 
+TEST_F(FormatTestJS, FunctionParametersTrailingComma) {
+  verifyFormat("function trailingComma(\n"
+               "    p1,\n"
+               "    p2,\n"
+               "    p3,\n"
+               ") {\n"
+               "  a;  //\n"
+               "}\n",
+               "function trailingComma(p1, p2, p3,) {\n"
+               "  a;  //\n"
+               "}\n");
+  verifyFormat("trailingComma(\n"
+               "    p1,\n"
+               "    p2,\n"
+               "    p3,\n"
+               ");\n",
+               "trailingComma(p1, p2, p3,);\n");
+  verifyFormat("trailingComma(\n"
+               "    p1  // hello\n"
+               ");\n",
+               "trailingComma(p1 // hello\n"
+               ");\n");
+}
+
 TEST_F(FormatTestJS, ArrayLiterals) {
   verifyFormat("var aaaaa: List<SomeThing> =\n"
                "    [new SomeThingAAAAAAAAAAAA(), new SomeThingBBBBBBBBB()];");
   verifyFormat("return [\n"
                "  aaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbbbbbbbbbbbbbbbbb,\n"
                "  ccccccccccccccccccccccccccc\n"
                "];");
   verifyFormat("return [\n"
verifyFormat("SomeFunction(function() {\n"
                "}.bind(this));");
 
   // FIXME: This is bad, we should be wrapping before "function() {".
   verifyFormat("someFunction(function() {\n"
                "  doSomething();  // break\n"
                "})\n"
                "    .doSomethingElse(\n"
                "        // break\n"
-               "        );");
+               "    );");
 
   Style.ColumnLimit = 33;
   verifyFormat("f({a: function() { return 1; }});", Style);
   Style.ColumnLimit = 32;
   verifyFormat("f({\n"
                "  a: function() { return 1; }\n"
                "});",
                Style);
verifyFormat("var a = a.aaaaaaa(\n"
                "        aaaaaaaaaaaaaaaaaaaaa(bbbbbbb));");
 
   // FIXME: This is bad, we should be wrapping before "() => {".
   verifyFormat("someFunction(() => {\n"
                "  doSomething();  // break\n"
                "})\n"
                "    .doSomethingElse(\n"
                "        // break\n"
-               "        );");
+               "    );");
 }
 
 TEST_F(FormatTestJS, ReturnStatements) {
   verifyFormat("function() {\n"
                "  return [hello, world];\n"
                "}");
 }