[clangd] Don't highlight ObjC `id` and `instancetype`

[dgoldman]

Summary

Even though they're implemented via typedefs, we typically
want to treat them like keywords (default editor behavior).

[kadircet]

thanks, it looks good as a contained fix. but it feels like we probably don't want these to be treated as decls in other places too (e.g. can we really provide any useful goto/hover on those id or instancetype decls?) maybe we should update ASTVisitors in FindTarget to not report these at all. WDYT?

[dgoldman]

In D108556#2962008, @kadircet wrote:

thanks, it looks good as a contained fix. but it feels like we probably don't want these to be treated as decls in other places too (e.g. can we really provide any useful goto/hover on those id or instancetype decls?) maybe we should update ASTVisitors in FindTarget to not report these at all. WDYT?

That's a good point - I guess it could be marginally useful?

Hovering over instancetype you'll see typedef id instancetype and can be brought via goto definition to objc.h which defines typedef struct objc_object *id;. Hovering over id you see typedef id id (which is quite confusing) and go-to definition brings you to objc.h as well. In theory the hover information should show you A pointer to an instance of a class. (comment from objc.h), seems to have worked previously, not sure why it doesn't atm. References doesn't appear to work and you'd really never want to use it if it did - there'd be way too many.

[kadircet]

Hmm, it sounds like you want them to be treated one way during semantic highlighting and another during other features, which is fine but somewhat confusing. (e.g. we want to surface hover/goto on these identifiers, but we're making them less visible by encouraging editors to highlight them as keywords).

[dgoldman]

Swap to ignore in FindTarget

[dgoldman]

In D108556#2981712, @kadircet wrote:

Hmm, it sounds like you want them to be treated one way during semantic highlighting and another during other features, which is fine but somewhat confusing. (e.g. we want to surface hover/goto on these identifiers, but we're making them less visible by encouraging editors to highlight them as keywords).

I went ahead and just ignored them in FindTarget (LMK if that's right) - I think you're right that it's unlikely to provide any real value so we might as well not support it.

[kadircet]

thanks, this looks great! just a question about the extra handling

clang-tools-extra/clangd/FindTarget.cpp

100

i think comment needs updating. rather than mentioning the editor, can we just talk about "these should be treated as keywords rather than decls" ?

187

why do we need this despite bailing out in the visitor?

[dgoldman]

Update comment + remove un-needed code

[dgoldman]

clang-tools-extra/clangd/FindTarget.cpp

187

Removed, not necessary

[dgoldman]

Friendly ping, PTAL

[kadircet]

sorry, I thought I've already LGTM'd it in previous iteration. Thanks!

clang-tools-extra/clangd/FindTarget.cpp

if (const auto *ED = dyn_cast<EnumDecl>(ECD->getDeclContext())) {
         for (const NamedDecl *BaseECD : Pattern->lookup(ECD->getDeclName()))
           return BaseECD;
       }
     }
   }
   return nullptr;
 }
 
+// Returns true if the `TypedefNameDecl` should not be reported.
+bool shouldSkipTypedef(const TypedefNameDecl *TD) {
+  // These should be treated as keywords rather than decls - the typedef is an
+  // odd implementation detail.
+  if (TD == TD->getASTContext().getObjCInstanceTypeDecl() ||
+      TD == TD->getASTContext().getObjCIdDecl())
+    return true;
+  return false;
+}
+
 // TargetFinder locates the entities that an AST node refers to.
 //
 // Typically this is (possibly) one declaration and (possibly) one type, but
 // may be more:
 //  - for ambiguous nodes like OverloadExpr
 //  - if we want to include e.g. both typedefs and the underlying type
 //
 // This is organized as a set of mutually recursive helpers for particular node
