[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

1464–1467

Why doesn't this trigger on function templates:

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

[lichray]

clang/lib/Format/TokenAnnotator.cpp

1464

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

1464–1467

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

1464–1467

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

1464–1467

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

void modifyContext(const FormatToken &Current) {
     } else if (Current.is(tok::kw_new)) {
       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;
     }
   }
 
+  static FormatToken *untilMatchingParen(FormatToken *Current) {
+    // Used when `MatchingParen` is not yet established.
+    int ParenLevel = 0;
+    while (Current) {
+      if (Current->is(tok::l_paren))
+        ParenLevel++;
+      if (Current->is(tok::r_paren))
+        ParenLevel--;
+      if (ParenLevel < 1)
+        break;
+      Current = Current->Next;
+    }
+    return Current;
+  }
+
+  static bool isDeductionGuide(FormatToken &Current) {
+    // Look for a deduction guide template<T> A(...) -> A<...>;
+    if (Current.Previous && Current.Previous->is(tok::r_paren) &&
+        Current.startsSequence(tok::arrow, tok::identifier, tok::less)) {
+      // Find the TemplateCloser.
+      FormatToken *TemplateCloser = Current.Next->Next;
+      int NestingLevel = 0;
+      while (TemplateCloser) {
+        // Skip over an expressions in parens  A<(3 < 2)>;
+        if (TemplateCloser->is(tok::l_paren)) {
+          // No Matching Paren yet so skip to matching paren
+          TemplateCloser = untilMatchingParen(TemplateCloser);
+        }
+        if (TemplateCloser->is(tok::less))
+          NestingLevel++;
+        if (TemplateCloser->is(tok::greater))
+          NestingLevel--;
+        if (NestingLevel < 1)
+          break;
+        TemplateCloser = TemplateCloser->Next;
+      }
+      // Assuming we have found the end of the template ensure its followed
+      // with a semi-colon.
+      if (TemplateCloser && TemplateCloser->Next &&
+          TemplateCloser->Next->is(tok::semi) &&
+          Current.Previous->MatchingParen) {
+        // Determine if the identifier `A` prior to the A<..>; is the same as
+        // prior to the A(..)
+        FormatToken *LeadingIdentifier =
+            Current.Previous->MatchingParen->Previous;
+
+        // Differentiate a deduction guide by seeing the
+        // > of the template prior to the leading identifier.
+        if (LeadingIdentifier) {
+          FormatToken *PriorLeadingIdentifier = LeadingIdentifier->Previous;
+          // Skip back past explicit decoration
+          if (PriorLeadingIdentifier &&
+              PriorLeadingIdentifier->is(tok::kw_explicit))
+            PriorLeadingIdentifier = PriorLeadingIdentifier->Previous;
+
+          return (PriorLeadingIdentifier &&
+                  PriorLeadingIdentifier->is(TT_TemplateCloser) &&
+                  LeadingIdentifier->TokenText == Current.Next->TokenText);
+        }
+      }
+    }
+    return false;
+  }
+
   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 &&
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 (isDeductionGuide(Current)) {
+      // Deduction guides trailing arrow " A(...) -> A<T>;".
+      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))

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)>;");
+  verifyFormat("template <class T> A() -> A<decltype(p->foo<3>())>;");
+  verifyFormat("template <class T> A() -> A<decltype(foo<traits<1>>)>;");
+  verifyFormat("template <class T> A() -> A<sizeof(p->foo<1>)>;");
+  verifyFormat("template <class T> A() -> A<(3 < 2)>;");
+  verifyFormat("template <class T> A() -> A<((3) < (2))>;");
+  verifyFormat("template <class T> x() -> x<1>;");
+  verifyFormat("template <class T> explicit x(T &) -> x<1>;");
+
+  // Ensure not deduction guides.
+  verifyFormat("c()->f<int>();");
+  verifyFormat("x()->foo<1>;");
+  verifyFormat("x = p->foo<3>();");
+  verifyFormat("x()->x<1>();");
+  verifyFormat("x()->x<1>;");
+}
+
 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));