[clangd] Improve hover scopes for Objective-C code

[dgoldman]

Summary

• Instead of AppDelegate::application:didFinishLaunchingWithOptions: you will now see -[AppDelegate application:didFinishLaunchingWithOptions:]
• Also include categories in the name when printing the scopes, e.g. Class(Category) and -[Class(Category) method]

[sammccall]

Thanks! This generally looks good, just need to find the right home for some of the logic.

clangd/XRefs.cpp

455 ↗

(On Diff #223646)

XRefs.cpp isn't really the right place for this.
If there isn't somewhere appropriate in clangAST, then making this a case of printName in AST.h is probably best.

It should have unit tests (rather than testing it all through hover)

532 ↗

(On Diff #223646)

Maybe "skip over Type/Function/ObjC methods, these are part of the local scope instead".

(Not sure about "objc decl" as it seems to cover a lot of other things too)

clangd/unittests/XRefsTests.cpp

1029 ↗

(On Diff #223646)

combine test cases with above.
Do we need so many? I think 2 plus some unit-tests of the "pretty-print a decl" code should be enough

1177 ↗

(On Diff #223646)

you've got a loop here but only one test case. Combine with the other loop or just assert directly?

[kadircet]

clangd/XRefs.cpp

461 ↗

(On Diff #223646)

it looks like DeclPrinter::VisitObjCMethodDecl already has a similar handling.
would it provide value if you printed class and category names in the general case? If so it might make sense to update the logic in DeclPrinter.
If not, it looks like printing selector via getAsString is not enough, there's some special handling in DeclPrinter. Maybe move it into a public
place and make use of the same logic also in here to be consistent?

470 ↗

(On Diff #223646)

again there are printers in for ObjCCategory{Impl}Decls that don't respect TerseOutput option in printing policies. It might be sensible to update them instead to print an output like what you propose in here, in the presence of TerseOutput option. WDYT?

509 ↗

(On Diff #223646)

instead of saying what you do, could you say why you do it. IIUC, it is because the relevant bits of the container is already printed in getNameForObjCMethod right?

[dgoldman]

Will revisit this once more critical fixes are in (crash fixes), I'm still not sure where this sort of stuff should belong

clangd/XRefs.cpp

461 ↗

(On Diff #223646)

DeclPrinter::VisitObjCMethodDecl prints the declaration itself, e.g.:

- (void)someMethod:(int)param;

it wouldn't make sense to instead of that do

-[MyClass someMethod:] for the declaration itself

470 ↗

(On Diff #223646)

What method names are you looking at? I wasn't able to find them

[dgoldman]

We'll also want to do something similar for DocumentSymbols, see here, which will lead to Objective-C categories showing up as either (anonymous) or <category name>

[dgoldman]

• Rebase + refactor, sorry for the long delay

[dgoldman]

I think there's still some more work to be done after this which might move some of this around:

• DocumentSymbol support: Currently categories show up as (anonymous) or <category name> (see here).
• QualifiedNames for ObjC, see the conversation here, possibly MyClass.property and MyClass(Category)->Ivar? Someone from Apple might know more. The current :: behavior is from here

[dgoldman]

Swap to isa

[dgoldman]

Add protocol test

[dgoldman]

I messed around with qualified name changes - lots of things internally rely upon the qualified names so that will be best for a separate change. I could potentially handle DocumentSymbol fixes in here though - LMK if you think it makes sense to move some of this logic in AST.cpp (clangd). objcContainerLocalScope seems like it would be useful to generalize support for objc container decls by ensuring that categories becoming fully qualified with their class name.

[dgoldman]

Friendly ping, I think this is still worth merging in even without the QName changes

[sammccall]

Sorry for losing this.

LMK if you think it makes sense to move some of this logic in AST.cpp (clangd). objcContainerLocalScope seems like it would be useful to generalize support for objc container decls by ensuring that categories becoming fully qualified with their class name

Yes, please move these to AST.cpp, maybe rename to printObjCMethod and printObjCContainer.

clang-tools-extra/clangd/Hover.cpp

64

this function's name doesn't really describe what it's for

it has 3 callsites and is only needed in 2, just inline it?

71

looking at the implementation, this is null iff the method is part of a protocol.
<<error-type>> doesn't seem appropriatefor that case

78

prefer CID->getName(), the operator<< is really surprising here

80

nit: Method->getSelector().print(OS << ' ');

(yeah, the implementation is dumb, but maybe they'll fix it)

85

you're neither flushing nor destroying the OS, this is fragile.

(From playing with godbolt, it looks like this works as long as move-elision kicks in, but adding if (false) return "" at the top prevents the stream from being flushed here)

96

can't you return objcContainerLocalScope(CI->getCategoryDecl()) ?

149

nit: isa<ObjCMethodDecl, ObjCContainerDecl>(DC)

I'm not sure the comment adds much here (particularly "we return an empty string") - maybe remove?

[dgoldman]

• Address review comments

Will move over to AST.cpp in next update

clang-tools-extra/clangd/Hover.cpp

64

Worth keeping, see below, Open to naming suggestions =)

71

It can also be NULL for invalid code (in category).

Since we only call this for Decls in this decl context protocols can't ever appear as protocols can't have method definitions. Do you think it's still worth handling?

96

As noted in getCategoryDecl() the class interface can be NULL when working with invalid code.

[sammccall]

clang-tools-extra/clangd/Hover.cpp

64

nameOrError?

71

ah, I see...

yes I think the protocol case is worth handling in order to give this a simple signature in AST.h (prettyprint an ObjC method, without any surprising caveats)

[dgoldman]

• Move over to AST.cpp

[dgoldman]

• Support protocols

clang-tools-extra/clangd/Hover.cpp

 #include "index/SymbolCollector.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
PrintingPolicy printingPolicyForDecls(PrintingPolicy Base) {
   Policy.TerseOutput = true;
   Policy.PolishForDeclaration = true;
   Policy.ConstantsAsWritten = true;
   Policy.SuppressTagKeyword = false;
 
   return Policy;
 }
 
+static llvm::StringRef getNameForObjCInterface(const ObjCInterfaceDecl *ID) {
+  return ID ? ID->getName() : "<<error-type>>";
+}
+
+static std::string objcMethodLocalScope(const ObjCMethodDecl *Method) {
+  std::string Name;
+  llvm::raw_string_ostream OS(Name);
+  const ObjCInterfaceDecl *Class = Method->getClassInterface();
+
+  OS << (Method->isInstanceMethod() ? '-' : '+') << '['
+     << getNameForObjCInterface(Class);
+
+  if (const ObjCCategoryImplDecl *CID =
+          dyn_cast<ObjCCategoryImplDecl>(Method->getDeclContext()))
+    OS << '(' << *CID << ')';
+
+  OS << ' ' << Method->getSelector().getAsString();
+  if (Method->isVariadic())
+    OS << ", ...";
+
+  OS << ']';
+  return Name;
+}
+
+static std::string objcContainerLocalScope(const ObjCContainerDecl *C) {
+  if (const ObjCCategoryDecl *Category = dyn_cast<ObjCCategoryDecl>(C)) {
+    std::string Name;
+    llvm::raw_string_ostream OS(Name);
+    const ObjCInterfaceDecl *Class = Category->getClassInterface();
+    OS << getNameForObjCInterface(Class) << '(' << Category->getName() << ')';
+    return Name;
+  }
+  if (const ObjCCategoryImplDecl *CI = dyn_cast<ObjCCategoryImplDecl>(C)) {
+    std::string Name;
+    llvm::raw_string_ostream OS(Name);
+    const ObjCInterfaceDecl *Class = CI->getClassInterface();
+    OS << getNameForObjCInterface(Class) << '(' << *CI << ')';
+    return Name;
+  }
+  return C->getNameAsString();
+}
+
 /// Given a declaration \p D, return a human-readable string representing the
 /// local scope in which it is declared, i.e. class(es) and method name. Returns
 /// an empty string if it is not local.
 std::string getLocalScope(const Decl *D) {
   std::vector<std::string> Scopes;
   const DeclContext *DC = D->getDeclContext();
+
+  // ObjC scopes won't have multiple components for us to join, instead:
+  // - Methods: "-[Class methodParam1:methodParam2]"
+  // - Classes, categories, and protocols: "MyClass(Category)"
+  if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(DC))
+    return objcMethodLocalScope(MD);
+  else if (const ObjCContainerDecl *CD = dyn_cast<ObjCContainerDecl>(DC))
+    return objcContainerLocalScope(CD);
+
   auto GetName = [](const TypeDecl *D) {
     if (!D->getDeclName().isEmpty()) {
       PrintingPolicy Policy = D->getASTContext().getPrintingPolicy();
       Policy.SuppressScope = true;
       return declaredType(D).getAsString(Policy);
     }
     if (auto RD = dyn_cast<RecordDecl>(D))
       return ("(anonymous " + RD->getKindName() + ")").str();
std::string getLocalScope(const Decl *D) {
   return llvm::join(llvm::reverse(Scopes), "::");
 }
 
 /// Returns the human-readable representation for namespace containing the
 /// declaration \p D. Returns empty if it is contained global namespace.
 std::string getNamespaceScope(const Decl *D) {
   const DeclContext *DC = D->getDeclContext();
 
+  // ObjC does not have the concept of namespaces, so we return an empty string
+  // and instead support local scopes.
+  if (isa<ObjCMethodDecl>(DC) || isa<ObjCContainerDecl>(DC))
+    return "";
+
   if (const TagDecl *TD = dyn_cast<TagDecl>(DC))
     return getNamespaceScope(TD);
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(DC))
     return getNamespaceScope(FD);
   if (const NamespaceDecl *NSD = dyn_cast<NamespaceDecl>(DC)) {
     // Skip inline/anon namespaces.
     if (NSD->isInline() || NSD->isAnonymousNamespace())
       return getNamespaceScope(NSD);

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

const std::function<void(HoverInfo &)> ExpectedBuilder;
            @interface ObjC {
              char [[da^ta]];
            }@end
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "data";
             HI.Type = "char";
             HI.Kind = index::SymbolKind::Field;
-            HI.NamespaceScope = "ObjC::"; // FIXME: fix it
+            HI.LocalScope = "ObjC::";
+            HI.NamespaceScope = "";
             HI.Definition = "char data";
           }},
       {
           R"cpp(
           @interface MYObject
           @end
           @interface Interface
           @property(retain) [[MYOb^ject]] *x;
const std::function<void(HoverInfo &)> ExpectedBuilder;
           @end
           )cpp",
           [](HoverInfo &HI) {
             HI.Name = "MYObject";
             HI.Kind = index::SymbolKind::Class;
             HI.NamespaceScope = "";
             HI.Definition = "@interface MYObject\n@end";
           }},
+      {
+          R"cpp(
+          @interface MYObject
+          @end
+          @interface MYObject (Private)
+          @property(nonatomic, assign) int privateField;
+          @end
+
+          int someFunction() {
+            MYObject *obj = [MYObject sharedInstance];
+            return obj.[[private^Field]];
+          }
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "privateField";
+            HI.Kind = index::SymbolKind::InstanceProperty;
+            HI.LocalScope = "MYObject(Private)::";
+            HI.NamespaceScope = "";
+            HI.Definition = "@property(nonatomic, assign, unsafe_unretained, "
+                            "readwrite) int privateField;";
+          }},
+      {
+          R"cpp(
+          @protocol MYProtocol
+          @property(nonatomic, assign) int prop1;
+          @end
+
+          int someFunction() {
+            id<MYProtocol> obj = 0;
+            return obj.[[pro^p1]];
+          }
+          )cpp",
+          [](HoverInfo &HI) {
+            HI.Name = "prop1";
+            HI.Kind = index::SymbolKind::InstanceProperty;
+            HI.LocalScope = "MYProtocol::";
+            HI.NamespaceScope = "";
+            HI.Definition = "@property(nonatomic, assign, unsafe_unretained, "
+                            "readwrite) int prop1;";
+          }},
+      {R"objc(
+        @interface Foo
+        @end
+
+        @implementation Foo(Private)
+        + (int)somePrivateMethod {
+          int [[res^ult]] = 2;
+          return result;
+        }
+        @end
+        )objc",
+       [](HoverInfo &HI) {
+         HI.Name = "result";
+         HI.Definition = "int result = 2";
+         HI.Kind = index::SymbolKind::Variable;
+         HI.Type = "int";
+         HI.LocalScope = "+[Foo(Private) somePrivateMethod]::";
+         HI.NamespaceScope = "";
+         HI.Value = "2";
+       }},
+      {R"objc(
+        @interface Foo
+        @end
+
+        @implementation Foo
+        - (int)variadicArgMethod:(id)first, ... {
+          int [[res^ult]] = 0;
+          return result;
+        }
+        @end
+        )objc",
+       [](HoverInfo &HI) {
+         HI.Name = "result";
+         HI.Definition = "int result = 0";
+         HI.Kind = index::SymbolKind::Variable;
+         HI.Type = "int";
+         HI.LocalScope = "-[Foo variadicArgMethod:, ...]::";
+         HI.NamespaceScope = "";
+         HI.Value = "0";
+       }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
   Symbol IndexSym = func("indexSymbol");
   IndexSym.Documentation = "comment from index";
   SymbolSlab::Builder Symbols;
   Symbols.insert(IndexSym);
   auto Index =