[PATCH] D116443: [clangd] Implement textDocument/typeDefinition
kadircet added inline comments. Herald added a project: All. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1292 +for (const LocatedSymbol : *Types) + Response.push_back(Sym.PreferredDeclaration); +return Reply(std::move(Response)); any particular reason for jumping to declaration here, rather than using definition when it's available or was this just an oversight? because this results in us jumping to forward declarations every now and then, and I can't see any use case where a declaration would be preferred to definition of a type. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116443/new/ https://reviews.llvm.org/D116443 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116443: [clangd] Implement textDocument/typeDefinition
This revision was automatically updated to reflect the committed changes.
sammccall marked 4 inline comments as done.
Closed by commit rG71a082f72674: [clangd] Implement textDocument/typeDefinition
(authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D116443?vs=396785=399761#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116443/new/
https://reviews.llvm.org/D116443
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/XRefs.cpp
clang-tools-extra/clangd/XRefs.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/test/type-definition.test
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -38,10 +38,12 @@
namespace {
using ::testing::AllOf;
+using ::testing::Contains;
using ::testing::ElementsAre;
using ::testing::Eq;
using ::testing::IsEmpty;
using ::testing::Matcher;
+using ::testing::Not;
using ::testing::UnorderedElementsAre;
using ::testing::UnorderedElementsAreArray;
using ::testing::UnorderedPointwise;
@@ -1781,6 +1783,69 @@
<< Test;
}
+TEST(FindType, All) {
+ Annotations HeaderA(R"cpp(
+struct [[Target]] { operator int() const; };
+struct Aggregate { Target a, b; };
+Target t;
+
+template class smart_ptr {
+ T& operator*();
+ T* operator->();
+ T* get();
+};
+ )cpp");
+ auto TU = TestTU::withHeaderCode(HeaderA.code());
+ for (const llvm::StringRef Case : {
+ "str^uct Target;",
+ "T^arget x;",
+ "Target ^x;",
+ "a^uto x = Target{};",
+ "namespace m { Target tgt; } auto x = m^::tgt;",
+ "Target funcCall(); auto x = ^funcCall();",
+ "Aggregate a = { {}, ^{} };",
+ "Aggregate a = { ^.a=t, };",
+ "struct X { Target a; X() : ^a() {} };",
+ "^using T = Target; ^T foo();",
+ "^template Target foo();",
+ "void x() { try {} ^catch(Target e) {} }",
+ "void x() { ^throw t; }",
+ "int x() { ^return t; }",
+ "void x() { ^switch(t) {} }",
+ "void x() { ^delete (Target*)nullptr; }",
+ "Target& ^tref = t;",
+ "void x() { ^if (t) {} }",
+ "void x() { ^while (t) {} }",
+ "void x() { ^do { } while (t); }",
+ "^auto x = []() { return t; };",
+ "Target* ^tptr = ",
+ "Target ^tarray[3];",
+ }) {
+Annotations A(Case);
+TU.Code = A.code().str();
+ParsedAST AST = TU.build();
+
+ASSERT_GT(A.points().size(), 0u) << Case;
+for (auto Pos : A.points())
+ EXPECT_THAT(findType(AST, Pos),
+ ElementsAre(Sym("Target", HeaderA.range(), HeaderA.range(
+ << Case;
+ }
+
+ // FIXME: We'd like these cases to work. Fix them and move above.
+ for (const llvm::StringRef Case : {
+ "smart_ptr ^tsmart;",
+ }) {
+Annotations A(Case);
+TU.Code = A.code().str();
+ParsedAST AST = TU.build();
+
+EXPECT_THAT(findType(AST, A.point()),
+Not(Contains(Sym("Target", HeaderA.range(), HeaderA.range()
+<< Case;
+ }
+}
+
void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) {
Annotations T(Test);
auto TU = TestTU::withCode(T.code());
Index: clang-tools-extra/clangd/test/type-definition.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/type-definition.test
@@ -0,0 +1,32 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,
+ "text":"class X {};\nauto x = X{};"
+}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/typeDefinition","params":{
+ "textDocument":{"uri":"test:///main.cpp"},
+ "position":{"line":1,"character":5}
+}}
+# CHECK: "id": 1
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT: "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT: "character": 7,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT: "character": 6,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT: },
+# CHECK-NEXT: "uri": "file://{{.*}}/clangd-test/main.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT: ]
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index:
[PATCH] D116443: [clangd] Implement textDocument/typeDefinition
sammccall marked 6 inline comments as done.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/XRefs.cpp:1883
+ QualType VisitIfStmt(const IfStmt *S) { return type(S->getCond()); }
+ QualType VisitCaseStmt(const CaseStmt *S) { return type(S->getLHS()); }
+ QualType VisitCXXForRangeStmt(const CXXForRangeStmt *S) {
usaxena95 wrote:
> I think this would be useful for enumerations primarily. Would it work as of
> now (would we return the enum definition loc) ?
Yes, it does work (tested manually).
Comment at: clang-tools-extra/clangd/XRefs.cpp:1925-1927
+// If we targeted something function-like, prefer its return type.
+if (auto FT = Type->getAs())
+ Type = FT->getReturnType();
usaxena95 wrote:
> Can this be accommodated in `typeForNode` ?
> It would be nicer if we keep this method only for plumbing.
Agree. It's not convenient to do it exactly in typeForNode though, as we have
to wrap every return with the unwrapping logic.
I added `unwrapFindType` to do this, and that's a natural place to fix
array/pointer cases too while here.
Comment at: clang-tools-extra/clangd/XRefs.h:110
+///
+/// For example, given `foo(b^ar())` where bar() returns unique_ptr,
+/// this should return the definition of class Foo.
usaxena95 wrote:
> This is sounds confusing to me about the current behaviour.
> We would return `unique_ptr` here atm. Maybe just have simple `returns
> Foo` here as of now.
Whoops, you're right. I'd love to have this behavior but decided not to bite it
off in this patch.
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1805
+ "namespace m { Target tgt; } auto x = m^::tgt;",
+ "Target funcCall(); auto x = ^funcCall();",
+ "Aggregate a = { {}, ^{} };",
usaxena95 wrote:
> Can you add a lambda as well
> `"au^to x = []() -> Target {return Target{};}"`
Done. This isn't handled yet (lambdas are CXXRecordType, not FunctionType) so
added it
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116443/new/
https://reviews.llvm.org/D116443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116443: [clangd] Implement textDocument/typeDefinition
usaxena95 accepted this revision.
usaxena95 added a comment.
This revision is now accepted and ready to land.
Thanks. LGTM.
Comment at: clang-tools-extra/clangd/XRefs.cpp:1869
+ QualType VisitDesignatedInitExpr(const DesignatedInitExpr *S) {
+for (auto : llvm::reverse(S->designators()))
+ if (D.isFieldDesignator())
Reason for `reverse` isn't clear to me. Can you add a comment.
Comment at: clang-tools-extra/clangd/XRefs.cpp:1880-1882
+ QualType VisitWhileStmt(const WhileStmt *S) { return type(S->getCond());
}
+ QualType VisitDoStmt(const DoStmt *S) { return type(S->getCond()); }
+ QualType VisitIfStmt(const IfStmt *S) { return type(S->getCond()); }
Can you add tests for these as well.
Comment at: clang-tools-extra/clangd/XRefs.cpp:1883
+ QualType VisitIfStmt(const IfStmt *S) { return type(S->getCond()); }
+ QualType VisitCaseStmt(const CaseStmt *S) { return type(S->getLHS()); }
+ QualType VisitCXXForRangeStmt(const CXXForRangeStmt *S) {
I think this would be useful for enumerations primarily. Would it work as of
now (would we return the enum definition loc) ?
Comment at: clang-tools-extra/clangd/XRefs.cpp:1925-1927
+// If we targeted something function-like, prefer its return type.
+if (auto FT = Type->getAs())
+ Type = FT->getReturnType();
Can this be accommodated in `typeForNode` ?
It would be nicer if we keep this method only for plumbing.
Comment at: clang-tools-extra/clangd/XRefs.h:110
+///
+/// For example, given `foo(b^ar())` where bar() returns unique_ptr,
+/// this should return the definition of class Foo.
This is sounds confusing to me about the current behaviour.
We would return `unique_ptr` here atm. Maybe just have simple `returns Foo`
here as of now.
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1805
+ "namespace m { Target tgt; } auto x = m^::tgt;",
+ "Target funcCall(); auto x = ^funcCall();",
+ "Aggregate a = { {}, ^{} };",
Can you add a lambda as well
`"au^to x = []() -> Target {return Target{};}"`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116443/new/
https://reviews.llvm.org/D116443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116443: [clangd] Implement textDocument/typeDefinition
sammccall created this revision.
sammccall added a reviewer: kbobyrev.
Herald added subscribers: usaxena95, kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
This reuses the type=>decl mapping from go-to-definition on auto.
(Which could stand some improvement, but that can happen later).
Fixes https://github.com/clangd/clangd/issues/367
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116443
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/XRefs.cpp
clang-tools-extra/clangd/XRefs.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/test/type-definition.test
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -38,10 +38,12 @@
namespace {
using ::testing::AllOf;
+using ::testing::Contains;
using ::testing::ElementsAre;
using ::testing::Eq;
using ::testing::IsEmpty;
using ::testing::Matcher;
+using ::testing::Not;
using ::testing::UnorderedElementsAre;
using ::testing::UnorderedElementsAreArray;
using ::testing::UnorderedPointwise;
@@ -1781,6 +1783,65 @@
<< Test;
}
+TEST(FindType, All) {
+ Annotations HeaderA(R"cpp(
+struct [[Target]] { operator int() const; };
+struct Aggregate { Target a, b; };
+Target t;
+
+template class smart_ptr {
+ T& operator*();
+ T* operator->();
+ T* get();
+};
+ )cpp");
+ auto TU = TestTU::withHeaderCode(HeaderA.code());
+ for (const llvm::StringRef Case : {
+ "str^uct Target;",
+ "T^arget x;",
+ "Target ^x;",
+ "a^uto x = Target{};",
+ "namespace m { Target tgt; } auto x = m^::tgt;",
+ "Target funcCall(); auto x = ^funcCall();",
+ "Aggregate a = { {}, ^{} };",
+ "Aggregate a = { ^.a=t, };",
+ "struct X { Target a; X() : ^a() {} };",
+ "^using T = Target; ^T foo();",
+ "^template Target foo();",
+ "void x() { try {} ^catch(Target e) {} }",
+ "void x() { ^throw t; }",
+ "int x() { ^return t; }",
+ "void x() { ^switch(t) {} }",
+ "void x() { ^delete (Target*)nullptr; }",
+ "Target& ^tref = t;",
+ }) {
+Annotations A(Case);
+TU.Code = A.code().str();
+ParsedAST AST = TU.build();
+
+ASSERT_GT(A.points().size(), 0u) << Case;
+for (auto Pos : A.points())
+ EXPECT_THAT(findType(AST, Pos),
+ ElementsAre(Sym("Target", HeaderA.range(), HeaderA.range(
+ << Case;
+ }
+
+ // FIXME: We'd like these cases to work. Fix them and move above.
+ for (const llvm::StringRef Case : {
+ "Target* ^tptr = ",
+ "smart_ptr ^tsmart;",
+ "Target ^tarray[3];",
+ }) {
+Annotations A(Case);
+TU.Code = A.code().str();
+ParsedAST AST = TU.build();
+
+EXPECT_THAT(findType(AST, A.point()),
+Not(Contains(Sym("Target", HeaderA.range(), HeaderA.range()
+<< Case;
+ }
+}
+
void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) {
Annotations T(Test);
auto TU = TestTU::withCode(T.code());
Index: clang-tools-extra/clangd/test/type-definition.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/type-definition.test
@@ -0,0 +1,32 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,
+ "text":"class X {};\nauto x = X{};"
+}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/typeDefinition","params":{
+ "textDocument":{"uri":"test:///main.cpp"},
+ "position":{"line":1,"character":5}
+}}
+# CHECK: "id": 1
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT: "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT: "character": 7,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT: "character": 6,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT: },
+# CHECK-NEXT: "uri": "file://{{.*}}/clangd-test/main.cpp"
+# CHECK-NEXT:}
+# CHECK-NEXT: ]
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/initialize-params.test
