[clang-format][NFC] Handle wrapping after => in mustBreakBefore()

[owenpan]

Summary

Move the handling of brace wrapping after => from unwrapped line parser to token annotator and clean up the parser.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

1640–1641

Should we add the isCSharp check?

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1640–1641

No. It was not there before, and adding it would fail several JavaScript test cases.

[MyDeveloperDay]

nice.

[owenpan]

Additional cleanups:

• Restrict parsing => to JavaScript and C#.
• Rename tryToParseCSharpLambda to tryToParseChildBlock.
• Remove redundant caret case.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1640–1641

Should we add the isCSharp check?

Added checking for either JavaScript or C#.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2005

Is this case not covered by this? I presume not as you didn't remove any unit tests

https://github.com/llvm/llvm-project/commit/516e054c05fcf92b89fbac3e22048f2c00cf218f

void f() {
   int i = {[operation setCompletionBlock : ^{
     [self onOperationDone];
   }] };
 }

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

2005

I had checked the commit you mentioned above before I decided to remove this case. Apparently it's redundant to the caret case in parseStructuralElement().

[MyDeveloperDay]

LGTM, (the original code had a unit test so we should be good).

[peterstys]

clang/lib/Format/UnwrappedLineParser.cpp

1955–1958

It seems that this block could be removed altogether, because this is already done on lines 1964-165, and it's done regardless if it the language is C# or not.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1955–1958

Lines 1964-1965 only apply to JavaScript because of line 1958, so lines 1955-1957 cannot be removed.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1963–1964

Removing these two lines doesn't fail FormatTests.

[owenpan]

Removed more redundant code.

[MyDeveloperDay]

LGTM

[peterstys]

clang/lib/Format/UnwrappedLineParser.cpp

1955–1958

Oh yes, sorry I missed the Javascript block. I wonder if in that case wouldn't be more consistent to add Javascript check on the line 1955 to be consistent with how the same block of logic is handled on the lines 1641-1645.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

1955–1958

I don't think so because here we must parse the optional async function part first for JavaScript only.

[HazardyKnusperkeks]

Good that you renamed the method.

clang/lib/Format/TokenAnnotator.cpp

 
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
                                      const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
   if (Right.NewlinesBefore > 1 && Style.MaxEmptyLinesToKeep > 0)
     return true;
 
   if (Style.isCSharp()) {
+    if (Left.is(TT_FatArrow) && Right.is(tok::l_brace) &&
+        Style.BraceWrapping.AfterFunction)
+      return true;
     if (Right.is(TT_CSharpNamedArgumentColon) ||
         Left.is(TT_CSharpNamedArgumentColon))
       return false;
     if (Right.is(TT_CSharpGenericTypeConstraint))
       return true;
     if (Right.Next && Right.Next->is(TT_FatArrow) &&
         (Right.is(tok::numeric_constant) ||
          (Right.is(tok::identifier) && Right.TokenText == "_")))

clang/lib/Format/UnwrappedLineParser.h

private:
   void parseJavaScriptEs6ImportExport();
   void parseStatementMacro();
   void parseCSharpAttribute();
   // Parse a C# generic type constraint: `where T : IComparable<T>`.
   // See:
   // https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/where-generic-type-constraint
   void parseCSharpGenericTypeConstraint();
   bool tryToParseLambda();
-  bool tryToParseCSharpLambda();
+  bool tryToParseChildBlock();
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
   bool tryToParseSimpleAttribute();
 
   // Used by addUnwrappedLine to denote whether to keep or remove a level
   // when resetting the line state.
   enum class LineLevel { Remove, Keep };

clang/lib/Format/UnwrappedLineParser.cpp

case tok::identifier: {
         if (FollowedByNewline && (Text.size() >= 5 || FunctionLike) &&
             tokenCanStartNewLine(*FormatTok) && Text == Text.upper()) {
           addUnwrappedLine();
           return;
         }
       }
       break;
     }
     case tok::equal:
