[clang-format] Do not break Objective-C string literals inside array literals

[benhamilton]

Summary

Concatenating Objective-C string literals inside an array literal
raises the warning -Wobjc-string-concatenation (which is enabled by default).

clang-format currently splits and concatenates string literals like
the following:

NSArray *myArray = @[ @"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" ];

into:

NSArray *myArray =
      @[ @"aaaaaaaaaaaaaaaaaaaaaaaaaaaa"
         @"aaaaaaaaa" ];

which raises the warning. This is https://bugs.llvm.org/show_bug.cgi?id=36153 .

The options I can think of to fix this are:

1. Have clang-format disable Wobjc-string-concatenation by emitting

pragmas around the formatted code

2. Have clang-format wrap the string literals in a macro (which

disables the warning)

3. Disable string splitting for Objective-C string literals inside

array literals

I think 1) has no precedent, and I couldn't find a good
identity() macro for 2). So, this diff implements 3).

Test Plan: make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests

[benhamilton]

Ping! Any comments?

[benhamilton]

Ping ping!

[stephanemoore]

The summary states that -Wobjc-string-concatenation is enabled by default? I looked through include/clang/Basic/DiagnosticGroups.td and did not see evidence that ObjCStringConcatenation is under All. Am I missing something? Is -Wobjc-string-concatenation enabled by default through some other mechanism?

[benhamilton]

@stephanemoore: It appears all warnings are enabled by default unless they are in class DefaultIgnore:

https://github.com/llvm-mirror/clang/blob/6de2efd1953adaa9a190b2cdfbe7b5c15f6d6efe/include/clang/Basic/Diagnostic.td#L105

This diagnostic is not in DefaultIgnore:

https://github.com/llvm-mirror/clang/blob/a58d0437d1e17d53d7bffea598d77789dd6d28b6/include/clang/Basic/DiagnosticGroups.td#L933

In fact, the All class is specifically for warnings which *are* in DefaultIgnore but which should be emitted with -Wall:

https://github.com/llvm-mirror/clang/blob/a58d0437d1e17d53d7bffea598d77789dd6d28b6/include/clang/Basic/DiagnosticGroups.td#L782

// Note that putting warnings in -Wall will not disable them by default. If a
// warning should be active _only_ when -Wall is passed in, mark it as
// DefaultIgnore in addition to putting it here.

[stephanemoore]

Thanks for the explanation 👌 I'm still learning the inner workings of clang 😅

The change looks good to me.

lib/Format/ContinuationIndenter.h

struct ParenState {
   ParenState(unsigned Indent, unsigned LastSpace, bool AvoidBinPacking,
              bool NoLineBreak)
       : Indent(Indent), LastSpace(LastSpace), NestedBlockIndent(Indent),
         BreakBeforeClosingBrace(false), AvoidBinPacking(AvoidBinPacking),
         BreakBeforeParameter(false), NoLineBreak(NoLineBreak),
         NoLineBreakInOperand(false), LastOperatorWrapped(true),
         ContainsLineBreak(false), ContainsUnwrappedBuilder(false),
         AlignColons(true), ObjCSelectorNameFound(false),
-        HasMultipleNestedBlocks(false), NestedBlockInlined(false) {}
+        HasMultipleNestedBlocks(false), NestedBlockInlined(false),
+        IsInsideObjCArrayLiteral(false) {}
 
   /// \brief The position to which a specific parenthesis level needs to be
   /// indented.
   unsigned Indent;
 
   /// \brief The position of the last space on each level.
   ///
   /// Used e.g. to break like:
struct ParenState {
   /// Not considered for memoization as it will always have the same value at
   /// the same token.
   bool HasMultipleNestedBlocks : 1;
 
   /// \brief The start of a nested block (e.g. lambda introducer in C++ or
   /// "function" in JavaScript) is not wrapped to a new line.
   bool NestedBlockInlined : 1;
 
+  /// \brief \c true if the current \c ParenState represents an Objective-C
+  /// array literal.
+  bool IsInsideObjCArrayLiteral : 1;
+
   bool operator<(const ParenState &Other) const {
     if (Indent != Other.Indent)
       return Indent < Other.Indent;
     if (LastSpace != Other.LastSpace)
       return LastSpace < Other.LastSpace;
     if (NestedBlockIndent != Other.NestedBlockIndent)
       return NestedBlockIndent < Other.NestedBlockIndent;
     if (FirstLessLess != Other.FirstLessLess)

lib/Format/ContinuationIndenter.cpp

bool NoLineBreak =
        State.Stack.back().NoLineBreakInOperand ||
        (Current.is(TT_TemplateOpener) &&
         State.Stack.back().ContainsUnwrappedBuilder));
   State.Stack.push_back(
       ParenState(NewIndent, LastSpace, AvoidBinPacking, NoLineBreak));
   State.Stack.back().NestedBlockIndent = NestedBlockIndent;
   State.Stack.back().BreakBeforeParameter = BreakBeforeParameter;
   State.Stack.back().HasMultipleNestedBlocks = Current.BlockParameterCount > 1;
+  State.Stack.back().IsInsideObjCArrayLiteral =
+      Current.is(TT_ArrayInitializerLSquare) && Current.Previous &&
+      Current.Previous->is(tok::at);
 }
 
 void ContinuationIndenter::moveStatePastScopeCloser(LineState &State) {
   const FormatToken &Current = *State.NextToken;
   if (!Current.closesScope())
     return;
 
   // If we encounter a closing ), ], } or >, we can remove a level from our
if (Current.isStringLiteral()) {
     // This way we avoid breaking code with line directives and unknown
     // preprocessor directives that contain long string literals.
     if (State.Line->Type == LT_PreprocessorDirective)
       return nullptr;
     // Exempts unterminated string literals from line breaking. The user will
     // likely want to terminate the string before any line breaking is done.
     if (Current.IsUnterminatedLiteral)
       return nullptr;
+    // Don't break string literals inside Objective-C array literals (doing so
+    // raises the warning -Wobjc-string-concatenation).
+    if (State.Stack.back().IsInsideObjCArrayLiteral) {
+      return nullptr;
+    }
 
     StringRef Text = Current.TokenText;
     StringRef Prefix;
     StringRef Postfix;
     // FIXME: Handle whitespace between '_T', '(', '"..."', and ')'.
     // FIXME: Store Prefix and Suffix (or PrefixLength and SuffixLength to
     // reduce the overhead) for each FormatToken, which is a string, so that we
     // don't run multiple checks here on the hot path.

unittests/Format/FormatTestObjC.cpp

TEST_F(FormatTestObjC, ObjCArrayLiterals) {
   verifyFormat(
       "- (NSAttributedString *)attributedStringForSegment:(NSUInteger)segment\n"
       "                                             index:(NSUInteger)index\n"
       "                                nonDigitAttributes:\n"
       "                                    (NSDictionary *)noDigitAttributes;");
   verifyFormat("[someFunction someLooooooooooooongParameter:@[\n"
                "  NSBundle.mainBundle.infoDictionary[@\"a\"]\n"
                "]];");
+  Style.ColumnLimit = 20;
+  // We can't break string literals inside NSArray literals
+  // (that raises -Wobjc-string-concatenation).
+  verifyFormat("NSArray *foo = @[\n"
+               "  @\"aaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
+               "];\n");
 }
 } // end namespace
 } // end namespace format
 } // end namespace clang