[clang-format] JSON formatting add new option for controlling newlines in json arrays

[MyDeveloperDay]

Summary

Working in a mixed environment of both vscode/vim with a team configured prettier configuration, this can leave clang-format and prettier fighting each other over the formatting of arrays, both simple arrays of elements.

This review aims to add some "control knobs" to the Json formatting in clang-format to help align the two tools so they can be used interchangeably.

This will allow simply arrays [1, 2, 3] to remain on a single line but will break those arrays based on context within that array.

Happy to change the name of the option (this is the third name I tried)

[curdeius]

clang/docs/ClangFormatStyleOptions.rst

3132

Why limiting to JSON only?
Could we name it in a general fashion (we comment that it's JSON only for the time being). I believe it may be an interesting option for various languages.

How about BreakMultilineArrays, or just BreakArrays to follow the naming of existing options a bit?

clang/lib/Format/TokenAnnotator.cpp

4408

Please elide braces in the ifs.

4418

You can elide braces here.

4425

You might elide braces in both ifs.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

4401–4417

Merge the check for comma below to avoid repeated code. Also, the check for l_brace is redundant.

4406–4415

Use a for loop instead.

4417–4418

This can be deleted now.

[MyDeveloperDay]

Addressed review comments, renamed the options

clang/docs/ClangFormatStyleOptions.rst

3132

I'm going to change the name to be BreakArrays but I'm not 100% sure who it might help other languages, but maybe we can look at this afterwards so having a good name now will help us later on.

clang/lib/Format/TokenAnnotator.cpp

4401–4417

@owenpan you have a very fine eye.. I didn't see that and had to reread your comments a couple of times! Thank you its much cleaner now

4418

This was funny I had RemoveBracesLLVM on, but it didn't get rid of them automatically, I wonder if this was my fault or if we are missing a case for the RemoveBraces option, but thanks you be done now

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

3132

Why limiting to JSON only?

I'm slightly wondering if it would get over rrules by code later on, but we could try I guess, I think we'd need to handle the [] / {} separately I think.

[HazardyKnusperkeks]

clang/.clang-format

2 ↗

(On Diff #459283)

Unrelated.

clang/include/clang/Format/Format.h

2080

2082

2093

Please sort.

2093

What about Leave?

3918

Please sort.

clang/lib/Format/TokenAnnotator.cpp

4401–4402

[MyDeveloperDay]

• address review comments

[MyDeveloperDay]

clang/include/clang/Format/Format.h

2093

I'm going to say that at present the boolean is probably enough as JSON formatting is really to do one or the other, but of course this can progress easily into an enum when there is a use case

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

4401–4416

The compiler will warn without the extra pair of parentheses?

4418

@MyDeveloperDay I'm very curious about this. Can you open an issue if it can be reproduced?

[HazardyKnusperkeks]

clang/lib/Format/Format.cpp

1968

These braces should stay, shouldn't they?

2196

It seems like there are a ton of unrelated brace removals.

[MyDeveloperDay]

clang/lib/Format/Format.cpp

2196

Oh rubbish I thought we were RemoveBraceLLVM clean, let me reproduce the patch

[MyDeveloperDay]

put back removed braces
address other review comments

clang/docs/ClangFormatStyleOptions.rst

**BreakAfterJavaFieldAnnotations** (``Boolean``) :versionbadge:`clang-format 3.8`
 
   .. code-block:: java
 
      true:                                  false:
      @Partial                       vs.     @Partial @Mock DataLoad loader;
      @Mock
      DataLoad loader;
 
+**BreakArrays** (``Boolean``) :versionbadge:`clang-format 16`
+  If ``true``, clang-format will always break after a Json array `[`
+  otherwise it will scan until the closing `]` to determine if it should add
+  newlines between elements (prettier compatible).
+
+  NOTE: This is currently only for formatting JSON.
+
+  .. code-block:: c++
+
+     true:                                  false:
+     [                          vs.      [1, 2, 3, 4]
+       1,
+       2,
+       3,
+       4
+     ]
+
 **BreakBeforeBinaryOperators** (``BinaryOperatorStyle``) :versionbadge:`clang-format 3.6`
   The way to wrap binary operators.
 
   Possible values:
 
   * ``BOS_None`` (in configuration: ``None``)
     Break after operators.
 
import {
          VeryLongImportsAreAnnoying,
          VeryLongImportsAreAnnoying,
          VeryLongImportsAreAnnoying,
      } from 'some/module.js'
 
      false:
      import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,} from "some/module.js"
 
 **KeepEmptyLinesAtTheStartOfBlocks** (``Boolean``) :versionbadge:`clang-format 3.7`
   If true, the empty line at the start of blocks is kept.
 
   .. code-block:: c++
 
      true:                                  false:
      if (foo) {                     vs.     if (foo) {
                                               bar();
        bar();                               }

clang/include/clang/Format/Format.h

struct FormatStyle {
   ///   false:
   ///   void f(int aaaaaaaaaaaaaaaaaaaa,
   ///          int aaaaaaaaaaaaaaaaaaaa,
   ///          int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
   /// \endcode
   /// \version 3.7
   bool BinPackParameters;
 
+  /// If ``true``, clang-format will always break after a Json array `[`
+  /// otherwise it will scan until the closing `]` to determine if it should add
+  /// newlines between elements (prettier compatible).
+  ///
+  /// NOTE: This is currently only for formatting JSON.
+  /// \code
+  ///    true:                                  false:
+  ///    [                          vs.      [1, 2, 3, 4]
+  ///      1,
+  ///      2,
+  ///      3,
+  ///      4
+  ///    ]
+  /// \endcode
+  /// \version 16
+  bool BreakArrays;
+
   /// The style of wrapping parameters on the same line (bin-packed) or
   /// on one line each.
   enum BinPackStyle : int8_t {
     /// Automatically determine parameter bin-packing behavior.
     BPS_Auto,
     /// Always bin-pack parameters.
     BPS_Always,
     /// Never bin-pack parameters.

   };
 
   /// Defines in which cases to put empty line before access modifiers.
   /// \version 12
   EmptyLineBeforeAccessModifierStyle EmptyLineBeforeAccessModifier;
 
   /// If ``true``, clang-format detects whether function calls and
   /// definitions are formatted with one parameter per line.
   ///
   /// Each call can be bin-packed, one-per-line or inconclusive. If it is
   /// inconclusive, e.g. completely on one line, but a decision needs to be
   /// made, clang-format analyzes whether there are other bin-packed cases in
   /// the input file and act accordingly.
   ///
   /// NOTE: This is an experimental flag, that might go away or be renamed. Do
   /// not use this in config files, etc. Use at your own risk.
   /// \version 3.7
   bool ExperimentalAutoDetectBinPacking;
 
   /// Different ways to try to fit all constructor initializers on a line.
   enum PackConstructorInitializersStyle : int8_t {
     /// Always put each constructor initializer on its own line.
     /// \code
     ///    Constructor()
     ///        : a(),
     ///          b()
     /// \endcode
     PCIS_Never,
     /// Bin-pack constructor initializers.
     /// \code
return AccessModifierOffset == R.AccessModifierOffset &&
            AlwaysBreakAfterReturnType == R.AlwaysBreakAfterReturnType &&
            AlwaysBreakBeforeMultilineStrings ==
                R.AlwaysBreakBeforeMultilineStrings &&
            AlwaysBreakTemplateDeclarations ==
                R.AlwaysBreakTemplateDeclarations &&
            AttributeMacros == R.AttributeMacros &&
            BinPackArguments == R.BinPackArguments &&
            BinPackParameters == R.BinPackParameters &&
+           BreakArrays == R.BreakArrays &&
            BreakBeforeBinaryOperators == R.BreakBeforeBinaryOperators &&
            BreakBeforeBraces == R.BreakBeforeBraces &&
            BreakBeforeConceptDeclarations == R.BreakBeforeConceptDeclarations &&
            BreakBeforeTernaryOperators == R.BreakBeforeTernaryOperators &&
            BreakConstructorInitializers == R.BreakConstructorInitializers &&
            CompactNamespaces == R.CompactNamespaces &&
            BreakAfterJavaFieldAnnotations == R.BreakAfterJavaFieldAnnotations &&
            BreakStringLiterals == R.BreakStringLiterals &&
            ColumnLimit == R.ColumnLimit && CommentPragmas == R.CommentPragmas &&
            BreakInheritanceList == R.BreakInheritanceList &&
            ConstructorInitializerIndentWidth ==
                R.ConstructorInitializerIndentWidth &&
            ContinuationIndentWidth == R.ContinuationIndentWidth &&
            Cpp11BracedListStyle == R.Cpp11BracedListStyle &&
            DeriveLineEnding == R.DeriveLineEnding &&
            DerivePointerAlignment == R.DerivePointerAlignment &&
            DisableFormat == R.DisableFormat &&
            EmptyLineAfterAccessModifier == R.EmptyLineAfterAccessModifier &&
            EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier &&
            ExperimentalAutoDetectBinPacking ==
                R.ExperimentalAutoDetectBinPacking &&
            PackConstructorInitializers == R.PackConstructorInitializers &&
            FixNamespaceComments == R.FixNamespaceComments &&
            ForEachMacros == R.ForEachMacros &&
            IncludeStyle.IncludeBlocks == R.IncludeStyle.IncludeBlocks &&
            IncludeStyle.IncludeCategories == R.IncludeStyle.IncludeCategories &&
            IncludeStyle.IncludeIsMainRegex ==
                R.IncludeStyle.IncludeIsMainRegex &&

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     // former for backwards compatibility.
     if (BreakConstructorInitializersBeforeComma &&
         Style.BreakConstructorInitializers == FormatStyle::BCIS_BeforeColon) {
       Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
     }
 
     IO.mapOptional("BreakAfterJavaFieldAnnotations",
                    Style.BreakAfterJavaFieldAnnotations);
+    IO.mapOptional("BreakArrays", Style.BreakArrays);
     IO.mapOptional("BreakStringLiterals", Style.BreakStringLiterals);
     IO.mapOptional("ColumnLimit", Style.ColumnLimit);
     IO.mapOptional("CommentPragmas", Style.CommentPragmas);
     IO.mapOptional("QualifierAlignment", Style.QualifierAlignment);
 
     // Default Order for Left/Right based Qualifier alignment.
     if (Style.QualifierAlignment == FormatStyle::QAS_Right)
       Style.QualifierOrder = {"type", "const", "volatile"};
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   // Off by default Qualifier ordering
   LLVMStyle.QualifierAlignment = FormatStyle::QAS_Leave;
 
   LLVMStyle.DeriveLineEnding = true;
   LLVMStyle.DerivePointerAlignment = false;
   LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
   LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
   LLVMStyle.ExperimentalAutoDetectBinPacking = false;
+  LLVMStyle.BreakArrays = true;
   LLVMStyle.PackConstructorInitializers = FormatStyle::PCIS_BinPack;
   LLVMStyle.FixNamespaceComments = true;
   LLVMStyle.ForEachMacros.push_back("foreach");
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IfMacros.push_back("KJ_IF_MAYBE");
   LLVMStyle.IncludeStyle.IncludeCategories = {
       {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
for (AnnotatedLine *Line : Lines) {
            FormatTok = FormatTok->Next) {
         StringRef Input = FormatTok->TokenText;
         if (FormatTok->Finalized || !FormatTok->isStringLiteral() ||
             // NB: testing for not starting with a double quote to avoid
             // breaking `template strings`.
             (Style.JavaScriptQuotes == FormatStyle::JSQS_Single &&
              !Input.startswith("\"")) ||
             (Style.JavaScriptQuotes == FormatStyle::JSQS_Double &&
              !Input.startswith("\'"))) {
           continue;
         }
 
         // Change start and end quote.
         bool IsSingle = Style.JavaScriptQuotes == FormatStyle::JSQS_Single;
         SourceLocation Start = FormatTok->Tok.getLocation();
         auto Replace = [&](SourceLocation Start, unsigned Length,
                            StringRef ReplacementText) {
for (AnnotatedLine *Line : Lines) {
            FormatTok = FormatTok->Next) {
         if (FormatTok->NewlinesBefore == 0)
           continue;
         FormatToken *Matching = FormatTok->MatchingParen;
         if (!Matching || !FormatTok->getPreviousNonComment())
           continue;
         if (!(FormatTok->is(tok::r_square) &&
               Matching->is(TT_ArrayInitializerLSquare)) &&
             !(FormatTok->is(tok::r_brace) && Matching->is(TT_DictLiteral))) {
           continue;
         }
         FormatToken *Prev = FormatTok->getPreviousNonComment();
         if (Prev->is(tok::comma) || Prev->is(tok::semi))
           continue;
         // getEndLoc is not reliably set during re-lexing, use text length
         // instead.
         SourceLocation Start =

clang/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
   }
 
   // Basic JSON newline processing.
   if (Style.isJson()) {
     // Always break after a JSON record opener.
     // {
     // }
     if (Left.is(TT_DictLiteral) && Left.is(tok::l_brace))
       return true;
-    // Always break after a JSON array opener.
-    // [
-    // ]
-    if (Left.is(TT_ArrayInitializerLSquare) && Left.is(tok::l_square) &&
-        !Right.is(tok::r_square)) {
+    // Always break after a JSON array opener based on BreakArrays.
+    if ((Left.is(TT_ArrayInitializerLSquare) && Left.is(tok::l_square) &&
+         Right.isNot(tok::r_square)) ||
+        Left.is(tok::comma)) {
+      if (Right.is(tok::l_brace))
         return true;
-    }
-    // Always break after successive entries.
-    // 1,
-    // 2
-    if (Left.is(tok::comma))
+      // scan to the right if an we see an object or an array inside
+      // then break.
+      for (const auto *Tok = &Right; Tok; Tok = Tok->Next) {
+        if (Tok->isOneOf(tok::l_brace, tok::l_square))
           return true;
+        if (Tok->isOneOf(tok::r_brace, tok::r_square))
+          break;
+      }
+      return Style.BreakArrays;
+    }
   }
 
   // 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. Import statements, especially in
   // JavaScript, can be an exception to this rule.
   if (Style.JavaScriptWrapImports || Line.Type != LT_ImportStatement) {
     const FormatToken *BeforeClosingBrace = nullptr;
     if ((Left.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
          (Style.isJavaScript() && Left.is(tok::l_paren))) &&
         Left.isNot(BK_Block) && Left.MatchingParen) {
       BeforeClosingBrace = Left.MatchingParen->Previous;
     } else if (Right.MatchingParen &&
                (Right.MatchingParen->isOneOf(tok::l_brace,
                                              TT_ArrayInitializerLSquare) ||
                 (Style.isJavaScript() &&

clang/unittests/Format/FormatTestJson.cpp

TEST_F(FormatTestJson, JsonArray) {
   verifyFormat("[\n"
                "  {\n"
                "    \"name\": 1\n"
                "  },\n"
                "  {}\n"
                "]");
 }
 
+TEST_F(FormatTestJson, JsonArrayOneLine) {
+  FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
+  Style.BreakArrays = false;
+  Style.SpacesInContainerLiterals = false;
+  verifyFormat("[]", Style);
+  verifyFormat("[1]", Style);
+  verifyFormat("[1, 2]", Style);
+  verifyFormat("[1, 2, 3]", Style);
+  verifyFormat("[1, 2, 3, 4]", Style);
+  verifyFormat("[1, 2, 3, 4, 5]", Style);
+
+  verifyFormat("[\n"
+               "  1,\n"
+               "  2,\n"
+               "  {\n"
+               "    A: 1\n"
+               "  }\n"
+               "]",
+               Style);
+}
+
 TEST_F(FormatTestJson, JsonNoStringSplit) {
   FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
   Style.IndentWidth = 4;
   verifyFormat(
       "[\n"
       "    {\n"
       "        "
       "\"naaaaaaaa\": \"foooooooooooooooooooooo oooooooooooooooooooooo\"\n"