[PATCH] D98984: [clangd] Improve handling of Objective-C protocols in types

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

LG apart from a few nits & reverting the whitespace changes to unrelated tests.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:435
       void VisitObjCObjectType(const ObjCObjectType *OOT) {
-        // FIXME: ObjCObjectTypeLoc has no children for the protocol list, so
-        // there is no node in id<Foo> that refers to ObjCProtocolDecl Foo.
-        if (OOT->isObjCQualifiedId() && OOT->getNumProtocols() == 1)
-          Outer.add(OOT->getProtocol(0), Flags);
+        unsigned NumProtocols = OOT->getNumProtocols();
+        for (unsigned I = 0; I < NumProtocols; I++)
----------------
dgoldman wrote:
> Let me know if I'm missing something here, not exactly sure what the old comment meant - is there a problem since this ObjCObjectType  can refer to multiple protocols?
Here's the way the AST should ideally look like (and what this comment is wishing for):

```
ObjCObjectTypeLoc C<Foo>
 - ObjCInterfaceTypeLoc C
 - ObjCProtocolLoc Foo // doesn't actually exist
```

And targetDecl() would work on the two children, but not the parent.

But there's no AST node for protocols. A piece of cleverness in this patch is getting go-to-definition to work on protocols by saying "if the cursor is on ObjCObjectType itself, then it must be on a protocol rather than the base (otherwise it'd be on the ObjCInterfaceType child)".

But it's a hack, as evidenced by the fact that operations that work on the *range* `[[C<Foo>]]` will still see the target as `<Foo>` (literally any of nothing, C, C+Foo would be better).

Nevertheless I think it's probably a net win.

---

Can you leave a comment here like 
```
// There's no child node for just a protocol, so make all the protocols targets.
// But not the base type, which *does* have a child ObjCInterfaceTypeLoc.
// This structure is a hack, but it works well for go-to-definition.
```


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:824
         return;
+      ReferenceLoc *Ref = &Refs.back();
+      // Fill in the qualifier.
----------------
this doesn't seem right: we now have a list, the Visit() call may have added things to the list, we need to adjust *all* the added things.

```
// Add qualifier for the newly-added refs.
for (unsigned I = InitialSize; I < Refs.size(); ++I) {
 // adjust Refs[I]
}
```


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:912
+      }
+      // FIXME: Maybe add references to protocols in ObjCObjectPointerTypeLoc.
+    }
----------------
dgoldman wrote:
> Doesn't look like that's needed, but if we wanted to decorate the type parameters in `ObjCObjectTypeLoc` with `typeParameter` would that just go in `SemanticHighlighting.cpp`?
That's not something we want to do - ObjCObjectTypeLoc has type arguments, not parameters.
(Not trying to be pedantic with terminology, but the difference is critical here)

e.g. in the function call `foo(42)` we don't decorate 42 as a parameter, we decorate it as an integer.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:952
   )cpp";
-  // FIXME: there's no AST node corresponding to 'Foo', so we're stuck.
-  EXPECT_DECLS("ObjCObjectTypeLoc");
+  EXPECT_DECLS("ObjCObjectTypeLoc", "@protocol Foo");
 }
----------------
can you add a second protocol so we can see the boundaries of this approach?

(I think C<[[Foo]], Bar> should resolve to both @protocol Foo and @protocol Bar)


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:1048
         )cpp",
-           "0: targets = {global}\n"
-           "1: targets = {param}\n"
----------------
there are a ton of whitespace changes in this patch, because the version at HEAD isn't clang-format clean :-(

Feel free to format it first in a separate commit, but I'd prefer not to reformat all the tests mixed in with other changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98984