Refactor getDeclAtPosition() to use SelectionTree + targetDecl()

[nridge]

Summary

This fixes issue #163, among other improvements to go-to-definition.

[nridge]

clang-tools-extra/clangd/XRefs.cpp

137

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?

138

I would appreciate some tips on how we could handle this in targetDecl(). Please see more specific comments below.

140

This part is intended to handle case #8 in LocateSymbol.Ambiguous, where you have a declaration of the form Foo abcd("asdf"); which invokes a constructor.

A naive attempt to add this logic to TargetFinder::add(const Decl*) resulted in the constructor being a target even for references to the variable, not just its declaration.

[sammccall]

Thanks! I'd been meaning to do this before my vacation (and had a draft I'll dig up in case there were other issues).

Generally I think we should be willing to change behavior in some marginal cases, and I do think it's important to care about the layering of seltree vs targetDecl because we'll want to plug them into other things in the future. (E.g. using targetDecl + recursiveASTVisitor for more flexible indexing)

clang-tools-extra/clangd/XRefs.cpp

137

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?

138

My thinking is basically:

• C++ syntax is vague and doesn't give us great ways of referring directly to constructors.
• there's value to simplicity, and I've found go-to-definition additionally finding implicit nodes to be confusing as often as useful. I think on balance and given the constraints of LSP, we should consider dropping this and having implicit references be returned by find-refs but not targetable by go-to-def/hover/etc. It's OK for simplicity of implementation to be a concern here.
• the node that targets the constructor is the CXXConstructExpr. Treating the VarDecl conditionally as a reference to the constructor, while clever, seems like a hack. Ideally the fix is to make SelectionTree yield the CXXConstructExpr, not to change TargetDecl.
• CXXConstructExpr is only sometimes implicit. SelectionTree should return it for the parens in Foo() or Foo x(). And ideally for the equals in Foo x = {}, though I think there's no CXXConstructExpr in the AST in that case :-(
• When the AST modelling is bad (as it seems to be for some aspects CXXConstructExpr) we should consider accepting some glitches and trying to improve things in clang if they're important. Hacking around them will lead to inconsistencies between different clangd features.

The TL;DR is I think I'd be happy to drop this special case and try to make SelectionTree surface CXXConstructExpr more often, even if it changes some behavior.

138

I'd prefer not to privilege any defaults here, I think this is unfortunately something that each caller needs to think about.

846

might be clearer to handle that case first then, and guard each with if (!HI)

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

2202

nit: can we add the trailing comma in the init-list to preserve the older, clearer formatting instead?

[ilya-biryukov]

clang-tools-extra/clangd/XRefs.cpp

138

+1 to the idea of landing this and changing behavior.
I don't think we're loosing much in terms of functionality and I personally like the new code much more.

[nridge]

clang-tools-extra/clangd/XRefs.cpp

138

FWIW, I do find being able to get from a variable declaration to the invoked constructor to be very useful.

If the class has several constructors, there's no other easy way to determine which one is being invoked (only other thing I can think of is to perform "find references" on each constructor and see which one includes the variable declaration as a reference, which is a lot more tedious), and I think that's an important thing to be able to do (just like for named functions).

So I'd definitely like to keep this case working. However, I'd be fine with only having the parens or braces target the constructor. (That's still slightly annoying, as you have to place the cursor more precisely, and it also may not be obvious to users, but at least there's a way to do it.) I'm also fine with changing the behaviour for now, and deferring constructor targeting to a follow-up, as there are other benefits this patch brings.

[ilya-biryukov]

clang-tools-extra/clangd/XRefs.cpp

138

We discussed this offline: finding constructor references is useful, but current approach does not generalize very well, e.g. for implicit constructor calls:

struct Foo {
  Foo(int);
};

int func(Foo a, Foo b);
int test() {
  func(1, 2); // no way to find Foo(1) and Foo(2) calls.
}

