[clang-format] Correctly annotate UDLs as OverloadedOperator

[rymiel]

Summary

While the opening parenthesis of an user-defined literal operator was
correctly annotated as OverloadedOperatorLParen, the "" and its suffix
wasn't annotated as OverloadedOperator.

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

[rymiel]

Note issue being fixed

[rymiel]

Notes:

As noted above, the left paren was correctly annotated, but the logic in rParenEndsCast goes based off of what is *before* the l paren. This resulted in a paren pair where the left one was OverloadedOperatorLParen and the right one was CastRParen.

This means this bug could really be fixed in two ways, I chose to annotate the UDL with OverloadedOperator, as I felt like that resulted in "more correct tokens".

Edit: While attempting to add token annotator tests for this fix, i noticed there aren't any at all for overloaded operators, and while writing them, I noticed more operators which aren't marked with OverloadedOperator, such as operator[] and UDLs which don't start with an underscore. (see below)

Unless any reviewers have any other opinions, I would leave fixing those out of this patch and leave the tests "incomplete" for now?

Also, UDLs that don't start with an underscore aren't considered a single "string_literal" token, instead becoming a string literal "" and an identifier following it (where as those with an underscore become one token, such as ""_a). I'm unsure if that's the expected case and if both tokens should just be considered part of the operator

[rymiel]

Also add a few format tests

[rymiel]

Reformat

[rymiel]

While this patch fixes the linked bug, but I encountered more questions that I would like second opinions on (see notes above)

[HazardyKnusperkeks]

Looks good. You could add a FIXME before the commented out tests.

[owenpan]

In D134853#3822842, @rymiel wrote:

Unless any reviewers have any other opinions, I would leave fixing those out of this patch and leave the tests "incomplete" for now?

+1.

Also, UDLs that don't start with an underscore aren't considered a single "string_literal" token, instead becoming a string literal "" and an identifier following it (where as those with an underscore become one token, such as ""_a). I'm unsure if that's the expected case and if both tokens should just be considered part of the operator

The operator "" and the identifier that follows should be two separate tokens regardless if the identifier starts with an underscore.

clang/lib/Format/TokenAnnotator.cpp

1189

We should check for "", not just any string_literal.

[rymiel]

The operator "" and the identifier that follows should be two separate tokens regardless if the identifier starts with an underscore.

So, after some investigation, it turns out the clang lexer only combines an underscore-less UDL to a single string_literal token if it's a valid standard library UDL (in the given standard library version)

https://github.com/llvm/llvm-project/blob/6f46ff3765dcdc178b9cf52ebd8c03437806798a/clang/lib/Lex/Lexer.cpp#L1960-L1975
https://github.com/llvm/llvm-project/blob/3f18f7c0072b642f5fe88d2fb7bb8ccf69a6c6f5/clang/lib/Lex/LiteralSupport.cpp#L1180-L1185

Which means only single-token string_literals should be considered

[owenpan]

In D134853#3829433, @rymiel wrote:

So, after some investigation, it turns out the clang lexer only combines an underscore-less UDL to a single string_literal token if it's a valid standard library UDL (in the given standard library version)

And only if there are no spaces between "" and the identifier. Otherwise, it's invalid. See https://en.cppreference.com/w/cpp/language/user_literal.

Which means only single-token string_literals should be considered

We don't want to merge e.g. "" _foo or "" if into a single token ""_foo or ""if. With the former, the user may want clang-format to keep the space. With the latter, I'm not sure if it's ok for clang-format to fix the syntax error.

[rymiel]

Handle UDLs with and without spaces

Thank you @owenpan, I was not actually aware UDLs without spaces were valid.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

1193–1198

[rymiel]

Full stops in comments

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

1188–1201

We can simply check if the previous token starts with "".

clang/unittests/Format/TokenAnnotatorTest.cpp

427

We need/should not annotate the suffix.

429–437

IMO they are redundant as both _a and _long_literal are identifiers starting with an underscore.

449

Delete.

[rymiel]

