[clang-format] [PR43338] C# clang format has space issues betweern C# only keywords

[MyDeveloperDay]

Summary

When formatting C# there can be issues with a lack of spaces between using ( , foreach ( and generic types

The C# code

public class Foo
{
    Dictionary<string,string> foo;
}

will be formatted as

public class Foo
{
    Dictionary<string, string>foo;
                           ^^^^^   missing a space
}

This revision also reverts some of D66662: [clang-format] [PR43100] clang-format C# support does not add a space between "using" and paren in order to make this cleaner and resolve an issues seen by @owenpan that the formatting didn't add a space when not in a code block

This also transforms C# foreach commands to be seen as tok::kw_for commands (to ensure foreach gets the same Brace Wrapping behavior as for without littering the code with if(Style.isCSharp())

[mitchell-stellar]

LGTM

clang/lib/Format/FormatTokenLexer.h

private:
 
   bool tryMergeLessLess();
   bool tryMergeNSStringLiteral();
   bool tryMergeJSPrivateIdentifier();
   bool tryMergeCSharpVerbatimStringLiteral();
   bool tryMergeCSharpKeywordVariables();
   bool tryMergeCSharpNullConditionals();
   bool tryMergeCSharpDoubleQuestion();
+  bool tryTransformCSharpForEach();
 
   bool tryMergeTokens(ArrayRef<tok::TokenKind> Kinds, TokenType NewType);
 
   // Returns \c true if \p Tok can only be followed by an operand in JavaScript.
   bool precedesOperand(FormatToken *Tok);
 
   bool canPrecedeRegexLiteral(FormatToken *Prev);
 

clang/lib/Format/FormatTokenLexer.cpp

if (Style.isCSharp()) {
     if (tryMergeCSharpKeywordVariables())
       return;
     if (tryMergeCSharpVerbatimStringLiteral())
       return;
     if (tryMergeCSharpDoubleQuestion())
       return;
     if (tryMergeCSharpNullConditionals())
       return;
+    if (tryTransformCSharpForEach())
+      return;
     static const tok::TokenKind JSRightArrow[] = {tok::equal, tok::greater};
     if (tryMergeTokens(JSRightArrow, TT_JsFatArrow))
       return;
   }
 
   if (tryMergeNSStringLiteral())
     return;
 
bool FormatTokenLexer::tryMergeCSharpNullConditionals() {
   Identifier->TokenText =
       StringRef(Identifier->TokenText.begin(),
                 Question->TokenText.end() - Identifier->TokenText.begin());
   Identifier->ColumnWidth += Question->ColumnWidth;
   Tokens.erase(Tokens.end() - 1);
   return true;
 }
 
