[clang-format] Fixed handling of requires clauses followed by attributes

[HazardyKnusperkeks]

Summary

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

[• Quuxplusone]

clang/lib/Format/UnwrappedLineParser.cpp

3019–3020

Nit: For this pattern, consider bool LambdaThisTimeAllowed = std::exchange(LambdaNextTimeAllowed, false);

3102

This seems like an overuse of fallthrough; how about simply

LambdaNextTimeAllowed = true;
nextToken();
break;

?

[curdeius]

+1 to Arthur's comments.
Does it fix any of the recently created issues?

[HazardyKnusperkeks]

In D119893#3325375, @curdeius wrote:

+1 to Arthur's comments.
Does it fix any of the recently created issues?

Yeah, I should have put that in the commit message, right? ;)

Maybe I would have discovered the bug myself, but this is from GitHub.

clang/lib/Format/UnwrappedLineParser.cpp

3019–3020

I was under the impression that exchange was C++17. If I'd known that it's 14 and I can use it, I'd done that in the first place.

3102

Can do. I'm just thinking if there is ever a change to what to do, not just nextToken(), I would have to change it only at 2 not 3 places. But probably that will never happen.

[curdeius]

In D119893#3325432, @HazardyKnusperkeks wrote:

In D119893#3325375, @curdeius wrote:

+1 to Arthur's comments.
Does it fix any of the recently created issues?

Yeah, I should have put that in the commit message, right? ;)

Maybe I would have discovered the bug myself, but this is from GitHub.

Just put Fixes https://github.com/llvm/llvm-project/issues/123456. in the description please so that the issues gets automatically closed when this patch lands.

[JohelEGP]

Thank you.

[curdeius]

LGTM. Maybe you can add a minimal test in FormatTest.cpp though.

[HazardyKnusperkeks]

Formatting Test added.

[curdeius]

In D119893#3326247, @HazardyKnusperkeks wrote:

Formatting Test added.

Thanks!

clang/lib/Format/UnwrappedLineParser.cpp

 
 #include "UnwrappedLineParser.h"
 #include "FormatToken.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <algorithm>
