[clangd][ObjC] Support ObjC class rename from implementation decls

[dgoldman]

Run clang format

[dgoldman]

• Implement discussed fixes + category support

[kadircet]

clang-tools-extra/clangd/FindTarget.cpp

719

can you also add tests for these into FindTargetTests?

720

we don't have the null check in other places, what's the significance here?

clang-tools-extra/clangd/SemanticHighlighting.cpp

131

let's do this in a separate change, with some tests

clang-tools-extra/clangd/refactor/Rename.cpp

175–181

sorry i don't follow what's the logic doing here and we're likely doing these in the wrong layer.

we should either:

• Fix selection tree to pick the correct ASTNode, if it's picking the wrong one due to not having special cases for these locations here
• Fix the targetDecl logic to make sure it emits all the declarations that might be referenced by this ast node, if it's missing ClassInterface.
• Fix the canonicalRenameDecl, if we should always prefer ClassInterface in these cases.

[dgoldman]

clang-tools-extra/clangd/SemanticHighlighting.cpp

131

Given the other changes, this is needed otherwise the semantic highlighting test fails.

clang-tools-extra/clangd/refactor/Rename.cpp

175–181

Replied to you in chat, LMK what you think.

[dgoldman]

Disable renaming categories

[dgoldman]

Add more FindTarget tests

[dgoldman]

clang-tools-extra/clangd/FindTarget.cpp

720

Removed, doesn't look like it's needed.

[kadircet]

clang-tools-extra/clangd/refactor/Rename.cpp

173

before calling canonicalRenameDecl here we can do a D = pickInterestingTarget(D); and map it to interface decl once (we'll probably need something similar for propertydecls?), rather than canonicalizing all ObjcCategoryDecls. that way we'll still get to rename proper references that target ObjcInterfaceDecls.

297

i am afraid we're doing this at the wrong layer. the discrepancy is actually arising from the fact that findExplicitReferences will report name locations for an entity whose primary location contains a different name (e.g. @interface ^Foo (^Bar) saying that there's a reference to objccategorydecl at Bar, but canonicalizing it to Foo).

so i think what we really want is not to canonicalize objccategory(impl)decls to objcinterfacedecl, but rather pick rename target as objcinterfacedecl when the user is trying to rename a categorydecl inside locatelDeclAt (put more comments near that part of the code).

sorry for the extra round trip here :/

792

this special casing should no longer be needed if we just map CategoryDecls to InterfaceDecls in locateDeclAt rather than at canonicalization time

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

2063

can you move this into a separate test case?

[dgoldman]

Fixes for review

clang-tools-extra/clangd/refactor/Rename.cpp

792

Yep, no longer needed since the Check the rename-triggering location is actually being renamed check catches this although the error is no symbol found instead of unsupported symbol.

[kadircet]

thanks!

clang-tools-extra/clangd/refactor/Rename.cpp

144

can you put a function level comment like:

Some AST nodes can reference multiple declarations. We try to pick the relevant one to rename here.

clang-tools-extra/clangd/unittests/RenameTests.cpp

1709

can you drop the ^ in here. we only iterate over points in the header file.

[dgoldman]

Fixes for review + rebase

clang-tools-extra/clangd/FindTarget.cpp

struct Visitor : ConstDeclVisitor<Visitor> {
 
     void VisitObjCCategoryImplDecl(const ObjCCategoryImplDecl *OCID) {
       Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   OCID->getLocation(),
                                   /*IsDecl=*/false,
                                   {OCID->getClassInterface()}});
       Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   OCID->getCategoryNameLoc(),
-                                  /*IsDecl=*/true,
+                                  /*IsDecl=*/false,
                                   {OCID->getCategoryDecl()}});
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+                                  OCID->getCategoryNameLoc(),
+                                  /*IsDecl=*/true,
+                                  {OCID}});
+    }
+
+    void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) {
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+                                  OIMD->getLocation(),
+                                  /*IsDecl=*/false,
+                                  {OIMD->getClassInterface()}});
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+                                  OIMD->getLocation(),
+                                  /*IsDecl=*/true,
+                                  {OIMD}});
     }
   };
 
   Visitor V{Resolver};
   V.Visit(D);
   return V.Refs;
 }
 

