[clang-format] PR48916 PointerAlignment not working when using C++20 init-statement in for loop

[MyDeveloperDay]

Summary

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

Left and Right Alignment inside a loop is misaligned.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

3054

Is this valid code? Or did we just wrongly assign PointerOrReference? I'd say after that there can not be a literal in valid code, thus we do not need to handle it.

3067

Do we need this just for auto? What when auto is replaced with int?

3072

Drop the outer paren?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

3054

Ok so as you see I broken out the compound statement, to be honest I find these compound if's unreadable and I can't for the life of me work out what case they are trying to handle. (I want to do this more)..

Please no one say doing it separately is slower! without measuring it..we are talking ns difference if anything at all.

I agree I truly believe we are mis assigning PointerOrReference/BinaryOperator this as I mentioned in
https://lists.llvm.org/pipermail/cfe-dev/2021-December/069486.html is a major issue and I think from time to time these rules try to correct that situation, in this case & is the logical operation on a reference.

3067

Man! you right...duh how did I miss that...

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

3054

I'm with you to split up these monsters!

[curdeius]

clang/lib/Format/TokenAnnotator.cpp

3056

Block, not line, comment

clang/unittests/Format/FormatTest.cpp

1950

How about pointers/references in the init? Also, please test sth else than auto, both in init and as the loop variable.

[MyDeveloperDay]

Address review comments

[MyDeveloperDay]

This fix has to go super specific to range for loops or we start manipulating pointer alignment all over the place in places perhaps we don't want to

[HazardyKnusperkeks]

You have reverted some of your changes. Including all the tests.

[MyDeveloperDay]

Fix the merge and add the tests back (which missed the patch)

[MyDeveloperDay]

In D115050#3172842, @HazardyKnusperkeks wrote:

You have reverted some of your changes. Including all the tests.

Yes oops!

[curdeius]

clang/unittests/Format/FormatTest.cpp

1950

I think that you're still missing non-auto variable declarations in init. Also, you can add a statement not being a variable declaration in the init.

[curdeius]

Requesting changes so that appears correctly in the review queue.

[MyDeveloperDay]

Add some more unit tests

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

1950

I think he meant something like
for (foo(); auto c : ...)

[curdeius]

clang/unittests/Format/FormatTest.cpp

1950

Indeed.
I'd like to see tests like these (some of them are already there):

for (auto x = 0; auto& c : {1, 2, 3})
for (auto x = 0; int& c : {1, 2, 3})
for (int x = 0; auto& c : {1, 2, 3})
for (int x = 0; int& c : {1, 2, 3})
for (f(); auto& c : {1, 2, 3})
for (f(); int& c : {1, 2, 3})

[MyDeveloperDay]

Add more unit tests

[curdeius]

LGTM! Thanks.

clang/lib/Format/FormatToken.h

public:
   bool canBePointerOrReferenceQualifier() const {
     return isOneOf(tok::kw_const, tok::kw_restrict, tok::kw_volatile,
                    tok::kw___attribute, tok::kw__Nonnull, tok::kw__Nullable,
                    tok::kw__Null_unspecified, tok::kw___ptr32, tok::kw___ptr64,
                    TT_AttributeMacro);
   }
 
   /// Determine whether the token is a simple-type-specifier.
-  bool isSimpleTypeSpecifier() const;
+  LLVM_NODISCARD bool isSimpleTypeSpecifier() const;
+
+  LLVM_NODISCARD bool isTypeOrIdentifier() const;
 
   bool isObjCAccessSpecifier() const {
     return is(tok::at) && Next &&
            (Next->isObjCAtKeyword(tok::objc_public) ||
             Next->isObjCAtKeyword(tok::objc_protected) ||
             Next->isObjCAtKeyword(tok::objc_package) ||
             Next->isObjCAtKeyword(tok::objc_private));
   }

clang/lib/Format/FormatToken.cpp

bool FormatToken::isSimpleTypeSpecifier() const {
   case tok::kw_decltype:
   case tok::kw__Atomic:
     return true;
   default:
     return false;
   }
 }
 
+bool FormatToken::isTypeOrIdentifier() const {
+  return isSimpleTypeSpecifier() || Tok.isOneOf(tok::kw_auto, tok::identifier);
+}
+
 TokenRole::~TokenRole() {}
 
 void TokenRole::precomputeFormattingInfos(const FormatToken *Token) {}
 
 unsigned CommaSeparatedList::formatAfterToken(LineState &State,
                                               ContinuationIndenter *Indenter,
                                               bool DryRun) {
   if (State.NextToken == nullptr || !State.NextToken->Previous)

clang/lib/Format/TokenAnnotator.cpp

if (Right.is(TT_PointerOrReference)) {
     }
     // Add a space if the previous token is a pointer qualifier or the closing
     // parenthesis of __attribute__(()) expression and the style requires spaces
     // after pointer qualifiers.
     if ((Style.SpaceAroundPointerQualifiers == FormatStyle::SAPQ_After ||
          Style.SpaceAroundPointerQualifiers == FormatStyle::SAPQ_Both) &&
         (Left.is(TT_AttributeParen) || Left.canBePointerOrReferenceQualifier()))
       return true;
+    if (Left.Tok.isLiteral())
+      return true;
+    // for (auto a = 0, b = 0; const auto & c : {1, 2, 3})
+    if (Left.isTypeOrIdentifier() && Right.Next && Right.Next->Next &&
+        Right.Next->Next->is(TT_RangeBasedForLoopColon))
+      return getTokenPointerOrReferenceAlignment(Right) !=
+             FormatStyle::PAS_Left;
     return (
-        Left.Tok.isLiteral() ||
         (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
          (getTokenPointerOrReferenceAlignment(Right) != FormatStyle::PAS_Left ||
           (Line.IsMultiVariableDeclStmt &&
            (Left.NestingLevel == 0 ||
             (Left.NestingLevel == 1 && Line.First->is(tok::kw_for)))))));
   }
   if (Right.is(TT_FunctionTypeLParen) && Left.isNot(tok::l_paren) &&
       (!Left.is(TT_PointerOrReference) ||
        (getTokenPointerOrReferenceAlignment(Left) != FormatStyle::PAS_Right &&
         !Line.IsMultiVariableDeclStmt)))
     return true;
   if (Left.is(TT_PointerOrReference)) {
     // Add a space if the next token is a pointer qualifier and the style
     // requires spaces before pointer qualifiers.
     if ((Style.SpaceAroundPointerQualifiers == FormatStyle::SAPQ_Before ||
          Style.SpaceAroundPointerQualifiers == FormatStyle::SAPQ_Both) &&
         Right.canBePointerOrReferenceQualifier())
       return true;
-    return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
-           (Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
-            !Right.is(TT_StartOfName)) ||
-           (Right.is(tok::l_brace) && Right.is(BK_Block)) ||
-           (!Right.isOneOf(TT_PointerOrReference, TT_ArraySubscriptLSquare,
-                           tok::l_paren) &&
-            (getTokenPointerOrReferenceAlignment(Left) !=
-                 FormatStyle::PAS_Right &&
-             !Line.IsMultiVariableDeclStmt) &&
-            Left.Previous &&
-            !Left.Previous->isOneOf(tok::l_paren, tok::coloncolon,
-                                    tok::l_square));
+    // & 1
+    if (Right.Tok.isLiteral())
+      return true;
+    // & /* comment
+    if (Right.is(TT_BlockComment))
+      return true;
+    // foo() -> const Bar * override/final
+    if (Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
+        !Right.is(TT_StartOfName))
+      return true;
+    // & {
+    if (Right.is(tok::l_brace) && Right.is(BK_Block))
+      return true;
+    // for (auto a = 0, b = 0; const auto& c : {1, 2, 3})
+    if (Left.Previous && Left.Previous->isTypeOrIdentifier() && Right.Next &&
+        Right.Next->is(TT_RangeBasedForLoopColon))
+      return getTokenPointerOrReferenceAlignment(Left) !=
+             FormatStyle::PAS_Right;
+    return !Right.isOneOf(TT_PointerOrReference, TT_ArraySubscriptLSquare,
+                                   tok::l_square);
   }
   // Ensure right pointer alignment with ellipsis e.g. int *...P
   if (Left.is(tok::ellipsis) && Left.Previous &&
       Left.Previous->isOneOf(tok::star, tok::amp, tok::ampamp))
     return Style.PointerAlignment != FormatStyle::PAS_Right;
 
   if (Right.is(tok::star) && Left.is(tok::l_paren))
     return false;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, SeparatePointerReferenceAlignment) {
   verifyFormat("int *f1(int *a, int &b, int &&c);", Style);
   verifyFormat("int &f2(int &&c, int *a, int &b);", Style);
   verifyFormat("int &&f3(int &b, int &&c, int *a);", Style);
   verifyFormat("int *f1(int &a) const &;", Style);
   verifyFormat("int *f1(int &a) const & = 0;", Style);
   verifyFormat("int *a = f1();", Style);
   verifyFormat("int &b = f2();", Style);
   verifyFormat("int &&c = f3();", Style);
+  verifyFormat("for (auto a = 0, b = 0; const auto &c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const int &c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const Foo &c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const Foo *c : {1, 2, 3})", Style);
 
   Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
   verifyFormat("Const unsigned int *c;\n"
                "const unsigned int *d;\n"
                "Const unsigned int &e;\n"
                "const unsigned int &f;\n"
                "const unsigned    &&g;\n"
                "Const unsigned      h;",
                Style);
 
   Style.PointerAlignment = FormatStyle::PAS_Left;
   Style.ReferenceAlignment = FormatStyle::RAS_Pointer;
   verifyFormat("int* f1(int* a, int& b, int&& c);", Style);
   verifyFormat("int& f2(int&& c, int* a, int& b);", Style);
   verifyFormat("int&& f3(int& b, int&& c, int* a);", Style);
   verifyFormat("int* f1(int& a) const& = 0;", Style);
   verifyFormat("int* a = f1();", Style);
   verifyFormat("int& b = f2();", Style);
   verifyFormat("int&& c = f3();", Style);
+  verifyFormat("for (auto a = 0, b = 0; const auto& c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const int& c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const Foo& c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const Foo* c : {1, 2, 3})", Style);
 
   Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
   verifyFormat("Const unsigned int* c;\n"
                "const unsigned int* d;\n"
                "Const unsigned int& e;\n"
                "const unsigned int& f;\n"
                "const unsigned&&    g;\n"
                "Const unsigned      h;",
                Style);
 
   Style.PointerAlignment = FormatStyle::PAS_Right;
   Style.ReferenceAlignment = FormatStyle::RAS_Left;
   verifyFormat("int *f1(int *a, int& b, int&& c);", Style);
   verifyFormat("int& f2(int&& c, int *a, int& b);", Style);
   verifyFormat("int&& f3(int& b, int&& c, int *a);", Style);
   verifyFormat("int *a = f1();", Style);
   verifyFormat("int& b = f2();", Style);
   verifyFormat("int&& c = f3();", Style);
+  verifyFormat("for (auto a = 0, b = 0; const Foo *c : {1, 2, 3})", Style);
 
   Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
   verifyFormat("Const unsigned int *c;\n"
                "const unsigned int *d;\n"
                "Const unsigned int& e;\n"
                "const unsigned int& f;\n"
                "const unsigned      g;\n"
                "Const unsigned      h;",
                Style);
 
   Style.PointerAlignment = FormatStyle::PAS_Left;
   Style.ReferenceAlignment = FormatStyle::RAS_Middle;
   verifyFormat("int* f1(int* a, int & b, int && c);", Style);
   verifyFormat("int & f2(int && c, int* a, int & b);", Style);
   verifyFormat("int && f3(int & b, int && c, int* a);", Style);
   verifyFormat("int* a = f1();", Style);
   verifyFormat("int & b = f2();", Style);
   verifyFormat("int && c = f3();", Style);
+  verifyFormat("for (auto a = 0, b = 0; const auto & c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const int & c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const Foo & c : {1, 2, 3})", Style);
+  verifyFormat("for (auto a = 0, b = 0; const Foo* c : {1, 2, 3})", Style);
 
   Style.AlignConsecutiveDeclarations = FormatStyle::ACS_Consecutive;
   verifyFormat("Const unsigned int*  c;\n"
                "const unsigned int*  d;\n"
                "Const unsigned int & e;\n"
                "const unsigned int & f;\n"
                "const unsigned &&    g;\n"
                "Const unsigned       h;",
                Style);
 
   Style.PointerAlignment = FormatStyle::PAS_Middle;
   Style.ReferenceAlignment = FormatStyle::RAS_Right;
   verifyFormat("int * f1(int * a, int &b, int &&c);", Style);
   verifyFormat("int &f2(int &&c, int * a, int &b);", Style);
   verifyFormat("int &&f3(int &b, int &&c, int * a);", Style);
   verifyFormat("int * a = f1();", Style);
   verifyFormat("int &b = f2();", Style);
   verifyFormat("int &&c = f3();", Style);
+  verifyFormat("for (auto a = 0, b = 0; const Foo * c : {1, 2, 3})", Style);
 
   // FIXME: we don't handle this yet, so output may be arbitrary until it's
   // specifically handled
   // verifyFormat("int Add2(BTree * &Root, char * szToAdd)", Style);
 }
 
 TEST_F(FormatTest, FormatsForLoop) {
   verifyFormat(