[clang-format] [PR35518] C++17 deduction guides are wrongly formatted

[MyDeveloperDay]

Summary

see https://bugs.llvm.org/show_bug.cgi?id=35518

clang-format removes spaces around deduction guides but not trailing return types, make the consistent

template <typename T> S(T)->S<T>;
auto f(int, int) -> double;

becomes

template <typename T> S(T) -> S<T>;
auto f(int, int) -> double;

[lichray]

Should we find a way to set ->'s type to TT_TrailingReturnArrow?

[MyDeveloperDay]

In D69577#1726587, @lichray wrote:

Should we find a way to set ->'s type to TT_TrailingReturnArrow?

that's possible then we might be able to use the existing spaces before rule

if (Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow) ||
    Left.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow))
  return true;

[MyDeveloperDay]

Detect deduction guides arrow as a TrailingReturn arrow, allowing use of existing space before/after rules

[klimek]

clang/lib/Format/TokenAnnotator.cpp

1400–1403

Why doesn't this trigger on function templates:

c()->f<int>();

[lichray]

clang/lib/Format/TokenAnnotator.cpp

1400

Comparing to the else if branch above, several questions can arise:

1. Has deduction-guide be considered a declaration (it is, of course, in standard)? If yes, without MustBeDeclaration, how x = p->foo<3>(); being formatted?
2. Without restrictions on NestingLevel, how A() -> A<decltype(p->foo<3>())>; being formatted?
3. How x()->foo<1>; being formatted? What's the difference between this and a deduction-guide? A deduction-guide has to follow TheSameType(...) -> TheSameType<....>; and appears only at namespace level, do these help?

