[clang-format] Add a space between an overloaded operator and '>'

[owenpan]

Summary

The token annotator doesn't annotate the template opener and closer as such if they enclose an overloaded operator. This causes the space between the operator and the closer to be removed, resulting
in invalid C++ code.

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

[vedgy]

Hi @owenpan. Thank you for fixing this bug!
Have you noticed this paragraph in my bug report?

I believe clang_getTypeSpelling(), or more likely QualType::print() used by it, should insert a tab character between such tokens to pretty-print compilable code. The tab character is preferable to the space character here, because the users may rely on the fact that pretty-printed binary operators are surrounded by spaces to distinguish them from angle brackets.

KDevelop parses the result of clang_getTypeSpelling() when libclang API is lacking. Since this recent commit KDevelop's parsing relies on the empirical fact that only operators are surrounded by spaces to distinguish them from angle brackets. Does this revision introduce angle brackets surrounded by spaces? Can tab characters be used instead? If not, do you know how else such angle brackets can be distinguished from operators?

[owenpan]

Added tryMergeGreaterGreater() to and fixed tryMergerLessLess() of FormatTokenLexer.

[owenpan]

In D143755#4118802, @vedgy wrote:

Hi @owenpan. Thank you for fixing this bug!
Have you noticed this paragraph in my bug report?

I believe clang_getTypeSpelling(), or more likely QualType::print() used by it, should insert a tab character between such tokens to pretty-print compilable code. The tab character is preferable to the space character here, because the users may rely on the fact that pretty-printed binary operators are surrounded by spaces to distinguish them from angle brackets.

Yes, but I'm not familiar with these functions.

KDevelop parses the result of clang_getTypeSpelling() when libclang API is lacking. Since this recent commit KDevelop's parsing relies on the empirical fact that only operators are surrounded by spaces to distinguish them from angle brackets. Does this revision introduce angle brackets surrounded by spaces? Can tab characters be used instead? If not, do you know how else such angle brackets can be distinguished from operators?

This patch should fix the reported invalid-code-generation bug by leaving a space between the overloaded operator and the closing angle bracket. clang-format doesn't insert tabs in the middle of a line (except when doing alignment if UseTab is set to Always).

[MyDeveloperDay]

LGTM

[kadircet]

Hi @owenpan, this seems to be crashing for:

struct Foo { operator enum foo{} };

with stack trace:

