[PATCH] D128826: Go-to-type on smart_ptr<Foo> now also shows Foo

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

TL;DR: I'm convinced that this patch (return multiple locations) is a good choice. Also talked to Kadir offline.

In D128826#3619339 <https://reviews.llvm.org/D128826#3619339>, @tom-anders wrote:

> I'm using this with neovim's builtin LSP, where when there are multiple results it takes me to the first one, and also opens the quickfix window that shows all results. But I can see how the current implementation could be annoying with other editors.

(Interesting, I'm using the same and find it annoying: it focuses the quickfix window and that's disruptive enough that I never noticed the cursor also moved. But https://github.com/neovim/neovim/pull/14878 lists some workarounds)

> So here's what I thought: Say you have a variable that's a smart pointer and trigger textDocument/typeDefinition on it and there's only a single result that takes you straight to the type. In this case, you might not even know that the type is actually wrapped inside a smart pointer and might make wrong assumptions about the code. Of course you could've done a Hover beforehand, but in the case where there are 2 results, you don't need that extra step.

Yeah. There's no guarantee the type in question is well-represented by a decl, but this is a bit confusing.
I think it might still be worth it for specific types like `unique_ptr<Foo>`, but for `vector<Foo>` it's too surprising even if you want Foo 99% of the time.

> Maybe it would be best to make this configurable via the config file? Since most people are using "most editors", I think taking you straight to the type would be better, but I'm really not sure.

Making it configurable doesn't actually solve the problem: we still need to think hard about what the best default is, because relatively few users ever configure anything.
So let's not rule it out, but there's not much point doing it in this patch.

> Btw, I think it would be useful to extend this feature to not only smart pointers, but also containers like std::vector. Then there'd also be the question what would happen for std::vector<std::unique_ptr<T>>: should there be 1, 2 or 3 results?

Yeah I find this pretty compelling. I think we should start with the approach in this patch, returning {pointer, pointee}, and iterate from there (add containers, or templates in general, or omit the pointer type in certain special cases...).



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1913
 // to target.
-static QualType unwrapFindType(QualType T) {
+static llvm::SmallVector<QualType> unwrapFindType(
+    QualType T, const HeuristicResolver* H) {
----------------
Ergonomically I might prefer `void unwrapFindType(QualType, ..., vector<QualType>& Out)` or so.
This tends to compose a bit better IMO by making all the cases look similar whether you're adding one type or several or combining lists.

You can still `return Out.push_back(T)` etc on a single line.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1916
   if (T.isNull())
-    return T;
+    return {T};
 
----------------
I think we want to return 0 types in this case


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1965
+    for (const QualType& Type : unwrapFindType(typeForNode(N), AST.getHeuristicResolver()))
+      if (!Type.isNull()) {
+        llvm::copy(locateSymbolForType(AST, Type), std::back_inserter(LocatedSymbols));
----------------
(if you modify unwrap above to not return null, you can skip this check)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1966
+      if (!Type.isNull()) {
+        llvm::copy(locateSymbolForType(AST, Type), std::back_inserter(LocatedSymbols));
+      }
----------------
we now have the potential for duplicates (already in this patch in fact: unique_ptr<unique_ptr<int>>.

Deduplicating types isn't that useful (unique_ptr<unique_ptr<int>> != unique_ptr<int>), I guess it's either deduplicate locations or allow the duplicates.
If you choose to allow them, probably worth a comment why.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128826