[clang-format] Fix C# nullable-related errors

[exv]

Summary

This fixes two errors:

Previously, clang-format was splitting up type identifiers from the
nullable ?. This changes this behavior so that the type name sticks with
the operator.

Additionally, nullable operators attached to return types in interface
functions were not parsed correctly. Digging deeper, it looks like
interface bodies were being parsed differently than classes and structs,
causing MustBeDeclaration to be incorrect for interface members. They
now share the same logic.

One other change is reintroducing the CSharpNullable type independent of
JsTypeOptionalQuestion. Despite having a similar semantic purpose, their
actual syntax differs quite a bit.

[curdeius]

You can add a parent revision so that this patch will be applied upon its parent.

[curdeius]

LGTM pending CI.

clang/lib/Format/UnwrappedLineParser.cpp

2531

Nit when you're here.

[exv]

• Incorporate curdeius's feedback

[MyDeveloperDay]

LGTM

[exv]

Rebase again in an attempt to fix CI

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(TrailingReturnArrow)                                                    \
   TYPE(TrailingUnaryOperator)                                                  \
   TYPE(TypeDeclarationParen)                                                   \
   TYPE(TypenameMacro)                                                          \
   TYPE(UnaryOperator)                                                          \
   TYPE(UntouchableMacroFunc)                                                   \
   TYPE(CSharpStringLiteral)                                                    \
   TYPE(CSharpNamedArgumentColon)                                               \
+  TYPE(CSharpNullable)                                                         \
   TYPE(CSharpNullConditionalLSquare)                                           \
   TYPE(CSharpGenericTypeConstraint)                                            \
   TYPE(CSharpGenericTypeConstraintColon)                                       \
   TYPE(CSharpGenericTypeConstraintComma)                                       \
   TYPE(Unknown)
 
 /// Determines the semantic type of a syntactic token, e.g. whether "<" is a
 /// template opener or binary operator.

clang/lib/Format/TokenAnnotator.cpp

case tok::question:
       if (Style.isCSharp()) {
         // `Type?)`, `Type?>`, `Type? name;` and `Type? name =` can only be
         // nullable types.
         // Line.MustBeDeclaration will be true for `Type? name;`.
         if ((!Contexts.back().IsExpression && Line.MustBeDeclaration) ||
             (Tok->Next && Tok->Next->isOneOf(tok::r_paren, tok::greater)) ||
             (Tok->Next && Tok->Next->is(tok::identifier) && Tok->Next->Next &&
              Tok->Next->Next->is(tok::equal))) {
-          Tok->setType(TT_JsTypeOptionalQuestion);
+          Tok->setType(TT_CSharpNullable);
           break;
         }
       }
       parseConditional();
       break;
     case tok::kw_template:
       parseTemplateDeclaration();
       break;
if (Style.isCpp()) {
     if (Left.is(TT_TemplateCloser) && Right.is(TT_StartOfName))
       return true;
 
     // spaces inside square brackets.
     if (Left.is(tok::l_square) || Right.is(tok::r_square))
       return Style.SpacesInSquareBrackets;
 
     // No space before ? in nullable types.
-    if (Right.is(TT_JsTypeOptionalQuestion))
+    if (Right.is(TT_CSharpNullable))
       return false;
 
     // No space before null forgiving '!'.
     if (Right.is(TT_JsNonNullAssertion))
       return false;
 
     // No space between consecutive commas '[,,]'.
     if (Left.is(tok::comma) && Right.is(tok::comma))
bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   // Language-specific stuff.
   if (Style.isCSharp()) {
     if (Left.isOneOf(TT_CSharpNamedArgumentColon, TT_AttributeColon) ||
         Right.isOneOf(TT_CSharpNamedArgumentColon, TT_AttributeColon))
       return false;
     // Only break after commas for generic type constraints.
     if (Line.First->is(TT_CSharpGenericTypeConstraint))
       return Left.is(TT_CSharpGenericTypeConstraintComma);
+    // Keep nullable operators attached to their identifiers.
+    if (Right.is(TT_CSharpNullable)) {
+      return false;
+    }
   } else if (Style.Language == FormatStyle::LK_Java) {
     if (Left.isOneOf(Keywords.kw_throws, Keywords.kw_extends,
                      Keywords.kw_implements))
       return false;
     if (Right.isOneOf(Keywords.kw_throws, Keywords.kw_extends,
                       Keywords.kw_implements))
       return true;
   } else if (Style.Language == FormatStyle::LK_JavaScript) {

clang/lib/Format/UnwrappedLineParser.h

private:
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
   void parseNew();
   void parseAccessSpecifier();
   bool parseEnum();
+  bool parseStructLike();
   void parseConcept();
   void parseRequires();
   void parseRequiresExpression(unsigned int OriginalLevel);
   void parseConstraintExpression(unsigned int OriginalLevel);
   void parseJavaEnumBody();
   // Parses a record (aka class) as a top level element. If ParseAsExpr is true,
   // parses the record as a child block, i.e. if the class declaration is an
   // expression.

clang/lib/Format/UnwrappedLineParser.cpp

case tok::kw_typedef:
                              Keywords.kw_CF_ENUM, Keywords.kw_CF_OPTIONS,
                              Keywords.kw_CF_CLOSED_ENUM,
                              Keywords.kw_NS_CLOSED_ENUM))
         parseEnum();
       break;
     case tok::kw_struct:
     case tok::kw_union:
     case tok::kw_class:
-      // parseRecord falls through and does not yet add an unwrapped line as a
-      // record declaration or definition can start a structural element.
-      parseRecord();
-      // This does not apply for Java, JavaScript and C#.
-      if (Style.Language == FormatStyle::LK_Java ||
-          Style.Language == FormatStyle::LK_JavaScript || Style.isCSharp()) {
-        if (FormatTok->is(tok::semi))
-          nextToken();
-        addUnwrappedLine();
+      if (parseStructLike()) {
         return;
       }
       break;
     case tok::period:
       nextToken();
       // In Java, classes have an implicit static member "class".
       if (Style.Language == FormatStyle::LK_Java && FormatTok &&
           FormatTok->is(tok::kw_class))
case tok::identifier: {
             break;
           }
         }
         parseRecord();
         addUnwrappedLine();
         return;
       }
 
+      if (FormatTok->is(Keywords.kw_interface)) {
+        if (parseStructLike()) {
+          return;
+        }
+        break;
+      }
+
       if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
         parseStatementMacro();
         return;
       }
 
       // See if the following token should start a new unwrapped line.
       StringRef Text = FormatTok->TokenText;
       nextToken();
bool UnwrappedLineParser::parseEnum() {
   }
   return true;
 
   // There is no addUnwrappedLine() here so that we fall through to parsing a
   // structural element afterwards. Thus, in "enum A {} n, m;",
   // "} n, m;" will end up in one unwrapped line.
 }
 
+bool UnwrappedLineParser::parseStructLike() {
+  // parseRecord falls through and does not yet add an unwrapped line as a
+  // record declaration or definition can start a structural element.
+  parseRecord();
+  // This does not apply to Java, JavaScript and C#.
+  if (Style.Language == FormatStyle::LK_Java ||
+      Style.Language == FormatStyle::LK_JavaScript || Style.isCSharp()) {
+    if (FormatTok->is(tok::semi))
+      nextToken();
+    addUnwrappedLine();
+    return true;
+  }
+  return false;
+}
+
 namespace {
 // A class used to set and restore the Token position when peeking
 // ahead in the token source.
 class ScopedTokenPosition {
   unsigned StoredPosition;
   FormatTokenSource *Tokens;
 
 public:

clang/unittests/Format/FormatTestCSharp.cpp

public class A {
   verifyFormat(R"(public float? Value;)", Style); // no space before `?`.
 
   verifyFormat(R"(int?[] arr = new int?[10];)",
                Style); // An array of a nullable type.
 
   verifyFormat(R"(var x = (int?)y;)", Style); // Cast to a nullable type.
 
   verifyFormat(R"(var x = new MyContainer<int?>();)", Style); // Generics.
+
+  verifyFormat(R"(//
+public interface I {
+  int? Function();
+})",
+               Style); // Interface methods.
+
+  Style.ColumnLimit = 10;
+  verifyFormat(R"(//
+public VeryLongType? Function(
+    int arg1,
+    int arg2) {
+  //
+})",
+               Style); // ? sticks with identifier.
 }
 
 TEST_F(FormatTestCSharp, CSharpArraySubscripts) {
   FormatStyle Style = getGoogleStyle(FormatStyle::LK_CSharp);
 
   // Do not format array subscript operators as attributes.
   verifyFormat(R"(//
 if (someThings[index].Contains(myThing)) {