[clang-format] Fix inconsistent identification of operator&

[dkt01]

Summary

Token annotator for clang-format incorrectly identifies operator& as a reference type in situations like Boost serialization archives.

Add annotation rules for standalone and chained operator& instances while preserving behavior for reference declarations at class scope. Add tests to validate annotation and formatting behavior.

[HazardyKnusperkeks]

Could this be added to Context instead of a new type?

clang/lib/Format/TokenAnnotator.cpp

1205

How does this happen? The only push I can see is before parseBrace, where a pop is following directly after.

1674

1675

Full stop at the end of your comments.

1937

You don't need to pass that, you can access it directly from determineStarAmpUsage, right?

2385

That const doesn't seem fit with the LLVM style.

2385

I'd prefer you write it out.

2490–2491

What about

struct S {
  auto Mem = C & D;
};

That would be annotated, or is there another rule above with would kick in? If there isn't a test with such a construct already please add one.

2496–2502

That seems better.

2544

Why static?

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

2544

The AnnotatingParser object lifetime is too short to track the scope information I wanted. From my testing, it seems like this object is created once per line. This is why I haven't added the scope information as part of the contexts member.

I would like to make this a non-static member, but I'm not sure which object it should be a member of. Do you have a recommendation?

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

2490–2491

This case was already handled properly by the IsExpression && !Contexts.back().CaretFound condition, but I'll add this test case as there isn't an equivalent test yet.

[dkt01]

Changes based on HazardyKnusperkeks' review

[dkt01]

Further changes based on HazardyKnusperkeks' review

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

1205

I previously had a double pop for some braces, but now the checks are no longer necessary as pushes and pops match.

[MyDeveloperDay]

clang/unittests/Format/TokenAnnotatorTest.cpp

181

how does this differentiate from

MyType & val2;

is that a binary operator? I don't think so?

[dkt01]

clang/unittests/Format/TokenAnnotatorTest.cpp

181

The heuristic I'm using is that [symbol] & [symbol]; outside a class/struct declaration is more likely an instance of operator& than an uninitialized reference. Tests on line 206 and 215 demonstrate how the two cases differ.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

2544

The TokenAnnotator? And then by reference to the parser.

2947

Unrelated.

clang/unittests/Format/TokenAnnotatorTest.cpp

181

The heuristic I'm using is that [symbol] & [symbol]; outside a class/struct declaration is more likely an instance of operator& than an uninitialized reference. Tests on line 206 and 215 demonstrate how the two cases differ.

I'm with this one. A reference without initialization is an error.

Can you add a test with an assignment, I didn't see one right now.

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

2544

I was over-thinking it... Adding to TokenAnnotator works great!
I did have to re-introduce the size checks before popping scopes because otherwise the UnbalancedStructuralElements test fails.

clang/unittests/Format/TokenAnnotatorTest.cpp

181

Added two new tests for assignment to pointer using unary operator & and initializing assignment to reference because neither of these assignments was in existing format tests I could find.

[dkt01]

Changes as suggested by HazardyKnusperkeks in review.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

850

See below

851

Is this check also necessary? Here we should only be when we have pushed before.
I'd change the if for an assert.

1205

Move the comment in the if? In general it hasn't to be a unbalanced brace, right?

[dkt01]

Changes recommended by HazardyKnusperkeks in review.

[HazardyKnusperkeks]

Looks good to me, but please wait until another one has given their blessing, or at least no veto for a few days.

[MyDeveloperDay]

There is part of me that feels we should not rush this in to get it into 16 (which was due to branch today), I feel it might be a good idea to commit this after branching, and let it live in 'main' so we can take for a real test drive

[dkt01]

Sounds good. I don't believe I have permissions to add commits to three repo anyway, so someone can add on my behalf after the 16 branch.

[MyDeveloperDay]

