[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356445: [clangd] Add support for type hierarchy (super types 
only for now) (authored by kadircet, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56370?vs=190755=191263#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/clangd/FindSymbols.cpp
  clang-tools-extra/trunk/clangd/FindSymbols.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/clangd/XRefs.cpp
  clang-tools-extra/trunk/clangd/XRefs.h
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.h
  clang-tools-extra/trunk/test/clangd/initialize-params.test
  clang-tools-extra/trunk/test/clangd/type-hierarchy.test
  clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
  clang-tools-extra/trunk/unittests/clangd/Matchers.h
  clang-tools-extra/trunk/unittests/clangd/TypeHierarchyTests.cpp

Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -368,6 +368,7 @@
   {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND,
ExecuteCommandParams::CLANGD_APPLY_TWEAK}},
  }},
+{"typeHierarchyProvider", true},
 );
 }
 
@@ -806,6 +807,13 @@
 std::move(Reply));
 }
 
+void ClangdLSPServer::onTypeHierarchy(
+const TypeHierarchyParams ,
+Callback> Reply) {
+  Server->typeHierarchy(Params.textDocument.uri.file(), Params.position,
+Params.resolve, Params.direction, std::move(Reply));
+}
+
 void ClangdLSPServer::applyConfiguration(
 const ConfigurationSettings ) {
   // Per-file update to the compilation database.
@@ -885,6 +893,7 @@
   MsgHandler->bind("workspace/didChangeWatchedFiles", ::onFileEvent);
   MsgHandler->bind("workspace/didChangeConfiguration", ::onChangeConfiguration);
   MsgHandler->bind("textDocument/symbolInfo", ::onSymbolInfo);
+  MsgHandler->bind("textDocument/typeHierarchy", ::onTypeHierarchy);
   // clang-format on
 }
 
Index: clang-tools-extra/trunk/clangd/FindSymbols.h
===
--- clang-tools-extra/trunk/clangd/FindSymbols.h
+++ clang-tools-extra/trunk/clangd/FindSymbols.h
@@ -21,9 +21,10 @@
 class SymbolIndex;
 
 /// Searches for the symbols matching \p Query. The syntax of \p Query can be
-/// the non-qualified name or fully qualified of a symbol. For example, "vector"
-/// will match the symbol std::vector and "std::vector" would also match it.
-/// Direct children of scopes (namepaces, etc) can be listed with a trailing
+/// the non-qualified name or fully qualified of a symbol. For example,
+/// "vector" will match the symbol std::vector and "std::vector" would also
+/// match it. Direct children of scopes (namepaces, etc) can be listed with a
+/// trailing
 /// "::". For example, "std::" will list all children of the std namespace and
 /// "::" alone will list all children of the global namespace.
 /// \p Limit limits the number of results returned (0 means no limit).
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -211,6 +211,61 @@
   }
 }
 
