[clang-format] Handle C# property accessors when parsing lines

[jbcoe]

Summary

Improve C# { get; set; } = default; formatting by handling it in the UnwrappedLineParser rather than trying to merge lines later.

Remove old logic to merge lines.

Update tests as formatting output has changed (as intended).

[jbcoe]

Format patch

[krasimir]

Thank you!

[MyDeveloperDay]

clang/unittests/Format/FormatTestCSharp.cpp

249

is this just a personal choice? or based on some rule that it shouldn't break?

I don't like us changing tests unless we understand otherwise we just keep flip-flopping the style?

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1344

previously set and get would break based on the setting of AfterFunction correct? now I assume it doesn't?

[jbcoe]

clang/lib/Format/UnwrappedLineParser.cpp

1344

That's right. There's a bunch more work needed here and current/previous behaviour is undertested and incorrect.

I'm focusing on this for the next few days so should get everything working well and configurable as we'd like.

MS examples are not very consistent so choice seems like the way forward: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/classes#properties

clang/unittests/Format/FormatTestCSharp.cpp

249

Agreed. This was oversight and merits discussion. I'll make this configurable in a follow-up patch.

Thanks for taking the time to review/comment.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1344

Sounds good

clang/unittests/Format/FormatTestCSharp.cpp

249

I the original C# work I mentioned we'd want to add specific styling for things like this, I feel this is going the the right direction thank you.

clang/lib/Format/UnwrappedLineFormatter.cpp

if (Style.CompactNamespaces) {
 
           // Indent like the outer-most namespace
           IndentTracker.nextLine(*I[i + 1]);
         }
         return i;
       }
     }
 
-    // Try to merge a CSharp property declaration like `{ get; private set }`.
-    if (Style.isCSharp()) {
-      unsigned CSPA = tryMergeCSharpPropertyAccessor(I, E, Limit);
-      if (CSPA > 0)
-        return CSPA;
-    }
-
     // Try to merge a function block with left brace unwrapped
     if (TheLine->Last->is(TT_FunctionLBrace) &&
         TheLine->First != TheLine->Last) {
       return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
     }
     // Try to merge a control statement block with left brace unwrapped
     if (TheLine->Last->is(tok::l_brace) && TheLine->First != TheLine->Last &&
         TheLine->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
tryFitMultipleLinesInOne(LevelIndentTracker &IndentTracker,
     }
     if (TheLine->InPPDirective &&
         (TheLine->First->HasUnescapedNewline || TheLine->First->IsFirst)) {
       return tryMergeSimplePPDirective(I, E, Limit);
     }
     return 0;
   }
 
-  // true for lines of the form [access-modifier] {get,set} [;]
-  bool isMergeablePropertyAccessor(const AnnotatedLine *Line) {
-    auto *Tok = Line->First;
-    if (!Tok)
-      return false;
-
-    if (Tok->isOneOf(tok::kw_public, tok::kw_protected, tok::kw_private,
-                     Keywords.kw_internal))
-      Tok = Tok->Next;
-
-    if (!Tok || (Tok->TokenText != "get" && Tok->TokenText != "set"))
-      return false;
-
-    if (!Tok->Next || Tok->Next->is(tok::semi))
-      return true;
-
-    return false;
-  }
-
-  unsigned tryMergeCSharpPropertyAccessor(
-      SmallVectorImpl<AnnotatedLine *>::const_iterator I,
-      SmallVectorImpl<AnnotatedLine *>::const_iterator E, unsigned /*Limit*/) {
-
-    auto CurrentLine = I;
-    // Does line start with `{`
-    if (!(*CurrentLine)->Last || (*CurrentLine)->Last->isNot(TT_FunctionLBrace))
-      return 0;
-    ++CurrentLine;
-
-    unsigned MergedLines = 0;
-    bool HasGetOrSet = false;
-    while (CurrentLine != E) {
-      bool LineIsGetOrSet = isMergeablePropertyAccessor(*CurrentLine);
-      HasGetOrSet = HasGetOrSet || LineIsGetOrSet;
-      if (LineIsGetOrSet) {
-        ++CurrentLine;
-        ++MergedLines;
-        continue;
-      }
-      auto *Tok = (*CurrentLine)->First;
-      if (Tok && Tok->is(tok::r_brace)) {
-        ++CurrentLine;
-        ++MergedLines;
-        // See if the next line is a default value so that we can merge `{ get;
-        // set } = 0`
-        if (CurrentLine != E && (*CurrentLine)->First &&
-            (*CurrentLine)->First->is(tok::equal)) {
-          ++MergedLines;
-        }
-        break;
-      }
-      // Not a '}' or a get/set line so do not merege lines.
-      return 0;
-    }
-
-    return HasGetOrSet ? MergedLines : 0;
-  }
-
   unsigned
   tryMergeSimplePPDirective(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
                             SmallVectorImpl<AnnotatedLine *>::const_iterator E,
                             unsigned Limit) {
     if (Limit == 0)
       return 0;
     if (I + 2 != E && I[2]->InPPDirective && !I[2]->First->HasUnescapedNewline)
       return 0;

clang/lib/Format/UnwrappedLineParser.h

private:
   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 tryToParseLambdaIntroducer();
+  bool tryToParsePropertyAccessor();
   void tryToParseJSFunction();
   void addUnwrappedLine();
   bool eof() const;
   // LevelDifference is the difference of levels after and before the current
   // token. For example:
   // - if the token is '{' and opens a block, LevelDifference is 1.
   // - if the token is '}' and closes a block, LevelDifference is -1.
   void nextToken(int LevelDifference = 0);

clang/lib/Format/UnwrappedLineParser.cpp

case tok::caret:
           FormatTok->isSimpleTypeSpecifier())
         nextToken();
       if (FormatTok->is(tok::l_paren))
         parseParens();
       if (FormatTok->is(tok::l_brace))
         parseChildBlock();
       break;
     case tok::l_brace:
-      if (!tryToParseBracedList()) {
+      if (!tryToParsePropertyAccessor() && !tryToParseBracedList()) {
         // A block outside of parentheses must be the last part of a
         // structural element.
         // FIXME: Figure out cases where this is not true, and add projections
         // for them (the one we know is missing are lambdas).
         if (Style.BraceWrapping.AfterFunction)
           addUnwrappedLine();
         FormatTok->Type = TT_FunctionLBrace;
         parseBlock(/*MustBeDeclaration=*/false);
         addUnwrappedLine();
         return;
       }
       // Otherwise this was a braced init list, and the structural
       // element continues.
       break;
     case tok::kw_try:
case tok::kw_new:
       break;
     default:
       nextToken();
       break;
     }
   } while (!eof());
 }
 
+bool UnwrappedLineParser::tryToParsePropertyAccessor() {
+  assert(FormatTok->is(tok::l_brace));
+  if (!Style.isCSharp())
+    return false;
+  // See if it's a property accessor.
+  if (FormatTok->Previous->isNot(tok::identifier))
+    return false;
+
+  // Try to parse the property accessor braces and contents:
+  // `{ get; set; } = new MyType(defaultValue);`
+  //  ^^^^^^^^^^^^^
+  //
+  // Record the current tokenPosition so that we can advance and
+  // reset the current token. `Next` is not set yet so we need
+  // another way to advance along the token stream.
+  unsigned int StoredPosition = Tokens->getPosition();
+  FormatToken *Tok = Tokens->getNextToken();
+
+  bool HasGetOrSet = false;
+  while (!eof()) {
+    if (Tok->isOneOf(tok::semi, tok::kw_public, tok::kw_private,
+                     tok::kw_protected, Keywords.kw_internal, Keywords.kw_get,
+                     Keywords.kw_set)) {
+      if (Tok->isOneOf(Keywords.kw_get, Keywords.kw_set))
+        HasGetOrSet = true;
+      Tok = Tokens->getNextToken();
+      continue;
+    }
+    if (Tok->is(tok::r_brace))
+      break;
+    Tokens->setPosition(StoredPosition);
+    return false;
+  }
+
+  if (!HasGetOrSet) {
+    Tokens->setPosition(StoredPosition);
+    return false;
+  }
+
+  Tokens->setPosition(StoredPosition);
+  while (FormatTok->isNot(tok::r_brace)) {
+    nextToken();
+  }
+
+  // Try to parse (optional) assignment to default value:
+  // `{ get; set; } = new MyType(defaultValue);`
+  //                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  // There may be some very complicated expressions inside default value
+  // assignment, the simple parse block below will not handle them.
+  // The parse block below would need extending to handle opening parens etc.
+  StoredPosition = Tokens->getPosition();
+  Tok = Tokens->getNextToken();
+  bool NextTokenIsEqual = Tok->is(tok::equal);
+  Tokens->setPosition(StoredPosition);
+
+  if (NextTokenIsEqual) {
+    do {
+      nextToken();
+      if (FormatTok->is(tok::semi))
+        break;
+    } while (!eof());
+  }
+
+  // Add an unwrapped line for the whole property accessor.
+  nextToken();
+  addUnwrappedLine();
+  return true;
+}
+
 bool UnwrappedLineParser::tryToParseLambda() {
   if (!Style.isCpp()) {
     nextToken();
     return false;
   }
   assert(FormatTok->is(tok::l_square));
   FormatToken &LSquare = *FormatTok;
   if (!tryToParseLambdaIntroducer())

clang/unittests/Format/FormatTestCSharp.cpp

TEST_F(FormatTestCSharp, Attributes) {
 
   verifyFormat("[Verb(\"start\", HelpText = \"Starts the server listening on "
                "provided port\")]\n"
                "class Test\n"
                "{\n"
                "}");
 
   verifyFormat("[TestMethod]\n"
-               "public string Host\n"
-               "{ set; get; }");
+               "public string Host { set; get; }");
 
   verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
                "listening on provided host\")]\n"
-               "public string Host\n"
-               "{ set; get; }");
+               "public string Host { set; get; }");
 
   verifyFormat(
       "[DllImport(\"Hello\", EntryPoint = \"hello_world\")]\n"
       "// The const char* returned by hello_world must not be deleted.\n"
       "private static extern IntPtr HelloFromCpp();)");
 
   // Class attributes go on their own line and do not affect layout of
   // interfaces. Line wrapping decisions previously caused each interface to be