$ ~/repos/llvm/build/bin/clang-format format_crash.cc --dry-run
clang-format: /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/TokenAnnotator.cpp:1229: bool clang::format::(anonymous namespace)::AnnotatingParser::consumeToken(): Assertion `CurrentToken' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /usr/local/google/home/kadircet/repos/llvm/build/bin/clang-format format_crash.cc --dry-run
 #0 0x0000000000397687 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Unix/Signals.inc:567:13
 #1 0x000000000039583e llvm::sys::RunSignalHandlers() /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Signals.cpp:105:18
 #2 0x0000000000397faa SignalHandler(int) /usr/local/google/home/kadircet/repos/llvm/llvm/lib/Support/Unix/Signals.inc:412:1
 #3 0x00007f49d145af90 (/lib/x86_64-linux-gnu/libc.so.6+0x3bf90)
 #4 0x00007f49d14a9ccc __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x00007f49d145aef2 raise ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x00007f49d1445472 abort ./stdlib/abort.c:81:7
 #7 0x00007f49d1445395 _nl_load_domain ./intl/loadmsgcat.c:1177:9
 #8 0x00007f49d1453df2 (/lib/x86_64-linux-gnu/libc.so.6+0x34df2)
 #9 0x000000000043a7c1 parseBrace /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/TokenAnnotator.cpp:867:9
#10 0x000000000043a7c1 clang::format::(anonymous namespace)::AnnotatingParser::consumeToken() /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/TokenAnnotator.cpp:1170:12
#11 0x000000000042e1f8 clang::format::(anonymous namespace)::AnnotatingParser::parseLine() /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/TokenAnnotator.cpp:1558:11
#12 0x000000000042dc37 clang::format::TokenAnnotator::annotate(clang::format::AnnotatedLine&) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/TokenAnnotator.cpp:2837:13
#13 0x000000000041b910 clang::format::TokenAnalyzer::process(bool) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/TokenAnalyzer.cpp:0:19
#14 0x00000000003d0576 ~TokenAnalyzer /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/TokenAnalyzer.h:88:7
#15 0x00000000003d0576 operator() /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/Format.cpp:3492:9
#16 0x00000000003d0576 __invoke_impl<std::pair<clang::tooling::Replacements, unsigned int>, (lambda at /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/Format.cpp:3491:27) &, const clang::format::Environment &> /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:61:14
#17 0x00000000003d0576 __invoke_r<std::pair<clang::tooling::Replacements, unsigned int>, (lambda at /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/Format.cpp:3491:27) &, const clang::format::Environment &> /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/invoke.h:114:9
#18 0x00000000003d0576 std::_Function_handler<std::pair<clang::tooling::Replacements, unsigned int> (clang::format::Environment const&), clang::format::internal::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus*)::$_8>::_M_invoke(std::_Any_data const&, clang::format::Environment const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_function.h:290:9
#19 0x00000000003b8a18 _M_is_engaged /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/optional:471:58
#20 0x00000000003b8a18 operator bool /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/optional:985:22
#21 0x00000000003b8a18 clang::format::internal::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, unsigned int, unsigned int, unsigned int, llvm::StringRef, clang::format::FormattingAttemptStatus*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/Format.cpp:3533:9
#22 0x00000000003b9c73 _Rb_tree_impl /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_tree.h:687:12
#23 0x00000000003b9c73 _Rb_tree /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_tree.h:954:7
#24 0x00000000003b9c73 set /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/stl_set.h:231:7
#25 0x00000000003b9c73 Replacements /usr/local/google/home/kadircet/repos/llvm/clang/include/clang/Tooling/Core/Replacement.h:212:7
#26 0x00000000003b9c73 clang::format::reformat(clang::format::FormatStyle const&, llvm::StringRef, llvm::ArrayRef<clang::tooling::Range>, llvm::StringRef, clang::format::FormattingAttemptStatus*) /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/Format.cpp:3557:10
#27 0x000000000033da7f clang::format::format(llvm::StringRef) /usr/local/google/home/kadircet/repos/llvm/clang/tools/clang-format/ClangFormat.cpp:0:7
#28 0x000000000033c0a5 main /usr/local/google/home/kadircet/repos/llvm/clang/tools/clang-format/ClangFormat.cpp:631:14
#29 0x00007f49d144618a __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#30 0x00007f49d1446245 call_init ./csu/../csu/libc-start.c:128:20
#31 0x00007f49d1446245 __libc_start_main ./csu/../csu/libc-start.c:368:5
#32 0x000000000033ac71 _start (/usr/local/google/home/kadircet/repos/llvm/build/bin/clang-format+0x33ac71)
Aborted

[owenpan]

@kadircet thanks for reporting the crash and reverting the commit. I will fix it and reland the patch.

clang/lib/Format/TokenAnnotator.cpp

1229

After next() or consumeToken(), CurrentToken would advance and could become null.

clang/lib/Format/FormatToken.h

#include "clang/Basic/TransformTypeTraits.def"
   [[nodiscard]] FormatToken *getPreviousNonComment() const {
     FormatToken *Tok = Previous;
     while (Tok && Tok->is(tok::comment))
       Tok = Tok->Previous;
     return Tok;
   }
 
   /// Returns the next token ignoring comments.
-  [[nodiscard]] const FormatToken *getNextNonComment() const {
-    const FormatToken *Tok = Next;
+  [[nodiscard]] FormatToken *getNextNonComment() const {
+    FormatToken *Tok = Next;
     while (Tok && Tok->is(tok::comment))
       Tok = Tok->Next;
     return Tok;
   }
 
   /// Returns \c true if this tokens starts a block-type list, i.e. a
   /// list that should be indented with a block indent.
   [[nodiscard]] bool opensBlockOrBlockTypeList(const FormatStyle &Style) const;

clang/lib/Format/FormatTokenLexer.h

public:
   ArrayRef<FormatToken *> lex();
 
   const AdditionalKeywords &getKeywords() { return Keywords; }
 
 private:
   void tryMergePreviousTokens();
 
   bool tryMergeLessLess();
+  bool tryMergeGreaterGreater();
   bool tryMergeNSStringLiteral();
   bool tryMergeJSPrivateIdentifier();
   bool tryMergeCSharpStringLiteral();
   bool tryMergeCSharpKeywordVariables();
   bool tryMergeNullishCoalescingEqual();
   bool tryTransformCSharpForEach();
   bool tryMergeForEach();
   bool tryTransformTryUsageForC();

clang/lib/Format/FormatTokenLexer.cpp

 
 void FormatTokenLexer::tryMergePreviousTokens() {
   if (tryMerge_TMacro())
     return;
   if (tryMergeConflictMarkers())
     return;
   if (tryMergeLessLess())
     return;
+  if (tryMergeGreaterGreater())
+    return;
   if (tryMergeForEach())
     return;
   if (Style.isCpp() && tryTransformTryUsageForC())
     return;
 
   if (Style.isJavaScript() || Style.isCSharp()) {
     static const tok::TokenKind NullishCoalescingOperator[] = {tok::question,
                                                                tok::question};
bool FormatTokenLexer::tryMergeLessLess() {
   if (First[0]->isNot(tok::less) || First[1]->isNot(tok::less))
     return false;
 
   // Only merge if there currently is no whitespace between the two "<".
   if (First[1]->hasWhitespaceBefore())
     return false;
 
   auto X = Tokens.size() > 3 ? First[-1] : nullptr;
-  auto Y = First[2];
-  if ((X && X->is(tok::less)) || Y->is(tok::less))
+  if (X && X->is(tok::less))
     return false;
 
-  // Do not remove a whitespace between the two "<" e.g. "operator< <>".
-  if (X && X->is(tok::kw_operator) && Y->is(tok::greater))
+  auto Y = First[2];
+  if ((!X || X->isNot(tok::kw_operator)) && Y->is(tok::less))
     return false;
 
   First[0]->Tok.setKind(tok::lessless);
   First[0]->TokenText = "<<";
   First[0]->ColumnWidth += 1;
   Tokens.erase(Tokens.end() - 2);
   return true;
 }
 
+bool FormatTokenLexer::tryMergeGreaterGreater() {
+  // Merge kw_operator,greater,greater into kw_operator,greatergreater.
+  if (Tokens.size() < 2)
+    return false;
+
+  auto First = Tokens.end() - 2;
+  if (First[0]->isNot(tok::greater) || First[1]->isNot(tok::greater))
+    return false;
+
+  // Only merge if there currently is no whitespace between the first two ">".
+  if (First[1]->hasWhitespaceBefore())
+    return false;
+
+  auto Tok = Tokens.size() > 2 ? First[-1] : nullptr;
+  if (Tok && Tok->isNot(tok::kw_operator))
+    return false;
+
+  First[0]->Tok.setKind(tok::greatergreater);
+  First[0]->TokenText = ">>";
+  First[0]->ColumnWidth += 1;
+  Tokens.erase(Tokens.end() - 1);
+  return true;
+}
+
 bool FormatTokenLexer::tryMergeTokens(ArrayRef<tok::TokenKind> Kinds,
                                       TokenType NewType) {
   if (Tokens.size() < Kinds.size())
     return false;
 
   SmallVectorImpl<FormatToken *>::const_iterator First =
       Tokens.end() - Kinds.size();
   for (unsigned i = 0; i < Kinds.size(); ++i)

clang/lib/Format/TokenAnnotator.cpp

case tok::kw_operator:
       if (Style.Language == FormatStyle::LK_TextProto ||
           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);
+        auto Next = CurrentToken->getNextNonComment();
+        if (!Next)
+          break;
+        if (Next->is(tok::less))
+          next();
+        else
           consumeToken();
-        if (!CurrentToken)
-          continue;
-        if (CurrentToken->is(tok::comma) &&
-            CurrentToken->Previous->isNot(tok::kw_operator)) {
+        assert(CurrentToken);
+        auto Previous = CurrentToken->getPreviousNonComment();
+        assert(Previous);
+        if (CurrentToken->is(tok::comma) && Previous->isNot(tok::kw_operator))
           break;
-        }
-        if (CurrentToken->Previous->isOneOf(TT_BinaryOperator, TT_UnaryOperator,
-                                            tok::comma, tok::star, tok::arrow,
-                                            tok::amp, tok::ampamp) ||
+        if (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);
+            Previous->TokenText.startswith("\"\"")) {
+          Previous->setType(TT_OverloadedOperator);
+          if (CurrentToken->isOneOf(tok::less, tok::greater))
+            break;
         }
       }
       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 (Keywords.isWordLike(Right) && Keywords.isWordLike(Left))
     return true;
 
   // Leave a space between * and /* to avoid C4138 `comment end` found outside
   // of comment.
   if (Left.is(tok::star) && Right.is(tok::comment))
     return true;
 
   if (Style.isCpp()) {
+    if (Left.is(TT_OverloadedOperator) &&
+        Right.isOneOf(TT_TemplateOpener, TT_TemplateCloser)) {
+      return true;
+    }
     // Space between UDL and dot: auto b = 4s .count();
     if (Right.is(tok::period) && Left.is(tok::numeric_constant))
       return true;
     // Space between import <iostream>.
     // or import .....;
     if (Left.is(Keywords.kw_import) && Right.isOneOf(tok::less, tok::ellipsis))
       return true;
     // Space between `module :` and `import :`.

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsOverloadedOperators) {
   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, SpaceBeforeTemplateCloser) {
+  verifyFormat("C<&operator- > minus;");
+  verifyFormat("C<&operator> > gt;");
+  verifyFormat("C<&operator>= > ge;");
+  verifyFormat("C<&operator<= > le;");
+  verifyFormat("C<&operator< <X>> lt;");
+}
+
 TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
   verifyFormat("void A::b() && {}");
   verifyFormat("void A::b() && noexcept {}");
   verifyFormat("Deleted &operator=(const Deleted &) & = default;");
   verifyFormat("Deleted &operator=(const Deleted &) && = delete;");
   verifyFormat("Deleted &operator=(const Deleted &) & noexcept = default;");
   verifyFormat("SomeType MemberFunction(const Deleted &) & = delete;");
   verifyFormat("SomeType MemberFunction(const Deleted &) && = delete;");

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsOverloadedOperators) {
   Tokens = annotate("auto operator()() {}");
   ASSERT_EQ(Tokens.size(), 9u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::kw_operator, TT_FunctionDeclarationName);
   EXPECT_TOKEN(Tokens[2], tok::l_paren, TT_OverloadedOperator);
   EXPECT_TOKEN(Tokens[3], tok::r_paren, TT_OverloadedOperator);
   EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_OverloadedOperatorLParen);
 }
 
+TEST_F(TokenAnnotatorTest, OverloadedOperatorInTemplate) {
+  struct {
+    const char *Text;
+    tok::TokenKind Kind;
+  } Operators[] = {{"+", tok::plus},
+                   {"-", tok::minus},
+                   // FIXME:
+                   // {"*", tok::star},
+                   {"/", tok::slash},
+                   {"%", tok::percent},
+                   {"^", tok::caret},
+                   // FIXME:
+                   // {"&", tok::amp},
+                   {"|", tok::pipe},
+                   {"~", tok::tilde},
+                   {"!", tok::exclaim},
+                   {"=", tok::equal},
+                   // FIXME:
+                   // {"<", tok::less},
+                   {">", tok::greater},
+                   {"+=", tok::plusequal},
+                   {"-=", tok::minusequal},
+                   {"*=", tok::starequal},
+                   {"/=", tok::slashequal},
+                   {"%=", tok::percentequal},
+                   {"^=", tok::caretequal},
+                   {"&=", tok::ampequal},
+                   {"|=", tok::pipeequal},
+                   {"<<", tok::lessless},
+                   {">>", tok::greatergreater},
+                   {">>=", tok::greatergreaterequal},
+                   {"<<=", tok::lesslessequal},
+                   {"==", tok::equalequal},
+                   {"!=", tok::exclaimequal},
+                   {"<=", tok::lessequal},
+                   {">=", tok::greaterequal},
+                   {"<=>", tok::spaceship},
+                   {"&&", tok::ampamp},
+                   {"||", tok::pipepipe},
+                   {"++", tok::plusplus},
+                   {"--", tok::minusminus},
+                   {",", tok::comma},
+                   {"->*", tok::arrowstar},
+                   {"->", tok::arrow}};
+
+  for (const auto &Operator : Operators) {
+    std::string Input("C<&operator");
+    Input += Operator.Text;
+    Input += " > a;";
+    auto Tokens = annotate(std::string(Input));
+    ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+    EXPECT_TOKEN(Tokens[1], tok::less, TT_TemplateOpener);
+    EXPECT_TOKEN(Tokens[4], Operator.Kind, TT_OverloadedOperator);
+    EXPECT_TOKEN(Tokens[5], tok::greater, TT_TemplateCloser);
+  }
+
+  auto Tokens = annotate("C<&operator< <X>> lt;");
+  ASSERT_EQ(Tokens.size(), 12u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[4], tok::less, TT_OverloadedOperator);
+  EXPECT_TOKEN(Tokens[5], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[7], tok::greater, TT_TemplateCloser);
+  EXPECT_TOKEN(Tokens[8], tok::greater, TT_TemplateCloser);
+}
+
 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);