[clang-format] Wrap lines for C# property accessors

[jbcoe]

Summary

Ensure that auto-implemented properties { get; private set } are wrapped on to one line for C# code.

[jbcoe]

Code and test to avoid wrapping accessors with expression body definitions.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineFormatter.cpp

443

I think we need to consider internal

456

and here

[jbcoe]

Handle C# access modifier internal .

Fix typo in test for expression-bodied get/set methods.

[MyDeveloperDay]

This LGTM, thank you for the patch

[jbcoe]

Simplify logic for merging property accessors

[krasimir]

This is pretty cool!

[MyDeveloperDay]

LGTM

clang/unittests/Format/FormatTestCSharp.cpp

246

Nit: I feel like this layout should really be an option, (maybe for the future).

When we originally did the C# work, we did say we might want in the future to add options that might be specific like laying out the accessors

I think VS has support for formatting them as either

public int Goo { set; get; }

and

public int Goo
{
     set; get;
}

and the following is equally valid in my eyes

public int Goo
{
     set; 
     get;
}

as well as what is being proposed here of

public int Goo
{   set; get; }

I'm not completely sure how much the other options are controlling this at present and how much is not in the control of your merging

and how does that change when there is actually code in the setter and getter?

[jbcoe]

clang/unittests/Format/FormatTestCSharp.cpp

246

Agreed that this should be configured by an option in a future revision.

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/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"
-               "{\n"
-               "    set;\n"
-               "    get;\n"
-               "}");
+               "{ set; get; }");
 
   verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
                "listening on provided host\")]\n"
                "public string Host\n"
-               "{\n"
-               "    set;\n"
-               "    get;\n"
-               "}");
+               "{ 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
PrintOrderDetails(orderNum: 31, productName: "Red Mug",
 
   // Ensure that trailing comments do not cause problems.
   verifyFormat(R"(//
 PrintOrderDetails(orderNum: 31, productName: "Red Mug",  // comment
                   sellerName: "Gift Shop");)",
                Style);
 }
 
+TEST_F(FormatTestCSharp, CSharpPropertyAccessors) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+
+  verifyFormat("int Value { get }", Style);
+  verifyFormat("int Value { get; }", Style);
+  verifyFormat("int Value { internal get; }", Style);
+  verifyFormat("int Value { get; } = 0", Style);
+  verifyFormat("int Value { set }", Style);
+  verifyFormat("int Value { set; }", Style);
+  verifyFormat("int Value { internal set; }", Style);
+  verifyFormat("int Value { set; } = 0", Style);
+  verifyFormat("int Value { get; set }", Style);
+  verifyFormat("int Value { set; get }", Style);
+  verifyFormat("int Value { get; private set; }", Style);
+  verifyFormat("int Value { get; set; }", Style);
+  verifyFormat("int Value { get; set; } = 0", Style);
+  verifyFormat("int Value { internal get; internal set; }", Style);
+
+  // Do not wrap expression body definitions.
+  verifyFormat(R"(//
+public string Name {
+  get => _name;
+  set => _name = value;
+})",
+               Style);
+}
+
 } // namespace format
 } // end namespace clang