clang-tools-extra/clangd/SemanticHighlighting.cpp

if (RD->isLambda())
       return std::nullopt;
     return HighlightingKind::Class;
   }
   if (isa<ClassTemplateDecl, RecordDecl, CXXConstructorDecl, ObjCInterfaceDecl,
           ObjCImplementationDecl>(D))
     return HighlightingKind::Class;
   if (isa<ObjCProtocolDecl>(D))
     return HighlightingKind::Interface;
-  if (isa<ObjCCategoryDecl>(D))
+  if (isa<ObjCCategoryDecl, ObjCCategoryImplDecl>(D))
     return HighlightingKind::Namespace;
   if (auto *MD = dyn_cast<CXXMethodDecl>(D))
     return MD->isStatic() ? HighlightingKind::StaticMethod
                           : HighlightingKind::Method;
   if (auto *OMD = dyn_cast<ObjCMethodDecl>(D))
     return OMD->isClassMethod() ? HighlightingKind::StaticMethod
                                 : HighlightingKind::Method;
   if (isa<FieldDecl, ObjCPropertyDecl>(D))

clang-tools-extra/clangd/refactor/Rename.cpp

 #include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ParentMapContext.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/STLExtras.h"
const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
   }
   if (const auto *UD = dyn_cast<UsingShadowDecl>(D)) {
     if (const auto *TargetDecl = UD->getTargetDecl())
       return canonicalRenameDecl(TargetDecl);
   }
   return dyn_cast<NamedDecl>(D->getCanonicalDecl());
 }
 
+// Some AST nodes can reference multiple declarations. We try to pick the
+// relevant one to rename here.
+const NamedDecl *pickInterestingTarget(const NamedDecl *D) {
+  // We only support renaming the class name, not the category name. This has
+  // to be done outside of canonicalization since we don't want a category name
+  // reference to be canonicalized to the class.
+  if (const auto *CD = dyn_cast<ObjCCategoryDecl>(D))
+    if (const auto CI = CD->getClassInterface())
+      return CI;
+  return D;
+}
+
 llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
                                                SourceLocation TokenStartLoc) {
   unsigned Offset =
       AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
 
   SelectionTree Selection = SelectionTree::createRight(
       AST.getASTContext(), AST.getTokens(), Offset, Offset);
   const SelectionTree::Node *SelectedNode = Selection.commonAncestor();
   if (!SelectedNode)
     return {};
 
   llvm::DenseSet<const NamedDecl *> Result;
   for (const NamedDecl *D :
        targetDecl(SelectedNode->ASTNode,
                   DeclRelation::Alias | DeclRelation::TemplatePattern,
                   AST.getHeuristicResolver())) {
+    D = pickInterestingTarget(D);
     Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
 
 void filterRenameTargets(llvm::DenseSet<const NamedDecl *> &Decls) {
   // For something like
   //     namespace ns { void foo(); }
   //     void bar() { using ns::f^oo; foo(); }
   // locateDeclAt() will return a UsingDecl and foo's actual declaration.
   // For renaming, we're only interested in foo's declaration, so drop the other
   // one. There should never be more than one UsingDecl here, otherwise the
   // rename would be ambiguos anyway.
   auto UD = std::find_if(Decls.begin(), Decls.end(), [](const NamedDecl *D) {
     return llvm::isa<UsingDecl>(D);
   });
   if (UD != Decls.end()) {
for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
     findExplicitReferences(
         TopLevelDecl,
         [&](ReferenceLoc Ref) {
           if (Ref.Targets.empty())
             return;
           for (const auto *Target : Ref.Targets) {
             if (canonicalRenameDecl(Target) == &ND) {
               Results.push_back(Ref.NameLoc);
               return;
             }
           }
         },
         AST.getHeuristicResolver());
   }
 
   return Results;
 }
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   if (ID->getName() == RInputs.NewName)
     return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, RInputs.NewName);
   if (Invalid)
     return makeError(std::move(*Invalid));
 
   auto Reject =
       renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index, Opts);
   if (Reject)
     return makeError(*Reject);
 
   // We have two implementations of the rename:
   //   - AST-based rename: used for renaming local symbols, e.g. variables
   //     defined in a function body;
   //   - index-based rename: used for renaming non-local symbols, and not
   //     feasible for local symbols (as by design our index don't index these
   //     symbols by design;

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Code = R"cpp(
     @interface Foo (Ext)
     @end
     @implementation [[Foo]] (Ext)
     @end
   )cpp";
   EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)");
 
   Code = R"cpp(
