[clangd] go-to-definition on auto unwraps array/pointer types

[sammccall]

We don't do this kind of unwrapping in targetDecl() because it seems a bit sloppy/approximate, but in this case the full type isn't written, so you can't choose to target the Foo rather than the * in Foo *.

Do you think we should try to handle unique_ptr/shared_ptr too? (Today they'd go to the unique_ptr template)

[qchateau]

Ah good catch, I agree this is the behavior we want.

Concerning cases like unique_ptr<T>, the only generic behavior I can think of would be to suggest multiple LocatedSymbol when auto is deduced to a template class: the template class itself, and all its template type parameters.

class A {};
class B {};

template<typename T, typename U>
class C {};

auto x = C<A, B>();

go-to-definition on auto could suggest jumping to C, A or B.

In a way, when you explicitly have C<A, B> x; you can go-to-def to either A, B or C in one click, it is simply made easier because all these types are visible in the code and you make the choice by choosing on where you click.

[sammccall]

In D93314#2455884, @qchateau wrote:

Ah good catch, I agree this is the behavior we want.

Concerning cases like unique_ptr<T>, the only generic behavior I can think of would be to suggest multiple LocatedSymbol when auto is deduced to a template class: the template class itself, and all its template type parameters.

Right. There are not-completely-generic options too though :-)

I'm inclined to agree that unique_ptr<MemoryBuffer> should resolve to both unique_ptr and MemoryBuffer.
But not std::default_deleter, and probably not even a custom deleter.

Similarly, maybe vector<MemoryBuffer> should resolve to both, but I'm not sure that holds for all templates (e.g. lock_guard).

So I'm leaning towards defining these behaviors for pointers and containers rather than templates per se. There's a nice symmetry with the raw pointer and array types that this patch.
(We have some precedent and code for detecting smart pointers, see getPointeeType() in FindTarget.cpp.)

We could land this as-is (just handling the built-in types) but it will need some structural changes for dealing with multiple results (including recursive cases like vector<Foo*>) so I'll take a stab at doing all that at once.

[qchateau]

So I'm leaning towards defining these behaviors for pointers and containers rather than templates per se. There's a nice symmetry with the raw pointer and array types that this patch.
(We have some precedent and code for detecting smart pointers, see getPointeeType() in FindTarget.cpp.)

If you think it's fine to have specific code using heuristics to try to find what the user expects, then pointers and containers sounds like a good target.

clang-tools-extra/clangd/XRefs.cpp

 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/Type.h"
+#include "clang/AST/TypeVisitor.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexSymbol.h"
if (Index && !ResultIndex.empty()) {
     });
   }
 
   auto Overrides = findOverrides(VirtualMethods, Index, MainFilePath);
   Result.insert(Result.end(), Overrides.begin(), Overrides.end());
   return Result;
 }
 
+// Unwrap a type for the purposes of go-to-definition on auto.
+// e.g. pointer types - there's no definition for Foo*, so show class Foo.
+// We could consider unwrapping unique_ptr<T> too - confusing? slippery slope?
+// We could also move this into targetDecl(), but it's semantically iffy.
+QualType unwrapType(QualType T) {
+  struct OneStepUnwrap : TypeVisitor<OneStepUnwrap, QualType> {
+    QualType VisitType(const Type *T) { return QualType(); }
+    QualType VisitArrayType(const ArrayType *T) { return T->getElementType(); }
+    QualType VisitPointerType(const PointerType *T) {
+      return T->getPointeeType();
+    }
+    QualType VisitReferenceType(const ReferenceType *T) {
+      return T->getPointeeTypeAsWritten();
+    }
+  } Unwrap;
+  if (T.isNull())
+    return T;
+  while (true) {
+    QualType Next = Unwrap.Visit(T.getTypePtr());
+    if (Next.isNull())
+      return T;
+    T = Next;
+  }
+}
+
 std::vector<LocatedSymbol> locateSymbolForType(const ParsedAST &AST,
                                                const QualType &Type) {
   const auto &SM = AST.getSourceManager();
   auto MainFilePath =
       getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!MainFilePath) {
     elog("Failed to get a path for the main file, so no symbol.");
     return {};
   }
 
-  auto Decls = targetDecl(DynTypedNode::create(Type.getNonReferenceType()),
+  auto Decls = targetDecl(DynTypedNode::create(unwrapType(Type)),
                           DeclRelation::TemplatePattern | DeclRelation::Alias);
   if (Decls.empty())
     return {};
 
   std::vector<LocatedSymbol> Results;
   const auto &ASTContext = AST.getASTContext();
 
   for (const NamedDecl *D : Decls) {

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

const char *Tests[] = {
       R"cpp(// auto on struct
         namespace ns1 {
         struct [[S1]] {};
         } // namespace ns1
 
         ^auto x = ns1::S1{};
       )cpp",
 
+      R"cpp(// auto unwraps pointers
+        struct [[S]] {};
+        S **const*&y();
+
+        ^auto x = y();
+      )cpp",
+
       R"cpp(// decltype on struct
         namespace ns1 {
         struct [[S1]] {};
         } // namespace ns1
 
         ns1::S1 i;
         ^decltype(i) j;
       )cpp",