[PATCH] D97617: [clangd] ObjC fixes for semantic highlighting and xref highlights

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:657
+    void VisitObjCInterfaceDecl(const ObjCInterfaceDecl *OID) {
+      if (OID->hasDefinition())
+        visitProtocolList(OID->protocols(), OID->protocol_locs());
----------------
dgoldman wrote:
> Should this be isThisDeclarationADefinition()? Is it even needed (will the protocol list just be empty otherwise)?
Indeed it should be isThisDeclarationADefinition().
And I think this is why we need the check - otherwise if this is a forward decl, we'll still traverse the protocols at any visible definition.

(getReferencedProtocols() asserts, and I was calling that at some point, which is why I originially had this check).


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:659
+        visitProtocolList(OID->protocols(), OID->protocol_locs());
+      Base::VisitObjCInterfaceDecl(OID); // Visit the interface itself.
+    }
----------------
dgoldman wrote:
> Hmm, for what? might be better to say why/what it does
Changed the comment.

(this walks back up the hierarchy, landing at VisitNamedDecl, which highlights the primary name token)


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:668
+                                  /*IsDecl=*/false,
+                                  {OCD->getClassInterface()}});
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
----------------
dgoldman wrote:
> IIRC I think it's possible that these could be NULL if the code was invalid, is this NULL safe?
Good point. But checking for nulls is tedious, and guessing which cases can't ever be null is a fool's game. Made it null-safe in https://github.com/llvm/llvm-project/commit/7556abf82137b57be9e32475a1995f936a22cd16 instead.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:774
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+                                  E->getSelectorStartLoc(),
+                                  /*IsDecl=*/false, Targets});
----------------
dgoldman wrote:
> What is this location used for? I guess it's not possible to have multiple Locs here like we have for a selector and you specifically handle for semantic highlighting below?
Yup, the abstraction only lets us report one location/token, and it's not worth complicating for one case IMO, so we hack around it at the caller.

There are two ideas for the specific location
 - it's a reasonable fallback behavior
 - callers that do handle the multi-token case themselves can use this location to match up the overlapping data. (As SemanticTokens does here and DocumentHighlight does too albeit it uses IndexDataConsumer code rather than findExplicitReferences)

Added a comment noting the limitation.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:927-930
+      // Possible it was something else referencing an ObjCMethodDecl.
+      // If the first token doesn't match, assume our guess was wrong.
+      if (!Locs.empty() && Locs.front() != Loc)
+        Locs.clear();
----------------
dgoldman wrote:
> Hmm, is this intentional? Not sure what else could reference it besides maybe a property?
I'm not sure either, but IME assuming we've covered every possible case in the AST tends to be a bad idea :-) This is mostly a defensive check (though now you mention it, I suppose property access might hit this.

Note that isa<ObjCMessageExpr>(OrigE) implies we're indexing a message expr, but isa<ObjCMethodDecl>(OrigD) *doesn't* imply we're indexing a method decl! (When indexing a message expr, OrigD is set to the target method, so the order of if/else is significant here).
This makes me feel like it would be quite plausible to go off the rails without this check.

I've tweaked the comments a bit here to reinforce this is a sanity check.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97617/new/

https://reviews.llvm.org/D97617