[clang][index] Fix processing of CompoundAssignOperator at setting up reference roles

[ArcsinX]

Without this patch in expressions like foo += 1 reference foo has no read and write roles.

This happens because CompoundAssignOperator is also a BinaryOperator, thus handling CompoindAssignOperator in else branch is a dead code.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aleksandr Platonov (ArcsinX)

Changes

Without this patch in expressions like foo += 1 reference foo has no read and write roles.

This happens because CompoundAssignOperator is also a BinaryOperator, thus handling CompoindAssignOperator in else branch is a dead code.

---

Full diff: https://github.com/llvm/llvm-project/pull/69370.diff

2 Files Affected:

• (modified) clang/lib/Index/IndexBody.cpp (+9-9)
• (modified) clang/unittests/Index/IndexTests.cpp (+25)

diff --git a/clang/lib/Index/IndexBody.cpp b/clang/lib/Index/IndexBody.cpp
index e88f321f18a7129..08136baa5d408e9 100644
--- a/clang/lib/Index/IndexBody.cpp
+++ b/clang/lib/Index/IndexBody.cpp
@@ -77,9 +77,15 @@ class BodyIndexer : public RecursiveASTVisitor<BodyIndexer> {
     const Stmt *Parent = *It;
 
     if (auto BO = dyn_cast<BinaryOperator>(Parent)) {
-      if (BO->getOpcode() == BO_Assign && BO->getLHS()->IgnoreParenCasts() == E)
-        Roles |= (unsigned)SymbolRole::Write;
-
+      if (BO->getOpcode() == BO_Assign) {
+        if (BO->getLHS()->IgnoreParenCasts() == E)
+          Roles |= (unsigned)SymbolRole::Write;
+      } else if (auto CA = dyn_cast<CompoundAssignOperator>(Parent)) {
+        if (CA->getLHS()->IgnoreParenCasts() == E) {
+          Roles |= (unsigned)SymbolRole::Read;
+          Roles |= (unsigned)SymbolRole::Write;
+        }
+      }
     } else if (auto UO = dyn_cast<UnaryOperator>(Parent)) {
       if (UO->isIncrementDecrementOp()) {
         Roles |= (unsigned)SymbolRole::Read;
@@ -88,12 +94,6 @@ class BodyIndexer : public RecursiveASTVisitor<BodyIndexer> {
         Roles |= (unsigned)SymbolRole::AddressOf;
       }
 
-    } else if (auto CA = dyn_cast<CompoundAssignOperator>(Parent)) {
-      if (CA->getLHS()->IgnoreParenCasts() == E) {
-        Roles |= (unsigned)SymbolRole::Read;
-        Roles |= (unsigned)SymbolRole::Write;
-      }
-
     } else if (auto CE = dyn_cast<CallExpr>(Parent)) {
       if (CE->getCallee()->IgnoreParenCasts() == E) {
         addCallRole(Roles, Relations);
diff --git a/clang/unittests/Index/IndexTests.cpp b/clang/unittests/Index/IndexTests.cpp
index 4d19f47283c2853..8e9a1c6bf88245c 100644
--- a/clang/unittests/Index/IndexTests.cpp
+++ b/clang/unittests/Index/IndexTests.cpp
@@ -428,6 +428,31 @@ TEST(IndexTest, NonTypeTemplateParameter) {
                              WrittenAt(Position(3, 15)))));
 }
 
+TEST(IndexTest, ReadWriteRoles) {
+  std::string Code = R"cpp(
+    int main() {
+      int foo = 0;
+      foo = 2;
+      foo += 1;
+      int bar = foo;
+  }
+  )cpp";
+  auto Index = std::make_shared<Indexer>();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique<IndexAction>(Index, Opts), Code);
+  EXPECT_THAT(
+      Index->Symbols,
+      AllOf(Contains(AllOf(QName("foo"), HasRole(SymbolRole::Write),
+                           WrittenAt(Position(4, 7)))),
+            Contains(AllOf(QName("foo"),
+                           HasRole(static_cast<unsigned>(SymbolRole::Read) |
+                                   static_cast<unsigned>(SymbolRole::Write)),
+                           WrittenAt(Position(5, 7)))),
+            Contains(AllOf(QName("foo"), HasRole(SymbolRole::Read),
+                           WrittenAt(Position(6, 17))))));
+}
+
 } // namespace
 } // namespace index
 } // namespace clang

[hokein]

good catch.

[hokein]

nit: consider inlining this if to the one above (like the original code) to the code less nested.

[ArcsinX]

I split this if to avoid this check else if (auto CA = dyn_cast<CompoundAssignOperator>(Parent)) if BO->getOpcode() == BO_Assign && BO->getLHS()->IgnoreParenCasts() != E

I.e. if BO->getOpcode() == BO_Assign then Parent is not a CompoundAssignOperator and we don't need to check this