-      // Fat arrows (=>) have tok::TokenKind tok::equal but TokenType
-      // TT_FatArrow. They always start an expression or a child block if
-      // followed by a curly brace.
-      if (FormatTok->is(TT_FatArrow)) {
-        nextToken();
-        if (FormatTok->is(tok::l_brace)) {
-          // C# may break after => if the next character is a newline.
-          if (Style.isCSharp() && Style.BraceWrapping.AfterFunction == true) {
-            // calling `addUnwrappedLine()` here causes odd parsing errors.
-            FormatTok->MustBreakBefore = true;
-          }
-          parseChildBlock();
-        }
+      if ((Style.Language == FormatStyle::LK_JavaScript || Style.isCSharp()) &&
+          FormatTok->is(TT_FatArrow)) {
+        tryToParseChildBlock();
         break;
       }
 
       nextToken();
       if (FormatTok->Tok.is(tok::l_brace)) {
         // Block kind should probably be set to BK_BracedInit for any language.
         // C# needs this change to ensure that array initialisers and object
         // initialisers are indented the same way.
bool UnwrappedLineParser::tryToParsePropertyAccessor() {
   if (!HasGetOrSet) {
     Tokens->setPosition(StoredPosition);
     return false;
   }
 
   // Try to parse the property accessor:
   // https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties
   Tokens->setPosition(StoredPosition);
-  if (!IsTrivialPropertyAccessor && Style.BraceWrapping.AfterFunction == true)
+  if (!IsTrivialPropertyAccessor && Style.BraceWrapping.AfterFunction)
     addUnwrappedLine();
   nextToken();
   do {
     switch (FormatTok->Tok.getKind()) {
     case tok::r_brace:
       nextToken();
       if (FormatTok->is(tok::equal)) {
         while (!eof() && FormatTok->isNot(tok::semi))
bool UnwrappedLineParser::tryToParseBracedList() {
   assert(FormatTok->isNot(BK_Unknown));
   if (FormatTok->is(BK_Block))
     return false;
   nextToken();
   parseBracedList();
   return true;
 }
 
-bool UnwrappedLineParser::tryToParseCSharpLambda() {
-  // Fat arrows (=>) have tok::TokenKind tok::equal but TokenType
-  // TT_FatArrow. They always start an expression or a child block if
-  // followed by a curly brace.
+bool UnwrappedLineParser::tryToParseChildBlock() {
+  assert(Style.Language == FormatStyle::LK_JavaScript || Style.isCSharp());
+  assert(FormatTok->is(TT_FatArrow));
+  // Fat arrows (=>) have tok::TokenKind tok::equal but TokenType TT_FatArrow.
+  // They always start an expression or a child block if followed by a curly
+  // brace.
   nextToken();
   if (FormatTok->isNot(tok::l_brace))
     return false;
-  // C# may break after => if the next character is a newline.
-  if (Style.BraceWrapping.AfterFunction) {
-    // calling `addUnwrappedLine()` here causes odd parsing errors.
-    FormatTok->MustBreakBefore = true;
-  }
   parseChildBlock();
   return true;
 }
 
 bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
                                           bool IsEnum,
                                           tok::TokenKind ClosingBraceKind) {
   bool HasError = false;
 
   // FIXME: Once we have an expression parser in the UnwrappedLineParser,
   // replace this by using parseAssignmentExpression() inside.
   do {
-    if (Style.isCSharp() && FormatTok->is(TT_FatArrow))
-      if (tryToParseCSharpLambda())
-        continue;
+    if (Style.isCSharp() && FormatTok->is(TT_FatArrow) &&
+        tryToParseChildBlock())
     if (Style.Language == FormatStyle::LK_JavaScript) {
       if (FormatTok->is(Keywords.kw_function) ||
           FormatTok->startsSequence(Keywords.kw_async, Keywords.kw_function)) {
         tryToParseJSFunction();
         continue;
       }
-      if (FormatTok->is(TT_FatArrow)) {
-        nextToken();
-        // Fat arrows can be followed by simple expressions or by child blocks
-        // in curly braces.
-        if (FormatTok->is(tok::l_brace)) {
-          parseChildBlock();
-          continue;
-        }
-      }
       if (FormatTok->is(tok::l_brace)) {
         // Could be a method inside of a braced list `{a() { return 1; }}`.
         if (tryToParseBracedList())
           continue;
         parseChildBlock();
       }
     }
     if (FormatTok->Tok.getKind() == ClosingBraceKind) {
       if (IsEnum && !Style.AllowShortEnumsOnASingleLine)
         addUnwrappedLine();
       nextToken();
       return !HasError;
     }
     switch (FormatTok->Tok.getKind()) {
-    case tok::caret:
-      nextToken();
-      if (FormatTok->is(tok::l_brace)) {
-        parseChildBlock();
-      }
-      break;
     case tok::l_square:
       if (Style.isCSharp())
         parseSquare();
       else
         tryToParseLambda();
       break;
     case tok::l_paren:
       parseParens();
case tok::at:
       nextToken();
       if (FormatTok->Tok.is(tok::l_brace)) {
         nextToken();
         parseBracedList();
       }
       break;
     case tok::equal:
       if (Style.isCSharp() && FormatTok->is(TT_FatArrow))
-        tryToParseCSharpLambda();
+        tryToParseChildBlock();
       else
         nextToken();
       break;
     case tok::kw_class:
       if (Style.Language == FormatStyle::LK_JavaScript)
         parseRecord(/*ParseAsExpr=*/true);
       else
         nextToken();