[clang-format] Remove special logic for parsing concept definitions.

[rymiel]

Summary

Previously, clang-format relied on a special method to parse concept
definitions, UnwrappedLineParser::parseConcept(), which deferred to
UnwrappedLineParser::parseConstraintExpression(). This is problematic,
because the C++ grammar treats concepts and requires clauses
differently, causing issues such as https://github.com/llvm/llvm-project/issues/55898 and https://github.com/llvm/llvm-project/issues/58130.

This patch removes parseConcept, letting the formatter parse concept
definitions as more like what they actually are, fancy bool definitions.

NOTE that because of this, some long concept definitions change in their
formatting, as can be seen in the changed tests. This is because of a
change in split penalties, caused by a change in MightBeFunctionDecl on
the concept definition line, which was previously true but with this
patch is now false.

One might argue that false is a more "correct" value for concept
definitions, but I'd be fine with setting it to true again to maintain
compatibility with previous versions.

Fixes https://github.com/llvm/llvm-project/issues/58130

Depends on D140330

[HazardyKnusperkeks]

I'm shocked and impressed, that it just works.

clang/lib/Format/TokenAnnotator.cpp

1683–1684

Does this change anything, besides the penalties and by that the wrapping?

While I can live with both, I think we should not format code differently, if it's not by a bugfix.

clang/unittests/Format/FormatTest.cpp

23945–23946

As I have learned in other changes, we have verifyNoCrash (or similar), this is what should be used. Didn't know about it back then.

[rymiel]

clang/lib/Format/TokenAnnotator.cpp

1683–1684

The only changes I could see:

• the change in Context.IsExpression, which causes the change in penalties;
• the fact that a new AnnotatedLine isn't created for the body of a concept (i.e. the stuff after the equals sign), where as it previously was.
• some invalid syntax is formatted differently

It's very likely I don't have a "sample size" large enough.

I am also okay with doing both; I made this issue to gauge what is the preferred direction by the reviewers.

[rymiel]

Use verifyNoCrash

[rymiel]

clang/unittests/Format/FormatTest.cpp

23945–23946

Thank you, now I know as well

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

1683–1684

Let's hear from @owenpan @MyDeveloperDay

[rymiel]

Adapt description of parseConstraintExpression

[owenpan]

clang/unittests/Format/FormatTest.cpp

23593–23594

What would happen with parentheses added?

concept DelayedCheck = (static_cast<bool>(0) || requires(T t) { t.bar(); }) && sizeof(T) <= 8;
concept DelayedCheck = static_cast<bool>(0) || (requires(T t) { t.bar(); } && sizeof(T) <= 8);

23946–23947

Just want to make sure that the line break didn't matter.

[rymiel]

clang/unittests/Format/FormatTest.cpp

23946–23947

Sorry, that line break was removed when verifyFormat was still used

[MyDeveloperDay]

Thank you for this patch

[rymiel]

clang/unittests/Format/FormatTest.cpp

23593–23594

No difference:

concept DelayedCheck =
    (static_cast<bool>(0) || requires(T t) { t.bar(); }) && sizeof(T) <= 8;
concept DelayedCheck =
    static_cast<bool>(0) || (requires(T t) { t.bar(); } && sizeof(T) <= 8);
concept DelayedCheck =
    static_cast<bool>(0) || requires(T t) { t.bar(); } && sizeof(T) <= 8;

clang/lib/Format/TokenAnnotator.cpp

auto AssignmentStartsExpression = [&]() {
 
         Tok = Tok->MatchingParen;
         if (!Tok)
           return false;
         Tok = Tok->getNextNonComment();
         if (!Tok)
           return false;
 
-        if (Tok->isOneOf(tok::kw_class, tok::kw_enum, tok::kw_concept,
-                         tok::kw_struct, tok::kw_using)) {
+        if (Tok->isOneOf(tok::kw_class, tok::kw_enum, tok::kw_struct,
+                         tok::kw_using)) {
           return false;
         }
 
         return true;
       }
 
       // Type aliases use `type X = ...;` in TypeScript and can be exported
       // using `export type ...`.

clang/lib/Format/UnwrappedLineParser.h

private:
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
   void parseModuleImport();
   void parseNew();
   void parseAccessSpecifier();
   bool parseEnum();
   bool parseStructLike();
-  void parseConcept();
   bool parseRequires();
   void parseRequiresClause(FormatToken *RequiresToken);
   void parseRequiresExpression(FormatToken *RequiresToken);
   void parseConstraintExpression();
   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::at:
         // This branch isn't strictly necessary (the kw_try case below would
         // do this too after the tok::at is parsed above).  But be explicit.
         parseTryCatch();
         return;
       default:
         break;
       }
       break;
