[clang-format] Disallow trailing return arrows to be operators

[rymiel]

Summary

In the following construction:
template <typename T> requires Foo<T> || Bar<T> auto func() -> int;

The -> of the trailing return type was actually considered as an
operator as part of the binary operation in the requires clause, with
the precedence level of PrecedenceArrowAndPeriod, leading to fake
parens being inserted in strange locations, that would never be closed.

Fixes one part of https://github.com/llvm/llvm-project/issues/56213
(the rest will probably be in a separate patch)

[MyDeveloperDay]

clang/unittests/Format/TokenAnnotatorTest.cpp

448

should you not check that Tokens[21] is a TT_TrailingReturnArrow?

[rymiel]

clang/unittests/Format/TokenAnnotatorTest.cpp

448

That's probably a good idea, thank you! (though the bug wasn't that -> was misannotated, it was still a trailing return arrow, it just caused fake parentheses to be inserted earlier on)

[rymiel]

Also test the annotated value of the arrow

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

2615

clang/unittests/Format/TokenAnnotatorTest.cpp

448

Can you add the test for ClosesRequiresClause?

[rymiel]

Address review comments

[owenpan]

Would you like to get commit access? See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

[rymiel]

In D134049#3798366, @owenpan wrote:

Would you like to get commit access? See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

I must humbly say that I do not think I qualify under "a track record of submitting high quality patches", I've only done incidental bug fixes that are in easily approachable places. Do you disagree with my self-assessment? (I also understand that I've made lots of extremely tiny patches that are probably a little annoying to manage)

[HazardyKnusperkeks]

In D134049#3798389, @rymiel wrote:

In D134049#3798366, @owenpan wrote:

Would you like to get commit access? See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

I must humbly say that I do not think I qualify under "a track record of submitting high quality patches", I've only done incidental bug fixes that are in easily approachable places. Do you disagree with my self-assessment? (I also understand that I've made lots of extremely tiny patches that are probably a little annoying to manage)

Yes you do, I had far less when I got my commit access.

[owenpan]

In D134049#3798389, @rymiel wrote:

In D134049#3798366, @owenpan wrote:

Would you like to get commit access? See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

I must humbly say that I do not think I qualify under "a track record of submitting high quality patches", I've only done incidental bug fixes that are in easily approachable places. Do you disagree with my self-assessment?

I think you should have commit access based on the patches you have submitted and wouldn't have asked otherwise.

(I also understand that I've made lots of extremely tiny patches that are probably a little annoying to manage)

Tiny patches, when appropriate, are actually more manageable. :)

[rymiel]

Thank you for the vote of confidence!! It went way more quickly than I expected; I now already have commit access!

[MyDeveloperDay]

@rymiel you definitely deserve the commit access: comments and initial investigations and possible bug locations in github issues, defect focused initial commits, way to go!, Welcome aboard..

clang/lib/Format/TokenAnnotator.cpp

if (Current) {
         return prec::Relational;
       }
       if (Style.isJavaScript() &&
           Current->isOneOf(Keywords.kw_in, Keywords.kw_as)) {
         return prec::Relational;
       }
       if (Current->is(TT_BinaryOperator) || Current->is(tok::comma))
         return Current->getPrecedence();
-      if (Current->isOneOf(tok::period, tok::arrow))
+      if (Current->isOneOf(tok::period, tok::arrow) &&
+          Current->isNot(TT_TrailingReturnArrow)) {
         return PrecedenceArrowAndPeriod;
+      }
       if ((Style.Language == FormatStyle::LK_Java || Style.isJavaScript()) &&
           Current->isOneOf(Keywords.kw_extends, Keywords.kw_implements,
                            Keywords.kw_throws)) {
         return 0;
       }
       // In Verilog case labels are not on separate lines straight out of
       // UnwrappedLineParser. The colon is not part of an expression.
       if (Style.isVerilog() && Current->is(tok::colon))

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsRequiresClausesAndConcepts) {
   EXPECT_TRUE(Tokens[33]->ClosesRequiresClause);
 
   Tokens = annotate("template <typename T>\n"
                     "void foo(T) noexcept requires Bar<T>;");
   ASSERT_EQ(Tokens.size(), 18u) << Tokens;
   EXPECT_TOKEN(Tokens[11], tok::kw_requires, TT_RequiresClause);
 
   Tokens = annotate("template <typename T>\n"
+                    "requires Bar<T> || Baz<T>\n"
+                    "auto foo(T) -> int;");
+  ASSERT_EQ(Tokens.size(), 24u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::kw_requires, TT_RequiresClause);
+  EXPECT_EQ(Tokens[11]->FakeLParens.size(), 0u);
+  EXPECT_TRUE(Tokens[14]->ClosesRequiresClause);
+  EXPECT_TOKEN(Tokens[20], tok::arrow, TT_TrailingReturnArrow);
+
+  Tokens = annotate("template <typename T>\n"
                     "struct S {\n"
                     "  void foo() const requires Bar<T>;\n"
                     "  void bar() const & requires Baz<T>;\n"
                     "  void bar() && requires Baz2<T>;\n"
                     "  void baz() const & noexcept requires Baz<T>;\n"
                     "  void baz() && noexcept requires Baz2<T>;\n"
                     "};\n"
                     "\n"