+#include <utility>
 
 #define DEBUG_TYPE "format-parser"
 
 namespace clang {
 namespace format {
 
 class FormatTokenSource {
 public:

 }
 
 /// \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.
 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;
   do {
+    bool LambdaThisTimeAllowed = std::exchange(LambdaNextTimeAllowed, false);
+
     switch (FormatTok->Tok.getKind()) {
     case tok::kw_requires: {
       auto RequiresToken = FormatTok;
       nextToken();
       parseRequiresExpression(RequiresToken);
       break;
     }
 
     case tok::l_paren:
       parseParens(/*AmpAmpTokenType=*/TT_BinaryOperator);
       break;
 
     case tok::l_square:
-      if (!tryToParseLambda())
+      if (!LambdaThisTimeAllowed || !tryToParseLambda())
         return;
       break;
 
     case tok::identifier:
       // We need to differentiate identifiers for a template deduction guide,
       // variables, or function return types (the constraint expression has
       // ended before that), and basically all other cases. But it's easier to
       // check the other way around.
do {
     case tok::l_brace:
       // Potential function body.
       return;
 
     case tok::ampamp:
     case tok::pipepipe:
       FormatTok->setType(TT_BinaryOperator);
       nextToken();
+      LambdaNextTimeAllowed = true;
+      break;
+
+    case tok::comma:
+    case tok::comment:
+      LambdaNextTimeAllowed = LambdaThisTimeAllowed;
+      nextToken();
       break;
 
-    case tok::kw_true:
-    case tok::kw_false:
     case tok::kw_sizeof:
     case tok::greater:
     case tok::greaterequal:
     case tok::greatergreater:
     case tok::less:
     case tok::lessequal:
     case tok::lessless:
     case tok::equalequal:
     case tok::exclaim:
     case tok::exclaimequal:
     case tok::plus:
     case tok::minus:
     case tok::star:
     case tok::slash:
-    case tok::numeric_constant:
     case tok::kw_decltype:
-    case tok::comment:
-    case tok::comma:
+      LambdaNextTimeAllowed = true;
+      // Just eat them.
+      nextToken();
+      break;
+
+    case tok::numeric_constant:
     case tok::coloncolon:
+    case tok::kw_true:
+    case tok::kw_false:
       // Just eat them.
       nextToken();
       break;
 
     case tok::kw_static_cast:
     case tok::kw_const_cast:
     case tok::kw_reinterpret_cast:
     case tok::kw_dynamic_cast:

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, Concepts) {
 
   verifyFormat("template <typename T>\n"
                "concept C = [] && true;");
 
   verifyFormat("template <typename T>\n"
                "concept C = [] && requires(T t) { typename T::size_type; };");
 }
 
-TEST_F(FormatTest, RequiresClauses) {
+TEST_F(FormatTest, RequiresClausesPositions) {
   auto Style = getLLVMStyle();
   EXPECT_EQ(Style.RequiresClausePosition, FormatStyle::RCPS_OwnLine);
   EXPECT_EQ(Style.IndentRequiresClause, true);
 
   verifyFormat("template <typename T>\n"
                "  requires(Foo<T> && std::trait<T>)\n"
                "struct Bar;",
                Style);
verifyFormat("template <typename AAAAAAA>\n"
                "requires Foo<AAAAAAAA>\n"
                "Bar(T) -> Bar<T>;\n"
                "template <typename AAAAAAA>\n"
                "requires Foo<AAAAAAAAAAAAAAAA>\n"
                "Bar(T) -> Bar<T>;",
                ColumnStyle);
 }
 
+TEST_F(FormatTest, RequiresClauses) {
+  verifyFormat("struct [[nodiscard]] zero_t {\n"
+               "  template <class T>\n"
+               "    requires requires { number_zero_v<T>; }\n"
+               "  [[nodiscard]] constexpr operator T() const {\n"
+               "    return number_zero_v<T>;\n"
+               "  }\n"
+               "};");
+}
+
 TEST_F(FormatTest, StatementAttributeLikeMacros) {
   FormatStyle Style = getLLVMStyle();
   StringRef Source = "void Foo::slot() {\n"
                      "  unsigned char MyChar = 'x';\n"
                      "  emit signal(MyChar);\n"
                      "  Q_EMIT signal(MyChar);\n"
                      "}";
 

clang/unittests/Format/TokenAnnotatorTest.cpp

Tokens =
       annotate("void Class::member() && requires(Namespace::Constant<T>) {}");
   ASSERT_EQ(Tokens.size(), 19u) << Tokens;
   EXPECT_TOKEN(Tokens[7], tok::kw_requires, TT_RequiresClause);
 
   Tokens = annotate("void Class::member() && requires(typename "
                     "Namespace::Outer<T>::Inner::Constant) {}");
   ASSERT_EQ(Tokens.size(), 24u) << Tokens;
   EXPECT_TOKEN(Tokens[7], tok::kw_requires, TT_RequiresClause);
+
+  Tokens = annotate("struct [[nodiscard]] zero_t {\n"
+                    "  template<class T>\n"
+                    "    requires requires { number_zero_v<T>; }\n"
+                    "  [[nodiscard]] constexpr operator T() const { "
+                    "return number_zero_v<T>; }\n"
+                    "};");
+  ASSERT_EQ(Tokens.size(), 44u);
+  EXPECT_TOKEN(Tokens[13], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TOKEN(Tokens[14], tok::kw_requires, TT_RequiresExpression);
+  EXPECT_TOKEN(Tokens[15], tok::l_brace, TT_RequiresExpressionLBrace);
+  EXPECT_TOKEN(Tokens[21], tok::r_brace, TT_Unknown);
+  EXPECT_EQ(Tokens[21]->MatchingParen, Tokens[15]);
+  EXPECT_TRUE(Tokens[21]->ClosesRequiresClause);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsRequiresExpressions) {
   auto Tokens = annotate("bool b = requires(int i) { i + 5; };");
   ASSERT_EQ(Tokens.size(), 16u) << Tokens;
   EXPECT_TOKEN(Tokens[3], tok::kw_requires, TT_RequiresExpression);
   EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_RequiresExpressionLParen);
   EXPECT_TOKEN(Tokens[8], tok::l_brace, TT_RequiresExpressionLBrace);
TEST_F(TokenAnnotatorTest, RequiresDoesNotChangeParsingOfTheRest) {
 
   for (auto I = 0u; I < NumberOfBaseTokens; ++I)
     if (I < NumberOfTokensBeforeRequires)
       EXPECT_EQ(*BaseTokens[I], *ConstrainedTokens[I]) << I;
     else
       EXPECT_EQ(*BaseTokens[I],
                 *ConstrainedTokens[I + NumberOfAdditionalRequiresClauseTokens])
           << I;
+
+  BaseTokens = annotate("struct [[nodiscard]] zero_t {\n"
+                        "  template<class T>\n"
+                        "  [[nodiscard]] constexpr operator T() const { "
+                        "return number_zero_v<T>; }\n"
+                        "};");
+
+  ConstrainedTokens = annotate("struct [[nodiscard]] zero_t {\n"
+                               "  template<class T>\n"
+                               "    requires requires { number_zero_v<T>; }\n"
+                               "  [[nodiscard]] constexpr operator T() const { "
+                               "return number_zero_v<T>; }\n"
+                               "};");
+  NumberOfBaseTokens = 35u;
+  NumberOfAdditionalRequiresClauseTokens = 9u;
+  NumberOfTokensBeforeRequires = 13u;
+
+  ASSERT_EQ(BaseTokens.size(), NumberOfBaseTokens) << BaseTokens;
+  ASSERT_EQ(ConstrainedTokens.size(),
+            NumberOfBaseTokens + NumberOfAdditionalRequiresClauseTokens)
+      << ConstrainedTokens;
+
+  for (auto I = 0u; I < NumberOfBaseTokens; ++I)
+    if (I < NumberOfTokensBeforeRequires)
+      EXPECT_EQ(*BaseTokens[I], *ConstrainedTokens[I]) << I;
+    else
+      EXPECT_EQ(*BaseTokens[I],
+                *ConstrainedTokens[I + NumberOfAdditionalRequiresClauseTokens])
+          << I;
 }
 
 } // namespace
 } // namespace format
 } // namespace clang