[clangd] Prefer definitions for gototype and implementation

[sammccall]

There's no test here, can you describe the cases you expect this to affect and why the new behavior is better?

For types this seems doubly-dubious at first glance:

• when we've decided to "prefer" a non-definition declaration, why make an exception here?
• I'd expect the definition to almost-always be the preferred declaration anyway.

For implementation, I think I there's a good argument for this behavior: the purpose of the command is to cut through abstractions to see what this method does in practice, and finding the definition fits with that. I'm happy with this behavior but I think it deserves a comment.
(There's also a bad argument: users think "implementation" means definition - just because the name is ambiguous doesn't mean we should implement both behaviors!)

[kadircet]

In D133843#3791127, @sammccall wrote:

There's no test here, can you describe the cases you expect this to affect and why the new behavior is better?

right, sorry for the obscure patch. giving examples/details in particular cases below.

For types this seems doubly-dubious at first glance:

• when we've decided to "prefer" a non-definition declaration, why make an exception here?
• I'd expect the definition to almost-always be the preferred declaration anyway.

So apart from the special cases this happens, at the moment we have a discrepancy between go-to-def on a type and go-to-type on a decl, which to me seems like a surprise and definitely unexpected. At least to me the main point of go-to-type interaction is to get rid of the extra jump, and when it takes me to declaration for whatever reason, that benefit is lost.
As for the particular cases I've noticed this happening, it's forward declarations visible from the definition of a type, which manifests in a couple different forms:

• A header depending on the type forward declaring it, and also being included by the TU defining the type (sort of a circular dependency, so might not be as important).
• A type that's being forward declared by the same header defining it, while the type still being public (which I think is common when there are interactions between two different types).
• A private type that's being forward declared in a header and defined in a private header or implementation file.

To me it seems like only the last case might warrant a jump to declaration, but I think it's still to prefer definition in that case. In other cases we most definitely want the definition directly.

For implementation, I think I there's a good argument for this behavior: the purpose of the command is to cut through abstractions to see what this method does in practice, and finding the definition fits with that. I'm happy with this behavior but I think it deserves a comment.
(There's also a bad argument: users think "implementation" means definition - just because the name is ambiguous doesn't mean we should implement both behaviors!)

Right, this was my motivation as well. Especially in UI-rich editors like VSCode, one actually gets snippets around the locations and seeing snippets around the definition definitely feels more helpful.

happy to add some comments for the reasoning if you (and possibly others seeing this change) also agree that this is more useful

[tom-anders]

happy to add some comments for the reasoning if you (and possibly others seeing this change) also agree that this is more useful

Can confirm that (at least to me) it's really annoying when it takes me to a forward declaration here. I 99% of the time will immediately trigger textDocument/definition when this happens.

[kadircet]

abandoning in favor of D155898

[sammccall]

Yeah, having used this feature for a while, you're right about all of this of course, the current behavior is infuriating.
Sorry!

clang-tools-extra/clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onGoToType(const TextDocumentPositionParams &Params,
   Server->findType(
       Params.textDocument.uri.file(), Params.position,
       [Reply = std::move(Reply)](
           llvm::Expected<std::vector<LocatedSymbol>> Types) mutable {
         if (!Types)
           return Reply(Types.takeError());
         std::vector<Location> Response;
         for (const LocatedSymbol &Sym : *Types)
-          Response.push_back(Sym.PreferredDeclaration);
+          Response.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
         return Reply(std::move(Response));
       });
 }
 
 void ClangdLSPServer::onGoToImplementation(
     const TextDocumentPositionParams &Params,
     Callback<std::vector<Location>> Reply) {
   Server->findImplementations(
       Params.textDocument.uri.file(), Params.position,
       [Reply = std::move(Reply)](
           llvm::Expected<std::vector<LocatedSymbol>> Overrides) mutable {
         if (!Overrides)
           return Reply(Overrides.takeError());
         std::vector<Location> Impls;
         for (const LocatedSymbol &Sym : *Overrides)
-          Impls.push_back(Sym.PreferredDeclaration);
+          Impls.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
         return Reply(std::move(Impls));
       });
 }
 
 void ClangdLSPServer::onSymbolInfo(const TextDocumentPositionParams &Params,
                                    Callback<std::vector<SymbolDetails>> Reply) {
   Server->symbolInfo(Params.textDocument.uri.file(), Params.position,
                      std::move(Reply));