-    case tok::kw_concept:
-      parseConcept();
-      return;
     case tok::kw_requires: {
       if (Style.isCpp()) {
         bool ParsedClause = parseRequires();
         if (ParsedClause)
           return;
       } else {
         nextToken();
       }
if (FormatTok->is(tok::colon)) {
     // Not a variable name nor namespace name.
     addUnwrappedLine();
   } else if (AccessSpecifierCandidate) {
     // Consider the access specifier to be a C identifier.
     AccessSpecifierCandidate->Tok.setKind(tok::identifier);
   }
 }
 
-/// \brief Parses a concept definition.
-/// \pre The current token has to be the concept keyword.
-///
-/// Returns if either the concept has been completely parsed, or if it detects
-/// that the concept definition is incorrect.
-void UnwrappedLineParser::parseConcept() {
-  assert(FormatTok->is(tok::kw_concept) && "'concept' expected");
-  nextToken();
-  if (!FormatTok->is(tok::identifier))
-    return;
-  nextToken();
-  if (!FormatTok->is(tok::equal))
-    return;
-  nextToken();
-  parseConstraintExpression();
-  if (FormatTok->is(tok::semi))
-    nextToken();
-  addUnwrappedLine();
-}
-
 /// \brief Parses a requires, decides if it is a clause or an expression.
 /// \pre The current token has to be the requires keyword.
 /// \returns true if it parsed a clause.
 bool clang::format::UnwrappedLineParser::parseRequires() {
   assert(FormatTok->is(tok::kw_requires) && "'requires' expected");
   auto RequiresToken = FormatTok;
 
   // We try to guess if it is a requires clause, or a requires expression. For
void UnwrappedLineParser::parseRequiresClause(FormatToken *RequiresToken) {
   bool InRequiresExpression =
       !RequiresToken->Previous ||
       RequiresToken->Previous->is(TT_RequiresExpressionLBrace);
 
   RequiresToken->setFinalizedType(InRequiresExpression
                                       ? TT_RequiresClauseInARequiresExpression
                                       : TT_RequiresClause);
 
+  // NOTE: parseConstraintExpression is only ever called from this function.
+  // It could be inlined into here.
   parseConstraintExpression();
 
   if (!InRequiresExpression)
     FormatTok->Previous->ClosesRequiresClause = true;
 }
 
 /// \brief Parses a requires expression.
 /// \param RequiresToken The requires keyword token, which starts this clause.
if (FormatTok->is(tok::l_brace)) {
     FormatTok->setFinalizedType(TT_RequiresExpressionLBrace);
     parseChildBlock(/*CanContainBracedList=*/false,
                     /*NextLBracesType=*/TT_CompoundRequirementLBrace);
   }
 }
 
 /// \brief Parses a constraint expression.
 ///
-/// This is either the definition of a concept, or the body of a requires
-/// clause. It returns, when the parsing is complete, or the expression is
-/// incorrect.
+/// This is the body of a requires clause. It returns, when the parsing is
+/// complete, or the expression is incorrect.
 void UnwrappedLineParser::parseConstraintExpression() {
   // The special handling for lambdas is needed since tryToParseLambda() eats a
   // token and if a requires expression is the last part of a requires clause
   // and followed by an attribute like [[nodiscard]] the ClosesRequiresClause is
   // not set on the correct token. Thus we need to be aware if we even expect a
   // lambda to be possible.
   // template <typename T> requires requires { ... } [[nodiscard]] ...;
   bool LambdaNextTimeAllowed = true;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, Concepts) {
 
   verifyFormat("template <typename T>\n"
                "concept DelayedCheck = Unit<T> && !!DerivedUnit<T>;");
 
   verifyFormat("template <typename T>\n"
                "concept DelayedCheck = !!false || requires(T t) { t.bar(); } "
                "&& sizeof(T) <= 8;");
 
-  verifyFormat(
-      "template <typename T>\n"
-      "concept DelayedCheck = static_cast<bool>(0) ||\n"
-      "                       requires(T t) { t.bar(); } && sizeof(T) <= 8;");
+  verifyFormat("template <typename T>\n"
+               "concept DelayedCheck =\n"
+               "    static_cast<bool>(0) || requires(T t) { t.bar(); } && "
+               "sizeof(T) <= 8;");
 
   verifyFormat("template <typename T>\n"
                "concept DelayedCheck = bool(0) || requires(T t) { t.bar(); } "
                "&& sizeof(T) <= 8;");
 
   verifyFormat(
       "template <typename T>\n"
-      "concept DelayedCheck = (bool)(0) ||\n"
-      "                       requires(T t) { t.bar(); } && sizeof(T) <= 8;");
+      "concept DelayedCheck =\n"
+      "    (bool)(0) || requires(T t) { t.bar(); } && sizeof(T) <= 8;");
 
   verifyFormat("template <typename T>\n"
                "concept DelayedCheck = (bool)0 || requires(T t) { t.bar(); } "
                "&& sizeof(T) <= 8;");
 
   verifyFormat("template <typename T>\n"
                "concept Size = sizeof(T) >= 5 && requires(T t) { t.bar(); } && "
                "sizeof(T) <= 8;");