So we'd probably want some other mechanism to expose *all* possible references.
In any case, changing behavior now and tweaking it with a follow-up patch is probably the best way to proceed with this change.

[nridge]

Removed constructor handling, deferring that to a follow-up

Addressed other comments

[nridge]

clang-tools-extra/clangd/XRefs.cpp

137

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.)

[nridge]

clang-tools-extra/clangd/XRefs.cpp

138

So we'd probably want some other mechanism to expose *all* possible references.

Any ideas for what such a mechanism might look like?

[ilya-biryukov]

clang-tools-extra/clangd/XRefs.cpp

138

We were discussing this offline, a few options that came up are:

Expose tree view of the AST (that links to the source code) with implicit nodes, allow to navigate from the conversion nodes to the references.
Code lens in VSCode, i.e. in-editor annotations right in the middle of the code

[nridge]

So, to summarize my understanding of the way forward here:

1. Land the refactoring without any constructor targeting (i.e. the current patch, which is ready to review in this state).
2. In a follow-up patch: target constructor in cases where this can be accomplished by having SelectionTree expose the CXXConstructorExpr on the parenthesis.
3. Longer-term: put in place a mechanism for exposing all implicit names via code lens or somesuch.

clang-tools-extra/clangd/XRefs.cpp

137

Ah, never mind, I see now that SelectionTree has existing logic for mapping a cursor position to a one-character selection, which in this case would result in mapping the cursor onto the parenthesis if it's just before the parenthesis.

[nridge]

Updated not to use the beginning-of-identifier location as input to SelectionTree

[nridge]

In D69237#1722399, @nridge wrote:

So, to summarize my understanding of the way forward here:

1. Land the refactoring without any constructor targeting (i.e. the current patch, which is ready to review in this state).
2. In a follow-up patch: target constructor in cases where this can be accomplished by having SelectionTree expose the CXXConstructorExpr on the parenthesis.

While trying to implement #2, I realized that SelectionTree already has this behaviour (exposes CXXConstructorExpr on the parenthesis), we just weren't benefitting from it because locateSymbolAt() was passing in the beginning-of-identifier location to getDeclAtPosition(), so Foo^(arg) and F^oo(arg) would yield the same result.

The updated patch fixes this and updates the tests to reflect the constructor targeting in cases where it now works.

[sammccall]

LG, just some style nits, thanks!

clang-tools-extra/clangd/XRefs.cpp

259

rather than justifying *not* doing something here, can we make it clearer that macros are the special case?
maybe move the "SourceLocationBeg" initialization under the "macros are simple" comment, and rename it to "MaybeMacroLocation" or so.

266

log should have more context, and be an elog instead.
Return afterward?

269

I don't really understand this comment.

If the intention is just not to look at the AST or index when we have a macro (which seems reasonable to me), then just return Result instead of HaveMacro = true above?

895–899

want -> show references to

1132

Are you sure you don't want type hierarchy to work on aliases to record types? (i.e. specify underlying rather than alias)

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

527

I'd suggest dropping these two lines of comment. The fact that it's testing constructors is clear from the range name. The FIXME comment describes the future change.

529

This could just be // FIXME: target constructor instead of class..
We already have a test, so the comment belongs above point("9") instead of here

[nridge]

Address latest comments

clang-tools-extra/clangd/XRefs.cpp

1132

Good catch! I added a test case to TypeHierarchyTests for this as well.

clang-tools-extra/clangd/XRefs.cpp

 //===--- XRefs.cpp -----------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "XRefs.h"
 #include "AST.h"
 #include "CodeCompletionStrings.h"
 #include "FindSymbols.h"
 #include "FindTarget.h"
 #include "FormattedString.h"
 #include "Logger.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
+#include "Selection.h"
 #include "SourceCode.h"
 #include "URI.h"
 #include "index/Index.h"
 #include "index/Merge.h"
 #include "index/Relation.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolLocation.h"
 #include "clang/AST/ASTContext.h"