clang/unittests/Format/TokenAnnotatorTest.cpp

427

Unless I change the logic in rParenEndsCast, the suffix does need to be an OverloadedOperator, since it goes off of the token immediately before left paren (https://reviews.llvm.org/D134853#3822842)

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

2120–2123

Perhaps add:

if (Tok.MatchingParen->is(TT_OverloadedOperatorLParen))
  return false;

clang/unittests/Format/TokenAnnotatorTest.cpp

427

After we annotate "" as TT_OverloadedOperator, rParenEndsCast() returns false, at least for your test cases and the example in https://github.com/llvm/llvm-project/issues/58035. If you can come up with test cases that still confuses rParenEndsCast(), we can add a simple check near the beginning of rParenEndsCast(). See line 2126 above.

[rymiel]

Do not annotate the trailing identifier of an UDL

[rymiel]

clang/lib/Format/TokenAnnotator.cpp

2120–2123

Note that in testing, adding isNot(TT_Unknown) did not cause any issues in tests, but that change felt a little too drastic

clang/unittests/Format/TokenAnnotatorTest.cpp

427

Oh, you are right; I did not notice the extra check which disallows identifiers in front of casts

429–437

I wanted to avoid all the tests being single-character names, as "single-character name" was sort of an issue for one format bug; but actually this isn't even single character anyway so you are right

[owenpan]

LGTM

clang/lib/Format/TokenAnnotator.cpp

case tok::kw_operator:
           Style.Language == FormatStyle::LK_Proto) {
         break;
       }
       while (CurrentToken &&
              !CurrentToken->isOneOf(tok::l_paren, tok::semi, tok::r_paren)) {
         if (CurrentToken->isOneOf(tok::star, tok::amp))
           CurrentToken->setType(TT_PointerOrReference);
         consumeToken();
-        if (CurrentToken && CurrentToken->is(tok::comma) &&
+        if (!CurrentToken)
+          continue;
+        if (CurrentToken->is(tok::comma) &&
             CurrentToken->Previous->isNot(tok::kw_operator)) {
           break;
         }
-        if (CurrentToken && CurrentToken->Previous->isOneOf(
-                                TT_BinaryOperator, TT_UnaryOperator, tok::comma,
-                                tok::star, tok::arrow, tok::amp, tok::ampamp)) {
+        if (CurrentToken->Previous->isOneOf(TT_BinaryOperator, TT_UnaryOperator,
+                                            tok::comma, tok::star, tok::arrow,
+                                            tok::amp, tok::ampamp) ||
+            // User defined literal.
+            CurrentToken->Previous->TokenText.startswith("\"\"")) {
           CurrentToken->Previous->setType(TT_OverloadedOperator);
         }
       }
       if (CurrentToken && CurrentToken->is(tok::l_paren))
         CurrentToken->setType(TT_OverloadedOperatorLParen);
       if (CurrentToken && CurrentToken->Previous->is(TT_BinaryOperator))
         CurrentToken->Previous->setType(TT_OverloadedOperator);
       break;
     case tok::question:
       if (Style.isJavaScript() && Tok->Next &&
           Tok->Next->isOneOf(tok::semi, tok::comma, tok::colon, tok::r_paren,
                              tok::r_brace)) {
         // Question marks before semicolons, colons, etc. indicate optional
         // types (fields, parameters), e.g.
         //   function(x?: string, y?) {...}
         //   class X { y?; }
         Tok->setType(TT_JsTypeOptionalQuestion);
         break;
       }
bool rParenEndsCast(const FormatToken &Tok) {
     if (!Style.isCSharp() && !Style.isCpp() &&
         Style.Language != FormatStyle::LK_Java) {
       return false;
     }
 
     // Empty parens aren't casts and there are no casts at the end of the line.
     if (Tok.Previous == Tok.MatchingParen || !Tok.Next || !Tok.MatchingParen)
       return false;
 
+    if (Tok.MatchingParen->is(TT_OverloadedOperatorLParen))
+      return false;
+
     FormatToken *LeftOfParens = Tok.MatchingParen->getPreviousNonComment();
     if (LeftOfParens) {
       // If there is a closing parenthesis left of the current
       // parentheses, look past it as these might be chained casts.
       if (LeftOfParens->is(tok::r_paren) &&
           LeftOfParens->isNot(TT_CastRParen)) {
         if (!LeftOfParens->MatchingParen ||
             !LeftOfParens->MatchingParen->Previous) {

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsOverloadedOperators) {
   verifyFormat("void f() { a.operator*(b & b); }");
   verifyFormat("void f() { a->operator&(a * b); }");
   verifyFormat("void f() { NS::a.operator+(*b * *b); }");
   // TODO: Calling an operator as a non-member function is hard to distinguish.
   // https://llvm.org/PR50629
   // verifyFormat("void f() { operator*(a & a); }");
   // verifyFormat("void f() { operator&(a, b * b); }");
 
+  verifyFormat("void f() { return operator()(x) * b; }");
+  verifyFormat("void f() { return operator[](x) * b; }");
+  verifyFormat("void f() { return operator\"\"_a(x) * b; }");
+  verifyFormat("void f() { return operator\"\" _a(x) * b; }");
+  verifyFormat("void f() { return operator\"\"s(x) * b; }");
+  verifyFormat("void f() { return operator\"\" s(x) * b; }");
+  verifyFormat("void f() { return operator\"\"if(x) * b; }");
+
   verifyFormat("::operator delete(foo);");
   verifyFormat("::operator new(n * sizeof(foo));");
   verifyFormat("foo() { ::operator delete(foo); }");
   verifyFormat("foo() { ::operator new(n * sizeof(foo)); }");
 }
 
 TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
   verifyFormat("void A::b() && {}");

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsFunctionRefQualifiers) {
   EXPECT_EQ(Tokens.size(), 12u) << Tokens;
   EXPECT_TOKEN(Tokens[9], tok::amp, TT_PointerOrReference);
 
   Tokens = annotate("template <typename T> void operator=(T) &;");
   EXPECT_EQ(Tokens.size(), 14u) << Tokens;
   EXPECT_TOKEN(Tokens[11], tok::amp, TT_PointerOrReference);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsOverloadedOperators) {
+  auto Tokens = annotate("x.operator+()");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::plus, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator=()");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::equal, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator+=()");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::plusequal, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator,()");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::comma, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator()()");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::l_paren, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[4], tok::r_paren, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[5], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator[]()");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  // EXPECT_TOKEN(Tokens[3], tok::l_square, TT_OverloadedOperator);
+  // EXPECT_TOKEN(Tokens[4], tok::r_square, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[5], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator\"\"_a()");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::string_literal, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator\"\" _a()");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::string_literal, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[5], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator\"\"if()");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::string_literal, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator\"\"s()");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::string_literal, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_OverloadedOperatorLParen);
+  Tokens = annotate("x.operator\"\" s()");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::string_literal, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[5], tok::l_paren, TT_OverloadedOperatorLParen);
+}
+
 TEST_F(TokenAnnotatorTest, UnderstandsRequiresClausesAndConcepts) {
   auto Tokens = annotate("template <typename T>\n"
                          "concept C = (Foo && Bar) && (Bar && Baz);");
 
   ASSERT_EQ(Tokens.size(), 21u) << Tokens;
   EXPECT_TOKEN(Tokens[10], tok::ampamp, TT_BinaryOperator);
   EXPECT_TOKEN(Tokens[13], tok::ampamp, TT_BinaryOperator);
   EXPECT_TOKEN(Tokens[16], tok::ampamp, TT_BinaryOperator);
 
   Tokens = annotate("template <typename T>\n"
                     "concept C = Foo && !Bar;");
 
   ASSERT_EQ(Tokens.size(), 14u) << Tokens;
   EXPECT_TOKEN(Tokens[9], tok::ampamp, TT_BinaryOperator);
   EXPECT_TOKEN(Tokens[10], tok::exclaim, TT_UnaryOperator);