[clang-format] successive C# attributes cause line breaking issues

[MyDeveloperDay]

Summary

D74265: [clang-format] Improve handling of C# attributes reduced the aggressiveness of line breaking following C# attributes, however this change removed any support for attributes on properties, causing significant ugliness to be introduced.

This revision goes some way to addressing that by re-introducing the more aggressive check to mustBreakBefore(), but constraining it to the most common cases where we use properties which should not impact the "caller info attributes" or the "[In , Out]" decorations that are normally put on pinvoke

It does not address my additional concerns of the original change regarding multiple C# attributes, as these are somewhat incorrectly handled by virtue of the fact its not recognising the second attribute as an attribute at all. But instead thinking its an array.

The purpose of this revision is to get back to where we were for the most common of cases as a stepping stone to resolving this. However D74265: [clang-format] Improve handling of C# attributes has broken a lot of C# code and this revision will go someway alone to addressing the majority.

[jbcoe]

Thanks for this!

[MyDeveloperDay]

In D103307#2786645, @jbcoe wrote:

Thanks for this!

I tried to handle the multiple attributes using the "parsing technique" you used, instead of the "mustBreaking technique" but every time I do this I end up putting a newline after every array!

[MyDeveloperDay]

Add additional unit test, and an additional check to ensure multiple attributes are broken, while @jbcoe this looks very similar to the original rule, the original rule actually had an || in the expression and not an &&, this I believe caused and issue where the adjacent [] [] caused an issue because we were breaking on an attribute and and array. By virtue of the fact the attributes are not correctly identified. Which concerns me a little,

void myFunction([In][Out] int a)

M=0 C=1 T=AttributeSquare S=0 F=0 B=0 BK=0 P=140 Name=l_square L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=79 Name=identifier L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x25b3990 Text='In'
M=0 C=0 T=AttributeSquare S=0 F=0 B=0 BK=0 P=63 Name=r_square L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
M=0 C=0 T=*ArraySubscriptLSquare* S=0 F=0 B=0 BK=0 P=240 Name=l_square L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=259 Name=identifier L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x25b39b0 Text='Out'
M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=243 Name=r_square L=25 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'

but this subsequent change seems to handle the cases where 2 attributes are decorating a property (for the most part)

M=0 C=0 T=AttributeSquare S=0 F=0 B=0 BK=0 P=43 Name=r_square L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
M=1 C=1 T=AttributeSquare S=1 F=0 B=0 BK=0 P=220 Name=l_square L=131 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['

My view is that [In][Out] should/could possibly be written as [In , Out] to avoid the error that at least your review seemed to want to be fixing.

And I think that using attributes on properties is perhaps more likely to be pervasive though out C# code than having them on pinvoke.

This update, adds a few more tests to try and uncover attitional places I would have expect this to break, (it doesn't) but I want to have more protection from the tests

From my tests I see one place where I see an issue, And seemingly for some reason this completely messes up even identifying that the attributes are themselves attributes. This is definitely a common problem. But the addition of either public/private/internal/protected immediately fixes that and I personally consider that a C# best practice anyway.

[JsonProperty("gender")] string gender { get; set; }


 M=0 C=0 T=ArraySubscriptLSquare S=1 F=0 B=0 BK=0 P=0 Name=l_square L=1 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='['
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=239 Name=identifier L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x25b37f0 Text='JsonProperty'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=223 Name=l_paren L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=259 Name=string_literal L=22 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='"gender"'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=243 Name=r_paren L=23 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=223 Name=r_square L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=']'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=identifier L=31 PPK=2 FakeLParens= FakeRParens=0 II=0x25b3418 Text='string'
 M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=38 PPK=2 FakeLParens= FakeRParens=0 II=0x25b3830 Text='gender'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=l_brace L=40 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='{'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=59 Name=identifier L=44 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x25b2df0 Text='get'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=semi L=45 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=40 Name=identifier L=49 PPK=2 FakeLParens= FakeRParens=0 II=0x25b2eb8 Text='set'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=semi L=50 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=';'
 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=40 Name=r_brace L=52 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='}'

If we are in agreement that this doesn't revert your intended changes, I'd like to commit this change as an interim step. I'd really like at least these aspect resolved before the 13 branch out.

[curdeius]

LGTM.

clang/lib/Format/TokenAnnotator.cpp

3529

Nit.

clang/lib/Format/TokenAnnotator.cpp

if (Right.NewlinesBefore > 1 && Style.MaxEmptyLinesToKeep > 0)
     return true;
 
   if (Style.isCSharp()) {
     if (Right.is(TT_CSharpNamedArgumentColon) ||
         Left.is(TT_CSharpNamedArgumentColon))
       return false;
     if (Right.is(TT_CSharpGenericTypeConstraint))
       return true;
+
+    // Break after C# [...] and before public/protected/private/internal.
+    if (Left.is(TT_AttributeSquare) && Left.is(tok::r_square) &&
+        (Right.isAccessSpecifier(/*ColonRequired=*/false) ||
+         Right.is(Keywords.kw_internal)))
+      return true;
+
   } else if (Style.Language == FormatStyle::LK_JavaScript) {
     // FIXME: This might apply to other languages and token kinds.
     if (Right.is(tok::string_literal) && Left.is(tok::plus) && Left.Previous &&
         Left.Previous->is(tok::string_literal))
       return true;
     if (Left.is(TT_DictLiteral) && Left.is(tok::l_brace) && Line.Level == 0 &&
         Left.Previous && Left.Previous->is(tok::equal) &&
         Line.First->isOneOf(tok::identifier, Keywords.kw_import, tok::kw_export,

clang/unittests/Format/FormatTestCSharp.cpp

verifyFormat("[Verb(\"start\", HelpText = \"Starts the server listening on "
                "provided port\")]\n"
                "class Test\n"
                "{\n"
                "}");
 
   verifyFormat("[TestMethod]\n"
                "public string Host { set; get; }");
 
+  // Adjacent attributes caused line wrapping issues
+  verifyFormat("[JsonProperty(\"foo\")]\n"
+               "public string Foo { set; get; }\n"
+               "[JsonProperty(\"bar\")]\n"
+               "public string Bar { set; get; }\n"
+               "[JsonProperty(\"bar\")]\n"
+               "protected string Bar { set; get; }\n"
+               "[JsonProperty(\"bar\")]\n"
+               "internal string Bar { set; get; }");
+
   verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
                "listening on provided host\")]\n"
                "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();)");