+SymbolKind indexSymbolKindToSymbolKind(index::SymbolKind Kind) {
+  switch (Kind) {
+  case index::SymbolKind::Unknown:
+return SymbolKind::Variable;
+  case index::SymbolKind::Module:
+return SymbolKind::Module;
+  case index::SymbolKind::Namespace:
+return SymbolKind::Namespace;
+  case index::SymbolKind::NamespaceAlias:
+return SymbolKind::Namespace;
+  case index::SymbolKind::Macro:
+return SymbolKind::String;
+  case index::SymbolKind::Enum:
+return SymbolKind::Enum;
+  case index::SymbolKind::Struct:
+return SymbolKind::Struct;
+  case index::SymbolKind::Class:
+return SymbolKind::Class;
+  case index::SymbolKind::Protocol:
+return SymbolKind::Interface;
+  case index::SymbolKind::Extension:
+return SymbolKind::Interface;
+  case index::SymbolKind::Union:
+return SymbolKind::Class;
+  case index::SymbolKind::TypeAlias:
+return SymbolKind::Class;
+  case index::SymbolKind::Function:
+return SymbolKind::Function;
+  case index::SymbolKind::Variable:
+return SymbolKind::Variable;
+  case 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Great, thanks for the reviews!

Could you commit the patch as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM, thanks Nathan!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.h:62
+/// Find the record type references at \p Pos.
+const CXXRecordDecl *findRecordTypeAt(ParsedAST , Position Pos);
+

nridge wrote:
> ilya-biryukov wrote:
> > This method looks like an implementation detail and does not align with 
> > other methods in `XRefs.h` which are high-level methods that implement LSP 
> > functionality.
> > 
> > It would be more appropriate to move it to `AST.h` or directly into the 
> > `XRefs.cpp`. WDYT?
> @sammccall asked for this method to be exposed in the header and some of the 
> tests written in terms of it.
LG, thanks for the clarification. I haven't been following the review closely.
Having methods that return pointers in `XRefs.h` seems like the wrong layering 
to me, but in any case it's a minor detail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 190755.
nridge marked an inline comment as done.
nridge added a comment.

Address latest review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/test/clangd/type-hierarchy.test
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp

Index: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
@@ -0,0 +1,462 @@
+//===-- TypeHierarchyTests.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 "Annotations.h"
+#include "ClangdUnit.h"
+#include "Compiler.h"
+#include "Matchers.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "TestTU.h"
+#include "XRefs.h"
+#include "index/FileIndex.h"
+#include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/Index/IndexingAction.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::AllOf;
+using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
+using testing::IsEmpty;
+using testing::Matcher;
+using testing::Pointee;
+using testing::UnorderedElementsAreArray;
+
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 7 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.h:62
+/// Find the record type references at \p Pos.
+const CXXRecordDecl *findRecordTypeAt(ParsedAST , Position Pos);
+

ilya-biryukov wrote:
> This method looks like an implementation detail and does not align with other 
> methods in `XRefs.h` which are high-level methods that implement LSP 
> functionality.
> 
> It would be more appropriate to move it to `AST.h` or directly into the 
> `XRefs.cpp`. WDYT?
@sammccall asked for this method to be exposed in the header and some of the 
tests written in terms of it.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:154
+// the SourceManager.
+std::string toURI(const SourceManager , llvm::StringRef Path,
+  llvm::StringRef FallbackDir);

ioeric wrote:
> why are we pulling this into the header? This still seems to be only used in 
> SymbolCollector.cpp.
You're right. This is a leftover from an earlier version of the patch.



Comment at: clang-tools-extra/test/clangd/type-hierarchy.test:1
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}

kadircet wrote:
> why strict-whitespace?
That's what the other lit tests seem to do, I just copied it :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

A few small NITs




Comment at: clang-tools-extra/clangd/ClangdServer.h:188
+  /// Get information about type hierarchy for a given position.
+  void findTypeHierarchy(PathRef File, Position Pos, int Resolve,
+ TypeHierarchyDirection Direction,

Could you rename it to `typeHierarchy`?
We try to keep these method names aligned with the LSP methods.



Comment at: clang-tools-extra/clangd/XRefs.h:62
+/// Find the record type references at \p Pos.
+const CXXRecordDecl *findRecordTypeAt(ParsedAST , Position Pos);
+

This method looks like an implementation detail and does not align with other 
methods in `XRefs.h` which are high-level methods that implement LSP 
functionality.

It would be more appropriate to move it to `AST.h` or directly into the 
`XRefs.cpp`. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

just a few drive-by comments ;)




Comment at: clang-tools-extra/clangd/FindSymbols.h:28
+// https://github.com/Microsoft/language-server-protocol/issues/344
+SymbolKind indexSymbolKindToSymbolKind(index::SymbolKind Kind);
+

This could probably live in `Protocol.h`.



Comment at: clang-tools-extra/clangd/XRefs.cpp:957
+  getTypeAncestors(*CXXRD, AST.getASTContext());
+  // TODO: Resolve type descendants if direction is Children or Both,
+  //   and ResolveLevels > 0.

nit: s/TODO/FIXME(owner)/

same elsewhere.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:154
+// the SourceManager.
+std::string toURI(const SourceManager , llvm::StringRef Path,
+  llvm::StringRef FallbackDir);

why are we pulling this into the header? This still seems to be only used in 
SymbolCollector.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D56370#1427075 , @nridge wrote:

> Ok, I filed a Theia issue  
> about it for now.


Thanks!

In D56370#1427076 , @nridge wrote:

> Are these tests written manually, or with the help or some kind of tool?


Yes unfortunately :(

Thanks for moving the tests!




Comment at: clang-tools-extra/test/clangd/type-hierarchy.test:1
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}

why strict-whitespace?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 190375.
nridge added a comment.

Address remaining review comments

(I figure out how to write a lit test)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/test/clangd/type-hierarchy.test
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp

Index: clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
@@ -0,0 +1,462 @@
+//===-- TypeHierarchyTests.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 "Annotations.h"
+#include "ClangdUnit.h"
+#include "Compiler.h"
+#include "Matchers.h"
+#include "SyncAPI.h"
+#include "TestFS.h"
+#include "TestTU.h"
+#include "XRefs.h"
+#include "index/FileIndex.h"
+#include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/Index/IndexingAction.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using testing::AllOf;
+using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
+using testing::IsEmpty;
+using testing::Matcher;
+using testing::Pointee;
+using testing::UnorderedElementsAreArray;
+
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+ 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56370#1426234 , @kadircet wrote:

> Unfortunately we usually test LSP layer with lit tests, and since we don't 
> have any lit tests for this use-case it wasn't caught. Could you add a lit 
> test for overall use case? You can see examples inside 
> clang-tools-extra/test/clangd/ folder.


Are these tests written manually, or with the help or some kind of tool?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

> In D56370#1424180 , @nridge wrote:
> 
>> Unfortunately, there is a further problem: the Theia client-side 
>> implementation of type hierarchy has recently merged, and their code has 
>> changed so that they do require `typeHierarchy/resolve` to be supported. 
>> They ask for 1 level in the initial request, ignore any extra levels the 
>> server might send, and ask for extra levels using `typeHierarchy/resolve` 
>> instead.
>>
>> What should we do here -- seek to change Theia, or implement 
>> `typeHierachy/resolve` for supertypes after all?
> 
> 
> Looking at 
> https://github.com/theia-ide/theia/pull/3802#issuecomment-455992523 inside 
> theia's implementation of this feature, I believe these are all subject to 
> change.

Note, that is a fairly old comment, and as mentioned the PR in question has 
recently merged.

> So let's leave it as it is, even if it would mean you will be able to get 
> only one level of parents knowledge in theia for now. I believe it should be 
> addressed in theia's implementation and proposal itself(which is acutally 
> addressed by sam, 
> https://github.com/Microsoft/vscode-languageserver-node/pull/426/files#r255416980)
>  And if the conclusion happens to be in favor of theia's current 
> implementation we can always change current implementation to respond to 
> those queries as well.

Ok, I filed a Theia issue  
about it for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1496
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(

Sorry for not pointing this out before, but it would be great if you could move 
related tests into a new file, XRefsTests is getting out of hand.

Feel free to ignore if you don't feel like it.



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1834
+
+TEST(TypeHierarchy, RecursiveHierarchy1) {
+  Annotations Source(R"cpp(

Could you also check for the response, since parent traversal for these cases 
are also disabled currently(due to dependent types issue).
So that we won't forget to fix any issues regarding these cases. 
I suppose, a check with only current symbol and empty parents and children 
should be enough. Same for other two cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added a comment.

In D56370#1424177 , @nridge wrote:

> Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a 
> fromJSON() implementation
>
> (Does the fact that this didn't cause any test failures suggest that the 
> fromJSON() functions aren't exercised by any tests?)


Unfortunately we usually test LSP layer with lit tests, and since we don't have 
any lit tests for this use-case it wasn't caught. Could you add a lit test for 
overall use case? You can see examples inside clang-tools-extra/test/clangd/ 
folder.

In D56370#1424180 , @nridge wrote:

> Unfortunately, there is a further problem: the Theia client-side 
> implementation of type hierarchy has recently merged, and their code has 
> changed so that they do require `typeHierarchy/resolve` to be supported. They 
> ask for 1 level in the initial request, ignore any extra levels the server 
> might send, and ask for extra levels using `typeHierarchy/resolve` instead.
>
> What should we do here -- seek to change Theia, or implement 
> `typeHierachy/resolve` for supertypes after all?


Looking at https://github.com/theia-ide/theia/pull/3802#issuecomment-455992523 
inside theia's implementation of this feature, I believe these are all subject 
to change.
So let's leave it as it is, even if it would mean you will be able to get only 
one level of parents knowledge in theia for now. I believe it should be 
addressed in theia's implementation and proposal itself(which is acutally 
addressed by sam, 
https://github.com/Microsoft/vscode-languageserver-node/pull/426/files#r255416980)




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-std::string TweakID,
-Expected InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  Expected InpAST) {

nridge wrote:
> kadircet wrote:
> > nridge wrote:
> > > kadircet wrote:
> > > > Can you revert this change?
> > > I tried, but clang-format automatically makes the change again.
> > > 
> > > Is there some particular clang-format configuration I should apply, so it 
> > > produces the same results? I don't currently have any configuration, i.e. 
> > > I think it's just reading the .clang-format file in the monorepo root.
> > That's what we use as well(and your formatted version is correct), but 
> > unfortunately sometimes people send out changes with wrong formatting and 
> > they are hard to notice during review. Generally we try not to touch those 
> > lines in irrelevant patches to keep blame clean.
> > 
> > If you are running clang-format directly you can instead try 
> > clang-format-diff to format only changed lines. But not that crucial.
> I'm not running clang-format directly, VSCode's clang-format extension is 
> doing so automatically upon every file-save. I have not found an option to 
> configure it to format only changed lines.
> 
> Would it help if I moved the formatting corrections to their own patch (and 
> made this patch depend on that)?
it is not that important, nvm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Unfortunately, there is a further problem: the Theia client-side implementation 
of type hierarchy has recently merged, and their code has changed so that they 
do require `typeHierarchy/resolve` to be supported. They ask for 1 level in the 
initial request, ignore any extra levels the server might send, and ask for 
extra levels using `typeHierarchy/resolve` instead.

What should we do here -- seek to change Theia, or implement 
`typeHierachy/resolve` after all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 190041.
nridge added a comment.

Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a fromJSON() 
implementation

(Does the fact that this didn't cause any test failures suggest that the 
fromJSON() functions aren't exercised by any tests?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,406 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

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

The updated patch addresses the infinite recursion issue by bailing on 
dependent bases for now, as Sam suggested. I will implement the more 
comprehensive suggested fix ("bail out once we see instantiations of the same 
template decl twice in a parent chain") in a follow-up patch. I did add all 
three testcases that have come up during discussion.

I believe all review comments to date are addressed now.




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-std::string TweakID,
-Expected InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  Expected InpAST) {

kadircet wrote:
> nridge wrote:
> > kadircet wrote:
> > > Can you revert this change?
> > I tried, but clang-format automatically makes the change again.
> > 
> > Is there some particular clang-format configuration I should apply, so it 
> > produces the same results? I don't currently have any configuration, i.e. I 
> > think it's just reading the .clang-format file in the monorepo root.
> That's what we use as well(and your formatted version is correct), but 
> unfortunately sometimes people send out changes with wrong formatting and 
> they are hard to notice during review. Generally we try not to touch those 
> lines in irrelevant patches to keep blame clean.
> 
> If you are running clang-format directly you can instead try 
> clang-format-diff to format only changed lines. But not that crucial.
I'm not running clang-format directly, VSCode's clang-format extension is doing 
so automatically upon every file-save. I have not found an option to configure 
it to format only changed lines.

Would it help if I moved the formatting corrections to their own patch (and 
made this patch depend on that)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 190040.
nridge added a comment.

Address the infinite recursion issue


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,406 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-std::string TweakID,
-Expected InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  Expected InpAST) {

nridge wrote:
> kadircet wrote:
> > Can you revert this change?
> I tried, but clang-format automatically makes the change again.
> 
> Is there some particular clang-format configuration I should apply, so it 
> produces the same results? I don't currently have any configuration, i.e. I 
> think it's just reading the .clang-format file in the monorepo root.
That's what we use as well(and your formatted version is correct), but 
unfortunately sometimes people send out changes with wrong formatting and they 
are hard to notice during review. Generally we try not to touch those lines in 
irrelevant patches to keep blame clean.

If you are running clang-format directly you can instead try clang-format-diff 
to format only changed lines. But not that crucial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[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:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl , ASTContext , int Levels,
+ TypeHierarchyDirection Direction) {

nridge wrote:
> sammccall wrote:
> > kadircet wrote:
> > > nridge wrote:
> > > > sammccall wrote:
> > > > > The scope and complexity of this function seems unneccesarily large:
> > > > >  - it's (in future?) capable of resolving in both directions
> > > > >  - it calls itself recursively, modifying TypeHierarchyDirection to 
> > > > > control the search
> > > > >  - it handles levels, even though this optimization is only important 
> > > > > for derived types
> > > > > 
> > > > > Can we restrict this to retrieving (all) recursive base types?
> > > > > i.e. `Optional getTypeAncestors(const 
> > > > > CXXRecordDecl , ASTContext )`
> > > > > Then later, subtype resolution can be the job of another function: 
> > > > > `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> > > > > 
> > > > > That way the recursion of getTypeAncestors is simpler to understand, 
> > > > > as it has much smaller scope. And code sharing between the two LSP 
> > > > > calls is clearer: fetching type hierarchy is a call to 
> > > > > `getTypeAncestors` and a call to `resolveTypeDescendants` (if 
> > > > > direction is children or both, and resolve is > 0), and resolving a 
> > > > > hierarchy is a call to `resolveTypeDescendants`.
> > > > If we remove "levels" here, should we introduce some kind of guard for 
> > > > infinite recursion, in case the user writes something like:
> > > > 
> > > > ```
> > > > template 
> > > > struct S : S {};
> > > > 
> > > > S<0> s;
> > > > ```
> > > clang should be limiting recursive template instantiations. Also since we 
> > > are just traversing the AST produced by clang, it should never be 
> > > infinite, but still a nice test case can you add one?
> > I think there is a point here...
> > 
> > Consider our `S` template with the direction reversed and a base case 
> > specialized:
> > ```
> > template 
> > struct S : S {};
> > template<>
> > struct S<0>{};
> > 
> > S<2> instance;
> > ```
> > 
> > Now the hierarchy of `S<2>` is well defined and finite: `S<2> : S<1> : 
> > S<0>`.
> > However IIUC the `CXXRecordDecl` for `S<2>` is the instantiation of the 
> > primary template, whose base is `S`, which is dependent[1], so the 
> > base's `CXXRecordDecl` is the primary template, whose base is `S`... 
> > and we never reach the base case.
> > 
> > Actually I'm not sure whether this happens if the base is dependent merely 
> > where it's spelled, or still dependent after instantiation? Even in the 
> > latter case one can construct examples where we'll infloop:
> > ```template 
> > struct Foo {
> >   S instance;
> > }```
> > Trying to get the hierarchy on `S` could infloop. (I agree these should 
> > both be test cases)
> > 
> > What's the Right Thing?
> >  - not sure about a recursion limit, as showing 10 `S`s in the 
> > hierarchy is silly.
> >  - not sure about bailing out on dependent types either, as knowing that 
> > e.g. my `SmallSet` inherits from `SmallMap` is meaningful or 
> > useful.
> >  - maybe we should bail out once we see instantiations of the same template 
> > decl twice in a parent chain. I.e. keep the seen non-null 
> > `CXXRecordDecl->getTemplateInstantiationPattern()`s in a set and stop 
> > recursing if insertion fails.
> > 
> > However for this patch I'd suggest just bailing out on dependent types with 
> > a TODO as the simplest, this is an edge case that can be fixed later.
> > 
> The current patch does actually recurse infinitely on this test case, likely 
> for the reasons Sam outlined (our handling of dependent types), and 
> eventually runs into the segfault.
> 
> I will update the patch to address this.
I meant to say "eventually runs into a stack overflow".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-std::string TweakID,
-Expected InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  Expected InpAST) {

kadircet wrote:
> Can you revert this change?
I tried, but clang-format automatically makes the change again.

Is there some particular clang-format configuration I should apply, so it 
produces the same results? I don't currently have any configuration, i.e. I 
think it's just reading the .clang-format file in the monorepo root.



Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl , ASTContext , int Levels,
+ TypeHierarchyDirection Direction) {

sammccall wrote:
> kadircet wrote:
> > nridge wrote:
> > > sammccall wrote:
> > > > The scope and complexity of this function seems unneccesarily large:
> > > >  - it's (in future?) capable of resolving in both directions
> > > >  - it calls itself recursively, modifying TypeHierarchyDirection to 
> > > > control the search
> > > >  - it handles levels, even though this optimization is only important 
> > > > for derived types
> > > > 
> > > > Can we restrict this to retrieving (all) recursive base types?
> > > > i.e. `Optional getTypeAncestors(const CXXRecordDecl 
> > > > , ASTContext )`
> > > > Then later, subtype resolution can be the job of another function: 
> > > > `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> > > > 
> > > > That way the recursion of getTypeAncestors is simpler to understand, as 
> > > > it has much smaller scope. And code sharing between the two LSP calls 
> > > > is clearer: fetching type hierarchy is a call to `getTypeAncestors` and 
> > > > a call to `resolveTypeDescendants` (if direction is children or both, 
> > > > and resolve is > 0), and resolving a hierarchy is a call to 
> > > > `resolveTypeDescendants`.
> > > If we remove "levels" here, should we introduce some kind of guard for 
> > > infinite recursion, in case the user writes something like:
> > > 
> > > ```
> > > template 
> > > struct S : S {};
> > > 
> > > S<0> s;
> > > ```
> > clang should be limiting recursive template instantiations. Also since we 
> > are just traversing the AST produced by clang, it should never be infinite, 
> > but still a nice test case can you add one?
> I think there is a point here...
> 
> Consider our `S` template with the direction reversed and a base case 
> specialized:
> ```
> template 
> struct S : S {};
> template<>
> struct S<0>{};
> 
> S<2> instance;
> ```
> 
> Now the hierarchy of `S<2>` is well defined and finite: `S<2> : S<1> : S<0>`.
> However IIUC the `CXXRecordDecl` for `S<2>` is the instantiation of the 
> primary template, whose base is `S`, which is dependent[1], so the 
> base's `CXXRecordDecl` is the primary template, whose base is `S`... 
> and we never reach the base case.
> 
> Actually I'm not sure whether this happens if the base is dependent merely 
> where it's spelled, or still dependent after instantiation? Even in the 
> latter case one can construct examples where we'll infloop:
> ```template 
> struct Foo {
>   S instance;
> }```
> Trying to get the hierarchy on `S` could infloop. (I agree these should 
> both be test cases)
> 
> What's the Right Thing?
>  - not sure about a recursion limit, as showing 10 `S`s in the hierarchy 
> is silly.
>  - not sure about bailing out on dependent types either, as knowing that e.g. 
> my `SmallSet` inherits from `SmallMap` is meaningful or useful.
>  - maybe we should bail out once we see instantiations of the same template 
> decl twice in a parent chain. I.e. keep the seen non-null 
> `CXXRecordDecl->getTemplateInstantiationPattern()`s in a set and stop 
> recursing if insertion fails.
> 
> However for this patch I'd suggest just bailing out on dependent types with a 
> TODO as the simplest, this is an edge case that can be fixed later.
> 
The current patch does actually recurse infinitely on this test case, likely 
for the reasons Sam outlined (our handling of dependent types), and eventually 
runs into the segfault.

I will update the patch to address this.



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1562
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }

kadircet wrote:
> Can you put a TODO?
I added a comment to clarify that a field does not unambiguously specify a 
record type. i don't think there's anything further "to do" here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189444.
nridge marked 10 inline comments as done.
nridge added a comment.

Address most of the latest review comments.

The infinite recursion issue remains to be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,361 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+// A field does not unambiguously specify a record type
+// (possible associated reocrd types could be the field's type,
+// or the type of the record that the field is a member of).
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for picking this up... just wanted to leave one last comment as I'd been 
thinking about the recursive template case too.




Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl , ASTContext , int Levels,
+ TypeHierarchyDirection Direction) {

kadircet wrote:
> nridge wrote:
> > sammccall wrote:
> > > The scope and complexity of this function seems unneccesarily large:
> > >  - it's (in future?) capable of resolving in both directions
> > >  - it calls itself recursively, modifying TypeHierarchyDirection to 
> > > control the search
> > >  - it handles levels, even though this optimization is only important for 
> > > derived types
> > > 
> > > Can we restrict this to retrieving (all) recursive base types?
> > > i.e. `Optional getTypeAncestors(const CXXRecordDecl 
> > > , ASTContext )`
> > > Then later, subtype resolution can be the job of another function: 
> > > `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> > > 
> > > That way the recursion of getTypeAncestors is simpler to understand, as 
> > > it has much smaller scope. And code sharing between the two LSP calls is 
> > > clearer: fetching type hierarchy is a call to `getTypeAncestors` and a 
> > > call to `resolveTypeDescendants` (if direction is children or both, and 
> > > resolve is > 0), and resolving a hierarchy is a call to 
> > > `resolveTypeDescendants`.
> > If we remove "levels" here, should we introduce some kind of guard for 
> > infinite recursion, in case the user writes something like:
> > 
> > ```
> > template 
> > struct S : S {};
> > 
> > S<0> s;
> > ```
> clang should be limiting recursive template instantiations. Also since we are 
> just traversing the AST produced by clang, it should never be infinite, but 
> still a nice test case can you add one?
I think there is a point here...

Consider our `S` template with the direction reversed and a base case 
specialized:
```
template 
struct S : S {};
template<>
struct S<0>{};

S<2> instance;
```

Now the hierarchy of `S<2>` is well defined and finite: `S<2> : S<1> : S<0>`.
However IIUC the `CXXRecordDecl` for `S<2>` is the instantiation of the primary 
template, whose base is `S`, which is dependent[1], so the base's 
`CXXRecordDecl` is the primary template, whose base is `S`... and we 
never reach the base case.

Actually I'm not sure whether this happens if the base is dependent merely 
where it's spelled, or still dependent after instantiation? Even in the latter 
case one can construct examples where we'll infloop:
```template 
struct Foo {
  S instance;
}```
Trying to get the hierarchy on `S` could infloop. (I agree these should both 
be test cases)

What's the Right Thing?
 - not sure about a recursion limit, as showing 10 `S`s in the hierarchy 
is silly.
 - not sure about bailing out on dependent types either, as knowing that e.g. 
my `SmallSet` inherits from `SmallMap` is meaningful or useful.
 - maybe we should bail out once we see instantiations of the same template 
decl twice in a parent chain. I.e. keep the seen non-null 
`CXXRecordDecl->getTemplateInstantiationPattern()`s in a set and stop recursing 
if insertion fails.

However for this patch I'd suggest just bailing out on dependent types with a 
TODO as the simplest, this is an edge case that can be fixed later.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
   Callback CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-std::string TweakID,
-Expected InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  Expected InpAST) {

Can you revert this change?



Comment at: clang-tools-extra/clangd/FindSymbols.h:16
 #include "Protocol.h"
+#include "index/Index.h"
 #include "llvm/ADT/StringRef.h"

this should rather be `clang/Index/IndexSymbol.h`. `index::SymbolKind` resides 
in that header.



Comment at: clang-tools-extra/clangd/FindSymbols.h:21
+class ASTContext;
+class NamedDecl;
+

I don't think these two are necessary inside header file.



Comment at: clang-tools-extra/clangd/Protocol.cpp:871
+  O.map("children", I.children);
+  return true;
+}

Shouldn't we fail if some of the above fields(like `name`) fails to decode ?



Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl , ASTContext , int Levels,
+ TypeHierarchyDirection Direction) {

nridge wrote:
> sammccall wrote:
> > The scope and complexity of this function seems unneccesarily large:
> >  - it's (in future?) capable of resolving in both directions
> >  - it calls itself recursively, modifying TypeHierarchyDirection to control 
> > the search
> >  - it handles levels, even though this optimization is only important for 
> > derived types
> > 
> > Can we restrict this to retrieving (all) recursive base types?
> > i.e. `Optional getTypeAncestors(const CXXRecordDecl 
> > , ASTContext )`
> > Then later, subtype resolution can be the job of another function: 
> > `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> > 
> > That way the recursion of getTypeAncestors is simpler to understand, as it 
> > has much smaller scope. And code sharing between the two LSP calls is 
> > clearer: fetching type hierarchy is a call to `getTypeAncestors` and a call 
> > to `resolveTypeDescendants` (if direction is children or both, and resolve 
> > is > 0), and resolving a hierarchy is a call to `resolveTypeDescendants`.
> If we remove "levels" here, should we introduce some kind of guard for 
> infinite recursion, in case the user writes something like:
> 
> ```
> template 
> struct S : S {};
> 
> S<0> s;
> ```
clang should be limiting recursive template instantiations. Also since we are 
just traversing the AST produced by clang, it should never be infinite, but 
still a nice test case can you add one?



Comment at: clang-tools-extra/clangd/XRefs.cpp:918
+
+  return CXXRD;
+}

Nit: Get rid of CXXRD and return inside if/else branches.



Comment at: clang-tools-extra/clangd/XRefs.cpp:924
+
+  for (auto It = CXXRD->bases_begin(); It != CXXRD->bases_end(); It++) {
+const CXXRecordDecl *ParentDecl = nullptr;

why not use a for-range loop ? (`CXXRD->bases()`)



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1562
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }

Can you put a TODO?



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1809
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));

I suppose parent resolving should not depend on level, so what about setting it 
to `0` instead of `10` so that test is consistent even after "child resolution" 
implementation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189114.
nridge added a comment.

Address a couple of outstanding TODOs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,337 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent1 =
+  dyn_cast((AST, "Parent1"));
+  const CXXRecordDecl *Parent2 =
+  dyn_cast((AST, 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189092.
nridge marked 2 inline comments as done.
nridge added a comment.

Address latest review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,323 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+TEST(TypeParents, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  EXPECT_THAT(typeParents(Parent), ElementsAre());
+  EXPECT_THAT(typeParents(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeParents(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeParents, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent1 =
+  dyn_cast((AST, "Parent1"));
+  const CXXRecordDecl 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 19 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/Protocol.cpp:830
+return false;
+  if (auto *Resolve = Params.getAsObject()->get("resolve")) {
+if (!fromJSON(*Resolve, R.resolve)) {

sammccall wrote:
> use ObjectMapper here? It's more idiomatic and avoids the issues below.
The reason I didn't use ObjectMapper is that I didn't know how to use it while 
also reusing the `fromJSON()` method for the base class, 
`TextDocumentPositionParams`.

I suppose I could just not reuse it (i.e. mention the fields of 
`TextDocumentPositionParams` in this function directly).



Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl , ASTContext , int Levels,
+ TypeHierarchyDirection Direction) {

sammccall wrote:
> The scope and complexity of this function seems unneccesarily large:
>  - it's (in future?) capable of resolving in both directions
>  - it calls itself recursively, modifying TypeHierarchyDirection to control 
> the search
>  - it handles levels, even though this optimization is only important for 
> derived types
> 
> Can we restrict this to retrieving (all) recursive base types?
> i.e. `Optional getTypeAncestors(const CXXRecordDecl 
> , ASTContext )`
> Then later, subtype resolution can be the job of another function: 
> `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> 
> That way the recursion of getTypeAncestors is simpler to understand, as it 
> has much smaller scope. And code sharing between the two LSP calls is 
> clearer: fetching type hierarchy is a call to `getTypeAncestors` and a call 
> to `resolveTypeDescendants` (if direction is children or both, and resolve is 
> > 0), and resolving a hierarchy is a call to `resolveTypeDescendants`.
If we remove "levels" here, should we introduce some kind of guard for infinite 
recursion, in case the user writes something like:

```
template 
struct S : S {};

S<0> s;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hi Nathan,
Really sorry about the delay here.
I'm actually out on leave at the moment, and didn't get things wrapped up 
before I went.
(I'm 1 week into 4 week parental leave, and got sick the week I was supposed to 
be handing things off).

@kadircet, can you finish the review here so we can get this landed?
My main concern is I think this can be further simplified by always fetching 
all ancestors, and treating "fill descendants" as basically a separate 
operation.
If it's not clear how to proceed there, we can land this and I can work on 
simplifying when I get back.
Cheers, Sam




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:530
+  auto Action = [Pos, Resolve,
+ Direction](Callback> CB,
+Expected InpAST) {

nit: `decltype(CB) CB` is I think a little more readable (once you're used to 
the idiom)



Comment at: clang-tools-extra/clangd/Protocol.cpp:830
+return false;
+  if (auto *Resolve = Params.getAsObject()->get("resolve")) {
+if (!fromJSON(*Resolve, R.resolve)) {

use ObjectMapper here? It's more idiomatic and avoids the issues below.



Comment at: clang-tools-extra/clangd/Protocol.cpp:831
+  if (auto *Resolve = Params.getAsObject()->get("resolve")) {
+if (!fromJSON(*Resolve, R.resolve)) {
+  R.resolve = 0; // default value if not specified

this seems confused - if there is a "resolve" field, but it's not a valid 
integer, then we should return false (fail to decode)



Comment at: clang-tools-extra/clangd/Protocol.cpp:832
+if (!fromJSON(*Resolve, R.resolve)) {
+  R.resolve = 0; // default value if not specified
+}

prefer to initialize it inline in the struct, rather than manually initializing 
in failure cases.
As written, if the "resolve" field isn't present at all, the field will be 
uninitialized.



Comment at: clang-tools-extra/clangd/Protocol.cpp:860
+Result["children"] = I.children;
+  // Older gcc cannot compile 'return Result', even though it is legal.
+  return llvm::json::Value(std::move(Result));

nit: this happens in lots of places, and we don't usually add a comment in each.
`return std::move(Result);` should be enough.

(technically I think this *isn't* legal in C++11, as DR1579 was only addressed 
in 14. But most compilers backport it...)



Comment at: clang-tools-extra/clangd/Protocol.h:1025
+  /// The hierarchy levels to resolve. `0` indicates no level.
+  int resolve;
+

`= 0`



Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional
+getTypeHierarchy(const CXXRecordDecl , ASTContext , int Levels,
+ TypeHierarchyDirection Direction) {

The scope and complexity of this function seems unneccesarily large:
 - it's (in future?) capable of resolving in both directions
 - it calls itself recursively, modifying TypeHierarchyDirection to control the 
search
 - it handles levels, even though this optimization is only important for 
derived types

Can we restrict this to retrieving (all) recursive base types?
i.e. `Optional getTypeAncestors(const CXXRecordDecl , 
ASTContext )`
Then later, subtype resolution can be the job of another function: 
`resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`

That way the recursion of getTypeAncestors is simpler to understand, as it has 
much smaller scope. And code sharing between the two LSP calls is clearer: 
fetching type hierarchy is a call to `getTypeAncestors` and a call to 
`resolveTypeDescendants` (if direction is children or both, and resolve is > 
0), and resolving a hierarchy is a call to `resolveTypeDescendants`.



Comment at: clang-tools-extra/clangd/XRefs.cpp:990
+
+  TypeHierarchyDirection ResolveDirection =
+  Direction.getValueOr(TypeHierarchyDirection::Parents);

this kind of interpreting of request messages should be a few layers up: 
probably TypeHierarchyRequest should just have a `TypeHierarchyDirection 
direction = Parents` and replace it if present while parsing.



Comment at: clang-tools-extra/clangd/XRefs.h:65
+/// Given a record type declaration, find its base (parent) types.
+std::vector typeAncestors(const CXXRecordDecl *CXXRD);
+

the name says "ancestors" (i.e. recursive) but the implementation is parents 
(non-recursive). I guess the name should change?



Comment at: clang-tools-extra/clangd/XRefs.h:72
+
+/// Convert a a URI (such as that returned by toURI()) into a form suitable
+/// for use in protocol replies (e.g. Location.uri, DocumentSymbol.uri).

XRefs.h isn't a suitable place to expose such a function.
It seems to be called only in one place in XRefs.cpp now, which doesn't need to 
be changed in this patch, can we revert the change?

If 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 189065.
nridge added a comment.

Rebased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1478,6 +1493,325 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+// TODO: FindRecordTypeAt, TemplateSpec
+
+TEST(TypeAncestors, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  EXPECT_THAT(typeAncestors(Parent), ElementsAre());
+  EXPECT_THAT(typeAncestors(Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeAncestors(Child2), ElementsAre(Child1));
+}
+
+TEST(TypeAncestors, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent1 =
+  dyn_cast((AST, "Parent1"));
+  const CXXRecordDecl *Parent2 =
+  

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I think this is ready to continue to be reviewed :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186947.
nridge added a comment.

Fix a test failure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/test/clangd/initialize-params.test
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1389,6 +1404,337 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+// TODO: FindRecordTypeAt, TemplateSpec
+
+TEST(TypeAncestors, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  ASTContext  = AST.getASTContext();
+
+  EXPECT_THAT(typeAncestors(Ctx, Parent), ElementsAre());
+  EXPECT_THAT(typeAncestors(Ctx, Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeAncestors(Ctx, Child2), ElementsAre(Child1));
+}
+
+TEST(TypeAncestors, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent1 =
+  

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1604
+  dyn_cast((AST, 
"Parent"))->getTemplatedDecl();
+  // TODO: This fails because findDecl() doesn't support template-ids
+  // const CXXRecordDecl *ParentSpec =

Any suggestions for how we can make `findDecl()` support //template-id//s?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186944.
nridge added a comment.

- Rework tests to test the lower-level functions like typeAncestors()
- Remove support for typeHierarchy/resolve


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -15,6 +15,8 @@
 #include "XRefs.h"
 #include "index/FileIndex.h"
 #include "index/SymbolCollector.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -25,9 +27,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +45,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1389,6 +1404,337 @@
   }
 }
 
+TEST(FindRecordTypeAt, TypeOrVariable) {
+  Annotations Source(R"cpp(
+struct Ch^ild2 {
+  int c;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Method) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+int main() {
+  Child2 child2;
+  child2.met^hod(5);
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ((AST, "Child2"), static_cast(RD));
+  }
+}
+
+TEST(FindRecordTypeAt, Field) {
+  Annotations Source(R"cpp(
+struct Child2 {
+  int fi^eld;
+};
+
+int main() {
+  Child2 child2;
+  child2.fi^eld = 5;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  for (Position Pt : Source.points()) {
+const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+EXPECT_EQ(nullptr, RD);
+  }
+}
+
+// TODO: FindRecordTypeAt, TemplateSpec
+
+TEST(TypeAncestors, SimpleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent {
+  int a;
+};
+
+struct Child1 : Parent {
+  int b;
+};
+
+struct Child2 : Child1 {
+  int c;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  const CXXRecordDecl *Parent =
+  dyn_cast((AST, "Parent"));
+  const CXXRecordDecl *Child1 =
+  dyn_cast((AST, "Child1"));
+  const CXXRecordDecl *Child2 =
+  dyn_cast((AST, "Child2"));
+
+  ASTContext  = AST.getASTContext();
+
+  EXPECT_THAT(typeAncestors(Ctx, Parent), ElementsAre());
+  EXPECT_THAT(typeAncestors(Ctx, Child1), ElementsAre(Parent));
+  EXPECT_THAT(typeAncestors(Ctx, Child2), ElementsAre(Child1));
+}
+
+TEST(TypeAncestors, MultipleInheritance) {
+  Annotations Source(R"cpp(
+struct Parent1 {
+  int a;
+};
+
+struct Parent2 {
+  int b;
+};
+
+struct Parent3 : Parent2 {
+  int c;
+};
+
+struct Child : Parent1, Parent3 {
+  int d;
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+ 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:842
+
+  StringRef Filename = SM.getFilename(BeginLoc);
+  std::string FileURI = toURI(SM, Filename, {});

sammccall wrote:
> This is more conversions than necessary.
> I think we just need:
> 
> ```auto FilePath = 
> getCanonicalPath(SM.getFileEntryForID(SM.getFileID(BeginLoc)), SM);
> auto TUPath = getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()));
> if (!FilePath || !TUPath)
>   return;
> THI.uri = URIForFile::Canonicalize(*FilePath, *TUPath);
> ```
> 
> (This is still more lines than I'd like, maybe we should have some helper for 
> going from (File ID, SourceManager) --> URI)
(I looked at adding such a helper, and I don't think it's a good idea - the few 
existing callsites that could use it currently benefit significantly from 
hoisting the getCanonicalPath from the TUPath outside of a loop. It may or may 
not be worth doing that here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+  return CB(InpAST.takeError());
+CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+Direction));

nridge wrote:
> sammccall wrote:
> > nridge wrote:
> > > sammccall wrote:
> > > > relying on the item range to resolve the actual symbol isn't reliable:
> > > >  - the source code may have changed
> > > >  - the range may not be within the TU, and might be e.g. in an indexed 
> > > > header we don't have a compile command for.
> > > Good point. I appreciate a bit better now why you suggested trying to 
> > > avoid `resolve` for now :)
> > > 
> > > What do you think of the following implementation approach:
> > > 
> > >  * Use the `data` field of `TypeHierarchyItem` (which the client will 
> > > send back to the server for `resolve` queries) to send the `SymbolID` of 
> > > the item to the client.
> > >  * When the client sends back the `SymbolID` in the `resolve` request, 
> > > look up the symbol in the index, and use the source range from the symbol.
> > > 
> > > (Later, when we start storing base <--> derived relationships in the 
> > > index for subtype support, we could just answer the entire `resolve` 
> > > query using the index.)
> > Thanks for exploring all the options here!
> > 
> > Generally we've tried to avoid relying on the index unless it's needed, 
> > using the AST where possible. There are failure modes here:
> >  - the base type is in the current file, which is actively edited. The 
> > ranges in the index may be off due to staleness.
> >  - the base type is not indexed, because it is e.g. a member class inside a 
> > class template
> >  - there's no index (`-index=0`, though I'm not sure why we still support 
> > this) or the index is stale and the type is missing (we're working on 
> > making index updates more async)
> >  - the base type is not encodable.
> > 
> > There are just more moving pieces here, I think. Is there a clear reason to 
> > support resolve for parents?
> > Is there a clear reason to support resolve for parents?
> 
> Just what I said earlier about a hypothetical client that relies on it.
> 
> However, given the complications involved in implementing it, I'm happy with 
> only being concerned with actual clients, not hypothetical ones. The only 
> client implementation I currently know of is Theia, and I checked that it 
> works fine without `resolve`, so I'm happy with deferring work on `resolve` 
> for now.
> 
> If another client comes along that relies on `resolve`, we can revisit this.
> Just what I said earlier about a hypothetical client that relies on it.

I'll try to get the spec clarified such that eager resolution is always fine.
But if we ever do need to deal with such clients, @ioeric had a neat idea: we 
can just stash the rest of the response in the `data` field, and then 
"resolution" can just pull it out. This should be easy to bolt on.



Comment at: clang-tools-extra/clangd/XRefs.cpp:842
+
+  StringRef Filename = SM.getFilename(BeginLoc);
+  std::string FileURI = toURI(SM, Filename, {});

This is more conversions than necessary.
I think we just need:

```auto FilePath = 
getCanonicalPath(SM.getFileEntryForID(SM.getFileID(BeginLoc)), SM);
auto TUPath = getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()));
if (!FilePath || !TUPath)
  return;
THI.uri = URIForFile::Canonicalize(*FilePath, *TUPath);
```

(This is still more lines than I'd like, maybe we should have some helper for 
going from (File ID, SourceManager) --> URI)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+  return CB(InpAST.takeError());
+CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+Direction));

sammccall wrote:
> nridge wrote:
> > sammccall wrote:
> > > relying on the item range to resolve the actual symbol isn't reliable:
> > >  - the source code may have changed
> > >  - the range may not be within the TU, and might be e.g. in an indexed 
> > > header we don't have a compile command for.
> > Good point. I appreciate a bit better now why you suggested trying to avoid 
> > `resolve` for now :)
> > 
> > What do you think of the following implementation approach:
> > 
> >  * Use the `data` field of `TypeHierarchyItem` (which the client will send 
> > back to the server for `resolve` queries) to send the `SymbolID` of the 
> > item to the client.
> >  * When the client sends back the `SymbolID` in the `resolve` request, look 
> > up the symbol in the index, and use the source range from the symbol.
> > 
> > (Later, when we start storing base <--> derived relationships in the index 
> > for subtype support, we could just answer the entire `resolve` query using 
> > the index.)
> Thanks for exploring all the options here!
> 
> Generally we've tried to avoid relying on the index unless it's needed, using 
> the AST where possible. There are failure modes here:
>  - the base type is in the current file, which is actively edited. The ranges 
> in the index may be off due to staleness.
>  - the base type is not indexed, because it is e.g. a member class inside a 
> class template
>  - there's no index (`-index=0`, though I'm not sure why we still support 
> this) or the index is stale and the type is missing (we're working on making 
> index updates more async)
>  - the base type is not encodable.
> 
> There are just more moving pieces here, I think. Is there a clear reason to 
> support resolve for parents?
> Is there a clear reason to support resolve for parents?

Just what I said earlier about a hypothetical client that relies on it.

However, given the complications involved in implementing it, I'm happy with 
only being concerned with actual clients, not hypothetical ones. The only 
client implementation I currently know of is Theia, and I checked that it works 
fine without `resolve`, so I'm happy with deferring work on `resolve` for now.

If another client comes along that relies on `resolve`, we can revisit this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+  return CB(InpAST.takeError());
+CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+Direction));

nridge wrote:
> sammccall wrote:
> > relying on the item range to resolve the actual symbol isn't reliable:
> >  - the source code may have changed
> >  - the range may not be within the TU, and might be e.g. in an indexed 
> > header we don't have a compile command for.
> Good point. I appreciate a bit better now why you suggested trying to avoid 
> `resolve` for now :)
> 
> What do you think of the following implementation approach:
> 
>  * Use the `data` field of `TypeHierarchyItem` (which the client will send 
> back to the server for `resolve` queries) to send the `SymbolID` of the item 
> to the client.
>  * When the client sends back the `SymbolID` in the `resolve` request, look 
> up the symbol in the index, and use the source range from the symbol.
> 
> (Later, when we start storing base <--> derived relationships in the index 
> for subtype support, we could just answer the entire `resolve` query using 
> the index.)
Thanks for exploring all the options here!

Generally we've tried to avoid relying on the index unless it's needed, using 
the AST where possible. There are failure modes here:
 - the base type is in the current file, which is actively edited. The ranges 
in the index may be off due to staleness.
 - the base type is not indexed, because it is e.g. a member class inside a 
class template
 - there's no index (`-index=0`, though I'm not sure why we still support this) 
or the index is stale and the type is missing (we're working on making index 
updates more async)
 - the base type is not encodable.

There are just more moving pieces here, I think. Is there a clear reason to 
support resolve for parents?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.
Herald added a subscriber: jdoerfert.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+  return CB(InpAST.takeError());
+CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+Direction));

sammccall wrote:
> relying on the item range to resolve the actual symbol isn't reliable:
>  - the source code may have changed
>  - the range may not be within the TU, and might be e.g. in an indexed header 
> we don't have a compile command for.
Good point. I appreciate a bit better now why you suggested trying to avoid 
`resolve` for now :)

What do you think of the following implementation approach:

 * Use the `data` field of `TypeHierarchyItem` (which the client will send back 
to the server for `resolve` queries) to send the `SymbolID` of the item to the 
client.
 * When the client sends back the `SymbolID` in the `resolve` request, look up 
the symbol in the index, and use the source range from the symbol.

(Later, when we start storing base <--> derived relationships in the index for 
subtype support, we could just answer the entire `resolve` query using the 
index.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:925
+CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl();
+  } else if (const CXXMethodDecl *Method = dyn_cast(D)) {
+// If this is a method, use the type of the class.

nridge wrote:
> kadircet wrote:
> > what about member fields ?
> It's not clear what the desired semantics would be for a member field: get 
> the type hierarchy of the enclosing class type, or the type hierarchy of the 
> field's type?
I think it is sensible to go for enclosing type. But up to you, in any case 
could you add a comment stating how `FieldDecl` are handled?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via cfe-commits]

(Sorry, hit enter too soon and truncated one of the comments)

On Mon, Feb 11, 2019 at 10:32 AM Sam McCall via Phabricator <
revi...@reviews.llvm.org> wrote:

> sammccall added a comment.
>
> In D56370#1391924 , @nridge
> wrote:
>
> > As far as reworking the tests to use these functions, I've thought about
> that a bit:
> >
> > - These functions return AST nodes. It's not clear to me how I would
> come up with "expected" AST nodes to test the return values against.
> See FindDecl
>
See the findDecl overloads in TestTU.h - we use these for such tests.

> - Even if we find a way to get "expected" AST nodes, we would be losing
> test coverage of functions like `declToTypeHierarchyItem()` (though I
> suppose we could write separate tests for that).
>
Yes, please do add unit tests for the functions separately - findDecl()
also words to get the input to that function.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56370#1391924 , @nridge wrote:

> As far as reworking the tests to use these functions, I've thought about that 
> a bit:
>
> - These functions return AST nodes. It's not clear to me how I would come up 
> with "expected" AST nodes to test the return values against.


See FindDecl

> - Even if we find a way to get "expected" AST nodes, we would be losing test 
> coverage of functions like `declToTypeHierarchyItem()` (though I suppose we 
> could write separate tests for that).
> - We could instead test against the //properties// of the AST nodes, such as 
> their names and source ranges, but then the test code to query those 
> properties would basically be duplicating code in 
> `declToTypeHierarchyItem()`. It would seem to make more sense to just test 
> the resulting `TypeHierarchyItem` instead (as the tests are doing now).






Comment at: clang-tools-extra/clangd/ClangdServer.cpp:543
+  return CB(InpAST.takeError());
+CB(clangd::getTypeHierarchy(InpAST->AST, Item.range.start, Resolve,
+Direction));

relying on the item range to resolve the actual symbol isn't reliable:
 - the source code may have changed
 - the range may not be within the TU, and might be e.g. in an indexed header 
we don't have a compile command for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56370#1390359 , @sammccall wrote:

> - in 'XRefs.h', I think the API to introduce is `findTypeAt(Position) -> 
> Decl*` +  `typeAncestors(Decl*)` and later `typeDescendants(Decl*)`, rather 
> than a single more complex `typeHierarchy` call. These two operations have 
> little in common implementation-wise, and it's easy to imagine editors 
> preferring to expose them separately. In clangdserver of course we need to 
> expose a single operation because of transactionality. The stitching together 
> could go in clangdserver, or a higher-level function exposed by xrefs - but I 
> think the separate functions are what we should be unit-testing.


In the latest update I introduced these helpers. I made the first one 
`findRecordTypeAt()` instead of `findTypeAt()`, since `findTypeAt()` suggests 
it also works for built-in types like `int`, but I don't think we can get a 
`Decl*` for `int`.

As far as reworking the tests to use these functions, I've thought about that a 
bit:

- These functions return AST nodes. It's not clear to me how I would come up 
with "expected" AST nodes to test the return values against.
- Even if we find a way to get "expected" AST nodes, we would be losing test 
coverage of functions like `declToTypeHierarchyItem()` (though I suppose we 
could write separate tests for that).
- We could instead test against the //properties// of the AST nodes, such as 
their names and source ranges, but then the test code to query those properties 
would basically be duplicating code in `declToTypeHierarchyItem()`. It would 
seem to make more sense to just test the resulting `TypeHierarchyItem` instead 
(as the tests are doing now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186139.
nridge added a comment.

Introduce and use findRecordTypeAt() and typeAncestors() helpers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +43,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1365,6 +1378,277 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]] {
+  int a;
+};
+
+struct $Child1Def[[Child1]] : Parent {
+  int b;
+};
+
+struct Ch^ild2 : Child1 {
+  int c;
+};
+
+struct Child3 : Child2 {
+  int d;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+
+  parent.a = 1;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (Position Pt : Source.points()) {
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]] {
+  int a;
+};
+
+struct $Parent2Def[[Parent2]] {
+  int b;
+};
+
+struct $Parent3Def[[Parent3]] : Parent2 {
+  int c;
+};
+
+struct Ch^ild : Parent1, Parent3 {
+  int d;
+};
+
+int main() {
+  Ch^ild  ch$c3^ild;
+
+  ch^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (Position Pt : Source.points()) {
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]] {
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent {
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+struct Child3 : Child2 {
+  void method (int x);
+};
+)cpp");
+
+  TestTU TU = 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56370#1390359 , @sammccall wrote:

> So on a concrete but still high-level:
>
> - I don't think we should implement the `resolve` operation, or resolution 
> bounds, at this point - this patch doesn't do anything slow. Returning 
> complete ancestors and never returning any children seems simplest.


A hypothetical client could always ask for one level at a time, and ignore any 
levels it didn't ask for; such a client would break if we did this.

I think the implementation of `resolve` is straightforward enough that we might 
as well keep it.

> - in 'XRefs.h', I think the API to introduce is `findTypeAt(Position) -> 
> Decl*` +  `typeAncestors(Decl*)` and later `typeDescendants(Decl*)`, rather 
> than a single more complex `typeHierarchy` call. These two operations have 
> little in common implementation-wise, and it's easy to imagine editors 
> preferring to expose them separately. In clangdserver of course we need to 
> expose a single operation because of transactionality. The stitching together 
> could go in clangdserver, or a higher-level function exposed by xrefs - but I 
> think the separate functions are what we should be unit-testing.

This sounds nice and clean, thanks for the suggestion! I will give it a try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186137.
nridge marked 4 inline comments as done.
nridge added a comment.

Address Kadir's review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +43,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1365,6 +1378,277 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]] {
+  int a;
+};
+
+struct $Child1Def[[Child1]] : Parent {
+  int b;
+};
+
+struct Ch^ild2 : Child1 {
+  int c;
+};
+
+struct Child3 : Child2 {
+  int d;
+};
+
+int main() {
+  Ch^ild2 ch^ild2;
+
+  parent.a = 1;
+  ch^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (Position Pt : Source.points()) {
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]] {
+  int a;
+};
+
+struct $Parent2Def[[Parent2]] {
+  int b;
+};
+
+struct $Parent3Def[[Parent3]] : Parent2 {
+  int c;
+};
+
+struct Ch^ild : Parent1, Parent3 {
+  int d;
+};
+
+int main() {
+  Ch^ild  ch$c3^ild;
+
+  ch^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (Position Pt : Source.points()) {
+llvm::Optional Result =
+getTypeHierarchy(AST, Pt, 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]] {
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent {
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1 {
+  void met^hod ();
+  void met^hod (int x);
+};
+
+struct Child3 : Child2 {
+  void method (int x);
+};
+)cpp");
+
+  TestTU TU = 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 19 inline comments as done.
nridge added a comment.

Thank you for the reviews!




Comment at: clang-tools-extra/clangd/Protocol.h:1026
+  /// When not defined, it is treated as `0`.
+  llvm::Optional resolve;
+

kadircet wrote:
> why not just use an int then ?
I was making the data structure correspond closely to the protocol, where the 
field is optional. But we can handle this in the serialization logic, yes.



Comment at: clang-tools-extra/clangd/XRefs.cpp:891
+if (Optional ParentSym =
+getTypeHierarchy(*ParentDecl, ASTCtx, Levels - 1, Direction)) {
+  Result->parents->emplace_back(std::move(*ParentSym));

kadircet wrote:
> A problem that might occur when you add children traversal:
> do we really want to include current decl in children of parent when we have 
> `BOTH` as direction? If so, maybe we should rather cache the results? Maybe 
> leave a todo/fixme ?
Actually, this is a bug: if the top-level call uses `Both`, the recursive calls 
should still just use `Parents` and `Children` (i.e. we never want children of 
parents or parents of children). Thanks for spotting that!



Comment at: clang-tools-extra/clangd/XRefs.cpp:925
+CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl();
+  } else if (const CXXMethodDecl *Method = dyn_cast(D)) {
+// If this is a method, use the type of the class.

kadircet wrote:
> what about member fields ?
It's not clear what the desired semantics would be for a member field: get the 
type hierarchy of the enclosing class type, or the type hierarchy of the 
field's type?



Comment at: clang-tools-extra/clangd/XRefs.cpp:935
+TypeHierarchyDirection ResolveDirection =
+Direction.getValueOr(TypeHierarchyDirection::Parents);
+return getTypeHierarchy(*CXXRD, ASTCtx, ResolveLevels, ResolveDirection);

kadircet wrote:
> Is this mentioned in proposal?
It's not specified; I left a [comment on the 
proposal](https://github.com/Microsoft/vscode-languageserver-node/pull/426/commits/876d7fe224e17d7c08258a6da21d11a2df9c1d0d#r252942213)
 asking about it.



Comment at: clang-tools-extra/unittests/clangd/Matchers.h:130
 
+// Implements the HasValue(m) matcher for matching an Optional whose
+// value matches matcher m.

sammccall wrote:
> nridge wrote:
> > Should I split this out into a separate patch? It's used by the tests being 
> > added for this functionality.
> This is nice! I think it should probably go in llvm/Testing/Support/..., but 
> it's OK here.
I'll leave it here for now. We can move it in a follow-up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D56370#1391904 , @nridge wrote:

> Add tests for scenarios involving class template specializations
>
> Also improve support for dependent bases


(This update is unrelated to the review comments, it's just improvements I had 
in the queue already. Another update to address review comments will come next.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 186136.
nridge added a comment.

Add tests for scenarios involving class template specializations

Also improve support for dependent bases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,13 @@
 namespace clangd {
 namespace {
 
+using testing::AllOf;
 using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +43,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1365,6 +1378,291 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  int a;
+};
+
+struct $Child1Def[[Child1]] : Parent
+{
+  int b;
+};
+
+struct Ch$p1^ild2 : Child1
+{
+  int c;
+};
+
+struct Child3 : Child2
+{
+  int d;
+};
+
+int main()
+{
+  Ch$p2^ild2 ch$p3^ild2;
+
+  parent.a = 1;
+  ch$p4^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]]
+{
+  int a;
+};
+
+struct $Parent2Def[[Parent2]]
+{
+  int b;
+};
+
+struct $Parent3Def[[Parent3]] : Parent2
+{
+  int c;
+};
+
+struct Ch$c1^ild : Parent1, Parent3
+{
+  int d;
+};
+
+int main()
+{
+  Ch$c2^ild  ch$c3^ild;
+
+  ch$c4^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent
+{
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1
+{
+  void met$p1^hod ();
+  

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Updated reviewers line to reflect (I think) current reality so this doesn't get 
lost, but feel free to reassign as you'd prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56370#1390283 , @kadircet wrote:

> Implementation LG, but I am not sure about adding a functionality on a 
> proposal that might change. WDYT @sammccall ?


We discussed this a bit on the thread, I think we should follow the proposed 
spec, but with some useful hedges:

- only implement enough bits to be useful
- I'm not sure the internal APIs/implementation should follow the "shape" of 
the spec so deeply.

So on a concrete but still high-level:

- I don't think we should implement the `resolve` operation, or resolution 
bounds, at this point - this patch doesn't do anything slow. Returning complete 
ancestors and never returning any children seems simplest.
- in 'XRefs.h', I think the API to introduce is `findTypeAt(Position) -> Decl*` 
+  `typeAncestors(Decl*)` and later `typeDescendants(Decl*)`, rather than a 
single more complex `typeHierarchy` call. These two operations have little in 
common implementation-wise, and it's easy to imagine editors preferring to 
expose them separately. In clangdserver of course we need to expose a single 
operation because of transactionality. The stitching together could go in 
clangdserver, or a higher-level function exposed by xrefs - but I think the 
separate functions are what we should be unit-testing.




Comment at: clang-tools-extra/unittests/clangd/Matchers.h:130
 
+// Implements the HasValue(m) matcher for matching an Optional whose
+// value matches matcher m.

nridge wrote:
> Should I split this out into a separate patch? It's used by the tests being 
> added for this functionality.
This is nice! I think it should probably go in llvm/Testing/Support/..., but 
it's OK here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a subscriber: sammccall.
kadircet added a comment.

Implementation LG, but I am not sure about adding a functionality on a proposal 
that might change. WDYT @sammccall ?




Comment at: clang-tools-extra/clangd/FindSymbols.cpp:16
 #include "SourceCode.h"
+#include "XRefs.h"
 #include "index/Index.h"

it seems like there is no change in file requiring this



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:218
   }
+
   return SI;

can you revert this one as well ?



Comment at: clang-tools-extra/clangd/Protocol.cpp:817
+bool fromJSON(const llvm::json::Value , TypeHierarchyDirection ) {
+  if (auto T = E.getAsInteger()) {
+if (*T < static_cast(TypeHierarchyDirection::Children) ||

nit: get rid of nesting by
```
auto T = E.getas..;
if(!T)
  return false;
if(*T not in rage)
  return false;
Out = cast(*T);
return true;
```



Comment at: clang-tools-extra/clangd/Protocol.h:1026
+  /// When not defined, it is treated as `0`.
+  llvm::Optional resolve;
+

why not just use an int then ?



Comment at: clang-tools-extra/clangd/Protocol.h:1046
+  /// It is `false` by default.
+  llvm::Optional deprecated;
+

again we can just store a bool, and only serialize if it is true



Comment at: clang-tools-extra/clangd/XRefs.cpp:876
+  Optional Result = declToTypeHierarchyItem(ASTCtx, CXXRD);
+  if (Result && Levels > 0) {
+if (Direction == TypeHierarchyDirection::Parents ||

again reduce nesting



Comment at: clang-tools-extra/clangd/XRefs.cpp:891
+if (Optional ParentSym =
+getTypeHierarchy(*ParentDecl, ASTCtx, Levels - 1, Direction)) {
+  Result->parents->emplace_back(std::move(*ParentSym));

A problem that might occur when you add children traversal:
do we really want to include current decl in children of parent when we have 
`BOTH` as direction? If so, maybe we should rather cache the results? Maybe 
leave a todo/fixme ?



Comment at: clang-tools-extra/clangd/XRefs.cpp:917
+  if (Symbols.Decls.empty())
+return {};
+

llvm::None



Comment at: clang-tools-extra/clangd/XRefs.cpp:925
+CXXRD = VD->getType().getTypePtr()->getAsCXXRecordDecl();
+  } else if (const CXXMethodDecl *Method = dyn_cast(D)) {
+// If this is a method, use the type of the class.

what about member fields ?



Comment at: clang-tools-extra/clangd/XRefs.cpp:932
+
+  if (CXXRD) {
+int ResolveLevels = Resolve.getValueOr(0);

reduce nesting



Comment at: clang-tools-extra/clangd/XRefs.cpp:935
+TypeHierarchyDirection ResolveDirection =
+Direction.getValueOr(TypeHierarchyDirection::Parents);
+return getTypeHierarchy(*CXXRD, ASTCtx, ResolveLevels, ResolveDirection);

Is this mentioned in proposal?



Comment at: clang-tools-extra/clangd/XRefs.cpp:939
+
+  return {};
+}

llvm::None



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1355
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{

can you format the code?



Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1387
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+llvm::Optional Result = getTypeHierarchy(

You can use `Source.points()` and use `Pt` directly instead of doing 
`Source.point(pt)`. Same for other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 3 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:820
 
+// TODO: Reduce duplication between this function and declToSym().
+static llvm::Optional

I am open to feedback on whether we want to reduce the duplication between 
these functions, and if so, suggestions for how.



Comment at: clang-tools-extra/clangd/XRefs.cpp:901
+
+  // TODO: Populate subtypes.
+}

I am deliberately leaving this part for a follow-up patch, as it will require 
index changes.



Comment at: clang-tools-extra/unittests/clangd/Matchers.h:130
 
+// Implements the HasValue(m) matcher for matching an Optional whose
+// value matches matcher m.

Should I split this out into a separate patch? It's used by the tests being 
added for this functionality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 184985.
nridge added a comment.

remove unrelated file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,11 @@
 namespace {
 
 using testing::ElementsAre;
+using testing::Eq;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +41,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1339,6 +1350,160 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  int a;
+};
+
+struct $Child1Def[[Child1]] : Parent
+{
+  int b;
+};
+
+struct Ch$p1^ild2 : Child1
+{
+  int c;
+};
+
+struct Child3 : Child2
+{
+  int d;
+};
+
+int main()
+{
+  Ch$p2^ild2 ch$p3^ild2;
+
+  parent.a = 1;
+  ch$p4^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]]
+{
+  int a;
+};
+
+struct $Parent2Def[[Parent2]]
+{
+  int b;
+};
+
+struct $Parent3Def[[Parent3]] : Parent2
+{
+  int c;
+};
+
+struct Ch$c1^ild : Parent1, Parent3
+{
+  int d;
+};
+
+int main()
+{
+  Ch$c2^ild  ch$c3^ild;
+
+  ch$c4^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent
+{
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1
+{
+  void met$p1^hod ();
+  void met$p2^hod (int x);
+};
+
+struct Child3 : Child2
+{
+  void method (int x);
+};
+)cpp");
+
+  TestTU TU = 

[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 184984.
nridge edited the summary of this revision.
nridge added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This completes the implementation of the revised spec (for supertypes only)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56370

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/FindSymbols.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/unittests/clangd/Matchers.h
  clang-tools-extra/unittests/clangd/XRefsTests.cpp

Index: clang-tools-extra/unittests/clangd/XRefsTests.cpp
===
--- clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -25,9 +25,11 @@
 namespace {
 
 using testing::ElementsAre;
+using testing::Eq;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
+using testing::Pointee;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -39,6 +41,15 @@
   return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
 }
 
+// GMock helpers for matching TypeHierarchyItem.
+MATCHER_P(WithName, N, "") { return arg.name == N; }
+MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
+MATCHER_P(SelectionRangeIs, R, "") { return arg.selectionRange == R; }
+template 
+testing::Matcher Parents(ParentMatchers... ParentsM) {
+  return Field(::parents, HasValue(ElementsAre(ParentsM...)));
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher &>
@@ -1339,6 +1350,160 @@
   }
 }
 
+TEST(SuperTypes, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  int a;
+};
+
+struct $Child1Def[[Child1]] : Parent
+{
+  int b;
+};
+
+struct Ch$p1^ild2 : Child1
+{
+  int c;
+};
+
+struct Child3 : Child2
+{
+  int d;
+};
+
+int main()
+{
+  Ch$p2^ild2 ch$p3^ild2;
+
+  parent.a = 1;
+  ch$p4^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child2"), WithKind(SymbolKind::Struct),
+Parents(AllOf(
+WithName("Child1"), WithKind(SymbolKind::Struct),
+SelectionRangeIs(Source.range("Child1Def")),
+Parents(AllOf(WithName("Parent"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("ParentDef")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+struct $Parent1Def[[Parent1]]
+{
+  int a;
+};
+
+struct $Parent2Def[[Parent2]]
+{
+  int b;
+};
+
+struct $Parent3Def[[Parent3]] : Parent2
+{
+  int c;
+};
+
+struct Ch$c1^ild : Parent1, Parent3
+{
+  int d;
+};
+
+int main()
+{
+  Ch$c2^ild  ch$c3^ild;
+
+  ch$c4^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+llvm::Optional Result = getTypeHierarchy(
+AST, Source.point(Pt), 10, TypeHierarchyDirection::Parents);
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(
+*Result,
+AllOf(
+WithName("Child"), WithKind(SymbolKind::Struct),
+Parents(AllOf(WithName("Parent1"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent1Def")),
+  Parents()),
+AllOf(WithName("Parent3"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent3Def")),
+  Parents(AllOf(
+  WithName("Parent2"), WithKind(SymbolKind::Struct),
+  SelectionRangeIs(Source.range("Parent2Def")),
+  Parents()));
+  }
+}
+
+TEST(SuperTypes, OnMethod) {
+  Annotations Source(R"cpp(
+struct $ParentDef[[Parent]]
+{
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+struct $Child1Def[[Child1]] : Parent
+{
+  void method