[PATCH] D99975: [clangd][ObjC] Improve support for class properties

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:309-313
+        // FIXME: visiting this here allows us to hover on UIColor in
+        // `UIColor.blackColor` but then `blackColor` no longer refers to the
+        // method.
+        // if (OPRE->isClassReceiver())
+        //   Outer.add(OPRE->getClassReceiver(), Flags);
----------------
dgoldman wrote:
> sammccall wrote:
> > dgoldman wrote:
> > > Is there a good way to handle this case where the expression can refer to multiple decls?
> > TL;DR: yes, fix the AST :-)
> > 
> > The design assumption is that an AST node (specifically: the tokens owned by exactly that AST node and not a descendant of it) refers to one thing. I don't see an easy way to hack around this.
> > 
> > Expressions like this are supposed to be split into multiple AST nodes. But ObjC seems to be missing AST nodes in a bunch of places (Protocol lists is another).
> > 
> > e.g. in C++ equivalent DeclRefExpr `UIColor::blackColor` there'd be both a NestedNameSpecifier for `UIColor::` and a TagTypeLoc for `UIColor`. And objc `[Test foo]` has an `ObjCInterfaceTypeLoc` for `Test`.
> > 
> > ---
> > 
> > I'm not sure this is the right place for this comment, the problem isn't what VisitObjCPropertyRefExpr is doing, the issue is that the caller is passing the wrong kind of node to findTarget() because there's no right one.
> That seems a bit invasive - is it really "fixing" the AST - is there anything wrong with the current AST design or rather any notable improvements by adding more nodes to the AST beside this one use case here? 
> 
> It seems like the design could be adapted if we had a SourceLocation hint here (from the caller) to disambiguate which one we mean, but I'm guessing not all of the callers/users of this would have one?
> That seems a bit invasive
I'm not sure it actually requires any changes to AST classes, just to RecursiveASTVisitor to wrap the getClassReceiverType() & getReceiverLocation() into n ObjCInterfaceTypeLoc (which is a value object) and visit it.

> is it really "fixing" the AST

It's inconsistent with the design of the rest of the AST.
There are very few other cases where AST nodes have this kind of compound meaning rather than being to split into sub-nodes.
(In fact ObjC protocol lists are the only example I can think of. I suspect this dates back to early clang days, maybe before the patterns became established)
And similarly, there are few/no places where types are referenced that don't have a TypeLoc node traversed by RecursiveASTVisitor.

> is there anything wrong with the current AST design or rather any notable improvements by adding more nodes to the AST beside this one use case here?

I guess the use case here is "precisely communicating a selection in an IDE".
Expand-selection features and show-ast-structure features don't work in clangd for a similar reason.

Outside clangd I'm less familiar of course! Matchers, clang-tidy checks and refactorings spring to mind.
For instance, if you want to write a tool to rename/split an ObjC class, then something like `typeLoc(loc(asString("FooClass")))` would match all usages... except this one, so your tool would have a bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99975