[clang-format] Correctly annotate */&/&& in operator function calls

[owenpan]

Summary

Reverts 4986f3f2f220 (but keeps its unit tests) and fixes #49973 differently.

Also fixes bugs that incorrectly annotate the operator keyword as TT_FunctionDeclarationName in function calls and as TT_Unknown in function declarations and definitions.

[owenpan]

Early break if operator is TT_FunctionDeclarationName.

[owenpan]

Handle operator preceded by return or co-return.

[owenpan]

Handle multiple */&/&& in a single operator function argument.

[owenpan]

Update a unit test to make it more rigorous.

[rymiel]

Thanks, this probably makes more sense than what I did before

clang/lib/Format/TokenAnnotator.cpp

3323

I assume this extra fixup step means that the context of the call's parens won't be set to IsExpression = true? That won't cause any problems?

[owenpan]

Don't fix the token type of */&/&& if the line is a function declaration.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

3323

This doesn't change IsExpression, which I believe has already been set by the annotator parser.

[owenpan]

Re-annotate */&/&& only if isCpp() is true.

[krasimir]

It looks like this causes a regression in the following example:

% cat test.cc
struct A {
  std::unique_ptr<B> operator()(const B& b) const;
};
% clang-format -style=google test.cc  # should be the same as the input
struct A {
  std::unique_ptr<B> operator()(const B & b) const;
};

@owenpan, I was planning to temporarily revert this until you have a chance to look into this.
However it seems I'd also have to revert https://github.com/llvm/llvm-project/commit/cc2ff02eadede988ab890b2d0f428bd1531a3803 and https://github.com/llvm/llvm-project/commit/0556ab33532964dd8720b3b3ebd87dfe459d81d2 along as they seem to depend on this commit. Fo you feel that there's a quick fix for this regression that will allow us to fix-forward without reverting all these commits?

[rymiel]

Shouldn't that regression already be handled by D155358?

[krasimir]

In D154184#4509744, @rymiel wrote:

Shouldn't that regression already be handled by D155358?

Ah, I missed that. That fixes it. Thank you!

[MyDeveloperDay]

In D154184#4509744, @rymiel wrote:

Shouldn't that regression already be handled by D155358?

nice catch @rymiel!

clang/lib/Format/TokenAnnotator.cpp