Oh no, auto x = p -> foo<1>(); this is a bug (I will look for bug reports, don't mind).

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

1400–1403

This case I agree is wrong (but that comes from the existing rule no?)

auto x = p -> foo<1>();

The other examples are currently as follows

c()->f<int>();
x()->foo<1>;
x = p->foo<3>();
A()->A<decltype(p->foo<3>())>;

This is how they look from the last build @hans made on the 17th October

auto x = p -> foo<1>();
c()->f<int>();
x()->foo<1>;
x = p->foo<3>();
A()->A<decltype(p->foo<3>())>;

Debug info

----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=auto L=4 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x2c52129f718 Text='auto'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=220 Name=identifier L=6 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2da8 Text='x'
 M=0 C=0 T=BinaryOperator S=1 B=0 BK=0 P=22 Name=equal L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=22 Name=identifier L=10 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2dd8 Text='p'
 M=0 C=1 T=TrailingReturnArrow S=1 B=0 BK=0 P=23 Name=arrow L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=23 Name=identifier L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d78 Text='foo'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=18 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=numeric_constant L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='1'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=l_paren L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=59 Name=r_paren L=22 PPK=2 FakeLParens= FakeRParens=2 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=23 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2e08 Text='c'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=140 Name=r_paren L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=170 Name=arrow L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=TrailingAnnotation S=0 B=0 BK=0 P=190 Name=identifier L=6 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d48 Text='f'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=int L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x2c52129fa50 Text='int'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=l_paren L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=140 Name=r_paren L=13 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2da8 Text='x'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=140 Name=r_paren L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=170 Name=arrow L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=TrailingAnnotation S=0 B=0 BK=0 P=190 Name=identifier L=8 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d78 Text='foo'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=9 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=numeric_constant L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='1'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=11 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=semi L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x2c5212a2da8 Text='x'
 M=0 C=0 T=BinaryOperator S=1 B=0 BK=0 P=22 Name=equal L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=22 Name=identifier L=5 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2dd8 Text='p'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=170 Name=arrow L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=identifier L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d78 Text='foo'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=numeric_constant L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='3'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=13 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=l_paren L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=59 Name=r_paren L=15 PPK=2 FakeLParens= FakeRParens=2 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=16 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----
AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=identifier L=1 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2cb8 Text='A'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=2 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=140 Name=r_paren L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=170 Name=arrow L=5 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=TrailingAnnotation S=0 B=0 BK=0 P=190 Name=identifier L=6 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2cb8 Text='A'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=30 Name=less L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=360 Name=decltype L=15 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a09f8 Text='decltype'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=263 Name=l_paren L=16 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=380 Name=identifier L=17 PPK=2 FakeLParens=0/ FakeRParens=0 II=0x2c5212a2dd8 Text='p'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=430 Name=arrow L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='->'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=283 Name=identifier L=22 PPK=2 FakeLParens= FakeRParens=0 II=0x2c5212a2d78 Text='foo'
 M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=290 Name=less L=23 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=620 Name=numeric_constant L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='3'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=530 Name=greater L=25 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=290 Name=l_paren L=26 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=400 Name=r_paren L=27 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=283 Name=r_paren L=28 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=270 Name=greater L=29 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text='>'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=30 Name=semi L=30 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

I should mention all existing unit tests pass, Beyonce rule!

[MyDeveloperDay]

Sorry ignore my last results, let me rerun, I was using the revision without the change.

Here is what they'll look like:

auto x = p -> foo<1>();
c() -> f<int>();
x() -> foo<1>;
x = p->foo<3>();
A() -> A<decltype(p->foo<3>())>;

[lichray]

clang/lib/Format/TokenAnnotator.cpp

1400–1403

New bug created: https://bugs.llvm.org/show_bug.cgi?id=43851

[MyDeveloperDay]

move detection of deduction guides into a function, add additional negative tests

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

1400–1403

let us cover that separately, as the logic for that is in the else if clause above and the bug is present prior to this revision.

[lichray]

clang/lib/Format/TokenAnnotator.cpp

1354

Parentheses not matching is comment.

1365

Does this work? What about A() -> A<(3 < 2)>;?

1366

What's this for? What about A() -> A<sizeof(p->foo<1>>);

I guess we don't have to look for the end of template, because a class X can't refer to its member called X with -> because that member is constructor. The approach to match the identifiers may not work in the future but may work fine for now.

clang/unittests/Format/FormatTest.cpp

4992

This looks like a deduction guide. It should be formatted as A() -> A<decltype(p->foo<3>())>; I assume? If you cannot cover this case in this patch, this test case needs a comment.

[MyDeveloperDay]

Address review comments, deduction guides with embedded parens

[lichray]

The functionality looks acceptable. Trying to parse the whole thing still looks fragile to me. I expect code owner to take a look at this change.

clang/lib/Format/TokenAnnotator.cpp

1371

Maybe make use of some TT_TemplateOpener?

clang/unittests/Format/FormatTest.cpp

4987

Does A() -> A<decltype(foo<traits<1>>)> (C++11 >>) work?

[MyDeveloperDay]

Add additional test case

clang/lib/Format/TokenAnnotator.cpp

1371

We can't use TT_TemplateOpener because like MatchParen it hasn't been set yet on the downstream tokens

clang/unittests/Format/FormatTest.cpp

4987

this should work because we are skipping everything in between the (....)

[curdeius]

Minor comments.

clang/lib/Format/TokenAnnotator.cpp

1354

Please write full-phrase comments.

clang/unittests/Format/FormatTest.cpp

4995

What about:

verifyFormat("x()->x<1>;");

i.e. a function x returning a pointer to a class having a template member x (for instance a template variable).

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

4995

I'm unsure if we are the limit of being able to differentiate between the two?

[curdeius]

clang/unittests/Format/FormatTest.cpp

4995

Well, deduction guides always have a template parameter list before class name (unless I'm mistaken), so we should be able to distinguish the two.

[MyDeveloperDay]

Ensure the deduction guides follow a template
Add additional test cases raised during the reviews

[curdeius]

LGTM. Just a small typo in comment. Otherwise, great job!

clang/lib/Format/TokenAnnotator.cpp

1369

Typo: template.

[merge_guards_bot]

Build result: pass - 59867 tests passed, 0 failed and 768 were skipped.
Log files: console-log.txt, CMakeCache.txt

clang/lib/Format/TokenAnnotator.cpp

if (Current.getPrecedence() == prec::Assignment &&
       Contexts.back().CanBeExpression = false;
     } else if (Current.isOneOf(tok::semi, tok::exclaim)) {
       // This should be the condition or increment in a for-loop.
       Contexts.back().IsExpression = true;
     }
   }
 
   void determineTokenType(FormatToken &Current) {
     if (!Current.is(TT_Unknown))
       // The token type is already known.
       return;
 
     if (Style.Language == FormatStyle::LK_JavaScript) {
       if (Current.is(tok::exclaim)) {
         if (Current.Previous &&
             (Current.Previous->isOneOf(tok::identifier, tok::kw_namespace,
                                        tok::r_paren, tok::r_square,
                                        tok::r_brace) ||
              Current.Previous->Tok.isLiteral())) {
           Current.Type = TT_JsNonNullAssertion;
           return;
         }
         if (Current.Next &&
             Current.Next->isOneOf(TT_BinaryOperator, Keywords.kw_as)) {
           Current.Type = TT_JsNonNullAssertion;
           return;
         }
       }
     }
 
     // Line.MightBeFunctionDecl can only be true after the parentheses of a
     // function declaration have been found. In this case, 'Current' is a
     // trailing token of this declaration and thus cannot be a name.
     if (Current.is(Keywords.kw_instanceof)) {
void determineTokenType(FormatToken &Current) {
     } else if (Current.is(tok::arrow) &&
                Style.Language == FormatStyle::LK_Java) {
       Current.Type = TT_LambdaArrow;
     } else if (Current.is(tok::arrow) && AutoFound && Line.MustBeDeclaration &&
                Current.NestingLevel == 0 &&
                !Current.Previous->is(tok::kw_operator)) {
       // not auto operator->() -> xxx;
       Current.Type = TT_TrailingReturnArrow;
     } else if (Current.isOneOf(tok::star, tok::amp, tok::ampamp)) {
       Current.Type = determineStarAmpUsage(Current,
                                            Contexts.back().CanBeExpression &&
                                                Contexts.back().IsExpression,
                                            Contexts.back().InTemplateArgument);
     } else if (Current.isOneOf(tok::minus, tok::plus, tok::caret)) {
       Current.Type = determinePlusMinusCaretUsage(Current);
       if (Current.is(TT_UnaryOperator) && Current.is(tok::caret))
         Contexts.back().CaretFound = true;
     } else if (Current.isOneOf(tok::minusminus, tok::plusplus)) {
       Current.Type = determineIncrementUsage(Current);
     } else if (Current.isOneOf(tok::exclaim, tok::tilde)) {
bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
 
   if (Left.is(tok::greater) && Right.is(tok::greater)) {
     if (Style.Language == FormatStyle::LK_TextProto ||
         (Style.Language == FormatStyle::LK_Proto && Left.is(TT_DictLiteral)))
       return !Style.Cpp11BracedListStyle;
     return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
            (Style.Standard < FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
   }
+
+  // Deduction guides add a space around the arrow  "...) -> A<T>".
+  if (Left.is(tok::r_paren) &&
+      Right.startsSequence(tok::arrow, TT_TrailingAnnotation,
+                           TT_TemplateOpener))
+    return true;
+  if (Left.is(tok::arrow) &&
+      Right.startsSequence(TT_TrailingAnnotation, TT_TemplateOpener))
+    return true;
+
   if (Right.isOneOf(tok::arrow, tok::arrowstar, tok::periodstar) ||
       Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
       (Right.is(tok::period) && Right.isNot(TT_DesignatedInitializerPeriod)))
     return false;
   if (!Style.SpaceBeforeAssignmentOperators && Left.isNot(TT_TemplateCloser) &&
       Right.getPrecedence() == prec::Assignment)
     return false;
   if (Style.Language == FormatStyle::LK_Java && Right.is(tok::coloncolon) &&

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, TrailingReturnType) {
   verifyFormat("template <typename T>\n"
                "auto aaaaaaaaaaaaaaaaaaaaaa(T t)\n"
                "    -> decltype(eaaaaaaaaaaaaaaa<T>(t.a).aaaaaaaa());");
 
   // Not trailing return types.
   verifyFormat("void f() { auto a = b->c(); }");
 }
 
+TEST_F(FormatTest, DeductionGuides) {
+  verifyFormat("template <class T> A(const T &, const T &) -> A<T &>;");
+  verifyFormat("template <class T> explicit A(T &, T &&) -> A<T>;");
+  verifyFormat("template <class... Ts> S(Ts...) -> S<Ts...>;");
+  verifyFormat(
+      "template <class... T>\n"
+      "array(T &&... t) -> array<std::common_type_t<T...>, sizeof...(T)>;");
+}
+
 TEST_F(FormatTest, BreaksFunctionDeclarationsWithTrailingTokens) {
   // Avoid breaking before trailing 'const' or other trailing annotations, if
   // they are not function-like.
   FormatStyle Style = getGoogleStyle();
   Style.ColumnLimit = 47;
   verifyFormat("void someLongFunction(\n"
                "    int someLoooooooooooooongParameter) const {\n}",
                getLLVMStyleWithColumns(47));
   verifyFormat("LoooooongReturnType\n"
                "someLoooooooongFunction() const {}",
                getLLVMStyleWithColumns(47));
   verifyFormat("LoooooongReturnType someLoooooooongFunction()\n"
                "    const {}",
                Style);
   verifyFormat("void SomeFunction(aaaaa aaaaaaaaaaaaaaaaaaaa,\n"