+    @interface Foo
+    @end
+    @interface Foo (Ext)
+    @end
+    @implementation Foo ([[Ext]])
+    @end
+  )cpp";
+  EXPECT_DECLS("ObjCCategoryImplDecl", "@interface Foo(Ext)");
+
+  Code = R"cpp(
     void test(id</*error-ok*/[[InvalidProtocol]]> p);
   )cpp";
   EXPECT_DECLS("ParmVarDecl", "id p");
 
   Code = R"cpp(
     @class C;
     @protocol Foo
     @end

 
 class FindExplicitReferencesTest : public ::testing::Test {
 protected:
   struct AllRefs {
     std::string AnnotatedCode;
     std::string DumpedReferences;
   };
 
-  /// Parses \p Code, finds function or namespace '::foo' and annotates its body
-  /// with results of findExplicitReferences.
-  /// See actual tests for examples of annotation format.
-  AllRefs annotateReferencesInFoo(llvm::StringRef Code) {
+  TestTU newTU(llvm::StringRef Code) {
     TestTU TU;
     TU.Code = std::string(Code);
 
     // FIXME: Auto-completion in a template requires disabling delayed template
     // parsing.
     TU.ExtraArgs.push_back("-std=c++20");
     TU.ExtraArgs.push_back("-xobjective-c++");
 
-    auto AST = TU.build();
-    auto *TestDecl = &findDecl(AST, "foo");
-    if (auto *T = llvm::dyn_cast<FunctionTemplateDecl>(TestDecl))
-      TestDecl = T->getTemplatedDecl();
-
-    std::vector<ReferenceLoc> Refs;
-    if (const auto *Func = llvm::dyn_cast<FunctionDecl>(TestDecl))
-      findExplicitReferences(
-          Func->getBody(),
-          [&Refs](ReferenceLoc R) { Refs.push_back(std::move(R)); },
-          AST.getHeuristicResolver());
-    else if (const auto *NS = llvm::dyn_cast<NamespaceDecl>(TestDecl))
-      findExplicitReferences(
-          NS,
-          [&Refs, &NS](ReferenceLoc R) {
-            // Avoid adding the namespace foo decl to the results.
-            if (R.Targets.size() == 1 && R.Targets.front() == NS)
-              return;
-            Refs.push_back(std::move(R));
-          },
-          AST.getHeuristicResolver());
-    else
-      ADD_FAILURE() << "Failed to find ::foo decl for test";
+    return TU;
+  }
 
+  AllRefs annotatedReferences(llvm::StringRef Code, ParsedAST &AST,
+                              std::vector<ReferenceLoc> Refs) {
     auto &SM = AST.getSourceManager();
     llvm::sort(Refs, [&](const ReferenceLoc &L, const ReferenceLoc &R) {
       return SM.isBeforeInTranslationUnit(L.NameLoc, R.NameLoc);
     });
 
     std::string AnnotatedCode;
     unsigned NextCodeChar = 0;
     for (unsigned I = 0; I < Refs.size(); ++I) {
AllRefs annotatedReferences(llvm::StringRef Code, ParsedAST &AST,
     AnnotatedCode += Code.substr(NextCodeChar);
 
     std::string DumpedReferences;
     for (unsigned I = 0; I < Refs.size(); ++I)
       DumpedReferences += std::string(llvm::formatv("{0}: {1}\n", I, Refs[I]));
 
     return AllRefs{std::move(AnnotatedCode), std::move(DumpedReferences)};
   }
+
+  /// Parses \p Code, and annotates its body with results of
+  /// findExplicitReferences on all top level decls.
+  /// See actual tests for examples of annotation format.
+  AllRefs annotateAllReferences(llvm::StringRef Code) {
+    TestTU TU = newTU(Code);
+    auto AST = TU.build();
+
+    std::vector<ReferenceLoc> Refs;
+    for (auto *TopLevel : AST.getLocalTopLevelDecls())
+      findExplicitReferences(
+          TopLevel, [&Refs](ReferenceLoc R) { Refs.push_back(std::move(R)); },
+          AST.getHeuristicResolver());
+    return annotatedReferences(Code, AST, std::move(Refs));
+  }
+
+  /// Parses \p Code, finds function or namespace '::foo' and annotates its body
+  /// with results of findExplicitReferences.
+  /// See actual tests for examples of annotation format.
+  AllRefs annotateReferencesInFoo(llvm::StringRef Code) {
+    TestTU TU = newTU(Code);
+    auto AST = TU.build();
+    auto *TestDecl = &findDecl(AST, "foo");
+    if (auto *T = llvm::dyn_cast<FunctionTemplateDecl>(TestDecl))
+      TestDecl = T->getTemplatedDecl();
+
+    std::vector<ReferenceLoc> Refs;
+    if (const auto *Func = llvm::dyn_cast<FunctionDecl>(TestDecl))
+      findExplicitReferences(
+          Func->getBody(),
+          [&Refs](ReferenceLoc R) { Refs.push_back(std::move(R)); },
+          AST.getHeuristicResolver());
+    else if (const auto *NS = llvm::dyn_cast<NamespaceDecl>(TestDecl))
+      findExplicitReferences(
+          NS,
+          [&Refs, &NS](ReferenceLoc R) {
+            // Avoid adding the namespace foo decl to the results.
+            if (R.Targets.size() == 1 && R.Targets.front() == NS)
+              return;
+            Refs.push_back(std::move(R));
+          },
+          AST.getHeuristicResolver());
+    else if (const auto *OC = llvm::dyn_cast<ObjCContainerDecl>(TestDecl))
+      findExplicitReferences(
+          OC, [&Refs](ReferenceLoc R) { Refs.push_back(std::move(R)); },
+          AST.getHeuristicResolver());
+    else
+      ADD_FAILURE() << "Failed to find ::foo decl for test";
+
+    return annotatedReferences(Code, AST, std::move(Refs));
+  }
 };
 
-TEST_F(FindExplicitReferencesTest, All) {
+TEST_F(FindExplicitReferencesTest, AllRefsInFoo) {
   std::pair</*Code*/ llvm::StringRef, /*References*/ llvm::StringRef> Cases[] =
       {// Simple expressions.
        {R"cpp(
         int global;
         int func();
         void foo(int param) {
           $0^global = $1^param + $2^func();
         }
for (const auto &C : Cases) {
     llvm::StringRef ExpectedRefs = C.second;
 
     auto Actual =
         annotateReferencesInFoo(llvm::Annotations(ExpectedCode).code());
     EXPECT_EQ(ExpectedCode, Actual.AnnotatedCode);
     EXPECT_EQ(ExpectedRefs, Actual.DumpedReferences) << ExpectedCode;
   }
 }
 
+TEST_F(FindExplicitReferencesTest, AllRefs) {
+  std::pair</*Code*/ llvm::StringRef, /*References*/ llvm::StringRef> Cases[] =
+      {{R"cpp(
+      @interface $0^MyClass
+      @end
+      @implementation $1^$2^MyClass
+      @end
+      )cpp",
+        "0: targets = {MyClass}, decl\n"
+        "1: targets = {MyClass}\n"
+        "2: targets = {MyClass}, decl\n"},
+       {R"cpp(
+      @interface $0^MyClass
+      @end
+      @interface $1^MyClass ($2^Category)
+      @end
+      @implementation $3^MyClass ($4^$5^Category)
+      @end
+      )cpp",
+        "0: targets = {MyClass}, decl\n"
+        "1: targets = {MyClass}\n"
+        "2: targets = {Category}, decl\n"
+        "3: targets = {MyClass}\n"
+        "4: targets = {Category}\n"
+        "5: targets = {Category}, decl\n"}};
+
+  for (const auto &C : Cases) {
+    llvm::StringRef ExpectedCode = C.first;
+    llvm::StringRef ExpectedRefs = C.second;
+
+    auto Actual = annotateAllReferences(llvm::Annotations(ExpectedCode).code());
+    EXPECT_EQ(ExpectedCode, Actual.AnnotatedCode);
+    EXPECT_EQ(ExpectedRefs, Actual.DumpedReferences) << ExpectedCode;
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/RenameTests.cpp

llvm::StringRef Tests[] = {
       R"cpp(
         namespace ns { int [[^foo]](int); char foo(char); }
         using ns::[[foo]];
         void f() {
           [[^foo]](42);
           foo('x');
         }
       )cpp",
+
+      // ObjC class with a category.
+      R"cpp(
+        @interface [[Fo^o]]
+        @end
+        @implementation [[F^oo]]
+        @end
+        @interface [[Fo^o]] (Category)
+        @end
+        @implementation [[F^oo]] (Category)
+        @end
+
+        void func([[Fo^o]] *f) {}
+      )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
     SCOPED_TRACE(T);
     Annotations Code(T);
     auto TU = TestTU::withCode(Code.code());
     TU.ExtraArgs.push_back("-xobjective-c++");
     auto AST = TU.build();
Case Cases[] = {
       )cpp",
        "not eligible for indexing", HeaderFile},
 
       {R"cpp(// disallow -- namespace symbol isn't supported
         namespace n^s {}
       )cpp",
        "not a supported kind", HeaderFile},
 
+      {R"cpp(// disallow - category rename.
+        @interface Foo
+        @end
+        @interface Foo (Cate^gory)
+        @end
+      )cpp",
+       "Cannot rename symbol: there is no symbol at the given location",
+       HeaderFile},
+
       {
           R"cpp(
          #define MACRO 1
          int s = MAC^RO;
        )cpp",
           "not a supported kind", HeaderFile},
 
       {
EXPECT_THAT(
       applyEdits(std::move(Results->GlobalChanges)),
       UnorderedElementsAre(
           Pair(Eq(BarPath), Eq(expectedResult(BarCode, NewName))),
           Pair(Eq(MainFilePath), Eq(expectedResult(MainCode, NewName)))));
 }
 
 TEST(CrossFileRenameTests, WithUpToDateIndex) {
   MockCompilationDatabase CDB;
-  CDB.ExtraClangFlags = {"-xc++"};
+  CDB.ExtraClangFlags = {"-xobjective-c++"};
   // rename is runnning on all "^" points in FooH, and "[[]]" ranges are the
   // expected rename occurrences.
   struct Case {
     llvm::StringRef FooH;
     llvm::StringRef FooCC;
   } Cases[] = {
       {
           // classes.
llvm::StringRef FooCC;
                   Derived2* d2, NotDerived* nd) {
           b->[[foo]]();
           d1->[[foo]]();
           d2->[[foo]]();
           nd->foo();
         }
       )cpp",
       },
-      {
-          // virtual templated method
-          R"cpp(
+      {// virtual templated method
         template <typename> class Foo { virtual void [[m]](); };
         class Bar : Foo<int> { void [[^m]]() override; };
       )cpp",
-          R"cpp(
           #include "foo.h"
 
           template<typename T> void Foo<T>::[[m]]() {}
           // FIXME: not renamed as the index doesn't see this as an override of
           // the canonical Foo<T>::m().
           // https://github.com/clangd/clangd/issues/1325
           class Baz : Foo<float> { void m() override; };
-        )cpp"
-      },
+        )cpp"},
       {
           // rename on constructor and destructor.
           R"cpp(
         class [[Foo]] {
           [[^Foo]]();
           ~[[Foo^]]();
         };
       )cpp",
llvm::StringRef FooCC;
         #include "foo.h"
         void bar() {
           [[Foo]] x;
           FOO y;
           FooFoo z;
         }
       )cpp",
       },
+      {
+          // Objective-C classes.
+          R"cpp(
+        @interface [[Fo^o]]
+        @end
+      )cpp",
+          R"cpp(
+        #include "foo.h"
+        @implementation [[Foo]]
+        @end
+
+        void func([[Foo]] *f) {}
+      )cpp",
+      },
   };
 
   trace::TestTracer Tracer;
   for (const auto &T : Cases) {
     SCOPED_TRACE(T.FooH);
     Annotations FooH(T.FooH);
     Annotations FooCC(T.FooCC);
     std::string FooHPath = testPath("foo.h");