I think they have branched (https://clang.llvm.org/docs/ClangFormatStyleOptions.html) the documentation has changed to 17.0

We would need your name and email in order to commit on your behalf.

I checked this patch out and tested it on my large project and I didn't see anything immediately jump out.

@owenpan, @rymiel any concerns?

[dkt01]

David Turner <[e-mail]>

Good to know it worked as expected on your project as well.

[MyDeveloperDay]

Good to know it worked as expected on your project as well.

I have some concerns (mainly because it feels like it could be quite invasive), hence I'm delaying on the LGTM, I really want to run this on a large code base, which I think if we commit post the branch (which is now branched) we would probably see more easily. Historically in the past I think the firefox team have kindly picked up recent builds (no obligations) and catch any regressions.

clang/lib/Format/TokenAnnotator.cpp

2766

was there a reason it could no longer be const?

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

118–119

Please keep them in the same order as that of the parameters above.

851

Should we assert that the token type of OpeningBrace matches the ScopeType before popping?

1161–1168

Do we really need these? If we add other special l_brace token types, we will have to update this list.

1207

Can you leave the comment at the top of the case? If we get rid of None for ScopeType, we don't need to pop here. (See below.)

2497–2499

Can you add a lambda for this so that it can be used below?

2500–2502

NextToken is guarded against null on line 2488 but not on lines 2489-2490.

2503–2504

Here we can use the lambda suggested above.

2799

Instead of pushing None, would it better to leave Scopes empty? Then we would not need None for the ScopeType, and instead of checking for Scopes.size() > 1, we could check if it's empty().

clang/lib/Format/TokenAnnotator.h

183 ↗

(On Diff #491823)

Drop std::.

clang/unittests/Format/TokenAnnotatorTest.cpp

192–194

Can you add a case for periodstar?

207–213

Can you add a case for arrowstar?

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

1161–1168

Agreed. This was left over from a prior implementation that didn't have a default case.

2500–2502

If NextToken is null on 2488, 2489-2490 will be short circuited.

2766

Member Scopes can be modified. Alternate would be to make Scopes mutable, but I thought removing const from this function is cleaner

[dkt01]

Changes as suggested in review from owenpan

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

115–117

Something like the above. (I prefer Scopes to TrackedScopes to be consistent with the naming of the other parameters.)

1205–1208

I don't think it's about unbalanced braces here.

2495–2496

To help simplify the if statement below.

2500–2502

You are right. The redundant parens threw me off.

2501

Redundant parens.

2502–2503

The lambda would check that PrevToken is nonnull.

2504–2508

Something like that.

clang/lib/Format/TokenAnnotator.h

36 ↗

(On Diff #492482)

Consider moving ScopeType out of TokenAnnotator and keeping it consistent with the style of LineType above (and replacing TokenAnnotator::ScopeType with ScopeType everywhere).

184–191 ↗

(On Diff #492482)

See above.

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

1205–1208

if (!Scopes.empty()) handles unbalanced braces. if(Tok->Previous) handles the case where a line starts with an rbrace.

2502–2503

!PrevToken->getPreviousNonComment() || is a different check than the null check in the lambda. It's acceptable to have no token two tokens prior because that indicates the ampersand is the second token of the line.

[dkt01]

Updates suggested in owenpan's review.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

122–124

128

851

1151–1163

1205–1208

I can't think of an example. Do you have one?

2495

Sorry about that!

2502–2503

I actually wanted fold this into the lambda. (See above.)

2502–2503

Now we only need the lambda.

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

1205–1208

The format unit test FormatUnbalancedStructuralElements feeds in strings that have only right braces, so without this check the pop fails.

[dkt01]

Updates recommended by owenpan in review.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

122

As suggested before.

1205–1208

Thanks!

[dkt01]

clang/lib/Format/TokenAnnotator.cpp

122

Oops... missed the consts last time...

[dkt01]

Changes requested in owenpan's review.
Rebased changes onto latest main branch.

clang/lib/Format/TokenAnnotator.cpp

 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
 /// store a parenthesis levels. It also tries to resolve matching "<" and ">"
 /// into template parameter lists.
 class AnnotatingParser {
 public:
   AnnotatingParser(const FormatStyle &Style, AnnotatedLine &Line,
                    const AdditionalKeywords &Keywords)
       : Style(Style), Line(Line), CurrentToken(Line.First), AutoFound(false),
         Keywords(Keywords) {
     Contexts.push_back(Context(tok::unknown, 1, /*IsExpression=*/false));
     resetTokenMetadata();
   }
 
 private:
   bool parseAngle() {
     if (!CurrentToken || !CurrentToken->Previous)
       return false;
     if (NonTemplateLess.count(CurrentToken->Previous))
       return false;
 
     const FormatToken &Previous = *CurrentToken->Previous; // The '<'.
     if (Previous.Previous) {
       if (Previous.Previous->Tok.isLiteral())
         return false;
       if (Previous.Previous->is(tok::r_paren) && Contexts.size() > 1 &&
           (!Previous.Previous->MatchingParen ||
            !Previous.Previous->MatchingParen->is(
                TT_OverloadedOperatorLParen))) {
         return false;
bool parseBrace() {
     if (Style.isJavaScript() && OpeningBrace.Previous &&
         OpeningBrace.Previous->is(TT_JsTypeColon)) {
       Contexts.back().IsExpression = false;
     }
 
     unsigned CommaCount = 0;
     while (CurrentToken) {
       if (CurrentToken->is(tok::r_brace)) {
+        if (Scopes.size() > 1)
+          Scopes.pop_back();
         assert(OpeningBrace.Optional == CurrentToken->Optional);
         OpeningBrace.MatchingParen = CurrentToken;
         CurrentToken->MatchingParen = &OpeningBrace;
         if (Style.AlignArrayOfStructures != FormatStyle::AIAS_None) {
           if (OpeningBrace.ParentBracket == tok::l_brace &&
               couldBeInStructArrayInitializer() && CommaCount > 0) {
             Contexts.back().ContextType = Context::StructArrayInitializer;
           }
case tok::l_square:
         return false;
       break;
     case tok::l_brace:
       if (Style.Language == FormatStyle::LK_TextProto) {
         FormatToken *Previous = Tok->getPreviousNonComment();
         if (Previous && Previous->getType() != TT_DictLiteral)
           Previous->setType(TT_SelectorName);
       }
-      if (!parseBrace())
+      switch (Tok->getType()) {
+      case TT_FunctionLBrace:
+      case TT_LambdaLBrace:
+        Scopes.push_back(ScopeType::Function);
+        break;
+      case TT_ClassLBrace:
+      case TT_StructLBrace:
+      case TT_UnionLBrace:
+        Scopes.push_back(ScopeType::Class);
+        break;
+      case TT_EnumLBrace:
+      case TT_ControlStatementLBrace:
+      case TT_ElseLBrace:
+      case TT_BracedListLBrace:
+      case TT_CompoundRequirementLBrace:
+      case TT_ObjCBlockLBrace:
+      case TT_RecordLBrace:
+      case TT_RequiresExpressionLBrace:
+      default:
+        Scopes.push_back(ScopeType::Other);
+      }
+      if (!parseBrace()) {
+        if (Scopes.size() > 1)
+          Scopes.pop_back();
         return false;
+      }
       break;
     case tok::less:
       if (parseAngle()) {
         Tok->setType(TT_TemplateOpener);
         // In TT_Proto, we must distignuish between:
         //   map<key, value>
         //   msg < item: data >
         //   msg: < item: data >
case tok::less:
         CurrentToken = Tok;
         next();
       }
       break;
     case tok::r_paren:
     case tok::r_square:
       return false;
     case tok::r_brace:
+      if (Scopes.size() > 1)
+        Scopes.pop_back();
       // Lines can start with '}'.
       if (Tok->Previous)
         return false;
       break;
     case tok::greater:
       if (Style.Language != FormatStyle::LK_TextProto)
         Tok->setType(TT_BinaryOperator);
       if (Tok->Previous && Tok->Previous->is(TT_TemplateCloser))
         Tok->SpacesRequiredBefore = 1;
       break;
enum {
       StructArrayInitializer,
       // Like in `static_cast<int>`.
       TemplateArgument,
       // C11 _Generic selection.
       C11GenericSelection,
     } ContextType = Unknown;
   };
 
+  enum class ScopeType {
+    // Not contained within scope block
+    None,
+    // Contained in class declaration/definition
+    Class,
+    // Contained within function definition
+    Function,
+    // Contained within other scope block (loop, if/else, etc)
+    Other,
+  };
+
   /// Puts a new \c Context onto the stack \c Contexts for the lifetime
   /// of each instance.
   struct ScopedContextCreator {
     AnnotatingParser &P;
 
     ScopedContextCreator(AnnotatingParser &P, tok::TokenKind ContextKind,
                          unsigned Increase)
         : P(P) {
if (Current.is(Keywords.kw_instanceof)) {
       // Concept implicit conversion constraint needs to be treated like
       // a trailing return type  ... } -> <type>.
       Current.setType(TT_TrailingReturnArrow);
     } else if (isDeductionGuide(Current)) {
       // Deduction guides trailing arrow " A(...) -> A<T>;".
       Current.setType(TT_TrailingReturnArrow);
     } else if (Current.isOneOf(tok::star, tok::amp, tok::ampamp)) {
       Current.setType(determineStarAmpUsage(
-          Current,
+          Current, Scopes.back(),
           Contexts.back().CanBeExpression && Contexts.back().IsExpression,
           Contexts.back().ContextType == Context::TemplateArgument));
     } else if (Current.isOneOf(tok::minus, tok::plus, tok::caret) ||
                (Style.isVerilog() && Current.is(tok::pipe))) {
       Current.setType(determinePlusMinusCaretUsage(Current));
       if (Current.is(TT_UnaryOperator) && Current.is(tok::caret))
         Contexts.back().CaretFound = true;
     } else if (Current.isOneOf(tok::minusminus, tok::plusplus)) {
bool determineUnaryOperatorByUsage(const FormatToken &Tok) {
     // There can't be two consecutive binary operators.
     if (PrevToken->is(TT_BinaryOperator))
       return true;
 
     return false;
   }
 
   /// Return the type of the given token assuming it is * or &.
-  TokenType determineStarAmpUsage(const FormatToken &Tok, bool IsExpression,
+  TokenType determineStarAmpUsage(const FormatToken &Tok,
+                                  const ScopeType TokScope, bool IsExpression,
                                   bool InTemplateArgument) {
     if (Style.isJavaScript())
       return TT_BinaryOperator;
 
     // && in C# must be a binary operator.
     if (Style.isCSharp() && Tok.is(tok::ampamp))
       return TT_BinaryOperator;
 
if (NextToken->Tok.isAnyIdentifier()) {
         return TT_BinaryOperator;
     }
 
     // It is very unlikely that we are going to find a pointer or reference type
     // definition on the RHS of an assignment.
     if (IsExpression && !Contexts.back().CaretFound)
       return TT_BinaryOperator;
 
+    // Opeartors at class scope are likely pointer or reference members
+    if (TokScope == ScopeType::Class)
+      return TT_PointerOrReference;
+
+    // It's more likely that & represents operator& than an uninitialized
+    // reference
+    if (Tok.is(tok::amp) && (PrevToken && PrevToken->Tok.isAnyIdentifier()) &&
+        ((PrevToken->getPreviousNonComment() &&
+          (PrevToken->getPreviousNonComment()->is(tok::amp) ||
+           PrevToken->getPreviousNonComment()->is(tok::period) ||
+           PrevToken->getPreviousNonComment()->is(tok::arrow) ||
+           PrevToken->getPreviousNonComment()->is(tok::arrowstar) ||
+           PrevToken->getPreviousNonComment()->is(tok::periodstar))) ||
+         !PrevToken->getPreviousNonComment()) &&
+        (NextToken && NextToken->Tok.isAnyIdentifier()) &&
+        (NextToken->getNextNonComment() &&
+         (NextToken->getNextNonComment()->is(tok::amp) ||
+          NextToken->getNextNonComment()->is(tok::semi) ||
+          NextToken->getNextNonComment()->is(tok::period) ||
+          NextToken->getNextNonComment()->is(tok::arrow) ||
+          NextToken->getNextNonComment()->is(tok::arrowstar) ||
+          NextToken->getNextNonComment()->is(tok::periodstar)))) {
+      return TT_BinaryOperator;
+    }
+
     return TT_PointerOrReference;
   }
 
   TokenType determinePlusMinusCaretUsage(const FormatToken &Tok) {
     if (determineUnaryOperatorByUsage(Tok))
       return TT_UnaryOperator;
 
     const FormatToken *PrevToken = Tok.getPreviousNonComment();
if (!PrevToken || PrevToken->is(TT_CastRParen))
       return TT_UnaryOperator;
     if (PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::identifier))
       return TT_TrailingUnaryOperator;
 
     return TT_UnaryOperator;
   }
 
   SmallVector<Context, 8> Contexts;
+  static SmallVector<ScopeType, 8> Scopes;
 
   const FormatStyle &Style;
   AnnotatedLine &Line;
   FormatToken *CurrentToken;
   bool AutoFound;
   const AdditionalKeywords &Keywords;
 
   // Set of "<" tokens that do not open a template parameter list. If parseAngle
private:
   }
 
   const FormatStyle &Style;
   const AdditionalKeywords &Keywords;
   const AnnotatedLine &Line;
   FormatToken *Current;
 };
 
+SmallVector<AnnotatingParser::ScopeType, 8> AnnotatingParser::Scopes = {
+    AnnotatingParser::ScopeType::None};
+
 } // end anonymous namespace
 
 void TokenAnnotator::setCommentLineLevels(
     SmallVectorImpl<AnnotatedLine *> &Lines) const {
   const AnnotatedLine *NextNonCommentLine = nullptr;
   for (AnnotatedLine *Line : llvm::reverse(Lines)) {
     assert(Line->First);
 
     // If the comment is currently aligned with the line immediately following
     // it, that's probably intentional and we should keep it.
     if (NextNonCommentLine && Line->isComment() &&
         NextNonCommentLine->First->NewlinesBefore <= 1 &&
         NextNonCommentLine->First->OriginalColumn ==
             Line->First->OriginalColumn) {

 
 static unsigned maxNestingDepth(const AnnotatedLine &Line) {
   unsigned Result = 0;
   for (const auto *Tok = Line.First; Tok != nullptr; Tok = Tok->Next)
     Result = std::max(Result, Tok->NestingLevel);
   return Result;
 }
 
 void TokenAnnotator::annotate(AnnotatedLine &Line) const {
   for (auto &Child : Line.Children)
     annotate(*Child);
 
   AnnotatingParser Parser(Style, Line, Keywords);
   Line.Type = Parser.parseLine();
 
   // With very deep nesting, ExpressionParser uses lots of stack and the
   // formatting algorithm is very slow. We're not going to do a good job here
static bool isFunctionDeclarationName(bool IsCpp, const FormatToken &Current,
   // Check whether parameter list can belong to a function declaration.
   if (!Next || !Next->is(tok::l_paren) || !Next->MatchingParen)
     return false;
   // If the lines ends with "{", this is likely a function definition.
   if (Line.Last->is(tok::l_brace))
     return true;
   if (Next->Next == Next->MatchingParen)
     return true; // Empty parentheses.
   // If there is an &/&& after the r_paren, this is likely a function.
   if (Next->MatchingParen->Next &&
       Next->MatchingParen->Next->is(TT_PointerOrReference)) {
     return true;
   }
 
   // Check for K&R C function definitions (and C++ function definitions with
   // unnamed parameters), e.g.:
   //   int f(i)

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   // operator invoked as a member function).
   verifyFormat("void f() { a.operator()(a * a); }");
   verifyFormat("void f() { a->operator()(a & a); }");
   verifyFormat("void f() { a.operator()(*a & *a); }");
   verifyFormat("void f() { a->operator()(*a * *a); }");
 
   verifyFormat("int operator()(T (&&)[N]) { return 1; }");
   verifyFormat("int operator()(T (&)[N]) { return 0; }");
+
+  verifyFormat("val1 & val2;");
+  verifyFormat("val1 & val2 & val3;");
+  verifyFormat("class c {\n"
+               "  void func(type &a) { a & member; }\n"
+               "  anotherType &member;\n"
+               "}");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
   verifyFormat("SomeType s __attribute__((unused)) (InitValue);");
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa __attribute__((unused))\n"
                "aaaaaaaaaaaaaaaaaaaaaaa(int i);");
   verifyFormat("__attribute__((nodebug)) ::qualified_type f();");
   FormatStyle AfterType = getLLVMStyle();

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsUsesOfStarAndAmp) {
   ASSERT_EQ(Tokens.size(), 17u) << Tokens;
   EXPECT_TOKEN(Tokens[9], tok::amp, TT_PointerOrReference);
   EXPECT_TOKEN(Tokens[12], tok::amp, TT_PointerOrReference);
 
   Tokens = annotate("void f7() { int f8(Foo&&, Bar&&); }");
   ASSERT_EQ(Tokens.size(), 17u) << Tokens;
   EXPECT_TOKEN(Tokens[9], tok::ampamp, TT_PointerOrReference);
   EXPECT_TOKEN(Tokens[12], tok::ampamp, TT_PointerOrReference);
+
+  Tokens = annotate("val1 & val2;");
+  ASSERT_EQ(Tokens.size(), 5u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1 & val2.member;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1 & val2 & val3;");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[3], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("val1 & val2 // comment\n"
+                    "     & val3;");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[4], tok::amp, TT_BinaryOperator);
+
+  Tokens =
+      annotate("val1 & val2.member & val3.member() & val4 & val5->member;");
+  ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[5], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[11], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[13], tok::amp, TT_BinaryOperator);
+
+  Tokens = annotate("class c {\n"
+                    "  void func(type &a) { a & member; }\n"
+                    "  anotherType &member;\n"
+                    "}");
+  ASSERT_EQ(Tokens.size(), 22u) << Tokens;
+  EXPECT_TOKEN(Tokens[7], tok::amp, TT_PointerOrReference);
+  EXPECT_TOKEN(Tokens[12], tok::amp, TT_BinaryOperator);
+  EXPECT_TOKEN(Tokens[17], tok::amp, TT_PointerOrReference);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsUsesOfPlusAndMinus) {
   auto Tokens = annotate("x - 0");
   ASSERT_EQ(Tokens.size(), 4u) << Tokens;
   EXPECT_TOKEN(Tokens[1], tok::minus, TT_BinaryOperator);
   Tokens = annotate("0 + 0");
   ASSERT_EQ(Tokens.size(), 4u) << Tokens;