+// In C# transform identifier foreach into kw_foreach
+bool FormatTokenLexer::tryTransformCSharpForEach() {
+  if (Tokens.size() < 1)
+    return false;
+  auto &Identifier = *(Tokens.end() - 1);
+  if (!Identifier->is(tok::identifier))
+    return false;
+  if (Identifier->TokenText != "foreach")
+    return false;
+
+  Identifier->Type = TT_ForEachMacro;
+  Identifier->Tok.setKind(tok::kw_for);
+  return true;
+}
+
 bool FormatTokenLexer::tryMergeLessLess() {
   // Merge X,less,less,Y into X,lessless,Y unless X or Y is less.
   if (Tokens.size() < 3)
     return false;
 
   bool FourthTokenIsLess = false;
   if (Tokens.size() > 3)
     FourthTokenIsLess = (Tokens.end() - 4)[0]->is(tok::less);

clang/lib/Format/TokenAnnotator.cpp

if ((Left.is(tok::l_brace) && Left.BlockKind != BK_Block) ||
       (Right.is(tok::r_brace) && Right.MatchingParen &&
        Right.MatchingParen->BlockKind != BK_Block))
     return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
     // No whitespace in x(/*foo=*/1), except for JavaScript.
     return Style.Language == FormatStyle::LK_JavaScript ||
            !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
-    // using (FileStream fs...
-    if (Style.isCSharp() && Left.is(tok::kw_using) &&
-        Style.SpaceBeforeParens != FormatStyle::SBPO_Never)
-      return true;
     if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
         (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
       return true;
     return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
            (Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
             (Left.isOneOf(tok::pp_elif, tok::kw_for, tok::kw_while,
                           tok::kw_switch, tok::kw_case, TT_ForEachMacro,
                           TT_ObjCForIn) ||
if (Left.MatchingParen &&
       return !Style.Cpp11BracedListStyle;
     // A percent is probably part of a formatting specification, such as %lld.
     if (Left.is(tok::percent))
       return false;
     // Preserve the existence of a space before a percent for cases like 0x%04x
     // and "%d %d"
     if (Left.is(tok::numeric_constant) && Right.is(tok::percent))
       return Right.WhitespaceRange.getEnd() != Right.WhitespaceRange.getBegin();
-  } else if (Style.Language == FormatStyle::LK_JavaScript || Style.isCSharp()) {
+  } else if (Style.isCSharp()) {
+    // space between type and variable e.g. Dictionary<string,string> foo;
+    if (Left.is(TT_TemplateCloser) && Right.is(TT_StartOfName))
+      return true;
+    // space between keywords and paren e.g. "using ("
+    if (Right.is(tok::l_paren))
+      if (Left.is(tok::kw_using))
+        return spaceRequiredBeforeParens(Left);
+  } else if (Style.Language == FormatStyle::LK_JavaScript) {
     if (Left.is(TT_JsFatArrow))
       return true;
     // for await ( ...
     if (Right.is(tok::l_paren) && Left.is(Keywords.kw_await) && Left.Previous &&
         Left.Previous->is(tok::kw_for))
       return true;
     if (Left.is(Keywords.kw_async) && Right.is(tok::l_paren) &&
         Right.MatchingParen) {

clang/unittests/Format/FormatTestCSharp.cpp

 TEST_F(FormatTestCSharp, CSharpClass) {
   verifyFormat("public class SomeClass {\n"
                "  void f() {}\n"
                "  int g() { return 0; }\n"
                "  void h() {\n"
                "    while (true) f();\n"
                "    for (;;) f();\n"
                "    if (true) f();\n"
+               "    foreach (x in y) f();\n"
                "  }\n"
                "}");
 }
 
 TEST_F(FormatTestCSharp, AccessModifiers) {
   verifyFormat("public String toString() {}");
   verifyFormat("private String toString() {}");
   verifyFormat("protected String toString() {}");

 }
 
 TEST_F(FormatTestCSharp, CSharpFatArrows) {
   verifyFormat("Task serverTask = Task.Run(async() => {");
   verifyFormat("public override string ToString() => \"{Name}\\{Age}\";");
 }
 
 TEST_F(FormatTestCSharp, CSharpNullConditional) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
+
   verifyFormat(
       "public Person(string firstName, string lastName, int? age=null)");
 
-  verifyFormat("switch(args?.Length)");
+  verifyFormat("foo () {\n"
+               "  switch (args?.Length) {}\n"
+               "}",
+               Style);
+
+  verifyFormat("switch (args?.Length) {}", Style);
 
   verifyFormat("public static void Main(string[] args) { string dirPath "
                "= args?[0]; }");
+
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Never;
+
+  verifyFormat("switch(args?.Length) {}", Style);
 }
 
 TEST_F(FormatTestCSharp, Attributes) {
   verifyFormat("[STAThread]\n"
                "static void\n"
                "Main(string[] args) {}");
 
   verifyFormat("[TestMethod]\n"
TEST_F(FormatTestCSharp, Attributes) {
   verifyFormat("[TestMethod(\"start\", HelpText = \"Starts the server "
                "listening on provided host\")]\n"
                "public string Host {\n  set;\n  get;\n}");
 }
 
 TEST_F(FormatTestCSharp, CSharpUsing) {
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
   Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
-  verifyFormat("public void foo() {\n"
+  verifyFormat("public void foo () {\n"
                "  using (StreamWriter sw = new StreamWriter (filenameA)) {}\n"
                "}",
                Style);
 
+  verifyFormat("using (StreamWriter sw = new StreamWriter (filenameB)) {}",
+               Style);
+
   Style.SpaceBeforeParens = FormatStyle::SBPO_Never;
   verifyFormat("public void foo() {\n"
                "  using(StreamWriter sw = new StreamWriter(filenameB)) {}\n"
                "}",
                Style);
+
+  verifyFormat("using(StreamWriter sw = new StreamWriter(filenameB)) {}",
+               Style);
 }
 
 TEST_F(FormatTestCSharp, CSharpRegions) {
   verifyFormat("#region aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaa "
                "aaaaaaaaaaaaaaa long region");
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
   verifyFormat("public enum var { none, @string, bool, @enum }");
 }
 
 TEST_F(FormatTestCSharp, CSharpNullCoalescing) {
   verifyFormat("var test = ABC ?? DEF");
   verifyFormat("string myname = name ?? \"ABC\";");
   verifyFormat("return _name ?? \"DEF\";");
 }
 
+TEST_F(FormatTestCSharp, CSharpSpaceBefore) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
+
+  verifyFormat("List<string> list;", Style);
+  verifyFormat("Dictionary<string, string> dict;", Style);
+
+  verifyFormat("for (int i = 0; i < size (); i++) {\n"
+               "}",
+               Style);
+  verifyFormat("foreach (var x in y) {\n"
+               "}",
+               Style);
+  verifyFormat("switch (x) {}", Style);
+  verifyFormat("do {\n"
+               "} while (x);",
+               Style);
+
+  Style.SpaceBeforeParens = FormatStyle::SBPO_Never;
+
+  verifyFormat("List<string> list;", Style);
+  verifyFormat("Dictionary<string, string> dict;", Style);
+
+  verifyFormat("for(int i = 0; i < size(); i++) {\n"
+               "}",
+               Style);
+  verifyFormat("foreach(var x in y) {\n"
+               "}",
+               Style);
+  verifyFormat("switch(x) {}", Style);
+  verifyFormat("do {\n"
+               "} while(x);",
+               Style);
+}
+
 } // namespace format
 } // end namespace clang