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

MyDeveloperDay via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri May 28 05:03:08 PDT 2021


MyDeveloperDay updated this revision to Diff 348495.
MyDeveloperDay added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103307/new/

https://reviews.llvm.org/D103307

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===================================================================
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -247,6 +247,37 @@
   verifyFormat("[TestMethod]\n"
                "public string Host { set; get; }");
 
+  // Adjacent properties should not cause 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; }");
+
+  // Multiple attributes should always be split (not just the first ones)
+  verifyFormat("[XmlIgnore]\n"
+               "[JsonProperty(\"foo\")]\n"
+               "public string Foo { set; get; }");
+
+  verifyFormat("[XmlIgnore]\n"
+               "[JsonProperty(\"foo\")]\n"
+               "public string Foo { set; get; }\n"
+               "[XmlIgnore]\n"
+               "[JsonProperty(\"bar\")]\n"
+               "public string Bar { set; get; }");
+
+  verifyFormat("[XmlIgnore]\n"
+               "[ScriptIgnore]\n"
+               "[JsonProperty(\"foo\")]\n"
+               "public string Foo { set; get; }\n"
+               "[XmlIgnore]\n"
+               "[ScriptIgnore]\n"
+               "[JsonProperty(\"bar\")]\n"
+               "public string Bar { set; get; }");
+
   verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
                "listening on provided host\")]\n"
                "public string Host { set; get; }");
@@ -271,6 +302,34 @@
                "{\n"
                "}");
 
+  verifyFormat("void MethodA([In, Out] ref double x)\n"
+               "{\n"
+               "}");
+
+  verifyFormat("void MethodA([In, Out] double[] x)\n"
+               "{\n"
+               "}");
+
+  verifyFormat("void MethodA([In] double[] x)\n"
+               "{\n"
+               "}");
+
+  verifyFormat("void MethodA(int[] x)\n"
+               "{\n"
+               "}");
+  verifyFormat("void MethodA(int[][] x)\n"
+               "{\n"
+               "}");
+  verifyFormat("void MethodA([] x)\n"
+               "{\n"
+               "}");
+
+  verifyFormat("public void Log([CallerLineNumber] int line = -1, "
+               "[CallerFilePath] string path = null,\n"
+               "                [CallerMemberName] string name = null)\n"
+               "{\n"
+               "}");
+
   // [] in an attribute do not cause premature line wrapping or indenting.
   verifyFormat(R"(//
 public class A
Index: clang/lib/Format/TokenAnnotator.cpp
===================================================================
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3520,6 +3520,17 @@
       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;
+    // Break between ] and [ but only when its really 2 attributes.
+    if (Left.is(TT_AttributeSquare) && Right.is(TT_AttributeSquare) &&
+        Left.is(tok::r_square) && Right.is(tok::l_square))
+      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 &&


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D103307.348495.patch
Type: text/x-patch
Size: 3742 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210528/86bd97ce/attachment-0001.bin>


More information about the cfe-commits mailing list