void add(const Decl *Dcl, RelSet Flags) {
     if (!Res.second && Res.first->second.contains(Flags))
       return;
     Res.first->second |= Flags;
 
     if (const UsingDirectiveDecl *UDD = llvm::dyn_cast<UsingDirectiveDecl>(D))
       D = UDD->getNominatedNamespaceAsWritten();
 
     if (const TypedefNameDecl *TND = dyn_cast<TypedefNameDecl>(D)) {
       add(TND->getUnderlyingType(), Flags | Rel::Underlying);
       Flags |= Rel::Alias; // continue with the alias.
     } else if (const UsingDecl *UD = dyn_cast<UsingDecl>(D)) {
       // no Underlying as this is a non-renaming alias.
       for (const UsingShadowDecl *S : UD->shadows())
         add(S->getUnderlyingDecl(), Flags);
       Flags |= Rel::Alias; // continue with the alias.
     } else if (const UsingEnumDecl *UED = dyn_cast<UsingEnumDecl>(D)) {
       add(UED->getEnumDecl(), Flags);
struct Visitor : public TypeVisitor<Visitor> {
         if (Outer.Resolver) {
           for (const NamedDecl *ND :
                Outer.Resolver->resolveTemplateSpecializationType(DTST)) {
             Outer.add(ND, Flags);
           }
         }
       }
       void VisitTypedefType(const TypedefType *TT) {
+        if (shouldSkipTypedef(TT->getDecl()))
+          return;
         Outer.add(TT->getDecl(), Flags);
       }
       void
       VisitTemplateSpecializationType(const TemplateSpecializationType *TST) {
         // Have to handle these case-by-case.
 
         // templated type aliases: there's no specialized/instantiated using
         // decl to point to. So try to find a decl for the underlying type
void VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
       Refs.push_back(
           ReferenceLoc{L.getQualifierLoc(), L.getNameLoc(),
                        /*IsDecl=*/false,
                        explicitReferenceTargets(
                            DynTypedNode::create(L.getType()), {}, Resolver)});
     }
 
     void VisitTypedefTypeLoc(TypedefTypeLoc L) {
+      if (shouldSkipTypedef(L.getTypedefNameDecl()))
+        return;
       Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
                                   L.getNameLoc(),
                                   /*IsDecl=*/false,
                                   {L.getTypedefNameDecl()}});
     }
 
     void VisitObjCInterfaceTypeLoc(ObjCInterfaceTypeLoc L) {
       Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),

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

Code = R"cpp(
     @implementation Foo
     + (id)sharedInstance { return 0; }
     @end
     void test() {
       id value = Foo.[[sharedInstance]];
     }
   )cpp";
   EXPECT_DECLS("ObjCPropertyRefExpr", "+ (id)sharedInstance");
+
+  Code = R"cpp(
+    @interface Foo
+    + ([[id]])sharedInstance;
+    @end
+  )cpp";
+  EXPECT_DECLS("TypedefTypeLoc");
+
+  Code = R"cpp(
+    @interface Foo
+    + ([[instancetype]])sharedInstance;
+    @end
+  )cpp";
+  EXPECT_DECLS("TypedefTypeLoc");
 }
 
 class FindExplicitReferencesTest : public ::testing::Test {
 protected:
   struct AllRefs {
     std::string AnnotatedCode;
     std::string DumpedReferences;
   };

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

)cpp",
       )cpp",
       R"cpp(
         // ObjC: Classes and methods
         @class $Class_decl[[Forward]];
 
         @interface $Class_decl[[Foo]]
         @end
         @interface $Class_decl[[Bar]] : $Class[[Foo]]
-        -($Class[[id]]) $Method_decl[[x]]:(int)$Parameter_decl[[a]] $Method_decl[[y]]:(int)$Parameter_decl[[b]];
+        -(id) $Method_decl[[x]]:(int)$Parameter_decl[[a]] $Method_decl[[y]]:(int)$Parameter_decl[[b]];
+        +(instancetype)$StaticMethod_decl_static[[sharedInstance]];
         +(void) $StaticMethod_decl_static[[explode]];
         @end
         @implementation $Class_decl[[Bar]]
-        -($Class[[id]]) $Method_decl[[x]]:(int)$Parameter_decl[[a]] $Method_decl[[y]]:(int)$Parameter_decl[[b]] {
+        -(id) $Method_decl[[x]]:(int)$Parameter_decl[[a]] $Method_decl[[y]]:(int)$Parameter_decl[[b]] {
           return self;
         }
+        +(instancetype)$StaticMethod_decl_static[[sharedInstance]] { return 0; }
         +(void) $StaticMethod_decl_static[[explode]] {}
         @end
 
         void $Function_decl[[m]]($Class[[Bar]] *$Parameter_decl[[b]]) {
           [$Parameter[[b]] $Method[[x]]:1 $Method[[y]]:2];
           [$Class[[Bar]] $StaticMethod_static[[explode]]];
         }
       )cpp",