[clang-format] Fix a crash (assertion) in qualifier alignment when matching template closer is null

[MyDeveloperDay]

Summary

https://github.com/llvm/llvm-project/issues/53008

template <class Id> using A = quantity /**/<kind<Id>, 1>;

the presence of the comment between identifier and template opener seems to be causing the qualifier alignment to fail

[MyDeveloperDay]

The cause is because Next is a comment, not a template opener, handle that case.

[curdeius]

clang/unittests/Format/QualifierFixerTest.cpp

830

Could you test with a line comment too?

830

And what about multiple comments? Maybe we need a while loop instead of if?

[curdeius]

clang/unittests/Format/QualifierFixerTest.cpp

830

And test with non-empty comments too.

[MyDeveloperDay]

Use getNextNonComment() which has some const-ness knock ons (but probably not a bad thing)

address the review concerns by adding more tests (which indeed highlighted new failure scenarios! Thanks @curdeius)

[curdeius]

LGTM! Thanks :).

[curdeius]

Just a last thought, you can maybe minimize some of the test cases.
It seems that even:

using X = Y /**/ <>;

reproduces the issue.

[MyDeveloperDay]

Simplify down the test to make it easier to read, keep perhaps slightly duplicated tests to just ensure we have the coverage

[curdeius]

Cool. Thanks!

clang/lib/Format/QualifierAlignmentFixer.h

public:
 
   std::pair<tooling::Replacements, unsigned>
   analyze(TokenAnnotator &Annotator,
           SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
           FormatTokenLexer &Tokens) override;
 
   static tok::TokenKind getTokenFromQualifier(const std::string &Qualifier);
 
-  FormatToken *analyzeRight(const SourceManager &SourceMgr,
+  const FormatToken *analyzeRight(const SourceManager &SourceMgr,
                                   const AdditionalKeywords &Keywords,
-                            tooling::Replacements &Fixes, FormatToken *Tok,
+                                  tooling::Replacements &Fixes,
+                                  const FormatToken *Tok,
                                   const std::string &Qualifier,
                                   tok::TokenKind QualifierType);
 
-  FormatToken *analyzeLeft(const SourceManager &SourceMgr,
+  const FormatToken *analyzeLeft(const SourceManager &SourceMgr,
                                  const AdditionalKeywords &Keywords,
-                           tooling::Replacements &Fixes, FormatToken *Tok,
+                                 tooling::Replacements &Fixes,
+                                 const FormatToken *Tok,
                                  const std::string &Qualifier,
                                  tok::TokenKind QualifierType);
 
   // is the Token a simple or qualifier type
   static bool isQualifierOrType(const FormatToken *Tok,
                                 const std::vector<tok::TokenKind> &Qualifiers);
 
   // is the Token likely a Macro
   static bool isPossibleMacro(const FormatToken *Tok);
 };
 
 } // end namespace format
 } // end namespace clang
 
 #endif

clang/lib/Format/QualifierAlignmentFixer.cpp

static void rotateTokens(const SourceManager &SourceMgr,
   }
 
   auto Range = CharSourceRange::getCharRange(First->getStartOfNonWhitespace(),
                                              Last->Tok.getEndLoc());
 
   replaceToken(SourceMgr, Fixes, Range, NewText);
 }
 
-FormatToken *LeftRightQualifierAlignmentFixer::analyzeRight(
+const FormatToken *LeftRightQualifierAlignmentFixer::analyzeRight(
     const SourceManager &SourceMgr, const AdditionalKeywords &Keywords,
-    tooling::Replacements &Fixes, FormatToken *Tok,
+    tooling::Replacements &Fixes, const FormatToken *Tok,
     const std::string &Qualifier, tok::TokenKind QualifierType) {
   // We only need to think about streams that begin with a qualifier.
   if (!Tok->is(QualifierType))
     return Tok;
   // Don't concern yourself if nothing follows the qualifier.
   if (!Tok->Next)
     return Tok;
   if (LeftRightQualifierAlignmentFixer::isPossibleMacro(Tok->Next))
if (Next && Next->isOneOf(tok::star, tok::amp, tok::ampamp) &&
       }
       return Next;
     }
   }
 
   return Tok;
 }
 
