[clangd] Improve printing of Objective-C categories and methods

[dgoldman]

Summary

• Categories will now show up as MyClass(Category) instead of Category and MyCategory() instead of (anonymous) in document symbols
• Methods will now be shown as -selector: or +selector: instead of selector: to differentiate between instance and class methods in document symbols

[njames93]

I'm not an objc guy, but can you add a test case demonstrating class(?) methods showing as +name:?

clang-tools-extra/clangd/AST.cpp

224–226 ↗

(On Diff #323349)

nit: Can make these const auto * as the type is spelt in the initialisation.

[dgoldman]

• Add class method test case + swap to auto

[sammccall]

clang-tools-extra/clangd/AST.cpp

224 ↗

(On Diff #323443)

I'm not sure this fits with the contract of printName, which really is to print the name of the symbol rather than describe it.

Examples:

• the title of a hovercard for a method will be "instance-method -[foo]" instead of "instance-method foo", which is inconsistent with c/c++
• the title of a hovercard for a category will be "extension PotentiallyLongClassName(CatName)" instead of extension ClassName
• the documentSymbol response will include the extra details in the name field rather than the detail field

We don't actually populate detail in documentSymbol, so maybe we do need a separate function that gives that level of detail.
Though I wonder whether it should mosty just print the decl (e.g. class X{};, @implementation PotentiallyLongClassName(CatName) etc. WDYT?

See also https://github.com/clangd/clangd/issues/520 https://github.com/clangd/clangd/issues/601 though both appear to have stalled.

228 ↗

(On Diff #323443)

in the other patch this was with -[brackets]

[dgoldman]

clang-tools-extra/clangd/AST.cpp

224 ↗

(On Diff #323443)

Maybe instead this should be split somehow into a separate specialization for Document Symbols?

e.g.

Hovercards:
instance-method foo
extension (anonymous) (current) or class-extension BTNButtonModel or BTNButtonModel()

Document Symbols:
-method
ClassName(Ext)

Since I think for the case here we would prefer to keep them different. Maybe if the LSP spec differentiated between instance methods and class/static methods -method that wouldn't be necessary? I guess I could revert the Method change here and just keep a special case for categories/class extensions?

The detail work seems worth doing too, but I think we would similarly just put the type information in there (for methods), for categories/class extensions I feel like it would be odd since it would at the very least repeat the name

[dgoldman]

clang-tools-extra/clangd/AST.cpp

228 ↗

(On Diff #323443)

We don't want to put the full form (e.g. including the container name) here - that would repeat the class name in the document symbols when the methods are already nested under the class.

[dgoldman]

Limit changes to document symbols

[sammccall]

Great, thanks!

clang-tools-extra/clangd/FindSymbols.cpp

176

can we motivate this with a comment?
// print X rather than Y

[dgoldman]

Add comment and rebase

clang-tools-extra/clangd/FindSymbols.cpp

Index->fuzzyFind(Req, [HintPath, &Top, &Filter, AnyScope = Req.AnyScope,
     Top.push({Score, std::move(Info)});
   });
   for (auto &R : std::move(Top).items())
     Result.push_back(std::move(R.second));
   return Result;
 }
 
 namespace {
+std::string getSymbolName(ASTContext &Ctx, const NamedDecl &ND) {
+  // Print `MyClass(Category)` instead of `Category` and `MyClass()` instead
+  // of `anonymous`.
+  if (const auto *Container = dyn_cast<ObjCContainerDecl>(&ND))
+    return printObjCContainer(*Container);
+  // Differentiate between class and instance methods: print `-foo` instead of
+  // `foo` and `+sharedInstance` instead of `sharedInstance`.
+  if (const auto *Method = dyn_cast<ObjCMethodDecl>(&ND)) {
+    std::string Name;
+    llvm::raw_string_ostream OS(Name);
+
+    OS << (Method->isInstanceMethod() ? '-' : '+');
+    Method->getSelector().print(OS);
+
+    OS.flush();
+    return Name;
+  }
+  return printName(Ctx, ND);
+}
+
 std::string getSymbolDetail(ASTContext &Ctx, const NamedDecl &ND) {
   PrintingPolicy P(Ctx.getPrintingPolicy());
   P.SuppressScope = true;
   P.SuppressUnwrittenScope = true;
   P.AnonymousTagLocations = false;
   P.PolishForDeclaration = true;
   std::string Detail;
   llvm::raw_string_ostream OS(Detail);
if (!SymbolRange)
     return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
   // FIXME: This is not classifying constructors, destructors and operators
   // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;
-  SI.name = printName(Ctx, ND);
+  SI.name = getSymbolName(Ctx, ND);
   SI.kind = SK;
   SI.deprecated = ND.isDeprecated();
   SI.range = Range{sourceLocToPosition(SM, SymbolRange->getBegin()),
                    sourceLocToPosition(SM, SymbolRange->getEnd())};
   SI.detail = getSymbolDetail(Ctx, ND);
 
   SourceLocation NameLoc = ND.getLocation();
   SourceLocation FallbackNameLoc;

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

EXPECT_THAT(
                 WithDetail("template auto (T, T) -> decltype(x + y)")),
           AllOf(WithName("Pair"), WithDetail("template class")),
           AllOf(WithName("Context"), WithDetail("template struct"),
                 Children(AllOf(
                     WithName("Pair<type-parameter-0-0, type-parameter-0-1>"),
                     WithDetail("<dependent type>"))))));
 }
 
+TEST(DocumentSymbolsTest, ObjCCategoriesAndClassExtensions) {
+  TestTU TU;
+  TU.ExtraArgs = {"-xobjective-c++", "-Wno-objc-root-class"};
+  Annotations Main(R"cpp(
+      $Cat[[@interface Cat
+      + (id)sharedCat;
+      @end]]
+      $SneakyCat[[@interface Cat (Sneaky)
+      - (id)sneak:(id)behavior;
+      @end]]
+
+      $MeowCat[[@interface Cat ()
+      - (void)meow;
+      @end]]
+      $PurCat[[@interface Cat ()
+      - (void)pur;
+      @end]]
+    )cpp");
+  TU.Code = Main.code().str();
+  EXPECT_THAT(
+      getSymbols(TU.build()),
+      ElementsAre(
+          AllOf(WithName("Cat"), SymRange(Main.range("Cat")),
+                Children(AllOf(WithName("+sharedCat"),
+                               WithKind(SymbolKind::Method)))),
+          AllOf(WithName("Cat(Sneaky)"), SymRange(Main.range("SneakyCat")),
+                Children(
+                    AllOf(WithName("-sneak:"), WithKind(SymbolKind::Method)))),
+          AllOf(
+              WithName("Cat()"), SymRange(Main.range("MeowCat")),
+              Children(AllOf(WithName("-meow"), WithKind(SymbolKind::Method)))),
+          AllOf(WithName("Cat()"), SymRange(Main.range("PurCat")),
+                Children(
+                    AllOf(WithName("-pur"), WithKind(SymbolKind::Method))))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang