[PATCH] D69237: Refactor getDeclAtPosition() to use SelectionTree + targetDecl()

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:147
   }
+  while ((N = N->Parent)) {
+    if (N->ASTNode.get<CallExpr>() || N->ASTNode.get<ImplicitCastExpr>() ||
----------------
sammccall wrote:
> nridge wrote:
> > This part is a super hacky way to keep case #9 in `LocateSymbol.Ambiguous` passing while not regressing other tests.
> > 
> > In that case, the selection tree looks like this:
> > 
> > ```
> >        DeclStmt Foo foox = Foo("asdf");
> > 
> >          VarDecl Foo foox = Foo("asdf")
> >            ExprWithCleanups Foo("asdf")
> >              CXXConstructExpr Foo("asdf")
> >                MaterializeTemporaryExpr Foo("asdf")
> >                  CXXFunctionalCastExpr Foo("asdf")
> >                   .RecordTypeLoc Foo
> > ```
> > 
> > The common ancestor is the `RecordTypeLoc`, but we'd like to find the constructor referred to by the `CXXConstructExpr` as a target as well. To achieve that, my current code walks up the parent tree until we encounter a `CXXConstructExpr`, but aborts the walk if certain node types are encountered to avoid certain other scenarios where we have a `CXXConstructExpr` ancestor.
> > 
> > This is almost certainly wrong, as there are likely many other cases where we have a `CXXConstructExpr` ancestor but don't want to find the constructor as a target which are not being handled correctly.
> > 
> > Is there a better way to identify the syntactic form at issue in this testcase?
> This case is gross, but as above I think it would be OK to have the targeting work like:
> ```
> Foo("asdf")
> FFFCSSSSSSC
> 
> F -> RecordTypeLoc    -> CXXRecordDecl Foo
> C -> CXXConstructExpr -> CXXConstructorDecl Foo(string)
> S -> StringLiteral    -> nullptr
> ```
> WDYT?
In your description, the target nodes are written underneath the characters. However, our input here is a cursor position (in between two characters).

Should we target the `CXXConstructExpr` if the cursor is just before the parenthesis, just after, or both?

(I am aware that `SelectionTree` can take a range as well as a position, so from the point of view of that API, "only when the selection is a range covering the parenthesis" is an option as well. However, this would not benefit go-to-definition, for which the request as specified in LSP only contains a position, not a range.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69237