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

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.


================
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);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99975