verifyFormat("template <typename T>\n"
                "concept C = foo(T{});");
 
   verifyFormat("template <typename T>\n"
                "concept Size = V<sizeof(T)>::Value > 5;");
 
   verifyFormat("template <typename T>\n"
                "concept True = S<T>::Value;");
 
+  verifyFormat("template <S T>\n"
+               "concept True = T.field;");
+
   verifyFormat(
       "template <typename T>\n"
       "concept C = []() { return true; }() && requires(T t) { t.bar(); } &&\n"
       "            sizeof(T) <= 8;");
 
   // FIXME: This is misformatted because the fake l paren starts at bool, not at
   // the lambda l square.
   verifyFormat("template <typename T>\n"
verifyFormat("template <typename T> concept HasFoo = requires(T t) {\n"
                "                                         { t.bar() };\n"
                "                                         t.bar();\n"
                "                                       };",
                Style);
 
   verifyFormat("template <typename T> concept True = true;", Style);
 
   verifyFormat(
-      "template <typename T> concept C = decltype([]() -> std::true_type {\n"
-      "                                    return {};\n"
-      "                                  }())::value &&\n"
-      "                                  requires(T t) { t.bar(); } && "
-      "sizeof(T) <= 8;",
+      "template <typename T> concept C =\n"
+      "    decltype([]() -> std::true_type { return {}; }())::value &&\n"
+      "    requires(T t) { t.bar(); } && sizeof(T) <= 8;",
       Style);
 
   verifyFormat("template <typename T> concept Semiregular =\n"
                "    DefaultConstructible<T> && CopyConstructible<T> && "
                "CopyAssignable<T> &&\n"
                "    requires(T a, std::size_t n) {\n"
                "      requires Same<T *, decltype(&a)>;\n"
                "      { a.~T() } noexcept;\n"
                "      requires Same<T *, decltype(new T)>;\n"
                "      requires Same<T *, decltype(new T[n])>;\n"
                "      { delete new T; };\n"
                "      { delete new T[n]; };\n"
                "    };",
                Style);
 
   // The following tests are invalid C++, we just want to make sure we don't
   // assert.
-  verifyFormat("template <typename T>\n"
+  verifyNoCrash("template <typename T>\n"
                 "concept C = requires C2<T>;");
 
-  verifyFormat("template <typename T>\n"
+  verifyNoCrash("template <typename T>\n"
                 "concept C = 5 + 4;");
 
-  verifyFormat("template <typename T>\n"
-               "concept C =\n"
-               "class X;");
+  verifyNoCrash("template <typename T>\n"
+                "concept C = class X;");
 
-  verifyFormat("template <typename T>\n"
+  verifyNoCrash("template <typename T>\n"
                 "concept C = [] && true;");
 
-  verifyFormat("template <typename T>\n"
+  verifyNoCrash("template <typename T>\n"
                 "concept C = [] && requires(T t) { typename T::size_type; };");
 }
 
 TEST_F(FormatTest, RequiresClausesPositions) {
   auto Style = getLLVMStyle();
   EXPECT_EQ(Style.RequiresClausePosition, FormatStyle::RCPS_OwnLine);
   EXPECT_EQ(Style.IndentRequiresClause, true);
 
   // The default in LLVM style is REI_OuterScope, but these tests were written