This is an archive of the discontinued LLVM Phabricator instance.

[clangd] ObjC fixes for semantic highlighting and xref highlights
ClosedPublic

Authored by sammccall on Feb 27 2021, 1:29 PM.

Details

Summary
  • highlight references to protocols in class/protocol/extension decls
  • support multi-token selector highlights in semantic + xref highlights (method calls and declarations only)
  • In @interface I(C), I now references the interface and C the category
  • highlight uses of interfaces as types
  • added semantic highlightings of protocol names (as "interface") and category names (as "namespace"). These are both standard kinds, maybe "extension" will be standardized...
  • highlight auto as "class" when it resolves to an ObjC pointer
  • don't highlight self as a variable even though the AST models it as one

Not fixed: uses of protocols in type names (needs some refactoring of
unrelated code first)

Diff Detail

Event Timeline

sammccall created this revision.Feb 27 2021, 1:29 PM
sammccall requested review of this revision.Feb 27 2021, 1:29 PM

(not really sure why this suddenly seemed important to me, but if you don't have semantic highlighting on, you're missing out!)

(not really sure why this suddenly seemed important to me, but if you don't have semantic highlighting on, you're missing out!)

Thanks!

clang-tools-extra/clangd/FindTarget.cpp
657

Should this be isThisDeclarationADefinition()? Is it even needed (will the protocol list just be empty otherwise)?

659

Hmm, for what? might be better to say why/what it does

668

IIRC I think it's possible that these could be NULL if the code was invalid, is this NULL safe?

687–689

Same comments here as the interface code above

775

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?

clang-tools-extra/clangd/XRefs.cpp
927–930

Hmm, is this intentional? Not sure what else could reference it besides maybe a property?

sammccall updated this revision to Diff 327445.Mar 2 2021, 7:17 AM
sammccall marked 6 inline comments as done.

address comments

clang-tools-extra/clangd/FindTarget.cpp
657

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).

659

Changed the comment.

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

668

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.

775

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.

clang-tools-extra/clangd/XRefs.cpp
927–930

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.

dgoldman accepted this revision.Mar 3 2021, 7:15 AM
This revision is now accepted and ready to land.Mar 3 2021, 7:15 AM
This revision was landed with ongoing or failed builds.Mar 3 2021, 11:16 AM
This revision was automatically updated to reflect the committed changes.