SymbolLocation getPreferredLocation(const Location &ASTLoc,
   Symbol ASTSym, IdxSym;
   ASTSym.ID = IdxSym.ID = SymbolID("dummy_id");
   ASTSym.CanonicalDeclaration = toIndexLocation(ASTLoc, Scratch);
   IdxSym.CanonicalDeclaration = IdxLoc;
   auto Merged = mergeSymbol(ASTSym, IdxSym);
   return Merged.CanonicalDeclaration;
 }
 
-/// Finds declarations locations that a given source location refers to.
-class DeclarationFinder : public index::IndexDataConsumer {
-  llvm::DenseSet<const Decl *> Decls;
-  const SourceLocation &SearchedLocation;
-
-public:
-  DeclarationFinder(const SourceLocation &SearchedLocation)
-      : SearchedLocation(SearchedLocation) {}
-
-  // The results are sorted by declaration location.
-  std::vector<const Decl *> getFoundDecls() const {
-    std::vector<const Decl *> Result;
-    for (const Decl *D : Decls)
-      Result.push_back(D);
-
-    llvm::sort(Result, [](const Decl *L, const Decl *R) {
-      return L->getBeginLoc() < R->getBeginLoc();
-    });
-    return Result;
-  }
-
-  bool
-  handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
-                      llvm::ArrayRef<index::SymbolRelation> Relations,
-                      SourceLocation Loc,
-                      index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
-    // Skip non-semantic references.
-    if (Roles & static_cast<unsigned>(index::SymbolRole::NameReference))
-      return true;
-
-    if (Loc == SearchedLocation) {
-      auto IsImplicitExpr = [](const Expr *E) {
-        if (!E)
-          return false;
-        // We assume that a constructor expression is implict (was inserted by
-        // clang) if it has an invalid paren/brace location, since such
-        // experssion is impossible to write down.
-        if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(E))
-          return CtorExpr->getParenOrBraceRange().isInvalid();
-        // Ignore implicit conversion-operator AST node.
-        if (const auto *ME = dyn_cast<MemberExpr>(E)) {
-          if (isa<CXXConversionDecl>(ME->getMemberDecl()))
-            return ME->getMemberLoc().isInvalid();
-        }
-        return isa<ImplicitCastExpr>(E);
-      };
-
-      if (IsImplicitExpr(ASTNode.OrigE))
-        return true;
-      // Find and add definition declarations (for GoToDefinition).
-      // We don't use parameter `D`, as Parameter `D` is the canonical
-      // declaration, which is the first declaration of a redeclarable
-      // declaration, and it could be a forward declaration.
-      if (const auto *Def = getDefinition(D)) {
-        Decls.insert(Def);
-      } else {
-        // Couldn't find a definition, fall back to use `D`.
-        Decls.insert(D);
-      }
-    }
-    return true;
-  }
-};
-
-std::vector<const Decl *> getDeclAtPosition(ParsedAST &AST,
-                                            SourceLocation Pos) {
-  DeclarationFinder Finder(Pos);
-  index::IndexingOptions IndexOpts;
-  IndexOpts.SystemSymbolFilter =
-      index::IndexingOptions::SystemSymbolFilterKind::All;
-  IndexOpts.IndexFunctionLocals = true;
-  IndexOpts.IndexParametersInDeclarations = true;
-  IndexOpts.IndexTemplateParameters = true;
-  indexTopLevelDecls(AST.getASTContext(), AST.getPreprocessor(),
-                     AST.getLocalTopLevelDecls(), Finder, IndexOpts);
-
-  return Finder.getFoundDecls();
+std::vector<const Decl *> getDeclAtPosition(ParsedAST &AST, SourceLocation Pos,
+                                            DeclRelationSet Relations) {
+  FileID FID;
+  unsigned Offset;
+  std::tie(FID, Offset) = AST.getSourceManager().getDecomposedSpellingLoc(Pos);
+  SelectionTree Selection(AST.getASTContext(), AST.getTokens(), Offset);
+  if (const SelectionTree::Node *N = Selection.commonAncestor()) {
+    auto Decls = targetDecl(N->ASTNode, Relations);
+    Result.assign(Decls.begin(), Decls.end());
+  return Result;
 }
 
 llvm::Optional<Location> makeLocation(ASTContext &AST, SourceLocation TokLoc,
                                       llvm::StringRef TUPath) {
   const SourceManager &SourceMgr = AST.getSourceManager();
   const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc));
   if (!F)
     return None;
if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line) {
       File.PreferredDeclaration = {
           URIForFile::canonicalize(Inc.Resolved, *MainFilePath), Range{}};
       File.Definition = File.PreferredDeclaration;
       // We're not going to find any further symbols on #include lines.
       return {std::move(File)};
     }
   }
 