if (OpeningParen.is(TT_OverloadedOperatorLParen)) {
       while (!Prev->is(tok::kw_operator)) {
         Prev = Prev->Previous;
         assert(Prev && "Expect a kw_operator prior to the OperatorLParen!");
       }
 
       // If faced with "a.operator*(argument)" or "a->operator*(argument)",
       // i.e. the operator is called as a member function,
       // then the argument must be an expression.
-      // If faced with "operator+(argument)", i.e. the operator is called as
-      // a free function, then the argument is an expression only if the current
-      // line can't be a declaration.
-      bool IsOperatorCallSite =
-          (Prev->Previous &&
-           Prev->Previous->isOneOf(tok::period, tok::arrow)) ||
-          (!Line.MustBeDeclaration && !Line.InMacroBody);
-      Contexts.back().IsExpression = IsOperatorCallSite;
+      bool OperatorCalledAsMemberFunction =
+          Prev->Previous && Prev->Previous->isOneOf(tok::period, tok::arrow);
+      Contexts.back().IsExpression = OperatorCalledAsMemberFunction;
     } else if (OpeningParen.is(TT_VerilogInstancePortLParen)) {
       Contexts.back().IsExpression = true;
       Contexts.back().ContextType = Context::VerilogInstancePortList;
     } else if (Style.isJavaScript() &&
                (Line.startsWith(Keywords.kw_type, tok::identifier) ||
                 Line.startsWith(tok::kw_export, Keywords.kw_type,
                                 tok::identifier))) {
       // type X = (...);
void TokenAnnotator::annotate(AnnotatedLine &Line) {
   Line.First->SpacesRequiredBefore = 1;
   Line.First->CanBreakBefore = Line.First->MustBreakBefore;
 }
 
 // This function heuristically determines whether 'Current' starts the name of a
 // function declaration.
 static bool isFunctionDeclarationName(bool IsCpp, const FormatToken &Current,
                                       const AnnotatedLine &Line) {
+  assert(Current.Previous);
   auto skipOperatorName = [](const FormatToken *Next) -> const FormatToken * {
     for (; Next; Next = Next->Next) {
       if (Next->is(TT_OverloadedOperatorLParen))
         return Next;
       if (Next->is(TT_OverloadedOperator))
         continue;
       if (Next->isOneOf(tok::kw_new, tok::kw_delete)) {
         // For 'new[]' and 'delete[]'.
for (; Next; Next = Next->Next) {
       break;
     }
     return nullptr;
   };
 
   // Find parentheses of parameter list.
   const FormatToken *Next = Current.Next;
   if (Current.is(tok::kw_operator)) {
-    if (Current.Previous && Current.Previous->is(tok::coloncolon))
+    const auto *Previous = Current.Previous;
+    if (Previous->Tok.getIdentifierInfo() &&
+        !Previous->isOneOf(tok::kw_return, tok::kw_co_return)) {
+      return true;
+    }
+    if (!Previous->isOneOf(tok::star, tok::amp, tok::ampamp))
       return false;
     Next = skipOperatorName(Next);
   } else {
     if (!Current.is(TT_StartOfName) || Current.NestingLevel != 0)
       return false;
     for (; Next; Next = Next->Next) {
       if (Next->is(TT_TemplateOpener) && Next->MatchingParen) {
         Next = Next->MatchingParen;
void TokenAnnotator::calculateFormattingInformation(AnnotatedLine &Line) const {
   FormatToken *Current = Line.First->Next;
   bool InFunctionDecl = Line.MightBeFunctionDecl;
   bool AlignArrayOfStructures =
       (Style.AlignArrayOfStructures != FormatStyle::AIAS_None &&
        Line.Type == LT_ArrayOfStructInitializer);
   if (AlignArrayOfStructures)
     calculateArrayInitializerColumnList(Line);
 
+  bool LineIsFunctionDeclaration = false;
   for (FormatToken *Tok = Current, *AfterLastAttribute = nullptr; Tok;
        Tok = Tok->Next) {
     if (isFunctionDeclarationName(Style.isCpp(), *Tok, Line)) {
+      LineIsFunctionDeclaration = true;
       Tok->setType(TT_FunctionDeclarationName);
       if (AfterLastAttribute &&
           mustBreakAfterAttributes(*AfterLastAttribute, Style)) {
         AfterLastAttribute->MustBreakBefore = true;
         Line.ReturnTypeWrapped = true;
       }
       break;
     }
     if (Tok->Previous->EndsCppAttributeGroup)
       AfterLastAttribute = Tok;
   }
 
+  if (Style.isCpp() && !LineIsFunctionDeclaration) {
+    // Annotate */&/&& in `operator` function calls as binary operators.
+    for (const auto *Tok = Line.First; Tok; Tok = Tok->Next) {
+      if (Tok->isNot(tok::kw_operator))
+        continue;
+      do {
+        Tok = Tok->Next;
+      } while (Tok && Tok->isNot(TT_OverloadedOperatorLParen));
+      if (!Tok)
+        break;
+      const auto *LeftParen = Tok;
+      for (Tok = Tok->Next; Tok && Tok != LeftParen->MatchingParen;
+           Tok = Tok->Next) {
+        if (Tok->isNot(tok::identifier))
+          continue;
+        auto *Next = Tok->Next;
+        const bool NextIsBinaryOperator =
+            Next && Next->isOneOf(tok::star, tok::amp, tok::ampamp) &&
+            Next->Next && Next->Next->is(tok::identifier);
+        if (!NextIsBinaryOperator)
+          continue;
+        Next->setType(TT_BinaryOperator);
+        Tok = Next;
+      }
+    }
+  }
+
   while (Current) {
     const FormatToken *Prev = Current->Previous;
     if (Current->is(TT_LineComment)) {
       if (Prev->is(BK_BracedInit) && Prev->opensScope()) {
         Current->SpacesRequiredBefore =
             (Style.Cpp11BracedListStyle && !Style.SpacesInParentheses) ? 0 : 1;
       } else if (Prev->is(TT_VerilogMultiLineListLParen)) {
         Current->SpacesRequiredBefore = 0;

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ("#define OP(x)                                    \\\n"
             "    return s << a.DebugString();                 \\\n"
             "  }",
             format("#define OP(x) \\\n"
                    "  ostream &operator<<(ostream &s, const A &a) { \\\n"
                    "    return s << a.DebugString(); \\\n"
                    "  }",
                    getLLVMStyleWithColumns(50)));
 
-// FIXME: We should be able to figure out this is an operator call
-#if 0
   verifyFormat("#define FOO             \\\n"
                "  void foo() {          \\\n"
                "    operator+(a * b);   \\\n"
-               "  }", getLLVMStyleWithColumns(25));
-#endif
+               "  }",
+               getLLVMStyleWithColumns(25));
 
   // FIXME: We cannot handle this case yet; we might be able to figure out that
   // foo<x> d > v; doesn't make sense.
   verifyFormat("foo<a<b && c> d> v;");
 
   FormatStyle PointerMiddle = getLLVMStyle();
   PointerMiddle.PointerAlignment = FormatStyle::PAS_Middle;
   verifyFormat("delete *x;", PointerMiddle);

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsFunctionRefQualifiers) {
   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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   // 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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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;
-  // FIXME
-  // EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   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;
-  // FIXME
-  // EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
   EXPECT_TOKEN(Tokens[3], tok::string_literal, TT_OverloadedOperator);
   EXPECT_TOKEN(Tokens[5], tok::l_paren, TT_OverloadedOperatorLParen);
 
   Tokens = annotate("int operator+(int);");
   ASSERT_EQ(Tokens.size(), 8u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::kw_operator, TT_FunctionDeclarationName);
   EXPECT_TOKEN(Tokens[2], tok::plus, TT_OverloadedOperator);
   EXPECT_TOKEN(Tokens[3], tok::l_paren, TT_OverloadedOperatorLParen);
Tokens = annotate("class Foo {\n"
                     "  int operator+(a* b);\n"
                     "}");
   ASSERT_EQ(Tokens.size(), 14u) << Tokens;
   EXPECT_TOKEN(Tokens[4], tok::kw_operator, TT_FunctionDeclarationName);
   EXPECT_TOKEN(Tokens[5], tok::plus, TT_OverloadedOperator);
   EXPECT_TOKEN(Tokens[6], tok::l_paren, TT_OverloadedOperatorLParen);
   EXPECT_TOKEN(Tokens[8], tok::star, TT_PointerOrReference);
 
+  Tokens = annotate("class Foo {\n"
+                    "  int c = operator+(a * b);\n"
+                    "}");
+  ASSERT_EQ(Tokens.size(), 16u) << Tokens;
+  EXPECT_TOKEN(Tokens[6], tok::kw_operator, TT_Unknown);
+  EXPECT_TOKEN(Tokens[7], tok::plus, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[8], tok::l_paren, TT_OverloadedOperatorLParen);
+  EXPECT_TOKEN(Tokens[10], tok::star, TT_BinaryOperator);
+
   Tokens = annotate("void foo() {\n"
                     "  operator+(a * b);\n"
                     "}");
   ASSERT_EQ(Tokens.size(), 15u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw_operator, TT_Unknown);
   EXPECT_TOKEN(Tokens[6], tok::plus, TT_OverloadedOperator);
   EXPECT_TOKEN(Tokens[7], tok::l_paren, TT_OverloadedOperatorLParen);
   EXPECT_TOKEN(Tokens[9], tok::star, TT_BinaryOperator);
 
+  Tokens = annotate("return operator+(a * b, c & d) + operator+(a && b && c);");
+  ASSERT_EQ(Tokens.size(), 24u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::kw_operator, TT_Unknown);
+  EXPECT_TOKEN(Tokens[2], tok::plus, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[3], tok::l_paren, TT_OverloadedOperatorLParen);
+  EXPECT_TOKEN(Tokens[5], tok::star, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[9], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[13], tok::kw_operator, TT_Unknown);
+  EXPECT_TOKEN(Tokens[14], tok::plus, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[15], tok::l_paren, TT_OverloadedOperatorLParen);
+  EXPECT_TOKEN(Tokens[17], tok::ampamp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[19], tok::ampamp, TT_BinaryOperator);
+
   Tokens = annotate("class Foo {\n"
                     "  void foo() {\n"
                     "    operator+(a * b);\n"
                     "  }\n"
                     "}");
   ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+  EXPECT_TOKEN(Tokens[8], tok::kw_operator, TT_Unknown);
   EXPECT_TOKEN(Tokens[9], tok::plus, TT_OverloadedOperator);
   EXPECT_TOKEN(Tokens[10], tok::l_paren, TT_OverloadedOperatorLParen);
   EXPECT_TOKEN(Tokens[12], tok::star, TT_BinaryOperator);
 }
 
 TEST_F(TokenAnnotatorTest, OverloadedOperatorInTemplate) {
   struct {
     const char *Text;