[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I sent you D93314  to extend the 
getNonReferenceType() a bit... curious what you think


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbda7d0af9707: [clangd] Improve goToDefinition on auto and 
dectype (authored by qchateau, committed by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  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
@@ -640,6 +640,134 @@
 struct Fo^o {};
   )cpp",
 
+  R"cpp(// auto builtin type (not supported)
+^auto x = 42;
+  )cpp",
+
+  R"cpp(// auto on lambda
+auto x = [[[]]]{};
+^auto y = x;
+  )cpp",
+
+  R"cpp(// auto on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+^auto x = ns1::S1{};
+  )cpp",
+
+  R"cpp(// decltype on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+^decltype(i) j;
+  )cpp",
+
+  R"cpp(// decltype(auto) on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+ns1::S1& j = i;
+^decltype(auto) k = j;
+  )cpp",
+
+  R"cpp(// auto on template class
+template class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};
+class X;
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on specialized template class
+template class Foo {};
+template<> class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on initializer list.
+namespace std
+{
+  template
+  class [[initializer_list]] {};
+}
+
+^auto i = {1,2};
+  )cpp",
+
+  R"cpp(// auto function return with trailing type
+struct [[Bar]] {};
+^auto test() -> decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype in trailing return type
+struct [[Bar]] {};
+auto test() -> ^decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto in function return
+struct [[Bar]] {};
+^auto test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto& in function return
+struct [[Bar]] {};
+^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// auto* in function return
+struct [[Bar]] {};
+^auto* test() {
+  Bar* x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// const auto& in function return
+struct [[Bar]] {};
+const ^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// decltype(auto) in function return
+struct [[Bar]] {};
+^decltype(auto) test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype of function with trailing return type.
+struct [[Bar]] {};
+auto test() -> decltype(Bar()) {
+  return Bar();
+}
+void foo() {
+  ^decltype(test()) i = test();
+}
+  )cpp",
+
   R"cpp(// Override specifier jumps to overridden method
 class Y { virtual void $decl[[a]]() = 0; };
 class X : Y { void a() ^override {} };
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -26,23 +26,168 @@
 namespace clangd {
 namespace {
 
-TEST(GetDeducedType, KwAutoExpansion) {
+TEST(GetDeducedType, KwAutoKwDecltypeExpansion) {
   struct Test {
 StringRef AnnotatedCode;
 const char *DeducedType;
   } Tests[] = {
   {"^auto i = 0;", "int"},
   {"^auto f(){ return 1;};", "int"},
+  {
+  R"cpp( // auto on struct in a namespace
+  namespace ns1 { struct S {}; }
+  ^auto v = ns1::S{};
+  )cpp",
+  "struct ns1::S",
+  },
+  {
+  R"cpp( // decltype on struct
+  namespace ns1 { struct S {}; }
+  ns1::S i;
+  ^decltype(i) j;
+  )cpp",
+  "ns1::S",
+  },
+  {
+  R"cpp(// decltype(auto) on struct&
+namespace ns1 {
+struct S {};
+} // namespace ns1
+
+ns1::S i;
+ns1::S& j = i;
+^decltype(auto) k = j;
+  

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

I've updated the patch with your latest comments.

Please commit this patch for me, using "quentin.chat...@gmail.com", I'll 
probably ask for commit access after a few successful reviews


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 311247.
qchateau marked an inline comment as done.
qchateau added a comment.

- Reintroduce test with a FIXME comment
- locateSymbolForType returns a vector instead of an optional


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  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
@@ -624,6 +624,134 @@
 struct Fo^o {};
   )cpp",
 
+  R"cpp(// auto builtin type (not supported)
+^auto x = 42;
+  )cpp",
+
+  R"cpp(// auto on lambda
+auto x = [[[]]]{};
+^auto y = x;
+  )cpp",
+
+  R"cpp(// auto on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+^auto x = ns1::S1{};
+  )cpp",
+
+  R"cpp(// decltype on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+^decltype(i) j;
+  )cpp",
+
+  R"cpp(// decltype(auto) on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+ns1::S1& j = i;
+^decltype(auto) k = j;
+  )cpp",
+
+  R"cpp(// auto on template class
+template class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};
+class X;
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on specialized template class
+template class Foo {};
+template<> class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on initializer list.
+namespace std
+{
+  template
+  class [[initializer_list]] {};
+}
+
+^auto i = {1,2};
+  )cpp",
+
+  R"cpp(// auto function return with trailing type
+struct [[Bar]] {};
+^auto test() -> decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype in trailing return type
+struct [[Bar]] {};
+auto test() -> ^decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto in function return
+struct [[Bar]] {};
+^auto test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto& in function return
+struct [[Bar]] {};
+^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// auto* in function return
+struct [[Bar]] {};
+^auto* test() {
+  Bar* x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// const auto& in function return
+struct [[Bar]] {};
+const ^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// decltype(auto) in function return
+struct [[Bar]] {};
+^decltype(auto) test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype of function with trailing return type.
+struct [[Bar]] {};
+auto test() -> decltype(Bar()) {
+  return Bar();
+}
+void foo() {
+  ^decltype(test()) i = test();
+}
+  )cpp",
+
   R"cpp(// Override specifier jumps to overridden method
 class Y { virtual void $decl[[a]]() = 0; };
 class X : Y { void a() ^override {} };
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -26,23 +26,168 @@
 namespace clangd {
 namespace {
 
-TEST(GetDeducedType, KwAutoExpansion) {
+TEST(GetDeducedType, KwAutoKwDecltypeExpansion) {
   struct Test {
 StringRef AnnotatedCode;
 const char *DeducedType;
   } Tests[] = {
   {"^auto i = 0;", "int"},
   {"^auto f(){ return 1;};", "int"},
+  {
+  R"cpp( // auto on struct in a namespace
+  namespace ns1 { struct S {}; }
+  ^auto v = ns1::S{};
+  )cpp",
+  "struct ns1::S",
+  },
+  {
+  R"cpp( // decltype on struct
+  namespace ns1 { struct S {}; }
+  ns1::S i;
+  ^decltype(i) j;
+  )cpp",
+  "ns1::S",
+  },
+  {
+  R"cpp(// decltype(auto) on struct&
+namespace ns1 {
+struct S {};
+} // namespace ns1
+
+ns1::S i;
+ns1::S& j = i;
+^decltype(auto) k = j;
+  )cpp",
+  "struct ns1::S &",
+  

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Sam McCall via Phabricator via cfe-commits]

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

Looks good, thanks again!
If you don't yet have LLVM commit access, I can commit this for you, let me 
know which email address to associate it with.
(https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access describes 
how you can get commit access if you're contributing regularly)




Comment at: clang-tools-extra/clangd/XRefs.cpp:442
+  const auto  = AST.getASTContext();
+  const NamedDecl *D = getPreferredDecl(Decls.front());
+

nit: we do often just use the first element (there's almost always just one), 
but here we can actually return multiple LocatedSymbols (client shows some sort 
of selector widget for them) so I'd suggest we loop and return all



Comment at: clang-tools-extra/clangd/XRefs.cpp:689
+const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+if (!D)
+  continue;

qchateau wrote:
> sammccall wrote:
> > there are many other types we could resolve to a decl: TypedefType, 
> > TemplateTypeParmtype, ...
> > If we're only going to handle tag types, it deserves a FIXME comment.
> > 
> > But we do have  a helper for this. Can you try calling 
> > `targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | 
> > DeclRelation::Alias)` instead of `Deduced->getTypePtr()->getAsTagDecl()`?
> > 
> > That should handle more cases, at a minimum, this case sholud pass:
> > ```
> > using [[T]] = int;
> > T x;
> > ^auto y = x;
> > ```
> > 
> > (I see you have a test case that aliases backed by tag types are resolved 
> > to the underlying tag decl, this change would offer the alias instead, 
> > which is more consistent with our current go-to-definition. You could also 
> > offer both by passing `DeclRelation::TemplatePattern | DeclRelation::Alias 
> > | DeclRelation::Underlying`... but I think we should value consistency here)
> `locateSymbolForType` uses `targetDecl`. Thanks for the tip, that's exactly 
> what I was looking for.
> 
> I also agree with you that go-to-definition should go to the alias instead of 
> the underlying type for consistency, but using 
> `targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | 
> DeclRelation::Alias)` is not enough to resolve this consistency issue: 
> `getDeducedType` already returns the underlying type. My current knowledge of 
> the clangd code is nou sufficient to understand why, but the root of the 
> problem seem to lie in the `DeducedTypeVisitor` class.
> 
> I removed the test that tested the undesirable behavior, should I open a bug 
> report for `getDeducedType` that should not "go through" aliases ? In which 
> case, what's the right place for that ? Github ?
Great!
Marginally better than removing a test is keeping it with a comment `// FIXME: 
it'd be nice if this resolved to the alias instead` or something like that

Github is the right place to file a bug, though I don't think this is really a 
big deal, so sometimes we just leave the comment.

(I'm not sure offhand if the AST actually preserves the sugar type or just the 
underlying one)



Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:669
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};

qchateau wrote:
> sammccall wrote:
> > There's a scalability problem with testing every corner of the C++ language 
> > (there are lots!) with every feature. Generally I prefer to cover some 
> > important/weird/bugfix cases here, but to cover as much as possible with 
> > unittests of the underlying functions.
> > 
> > In this case, that would mean
> >  - moving tests that are about `auto` in different contexts to ASTTests.cpp 
> > (current test coverage there is poor).
> >  - (assuming we can use `targetDecl()`) moving tests that are about which 
> > decl a type should resolve to to `FindTargetTests.cpp` - but current 
> > coverage there is pretty good so we can probably just drop many of them.
> I added the relevant tests to `ASTTests.cpp` but I did not remove the tests 
> here for the moment. I've always had the habit to keep tests that are already 
> written, even if redundant.
> 
> If you'd like me to remove some of them, I'd say the only tests that are not 
> redundant with the ones in `ASTTests.cpp` are the ones that test template 
> specializations. Shoud I keep only these ?
Keeping them around is fine too, we're not strict about this.

Thanks for adding all the tests for deduced types!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/XRefs.cpp:689
+const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+if (!D)
+  continue;

sammccall wrote:
> there are many other types we could resolve to a decl: TypedefType, 
> TemplateTypeParmtype, ...
> If we're only going to handle tag types, it deserves a FIXME comment.
> 
> But we do have  a helper for this. Can you try calling 
> `targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | 
> DeclRelation::Alias)` instead of `Deduced->getTypePtr()->getAsTagDecl()`?
> 
> That should handle more cases, at a minimum, this case sholud pass:
> ```
> using [[T]] = int;
> T x;
> ^auto y = x;
> ```
> 
> (I see you have a test case that aliases backed by tag types are resolved to 
> the underlying tag decl, this change would offer the alias instead, which is 
> more consistent with our current go-to-definition. You could also offer both 
> by passing `DeclRelation::TemplatePattern | DeclRelation::Alias | 
> DeclRelation::Underlying`... but I think we should value consistency here)
`locateSymbolForType` uses `targetDecl`. Thanks for the tip, that's exactly 
what I was looking for.

I also agree with you that go-to-definition should go to the alias instead of 
the underlying type for consistency, but using 
`targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | 
DeclRelation::Alias)` is not enough to resolve this consistency issue: 
`getDeducedType` already returns the underlying type. My current knowledge of 
the clangd code is nou sufficient to understand why, but the root of the 
problem seem to lie in the `DeducedTypeVisitor` class.

I removed the test that tested the undesirable behavior, should I open a bug 
report for `getDeducedType` that should not "go through" aliases ? In which 
case, what's the right place for that ? Github ?



Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:669
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};

sammccall wrote:
> There's a scalability problem with testing every corner of the C++ language 
> (there are lots!) with every feature. Generally I prefer to cover some 
> important/weird/bugfix cases here, but to cover as much as possible with 
> unittests of the underlying functions.
> 
> In this case, that would mean
>  - moving tests that are about `auto` in different contexts to ASTTests.cpp 
> (current test coverage there is poor).
>  - (assuming we can use `targetDecl()`) moving tests that are about which 
> decl a type should resolve to to `FindTargetTests.cpp` - but current coverage 
> there is pretty good so we can probably just drop many of them.
I added the relevant tests to `ASTTests.cpp` but I did not remove the tests 
here for the moment. I've always had the habit to keep tests that are already 
written, even if redundant.

If you'd like me to remove some of them, I'd say the only tests that are not 
redundant with the ones in `ASTTests.cpp` are the ones that test template 
specializations. Shoud I keep only these ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 311219.
qchateau added a comment.

- Remove the patches affecting hover
- Add tests in ASTTests.cpp to test the behavior of getDeducedType for auto and 
decltype
- Add missing comment
- Create a locateSymbolForType function (which uses targetDecl)
- Remove the unit tests that were testing undesirable behaviors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  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
@@ -624,6 +624,134 @@
 struct Fo^o {};
   )cpp",
 
+  R"cpp(// auto builtin type (not supported)
+^auto x = 42;
+  )cpp",
+
+  R"cpp(// auto on lambda
+auto x = [[[]]]{};
+^auto y = x;
+  )cpp",
+
+  R"cpp(// auto on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+^auto x = ns1::S1{};
+  )cpp",
+
+  R"cpp(// decltype on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+^decltype(i) j;
+  )cpp",
+
+  R"cpp(// decltype(auto) on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+ns1::S1& j = i;
+^decltype(auto) k = j;
+  )cpp",
+
+  R"cpp(// auto on template class
+template class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};
+class X;
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on specialized template class
+template class Foo {};
+template<> class [[Foo]] {};
+
+^auto x = Foo();
+  )cpp",
+
+  R"cpp(// auto on initializer list.
+namespace std
+{
+  template
+  class [[initializer_list]] {};
+}
+
+^auto i = {1,2};
+  )cpp",
+
+  R"cpp(// auto function return with trailing type
+struct [[Bar]] {};
+^auto test() -> decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype in trailing return type
+struct [[Bar]] {};
+auto test() -> ^decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto in function return
+struct [[Bar]] {};
+^auto test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto& in function return
+struct [[Bar]] {};
+^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// auto* in function return
+struct [[Bar]] {};
+^auto* test() {
+  Bar* x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// const auto& in function return
+struct [[Bar]] {};
+const ^auto& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// decltype(auto) in function return
+struct [[Bar]] {};
+^decltype(auto) test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype of function with trailing return type.
+struct [[Bar]] {};
+auto test() -> decltype(Bar()) {
+  return Bar();
+}
+void foo() {
+  ^decltype(test()) i = test();
+}
+  )cpp",
+
   R"cpp(// Override specifier jumps to overridden method
 class Y { virtual void $decl[[a]]() = 0; };
 class X : Y { void a() ^override {} };
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -26,23 +26,159 @@
 namespace clangd {
 namespace {
 
-TEST(GetDeducedType, KwAutoExpansion) {
+TEST(GetDeducedType, KwAutoKwDecltypeExpansion) {
   struct Test {
 StringRef AnnotatedCode;
 const char *DeducedType;
   } Tests[] = {
   {"^auto i = 0;", "int"},
   {"^auto f(){ return 1;};", "int"},
+  {
+  R"cpp( // auto on struct in a namespace
+  namespace ns1 { struct S {}; }
+  ^auto v = ns1::S{};
+  )cpp",
+  "struct ns1::S",
+  },
+  {
+  R"cpp( // decltype on struct
+  namespace ns1 { struct S {}; }
+  ns1::S i;
+  ^decltype(i) j;
+  )cpp",
+  "ns1::S",
+  },
+  {
+  R"cpp(// decltype(auto) on struct&
+namespace ns1 {
+struct S {};
+} // namespace ns1
+
+  

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this! Go-to-def-on-auto has been bugging me, it'll be great to 
have it :-)
Overall the patch looks really solid, with nice tests. The one thing I'd 
suggest is splitting the two features (hover and go-to-def) into separate 
patches.
e.g. here, I'm not totally sure about the hover change yet, and want to think 
and get some input from others. But I don't want to delay the go-to-definition 
part. So I've left comments on the go-to-definition changes, and if you want to 
drop hover from this patch and upload it as a separate review, that'd be really 
helpful.

---

My concern with hover is mostly around consistency and clarity. In isolation, 
the new hover certainly looks better than the old one for structs (e.g. syntax 
highligthing!).
However now we have a different style for struct vs non-struct types, and it's 
not just a surface thing: for structs we show the declaration, but many types 
have no declaration to show. I'm not sure how you can show a hover for `auto = 
char *const*` or so that feels consistent with this one.

We should also consider consistency and clarity with different types of `auto`. 
Currently our hover for undeduced auto is just "auto", and our hover for auto 
params is a mess. It would be nice about this distinction, as when the same 
keyword has multiple meanings people can conflate them.

One solution would be to make `auto` the name, and the type the definition. 
This provides less information (but I suspect that information is often noise):

  ### deduced-type `auto`
  
  ---
  
  struct cv::Ptr

(For context, the `auto` hover was one of the first implemented. Later we went 
back and added some structure (type/name/definition/documentation) but the old 
`auto` hover was mostly shoehorned into this. So if we're going to change auto 
hover, we shuold consider trying to make it fit this schema, too)




Comment at: clang-tools-extra/clangd/XRefs.cpp:685
+
+if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+  if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {

This deserves a high-level comment: // go-to-definition on auto should find the 
definition of the deduced type, if possible



Comment at: clang-tools-extra/clangd/XRefs.cpp:686
+if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+  if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
+Deduced = Deduced->getNonReferenceType();

I think we should pull the contents of this if() into a function 
findSymbolForType or so.

LSP has a `textDocument/typeDefinition` request that could also make use of 
this logic.
(You don't need to implement that, and I'm very much in favor of having this 
`textDocument/definition` be the do-what-i-mean swiss army knife like this 
patch does. But I'd just like to factor this code with potential reuse in mind)



Comment at: clang-tools-extra/clangd/XRefs.cpp:689
+const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+if (!D)
+  continue;

there are many other types we could resolve to a decl: TypedefType, 
TemplateTypeParmtype, ...
If we're only going to handle tag types, it deserves a FIXME comment.

But we do have  a helper for this. Can you try calling 
`targetDecl(DynTypedNode::create(Deduced), DeclRelation::TemplatePattern | 
DeclRelation::Alias)` instead of `Deduced->getTypePtr()->getAsTagDecl()`?

That should handle more cases, at a minimum, this case sholud pass:
```
using [[T]] = int;
T x;
^auto y = x;
```

(I see you have a test case that aliases backed by tag types are resolved to 
the underlying tag decl, this change would offer the alias instead, which is 
more consistent with our current go-to-definition. You could also offer both by 
passing `DeclRelation::TemplatePattern | DeclRelation::Alias | 
DeclRelation::Underlying`... but I think we should value consistency here)



Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:669
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};

There's a scalability problem with testing every corner of the C++ language 
(there are lots!) with every feature. Generally I prefer to cover some 
important/weird/bugfix cases here, but to cover as much as possible with 
unittests of the underlying functions.

In this case, that would mean
 - moving tests that are about `auto` in different contexts to ASTTests.cpp 
(current test coverage there is poor).
 - (assuming we can use `targetDecl()`) moving tests that are about which decl 
a type should resolve to to `FindTargetTests.cpp` - but current coverage there 
is pretty good so we can probably just drop many of them.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Not sure what's wrong with the unit test "x64 windows > 
LLVM.CodeGen/XCore::threads.ll", I don't see how it's related to my patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

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

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau updated this revision to Diff 310872.
qchateau added a comment.

Fix unittests and behavior

I've fixed the failing unittests, added some more
to test the new behavior, and fixed some bugs in
the new code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  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
@@ -624,6 +624,140 @@
 struct Fo^o {};
   )cpp",
 
+  R"cpp(// auto builtin type (not supported)
+au^to x = 42;
+  )cpp",
+
+  R"cpp(// auto on lambda
+auto x = [[[]]]{};
+au^to y = x;
+  )cpp",
+
+  R"cpp(// auto on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+au^to x = ns1::S1{};
+  )cpp",
+
+  R"cpp(// decltype on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+decl^type(i) j;
+  )cpp",
+
+  R"cpp(// decltype(auto) on struct
+namespace ns1 {
+struct [[S1]] {};
+} // namespace ns1
+
+ns1::S1 i;
+ns1::S1& j = i;
+decl^type(auto) k = j;
+  )cpp",
+
+  R"cpp(// auto on template class
+template class [[Foo]] {};
+
+au^to x = Foo();
+  )cpp",
+
+  R"cpp(// auto on template class with forward declared class
+template class [[Foo]] {};
+class X;
+
+au^to x = Foo();
+  )cpp",
+
+  R"cpp(// auto on specialized template class
+template class Foo {};
+template<> class [[Foo]] {};
+
+au^to x = Foo();
+  )cpp",
+
+  R"cpp(// auto on initializer list.
+namespace std
+{
+  template
+  class [[initializer_list]] {};
+}
+
+au^to i = {1,2};
+  )cpp",
+
+  R"cpp(// auto function return with trailing type
+struct [[Bar]] {};
+au^to test() -> decltype(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype in trailing return type
+struct [[Bar]] {};
+auto test() -> decl^type(Bar()) {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto in function return
+struct [[Bar]] {};
+au^to test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// auto& in function return
+struct [[Bar]] {};
+au^to& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// auto* in function return
+struct [[Bar]] {};
+au^to* test() {
+  Bar* x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// const auto& in function return
+struct [[Bar]] {};
+const au^to& test() {
+  static Bar x;
+  return x;
+}
+  )cpp",
+
+  R"cpp(// decltype(auto) in function return
+struct [[Bar]] {};
+decl^type(auto) test() {
+  return Bar();
+}
+  )cpp",
+
+  R"cpp(// decltype of function with trailing return type.
+struct [[Bar]] {};
+auto test() -> decltype(Bar()) {
+  return Bar();
+}
+void foo() {
+  decl^type(test()) i = test();
+}
+  )cpp",
+
+  R"cpp(// auto on alias
+struct [[cls]] {};
+typedef cls cls_type;
+au^to y = cls_type();
+  )cpp",
+
   R"cpp(// Override specifier jumps to overridden method
 class Y { virtual void $decl[[a]]() = 0; };
 class X : Y { void a() ^override {} };
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -388,6 +388,9 @@
[](HoverInfo ) {
  HI.Name = "(lambda)";
  HI.Kind = index::SymbolKind::Class;
+ HI.NamespaceScope = "";
+ HI.LocalScope = "foo::";
+ HI.Definition = "class {}";
}},
   // auto on template instantiation
   {R"cpp(
@@ -399,6 +402,8 @@
[](HoverInfo ) {
  HI.Name = "Foo";
  HI.Kind = index::SymbolKind::Class;
+ HI.NamespaceScope = "";
+ HI.Definition = "template <> class Foo {}";
}},
   // auto on specialized template
   {R"cpp(
@@ -411,6 +416,8 @@
[](HoverInfo ) {
  HI.Name = "Foo";
  HI.Kind = index::SymbolKind::Class;
+ HI.NamespaceScope = "";
+ HI.Definition = "template <> class Foo {}";
}},
 
   // macro
@@ -584,6 +591,8 

[PATCH] D92977: [clangd] Improve hover and goToDefinition on auto and dectype

[Quentin Chateau via Phabricator via cfe-commits]

qchateau created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
qchateau requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

Make the hover content on auto and decltype
look like the hover content we would get on the
deduced type.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92977

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -667,21 +667,44 @@
 return {};
   }
 
-  const syntax::Token *TouchedIdentifier =
-  syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
-  if (TouchedIdentifier)
-if (auto Macro =
-locateMacroReferent(*TouchedIdentifier, AST, *MainFilePath))
-  // Don't look at the AST or index if we have a macro result.
-  // (We'd just return declarations referenced from the macro's
-  // expansion.)
-  return {*std::move(Macro)};
-
-  ASTNodeKind NodeKind;
-  auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST,
-  *MainFilePath, Index, );
-  if (!ASTResults.empty())
-return ASTResults;
+  auto TokensTouchingCursor =
+  syntax::spelledTokensTouching(*CurLoc, AST.getTokens());
+  for (const auto  : TokensTouchingCursor) {
+if (Tok.kind() == tok::identifier) {
+  if (auto Macro = locateMacroReferent(Tok, AST, *MainFilePath))
+// Don't look at the AST or index if we have a macro result.
+// (We'd just return declarations referenced from the macro's
+// expansion.)
+return {*std::move(Macro)};
+
+  ASTNodeKind NodeKind;
+  auto ASTResults = locateASTReferent(*CurLoc, , AST, *MainFilePath,
+  Index, );
+  if (!ASTResults.empty())
+return ASTResults;
+} else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
+  if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
+const NamedDecl *D = Deduced->getTypePtr()->getAsTagDecl();
+if (!D)
+  continue;
+
+D = getPreferredDecl(D);
+auto Loc = makeLocation(AST.getASTContext(), nameLocation(*D, SM),
+*MainFilePath);
+if (!Loc)
+  continue;
+
+LocatedSymbol LocSym;
+LocSym.Name = printName(AST.getASTContext(), *D);
+LocSym.PreferredDeclaration = *Loc;
+if (const NamedDecl *Def = getDefinition(D))
+  LocSym.Definition = makeLocation(
+  AST.getASTContext(), nameLocation(*Def, SM), *MainFilePath);
+
+return {std::move(LocSym)};
+  }
+}
+  }
 
   // If the cursor can't be resolved directly, try fallback strategies.
   auto Word =
@@ -695,7 +718,7 @@
 Word->Text);
 return {*std::move(Macro)};
   }
-  ASTResults =
+  auto ASTResults =
   locateASTReferent(NearbyIdent->location(), NearbyIdent, AST,
 *MainFilePath, Index, /*NodeKind=*/nullptr);
   if (!ASTResults.empty()) {
@@ -708,6 +731,7 @@
   }
 }
 // No nearby word, or it didn't refer to anything either. Try the index.
+ASTNodeKind NodeKind;
 auto TextualResults =
 locateSymbolTextually(*Word, AST, Index, *MainFilePath, NodeKind);
 if (!TextualResults.empty())
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -556,12 +556,7 @@
   HoverInfo HI;
 
   if (const auto *D = T->getAsTagDecl()) {
-HI.Name = printName(ASTCtx, *D);
-HI.Kind = index::getSymbolInfo(D).Kind;
-
-const auto *CommentD = getDeclForComment(D);
-HI.Documentation = getDeclComment(ASTCtx, *CommentD);
-enhanceFromIndex(HI, *CommentD, Index);
+return getHoverContents(D, Index);
   } else {
 // Builtin types
 auto Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -667,21 +667,44 @@
 return {};
   }
 
-  const syntax::Token *TouchedIdentifier =
-  syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
-  if (TouchedIdentifier)
-if (auto Macro =
-locateMacroReferent(*TouchedIdentifier, AST, *MainFilePath))
-  // Don't look at the AST or index if we have a macro result.
-  // (We'd just return declarations referenced from the macro's
-  // expansion.)
-  return {*std::move(Macro)};
-
-  ASTNodeKind NodeKind;
-  auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST,
-