-  SourceLocation SourceLocationBeg =
-      SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
-          Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts()));
-
   // Macros are simple: there's no declaration/definition distinction.
   // As a consequence, there's no need to look them up in the index either.
+  SourceLocation MaybeMacroLocation =
+      SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+          Pos, AST.getSourceManager(), AST.getASTContext().getLangOpts()));
   std::vector<LocatedSymbol> Result;
-  if (auto M = locateMacroAt(SourceLocationBeg, AST.getPreprocessor())) {
+  if (auto M = locateMacroAt(MaybeMacroLocation, AST.getPreprocessor())) {
     if (auto Loc = makeLocation(AST.getASTContext(),
                                 M->Info->getDefinitionLoc(), *MainFilePath)) {
       LocatedSymbol Macro;
       Macro.Name = M->Name;
       Macro.PreferredDeclaration = *Loc;
       Macro.Definition = Loc;
       Result.push_back(std::move(Macro));
+
+      // Don't look at the AST or index if we have a macro result.
+      // (We'd just return declarations referenced from the macro's
+      // expansion.)
+      return Result;
     }
   }
 
   // Decls are more complicated.
   // The AST contains at least a declaration, maybe a definition.
   // These are up-to-date, and so generally preferred over index results.
   // We perform a single batch index lookup to find additional definitions.
 
   // Results follow the order of Symbols.Decls.
   // Keep track of SymbolID -> index mapping, to fill in index data later.
   llvm::DenseMap<SymbolID, size_t> ResultIndex;
 
+  SourceLocation SourceLoc;
+  if (auto L = sourceLocationInMainFile(SM, Pos)) {
+    SourceLoc = *L;
+  } else {
+    elog("locateSymbolAt failed to convert position to source location: {0}",
+         L.takeError());
+    return Result;
+  }
+
   // Emit all symbol locations (declaration or definition) from AST.
-  for (const Decl *D : getDeclAtPosition(AST, SourceLocationBeg)) {
-    auto Loc =
-        makeLocation(AST.getASTContext(), spellingLocIfSpelled(findName(D), SM),
+  DeclRelationSet Relations =
+      DeclRelation::TemplatePattern | DeclRelation::Alias;
+  for (const Decl *D : getDeclAtPosition(AST, SourceLoc, Relations)) {
+    const Decl *Def = getDefinition(D);
+    const Decl *Preferred = Def ? Def : D;
+    auto Loc = makeLocation(AST.getASTContext(),
+                            spellingLocIfSpelled(findName(Preferred), SM),
                             *MainFilePath);
     if (!Loc)
       continue;
 
     Result.emplace_back();
-    if (auto *ND = dyn_cast<NamedDecl>(D))
+    if (auto *ND = dyn_cast<NamedDecl>(Preferred))
       Result.back().Name = printName(AST.getASTContext(), *ND);
     Result.back().PreferredDeclaration = *Loc;
-    // DeclInfo.D is always a definition if possible, so this check works.
-    if (getDefinition(D) == D)
+    // Preferred is always a definition if possible, so this check works.
+    if (Def == Preferred)
       Result.back().Definition = *Loc;
 
     // Record SymbolID for index lookup later.
-    if (auto ID = getSymbolID(D))
+    if (auto ID = getSymbolID(Preferred))
       ResultIndex[*ID] = Result.size() - 1;
   }
 
   // Now query the index for all Symbol IDs we found in the AST.
   if (Index && !ResultIndex.empty()) {
     LookupRequest QueryRequest;
     for (auto It : ResultIndex)
       QueryRequest.IDs.insert(It.first);
     std::string Scratch;
     Index->lookup(QueryRequest, [&](const Symbol &Sym) {
       auto &R = Result[ResultIndex.lookup(Sym.ID)];
 
       if (R.Definition) { // from AST
         // Special case: if the AST yielded a definition, then it may not be
         // the right *declaration*. Prefer the one from the index.
         if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, *MainFilePath))
           R.PreferredDeclaration = *Loc;
 
         // We might still prefer the definition from the index, e.g. for

 }
 
 } // namespace
 
 std::vector<DocumentHighlight> findDocumentHighlights(ParsedAST &AST,
                                                       Position Pos) {
   const SourceManager &SM = AST.getSourceManager();
   // FIXME: show references to macro within file?
-  auto References =
-      findRefs(getDeclAtPosition(
-                   AST, SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
-                            Pos, SM, AST.getASTContext().getLangOpts()))),
-               AST);
+  DeclRelationSet Relations =
+      DeclRelation::TemplatePattern | DeclRelation::Alias;
+  auto References = findRefs(
+      getDeclAtPosition(AST,
+                        SM.getMacroArgExpandedLocation(getBeginningOfIdentifier(
+                            Pos, SM, AST.getASTContext().getLangOpts())),
+                        Relations),
 
   // FIXME: we may get multiple DocumentHighlights with the same location and
   // different kinds, deduplicate them.
   std::vector<DocumentHighlight> Result;
   for (const auto &Ref : References) {
     if (auto Range =
             getTokenRange(AST.getASTContext().getSourceManager(),
                           AST.getASTContext().getLangOpts(), Ref.Loc)) {

 llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
                                    format::FormatStyle Style,
                                    const SymbolIndex *Index) {
   const SourceManager &SM = AST.getSourceManager();
   llvm::Optional<HoverInfo> HI;
   SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getASTContext().getLangOpts()));
 
+  if (hasDeducedType(AST, SourceLocationBeg)) {
+    DeducedTypeVisitor V(SourceLocationBeg);
+    V.TraverseAST(AST.getASTContext());
+    if (!V.DeducedType.isNull())
+      HI = getHoverContents(V.DeducedType, V.D, AST.getASTContext(), Index);
+  }
+
+  if (!HI) {
     if (auto M = locateMacroAt(SourceLocationBeg, AST.getPreprocessor())) {
       HI = getHoverContents(*M, AST);
     } else {
-    auto Decls = getDeclAtPosition(AST, SourceLocationBeg);
+      DeclRelationSet Relations =
+          DeclRelation::TemplatePattern | DeclRelation::Alias;
+      auto Decls = getDeclAtPosition(AST, SourceLocationBeg, Relations);
       if (!Decls.empty())
         HI = getHoverContents(Decls.front(), Index);
     }
-  if (!HI && hasDeducedType(AST, SourceLocationBeg)) {
-    DeducedTypeVisitor V(SourceLocationBeg);
-    V.TraverseAST(AST.getASTContext());
-    if (!V.DeducedType.isNull())
-      HI = getHoverContents(V.DeducedType, V.D, AST.getASTContext(), Index);
   }
+
   if (!HI)
     return llvm::None;
 
   auto Replacements = format::reformat(
       Style, HI->Definition, tooling::Range(0, HI->Definition.size()));
   if (auto Formatted =
           tooling::applyAllReplacements(HI->Definition, Replacements))
     HI->Definition = *Formatted;
auto MainFilePath =
       getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!MainFilePath) {
     elog("Failed to get a path for the main file, so no references");
     return Results;
   }
   auto Loc = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getASTContext().getLangOpts()));
   // TODO: should we handle macros, too?
-  auto Decls = getDeclAtPosition(AST, Loc);
+  // We also show references to the targets of using-decls, so we include
+  // DeclRelation::Underlying.
+  DeclRelationSet Relations = DeclRelation::TemplatePattern |
+                              DeclRelation::Alias | DeclRelation::Underlying;
+  auto Decls = getDeclAtPosition(AST, Loc, Relations);
 
   // We traverse the AST to find references in the main file.
   auto MainFileRefs = findRefs(Decls, AST);
   // We may get multiple refs with the same location and different Roles, as
   // cross-reference is only interested in locations, we deduplicate them
   // by the location to avoid emitting duplicated locations.
   MainFileRefs.erase(std::unique(MainFileRefs.begin(), MainFileRefs.end(),
                                  [](const ReferenceFinder::Reference &L,

 
 std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {
   const SourceManager &SM = AST.getSourceManager();
   auto Loc = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getASTContext().getLangOpts()));
 
   std::vector<SymbolDetails> Results;
 
-  for (const Decl *D : getDeclAtPosition(AST, Loc)) {
+  // We also want the targets of using-decls, so we include
+  // DeclRelation::Underlying.
+  DeclRelationSet Relations = DeclRelation::TemplatePattern |
+                              DeclRelation::Alias | DeclRelation::Underlying;
+  for (const Decl *D : getDeclAtPosition(AST, Loc, Relations)) {
     SymbolDetails NewSymbol;
     if (const NamedDecl *ND = dyn_cast<NamedDecl>(D)) {
       std::string QName = printQualifiedName(*ND);
       std::tie(NewSymbol.containerName, NewSymbol.name) =
           splitQualifiedName(QName);
 
       if (NewSymbol.containerName.empty()) {
         if (const auto *ParentND =
if (Pattern) {
     RPSet.erase(Pattern);
   }
 }
 
 const CXXRecordDecl *findRecordTypeAt(ParsedAST &AST, Position Pos) {
   const SourceManager &SM = AST.getSourceManager();
   SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
       getBeginningOfIdentifier(Pos, SM, AST.getASTContext().getLangOpts()));
-  auto Decls = getDeclAtPosition(AST, SourceLocationBeg);
+  DeclRelationSet Relations =
+      DeclRelation::TemplatePattern | DeclRelation::Underlying;
+  auto Decls = getDeclAtPosition(AST, SourceLocationBeg, Relations);
   if (Decls.empty())
     return nullptr;
 
   const Decl *D = Decls[0];
 
   if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     // If this is a variable, use the type of the variable.
     return VD->getType().getTypePtr()->getAsCXXRecordDecl();

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

 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
-using ::testing::ElementsAreArray;
+using ::testing::UnorderedElementsAreArray;
 
 auto CreateExpectedSymbolDetails = [](const std::string &name,
                                       const std::string &container,
                                       const std::string &USR) {
   return SymbolDetails{name, container, USR, SymbolID(USR)};
 };
 
 TEST(SymbolInfoTests, All) {
std::pair<const char *, std::vector<SymbolDetails>>
               {/* not implemented */}},
       };
 
   for (const auto &T : TestInputExpectedOutput) {
     Annotations TestInput(T.first);
     auto AST = TestTU::withCode(TestInput.code()).build();
 
     EXPECT_THAT(getSymbolInfo(AST, TestInput.point()),
-                ElementsAreArray(T.second))
+                UnorderedElementsAreArray(T.second))
         << T.first;
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 MATCHER(ChildrenNotResolved, "") { return !arg.children; }
 
 TEST(FindRecordTypeAt, TypeOrVariable) {
   Annotations Source(R"cpp(
 struct Ch^ild2 {
   int c;
 };
 
+using A^lias = Child2;
+
 int main() {
   Ch^ild2 ch^ild2;
   ch^ild2.c = 1;
 }
 )cpp");
 
   TestTU TU = TestTU::withCode(Source.code());
   auto AST = TU.build();

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

TEST(LocateSymbol, All) {
   }
 }
 
 TEST(LocateSymbol, Ambiguous) {
   auto T = Annotations(R"cpp(
     struct Foo {
       Foo();
       Foo(Foo&&);
-      Foo(const char*);
+      $ConstructorLoc[[Foo]](const char*);
     };
 
     Foo f();
 
     void g(Foo foo);
 
     void call() {
       const char* str = "123";
       Foo a = $1^str;
       Foo b = Foo($2^str);
       Foo c = $3^f();
       $4^g($5^f());
       g($6^str);
       Foo ab$7^c;
       Foo ab$8^cd("asdf");
       Foo foox = Fo$9^o("asdf");
+      Foo abcde$10^("asdf");
+      Foo foox2 = Foo$11^("asdf");
     }
   )cpp");
   auto AST = TestTU::withCode(T.code()).build();
   // Ordered assertions are deliberate: we expect a predictable order.
   EXPECT_THAT(locateSymbolAt(AST, T.point("1")), ElementsAre(Sym("str")));
   EXPECT_THAT(locateSymbolAt(AST, T.point("2")), ElementsAre(Sym("str")));
   EXPECT_THAT(locateSymbolAt(AST, T.point("3")), ElementsAre(Sym("f")));
   EXPECT_THAT(locateSymbolAt(AST, T.point("4")), ElementsAre(Sym("g")));
   EXPECT_THAT(locateSymbolAt(AST, T.point("5")), ElementsAre(Sym("f")));
   EXPECT_THAT(locateSymbolAt(AST, T.point("6")), ElementsAre(Sym("str")));
+  // FIXME: Target the constructor as well.
   EXPECT_THAT(locateSymbolAt(AST, T.point("7")), ElementsAre(Sym("abc")));
-  EXPECT_THAT(locateSymbolAt(AST, T.point("8")),
-              ElementsAre(Sym("Foo"), Sym("abcd")));
-  EXPECT_THAT(locateSymbolAt(AST, T.point("9")),
-              // First one is class definition, second is the constructor.
-              ElementsAre(Sym("Foo"), Sym("Foo")));
+  // FIXME: Target the constructor as well.
+  EXPECT_THAT(locateSymbolAt(AST, T.point("8")), ElementsAre(Sym("abcd")));
+  // FIXME: Target the constructor as well.
+  EXPECT_THAT(locateSymbolAt(AST, T.point("9")), ElementsAre(Sym("Foo")));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("10")),
+              ElementsAre(Sym("Foo", T.range("ConstructorLoc"))));
+  EXPECT_THAT(locateSymbolAt(AST, T.point("11")),
+              ElementsAre(Sym("Foo", T.range("ConstructorLoc"))));
 }
 
 TEST(LocateSymbol, TemplateTypedefs) {
   auto T = Annotations(R"cpp(
     template <class T> struct function {};
     template <class T> using callback = function<T()>;
 
     c^allback<int> foo;

 }
 
 TEST(GetDeducedType, KwAutoExpansion) {
   struct Test {
     StringRef AnnotatedCode;
     const char *DeducedType;
   } Tests[] = {
       {"^auto i = 0;", "int"},
-      {"^auto f(){ return 1;};", "int"}
+      {"^auto f(){ return 1;};", "int"},
   };
   for (Test T : Tests) {
     Annotations File(T.AnnotatedCode);
     auto AST = TestTU::withCode(File.code()).build();
     ASSERT_TRUE(AST.getDiagnostics().empty())
         << AST.getDiagnostics().begin()->Message;
     SourceManagerForFile SM("foo.cpp", File.code());
 
     for (Position Pos : File.points()) {