[clangd] Add ObjC method support to prepareCallHierarchy

[sheldonneuberger-sc]

Summary

This fixes "textDocument/prepareCallHierarchy" in clangd for ObjC methods. Details at https://github.com/clangd/vscode-clangd/issues/247.

clangd uses Decl::isFunctionOrFunctionTemplate to check if the decl given in a prepareCallHierarchy request is eligible for prepareCallHierarchy. We change to use isFunctionOrMethod which includes functions and ObjC methods.

[nridge]

Thanks for the patch.

Could you add a test exercising call hierarchy for obj-c methods to CallHierarchyTests.cpp please?

In terms of the actual change, this function has some other callers in Parser and Sema code, and I don't know how this change will affect them. It may be better to limit the change to the clangd call site for now (that is, change that call site to use a local helper function, which checks isFunctionOrFunctionTemplate() || <obj-c stuff>), and defer changing Decl::isFunctionOrFunctionTemplate() itself to a subsequent refactor.

[kadircet]

I agree with Nathan on this one. It's unclear why there are two functions that does the same thing in different ways and some usages in sema looks suspicious enough to require caution (there are subtle checks for shadowing and whatnot and I don't know how these rules come into play in objc contexts).

So I would suggest updating clangd side to look like:

(isa<DeclContext>(Decl) && cast<DeclContext>(Decl)->isFunctionOrMethod()) || Decl->getlKind() == FunctionTemplate

[sheldonneuberger-sc]

• move change to clangd callsite
• add tests

[sheldonneuberger-sc]

• clang-format

[sheldonneuberger-sc]

• fix test name

[sheldonneuberger-sc]

Thanks for the detailed comments. I moved the patch into the clangd callsite. I also added a couple ObjC tests for CallHierarchy (I basically parameterized two of the existing tests).

[nridge]

Thanks, LGTM!

I have a couple of nits about the test changes, but with those I think this is good to merged.

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

54 ↗

(On Diff #388101)

nit: rather than passing std::string by value, let's use llvm::StringRef

(const std::string& would also be fine, but I think llvm::StringRef is more conventional in this codebase)

125 ↗

(On Diff #388101)

nit: the intendation here seems a bit inconsistent. Can we either indent everything between @implementation and @end, or alternatively un-indent just this line?

[kadircet]

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

54 ↗

(On Diff #388101)

i am not sure if these helpers makes tests easier to read (i believe they're actually making it harder).

please inline them back. if we want to have better infrastructure for testing this, we need to do it in its own patch (and I definitely agree that we need a better infra for testing here. but we need to think about something that can generalize rather than forcing a particular pattern).

[sheldonneuberger-sc]

• inline test helper for readability
• fix indentation

[sheldonneuberger-sc]

• revert whitespace change

[sheldonneuberger-sc]

Made suggested changes, ready for another review.

[kadircet]

thanks, lgtm! let me know of your email address (for commit attribution) if you want me to land this for you.

[nridge]

In D114058#3150606, @kadircet wrote:

thanks, lgtm! let me know of your email address (for commit attribution) if you want me to land this for you.

While I don't see it surfaced anywhere in the Phabricator UI, there is in fact an email address associated with this changeset, and it shows up in the commit created by arc patch D114058.

clang/include/clang/AST/DeclBase.h

public:
   /// Whether this declaration is a parameter pack.
   bool isParameterPack() const;
 
   /// returns true if this declaration is a template
   bool isTemplateDecl() const;
 
   /// Whether this declaration is a function or function template.
   bool isFunctionOrFunctionTemplate() const {
-    return (DeclKind >= Decl::firstFunction &&
-            DeclKind <= Decl::lastFunction) ||
-           DeclKind == FunctionTemplate;
+    switch (DeclKind) {
+    case Decl::Block:
+    case Decl::Captured:
+    case Decl::ObjCMethod:
+    case Decl::FunctionTemplate:
+      return true;
+    default:
+      return DeclKind >= Decl::firstFunction && DeclKind <= Decl::lastFunction;
+    }
   }
 
   /// If this is a declaration that describes some template, this
   /// method returns that template declaration.
   ///
   /// Note that this returns nullptr for partial specializations, because they
   /// are not modeled as TemplateDecls. Use getDescribedTemplateParams to handle
   /// those cases.