-FormatToken *LeftRightQualifierAlignmentFixer::analyzeLeft(
+const FormatToken *LeftRightQualifierAlignmentFixer::analyzeLeft(
     const SourceManager &SourceMgr, const AdditionalKeywords &Keywords,
-    tooling::Replacements &Fixes, FormatToken *Tok,
+    tooling::Replacements &Fixes, const FormatToken *Tok,
     const std::string &Qualifier, tok::TokenKind QualifierType) {
   // if Tok is an identifier and possibly a macro then don't convert.
   if (LeftRightQualifierAlignmentFixer::isPossibleMacro(Tok))
     return Tok;
 
-  FormatToken *Qual = Tok;
-  FormatToken *LastQual = Qual;
+  const FormatToken *Qual = Tok;
+  const FormatToken *LastQual = Qual;
   while (Qual && isQualifierOrType(Qual, ConfiguredQualifierTokens)) {
     LastQual = Qual;
     Qual = Qual->Next;
     if (Qual && Qual->is(QualifierType))
       break;
   }
 
   if (!Qual) {
if (Tok->is(TT_TemplateOpener) && Tok->Next &&
       Tok->Next->Next && Tok->Next->Next->is(QualifierType)) {
     rotateTokens(SourceMgr, Fixes, Tok->Next, Tok->Next->Next, /*Left=*/true);
   }
   if (Tok->startsSequence(tok::identifier) && Tok->Next) {
     if (Tok->Previous &&
         Tok->Previous->isOneOf(tok::star, tok::ampamp, tok::amp)) {
       return Tok;
     }
-    FormatToken *Next = Tok->Next;
+    const FormatToken *Next = Tok->Next;
     // The case  `std::Foo<T> const` -> `const std::Foo<T> &&`
     while (Next && Next->isOneOf(tok::identifier, tok::coloncolon))
       Next = Next->Next;
     if (Next && Next->Previous &&
         Next->Previous->startsSequence(tok::identifier, TT_TemplateOpener)) {
       // Read from to the end of the TemplateOpener to
       // TemplateCloser const ArrayRef<int> a; const ArrayRef<int> &a;
+      if (Next->is(tok::comment) && Next->getNextNonComment())
+        Next = Next->getNextNonComment();
       assert(Next->MatchingParen && "Missing template closer");
       Next = Next->MatchingParen->Next;
 
       // Move to the end of any template class members e.g.
       // `Foo<int>::iterator`.
       if (Next && Next->startsSequence(tok::coloncolon, tok::identifier))
         Next = Next->Next->Next;
       if (Next && Next->is(QualifierType)) {
LeftRightQualifierAlignmentFixer::analyze(
 
   tok::TokenKind QualifierToken = getTokenFromQualifier(Qualifier);
   assert(QualifierToken != tok::identifier && "Unrecognised Qualifier");
 
   for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
     FormatToken *First = AnnotatedLines[I]->First;
     const auto *Last = AnnotatedLines[I]->Last;
 
-    for (auto *Tok = First; Tok && Tok != Last && Tok->Next; Tok = Tok->Next) {
+    for (const auto *Tok = First; Tok && Tok != Last && Tok->Next;
+         Tok = Tok->Next) {
       if (Tok->is(tok::comment))
         continue;
       if (RightAlign)
         Tok = analyzeRight(SourceMgr, Keywords, Fixes, Tok, Qualifier,
                            QualifierToken);
       else
         Tok = analyzeLeft(SourceMgr, Keywords, Fixes, Tok, Qualifier,
                           QualifierToken);

clang/unittests/Format/QualifierFixerTest.cpp

TEST_F(QualifierFixerTest, NoOpQualifierReplacements) {
   Style.QualifierOrder = {"static", "const", "type"};
 
   ReplacementCount = 0;
   EXPECT_EQ(ReplacementCount, 0);
   verifyFormat("static const uint32 foo[] = {0, 31};", Style);
   EXPECT_EQ(ReplacementCount, 0);
 }
 
+TEST_F(QualifierFixerTest, QualifierTemplates) {
+  FormatStyle Style = getLLVMStyle();
+  Style.QualifierAlignment = FormatStyle::QAS_Custom;
+  Style.QualifierOrder = {"static", "const", "type"};
+
+  ReplacementCount = 0;
+  EXPECT_EQ(ReplacementCount, 0);
+  verifyFormat("using A = B<>;", Style);
+  verifyFormat("using A = B /**/<>;", Style);
+  verifyFormat("template <class C> using A = B<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B /**/<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B /* */<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B /*foo*/<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B /**/ /**/<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B<Foo</**/ C>, 1>;", Style);
+  verifyFormat("template <class C> using A = /**/ B<Foo<C>, 1>;", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+  verifyFormat("template <class C>\n"
+               "using A = B // foo\n"
+               "    <Foo<C>, 1>;",
+               Style);
+
+  ReplacementCount = 0;
+  Style.QualifierOrder = {"type", "static", "const"};
+  verifyFormat("using A = B<>;", Style);
+  verifyFormat("using A = B /**/<>;", Style);
+  verifyFormat("template <class C> using A = B<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B /**/<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B /* */<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B /*foo*/<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B /**/ /**/<Foo<C>, 1>;", Style);
+  verifyFormat("template <class C> using A = B<Foo</**/ C>, 1>;", Style);
+  verifyFormat("template <class C> using A = /**/ B<Foo<C>, 1>;", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+  verifyFormat("template <class C>\n"
+               "using A = B // foo\n"
+               "    <Foo<C>, 1>;",
+               Style);
+}
+
 } // namespace format
 } // namespace clang