[clang-format] Annotate lambdas with requires clauses.

[rymiel]

Summary

The C++ grammar allows lambdas to have a *requires-clause* in two
places, either directly after the *template-parameter-list*, such as:

[] <typename T> requires foo<T> (T t) { ... };

Or, at the end of the *lambda-declarator* (before the lambda's body):

[] <typename T> (T t) requires foo<T> { ... };

Previously, these cases weren't handled at all, resulting in weird
results.

Note that this commit only handles token annotation, so the actual
formatting still ends up suboptimal. This is mostly because I do not yet
know how to approach making the requires clause formatting of lambdas
match the formatting for functions.

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

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

3393

don't need else after break.
In fact I would negate the condition and return, and keep the remainder out of any if or else.

[rymiel]

Improve code flow in parseConstraintExpression

[rymiel]

clang/lib/Format/UnwrappedLineParser.cpp

3393

Thank you! I clearly wasn't thinking clearly there

[owenpan]

Would this patch make the end result look worse without "making the requires clause formatting of lambdas
match the formatting for functions"? If yes, then we should not land it just yet.

[rymiel]

In D145642#4183539, @owenpan wrote:

Would this patch make the end result look worse without "making the requires clause formatting of lambdas
match the formatting for functions"? If yes, then we should not land it just yet.

Depends on your definition of "worse" :)
I outlined this on the github issue (https://github.com/llvm/llvm-project/issues/61269), essentially:

The result for the example provided on the github issue something like this:

[&]<typename Callable>(Callable&& callable)
  requires std::is_invocable_v<Callable>
{ static_cast<void>(callable); };

Unlike function declarations, lambdas with a requires clause still have the body on one line. I don't yet know how to go about changing this, and if I should at all. This of course isn't an issue if the lambda body has multiple lines.

There is also the case of requires clauses before the parameters, which results in this:

[&]<typename Callable>
  requires std::is_invocable_v<Callable>
(Callable &&callable) { static_cast<void>(callable); };

This poses a whole new set of problems (like, starting a line with an opening paren is pretty weird!)
But, this sort of syntax seems fairly obscure: I only found out about it reading the C++ grammar, and the clang frontend itself doesn't actually parse it at all in some cases (I filed https://github.com/llvm/llvm-project/issues/61278 for this)

[owenpan]

In D145642#4186304, @rymiel wrote:

In D145642#4183539, @owenpan wrote:

Would this patch make the end result look worse without "making the requires clause formatting of lambdas
match the formatting for functions"? If yes, then we should not land it just yet.

Depends on your definition of "worse" :)
I outlined this on the github issue (https://github.com/llvm/llvm-project/issues/61269), essentially:

The result for the example provided on the github issue something like this:

[&]<typename Callable>(Callable&& callable)
  requires std::is_invocable_v<Callable>
{ static_cast<void>(callable); };

Unlike function declarations, lambdas with a requires clause still have the body on one line. I don't yet know how to go about changing this, and if I should at all. This of course isn't an issue if the lambda body has multiple lines.

I'm more concerned with the above case. If the body of the lambda (including the enclosing braces) should not be merged into a single line, then I would say the token-annotation-only fix might trigger regressions. Otherwise, we can safely land this patch now.

[rymiel]

Could you please clarify what you mean by "regressions" here? Isn't the behaviour of this syntax broken to begin with? It doesn't change anything about lambdas without requires-clauses

[owenpan]

In D145642#4186462, @rymiel wrote:

Could you please clarify what you mean by "regressions" here? Isn't the behaviour of this syntax broken to begin with? It doesn't change anything about lambdas without requires-clauses

Let's use you example:

$ cat test.cpp
#include <type_traits>

template <typename T>
struct foo {
    static const bool value = true;
};

template <typename T>
inline constexpr bool foo_v = foo<T>::value;

template <typename T>
concept foo_c = foo_v<T>;

int main() {
  [&]<typename Callable>
    requires foo<Callable>::value
  (Callable&& callable)
  {
    static_cast<void>(callable);
  };

  [&]<typename Callable>
    requires foo_c<Callable>
  (Callable&& callable)
  {
    static_cast<void>(callable);
  };

  [&]<typename Callable>
    requires foo_v<Callable>
  (Callable&& callable)
  {
    static_cast<void>(callable);
  };
}
$ clang-format -version
clang-format version 17.0.0 (https://github.com/llvm/llvm-project f6e7a5c29221f445e4cbddc32667a1e12a1446db)
$ clang-format test.cpp
#include <type_traits>

template <typename T> struct foo {
  static const bool value = true;
};

template <typename T> inline constexpr bool foo_v = foo<T>::value;

template <typename T>
concept foo_c = foo_v<T>;

int main() {
  [&]<typename Callable>
    requires foo<Callable>::value(Callable && callable)
  {
    static_cast<void>(callable);
  };

  [&]<typename Callable>
    requires foo_c<Callable>(Callable && callable)
  {
    static_cast<void>(callable);
  };

  [&]<typename Callable>
    requires foo_v<Callable>(Callable && callable)
  {
    static_cast<void>(callable);
  };
}

If this patch would merge the lambda bodies into single lines, would that be considered a possible regression? My guess is that it would be unless the lambda bodies should be merged in the first place, in which case the patch would also fix a formatting bug in addition to the annotation bug.

[usaxena95]

starting a line with an opening paren is pretty weird!)

I do not think this is weird. On the contrary, this is more readable to me and separates the requires clause from the parameters list. For example this one looks so much better:

// trunk.
template <typename T>
void func() {
  auto x = []<typename U>
    requires Foo<T> && Foo<T>(T t, U u) requires BarBar<U> && BarBar<U> ||
                 BarBar<U>
  {
    return u + t;
  };
}

// patch.
template <typename T> void func() {
  auto x = []<typename U>
    requires Foo<T> && Foo<T>
  (T t, U u)
    requires BarBar<U> && BarBar<U> || BarBar<U>
  { return u + t; };
}

Discussion on wrapping the lambda body with single statement. FWIW: I do not think this is a regression and we are fixing things as seen in my first example.

Another point:
While testing this patch, the following still fails to recognise. Might be something special with true.

auto y = [&]<typename Callable>
    requires true(Callable && callable)
  { static_cast<void>(callable); };

clang/unittests/Format/TokenAnnotatorTest.cpp

1352

This is valid and is accepted by the compilers https://godbolt.org/z/EPbrWbrsv

[rymiel]

Fix true edge case

[rymiel]

Remove unnecessary musing

[rymiel]

In D145642#4213256, @usaxena95 wrote:

Another point:
While testing this patch, the following still fails to recognise. Might be something special with true.

auto y = [&]<typename Callable>
    requires true(Callable && callable)
  { static_cast<void>(callable); };

Thank you for finding that edge case, I've fixed it and added a test for it

[rymiel]

clang/unittests/Format/TokenAnnotatorTest.cpp

1352

Thank you!

[rymiel]

So, it took me a while but I finally found where the logic is that makes the lambda braces stay on one line, but, now I'm not so sure if I should change it:

The thing I wanted to avoid was cases like

[&]<typename T>(T&& t)
requires T
{ t; };

Simply because "those braces don't really look nice", but it turns out there's a sort of "prior precedent" for this:

For example, FormatTest.cpp line 22046, LambdaWithLineComments:

verifyFormat("auto k = []() // X\n"
             "{ return; }",
             LLVMWithBeforeLambdaBody);
verifyFormat(
    "auto k = []() // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n"
    "{ return; }",
    LLVMWithBeforeLambdaBody);

(this is with combination of BraceWrapping.BeforeLambdaBody = true and AllowShortLambdaOnASingleLine)
(+ a change in C# which I would like to avoid touching since I exclusively do C++)

So, I guess I have the following choices:

1. do nothing! (allow those braces to stay on one line)
2. change the behavior of those existing tests, with whatever impact that has
3. make a very specific exception for just requires-clauses

(+ maybe other compromise options, i.e. making it configurable or something)

@owenpan do you perhaps have insight for this?

[HazardyKnusperkeks]

I think this is okay. A followup could handle single line lambdas. But I have a personal struggle with them, especially the name AllowShortLambdasOnASingleLine no it is not allow if turned on it is force. And in combination with ColumnLimit 0 there seem to be some mismatches (never bothered enough to investigate, or if it is just a feeling).

The question would be if something with a requires clause should be merges into one line, then I think it should put everything in one line. But what if that now is in conflict with RequiresClausePosition?

[owenpan]

@rymiel I'm deferring to you and @HazardyKnusperkeks, so doing nothing for now is fine.

clang/lib/Format/UnwrappedLineParser.cpp

while (FormatTok->isNot(tok::l_brace)) {
     if (FormatTok->isSimpleTypeSpecifier()) {
       nextToken();
       continue;
     }
     switch (FormatTok->Tok.getKind()) {
     case tok::l_brace:
       break;
     case tok::l_paren:
-      parseParens();
+      parseParens(/*AmpAmpTokenType=*/TT_PointerOrReference);
       break;
     case tok::l_square:
       parseSquare();
       break;
     case tok::less:
       assert(FormatTok->Previous);
       if (FormatTok->Previous->is(tok::r_square))
         InTemplateParameterList = true;
while (FormatTok->isNot(tok::l_brace)) {
     case tok::arrow:
       // This might or might not actually be a lambda arrow (this could be an
       // ObjC method invocation followed by a dereferencing arrow). We might
       // reset this back to TT_Unknown in TokenAnnotator.
       FormatTok->setFinalizedType(TT_LambdaArrow);
       SeenArrow = true;
       nextToken();
       break;
+    case tok::kw_requires: {
+      auto *RequiresToken = FormatTok;
+      nextToken();
+      parseRequiresClause(RequiresToken);
+      break;
+    }
     default:
       return true;
     }
   }
   FormatTok->setFinalizedType(TT_LambdaLBrace);
   LSquare.setFinalizedType(TT_LambdaLSquare);
   parseChildBlock();
   return true;

 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;
+
+  // Within lambda declarations, it is permitted to put a requires clause after
+  // its template parameter list, which would place the requires clause right
+  // before the parentheses of the parameters of the lambda declaration. Thus,
+  // we track if we expect to see grouping parentheses at all.
+  // Without this check, `requires foo<T> (T t)` in the below example would be
+  // seen as the whole requires clause, accidentally eating the parameters of
+  // the lambda.
+  // [&]<typename T> requires foo<T> (T t) { ... };
+  bool TopLevelParensAllowed = 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:
+      if (TopLevelParensAllowed) {
         parseParens(/*AmpAmpTokenType=*/TT_BinaryOperator);
+        TopLevelParensAllowed = false;
         break;
+      } else {
+        return;
+      }
 
     case tok::l_square:
       if (!LambdaThisTimeAllowed || !tryToParseLambda())
         return;
       break;
 
     case tok::kw_const:
     case tok::semi:
     case tok::kw_class:
     case tok::kw_struct:
     case tok::kw_union:
       return;
 
     case tok::l_brace:
       // Potential function body.
       return;
 
     case tok::ampamp:
     case tok::pipepipe:
       FormatTok->setFinalizedType(TT_BinaryOperator);
       nextToken();
       LambdaNextTimeAllowed = true;
+      TopLevelParensAllowed = true;
       break;
 
     case tok::comma:
     case tok::comment:
       LambdaNextTimeAllowed = LambdaThisTimeAllowed;
       nextToken();
       break;
 
     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:
       LambdaNextTimeAllowed = true;
+      TopLevelParensAllowed = true;
       // Just eat them.
       nextToken();
       break;
 
     case tok::numeric_constant:
     case tok::coloncolon:
     case tok::kw_true:
     case tok::kw_false:
default:
 
       // Read identifier with optional template declaration.
       nextToken();
       if (FormatTok->is(tok::less)) {
         nextToken();
         parseBracedList(/*ContinueOnSemicolons=*/false, /*IsEnum=*/false,
                         /*ClosingBraceKind=*/tok::greater);
       }
+      TopLevelParensAllowed = false;
       break;
     }
   } while (!eof());
 }
 
 bool UnwrappedLineParser::parseEnum() {
   const FormatToken &InitialToken = *FormatTok;
 

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsLambdas) {
   EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
   EXPECT_TOKEN(Tokens[9], tok::l_brace, TT_LambdaLBrace);
 
   Tokens = annotate("[] <Foo... T> {}");
   ASSERT_EQ(Tokens.size(), 10u) << Tokens;
   EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
   EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
   EXPECT_TOKEN(Tokens[7], tok::l_brace, TT_LambdaLBrace);
+
+  // Lambdas with a requires-clause
+  Tokens = annotate("[] <typename T> (T t) requires Bar<T> {}");
+  ASSERT_EQ(Tokens.size(), 18u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[10], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[14]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[15], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> (T &&t) requires Bar<T> {}");
+  ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[8], tok::ampamp, TT_PointerOrReference);
+  EXPECT_TOKEN(Tokens[11], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[15]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[16], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> (T t) requires Foo<T> || Bar<T> {}");
+  ASSERT_EQ(Tokens.size(), 23u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[10], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[19]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[20], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> (T t) -> T requires Bar<T> {}");
+  ASSERT_EQ(Tokens.size(), 20u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[10], tok::arrow, TT_LambdaArrow);
+  EXPECT_TOKEN(Tokens[12], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[16]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[17], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> requires Bar<T> (T t) {}");
+  ASSERT_EQ(Tokens.size(), 18u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[6], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[10]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[15], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> requires Bar<T> (T &&t) {}");
+  ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[6], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[10]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[13], tok::ampamp, TT_PointerOrReference);
+  EXPECT_TOKEN(Tokens[16], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> requires Foo<T> || Bar<T> (T t) {}");
+  ASSERT_EQ(Tokens.size(), 23u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[6], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[15]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[20], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> requires Bar<T> {}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[6], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[10]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[11], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> requires Bar<T> noexcept {}");
+  ASSERT_EQ(Tokens.size(), 15u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[6], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[10]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[12], tok::l_brace, TT_LambdaLBrace);
+
+  Tokens = annotate("[] <typename T> requires Bar<T> -> T {}");
+  ASSERT_EQ(Tokens.size(), 16u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[6], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[10]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[11], tok::arrow, TT_LambdaArrow);
+  EXPECT_TOKEN(Tokens[13], tok::l_brace, TT_LambdaLBrace);
+
+  // Both at once? Probably not even valid.
+  Tokens = annotate("[] <typename T> requires Foo<T> (T t) requires Bar<T> {}");
+  ASSERT_EQ(Tokens.size(), 23u) << Tokens;
+  EXPECT_TOKEN(Tokens[0], tok::l_square, TT_LambdaLSquare);
+  EXPECT_TOKEN(Tokens[2], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[6], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[10]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[15], tok::kw_requires, TT_RequiresClause);
+  EXPECT_TRUE(Tokens[19]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[20], tok::l_brace, TT_LambdaLBrace);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsFunctionAnnotations) {
   auto Tokens = annotate("template <typename T>\n"
                          "DEPRECATED(\"Use NewClass::NewFunction instead.\")\n"
                          "string OldFunction(const string &parameter) {}");
   ASSERT_EQ(Tokens.size(), 20u) << Tokens;
   EXPECT_TOKEN(Tokens[8], tok::r_paren, TT_FunctionAnnotationRParen);