[clangd] Populate detail field in document symbols

[lightmelodies]

Summary

This commit fix https://github.com/clangd/clangd/issues/520 and https://github.com/clangd/clangd/issues/601.

[sammccall]

Thanks for doing this!

I think this behavior is a great starting point.

I suspect printing the decl is more verbose than desired in a lot of cases, at least avoiding repeating the identifier would be nice. e.g.
x | int x --> x | int
foo | void foo() --> foo | void()
bar | struct bar {} --> bar | struct
This can be tweaked later, the simple implementation seems like a big improvement already.
If you feel like it though, I think this is mostly just printing the type instead of the decl for ValueDecls.

When you upload snapshots, please prepare the diffs with full context -U9999 to make review easier. (the Arcanist arc tool will do this automatically)

clang-tools-extra/clangd/FindSymbols.cpp

201

nit: limit the scope of raw_string_ostream by putting it in an anonymous block.
It does not flush until destroyed (or explicitly flushed)

201

Can we pull out a helper function for this? getSymbolDetail or so?

For now it's just a few lines but I imagine it could grow as we refine the behavior.

clang-tools-extra/clangd/Hover.h

20 ↗

(On Diff #323882)

this isn't a reasonable thing to expose & depend on from FindSymbols.cpp.

It's fine to add a copy into FindSymbols.cpp, especially since you want a slightly different policy anyway

clang-tools-extra/clangd/test/symbols.test

1

Can you add unit-tests for this feature in FindSymbolsTests.cpp?
Don't need to assert the detail for every symbol we test there, but adding a WithDetail matcher and adding it to a few nodes in the testcases would be useful. In particular, at least one that shows the getDescribedTemplateParams bit.

(It's nice to have a basic smoke-test here but unit-tests are where we try to cover more of the cases)

[lightmelodies]

Thanks for your response. Yes decl is too verbose in many cases. In fact I have tried another concise version, but I can not decide which one is better. Unit test in FindSymbolsTests.cpp is also done, but I would like upload them after we reach an agreement on what behavior is prefered.

[sammccall]

This looks really good. Only thing i'd really think about changing in behavior is dropping the template params.
Please go ahead and upload the unittests!

(And thanks for including the diff context)

clang-tools-extra/clangd/FindSymbols.cpp

184

As a tradeoff between brevity and information, we might consider printing template but not the param list.

Particularly for functions this is useful, as template params are mostly types repeated in the signature:

- DenseMap | template <typename InputIt> void (const InputIt&, const InputIt&)
+ DenseMap | template void (const InputIt&, const InputIt&)

But I think this may be fine for classes as well:

- DenseMap | template <typename KeyT, typename ValueT, typename KeyInfoT = ...> class
+ DenseMap | template class

188

It might not makes sense to print the placeholder DependentType, types that contain undeduced auto, and maybe some other cases.

(This can definitely be a fixme comment for later - it'll be hard to find exactly the right cases in one pass)

192

What you have here is probably best, just thinking out loud...

we could also consider including the underlying type like = int but I suspect the difference between ValueDecl is too subtle and sometimes the types are really long anyway.

194

(if we drop the template params then I don't think we need to even print "template" in this case because concepts are always templates)

197

nit: either OS.flush(); return Detail; or equivalently return std::move(OS.str());

(Otherwise whether OS's buffer gets flushed into the right string or not is fragile, I believe relying on move elision)

[lightmelodies]

Change behavior of template and add unit tests.

[sammccall]

Nice! This looks good to land as-is, I just have some suggestions where we may want to mark behavior to revisit later, and some places where we could trim the tests a bit.

Do you have commit access, or want me to land this for you? (I'd need an address for the commit)

clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp

384 ↗

(On Diff #324599)

the void return type isn't really meaningful

may want a FIXME here that () or constructor() would probably be better

(no need to fix though)

392 ↗

(On Diff #324599)

for destructors I think we should probably suppress detail entirely - signature is always the same and it's clear from the name it's a destructor.

490 ↗

(On Diff #324599)

up to you, but we don't need to add WithDetail() assertions everywhere, probably just enough to ensure we cover the interesting cases.

We could leave out these ones, exportContext, noLocals etc that are really testing other things.

588 ↗

(On Diff #324599)

feels like this could benefit from being "specialization struct" or so, "specialization int" below etc.

Again, this is a possible FIXME comment, not something that needs to be done now.

630 ↗

(On Diff #324599)

(all these WithDetail("")s seem pretty redundant)

[lightmelodies]

Better printing for C++ constructor and destructor. Remove unnecessary test cases.

[lightmelodies]

In D96751#2571346, @sammccall wrote:

Nice! This looks good to land as-is, I just have some suggestions where we may want to mark behavior to revisit later, and some places where we could trim the tests a bit.

Do you have commit access, or want me to land this for you? (I'd need an address for the commit)

I don't have commit access yet, it will be appreciated if you can land this for me (just use lightmelodies([e-mail]))

[sammccall]

Fantastic, thank you!

clang-tools-extra/clangd/FindSymbols.cpp

189

I think equivalent and a little safer is

std::string ConstructorType = VD->getType().getAsString(P);
llvm::StringRef WithoutVoid = ConstructorType;
WithoutVoid.consume_front("void");
OS << WithoutVoid;

(I'll apply this when landing)

[lightmelodies]

In D96751#2571563, @sammccall wrote:

Fantastic, thank you!

Maybe WithoutVoid.consume_front("void ");

[sammccall]

In D96751#2571579, @lightmelodies wrote:

In D96751#2571563, @sammccall wrote:

Fantastic, thank you!

Maybe WithoutVoid.consume_front("void ");

Oh wow, yes... sizeof() was really doing some heavy lifting :-)

clang-tools-extra/clangd/FindSymbols.cpp

Index->fuzzyFind(Req, [HintPath, &Top, &Filter, AnyScope = Req.AnyScope,
     Top.push({Score, std::move(Info)});
   });
   for (auto &R : std::move(Top).items())
     Result.push_back(std::move(R.second));
   return Result;
 }
 
 namespace {
+
+std::string getSymbolDetail(ASTContext &Ctx, const NamedDecl &ND) {
+  PrintingPolicy P(Ctx.getPrintingPolicy());
+  P.SuppressScope = true;
+  P.SuppressUnwrittenScope = true;
+  P.AnonymousTagLocations = false;
+  P.PolishForDeclaration = true;
+  std::string Detail;
+  llvm::raw_string_ostream OS(Detail);
+  if (const auto *TP = ND.getDescribedTemplateParams()) {
+    TP->print(OS, Ctx, P);
+  }
+  if (const auto *VD = dyn_cast<ValueDecl>(&ND)) {
+    VD->getType().print(OS, P);
+  } else if (const auto *TD = dyn_cast<TagDecl>(&ND)) {
+    OS << TD->getKindName();
+  } else if (const auto *TD = dyn_cast<TypedefNameDecl>(&ND)) {
+    OS << "type alias";
+  } else if (const auto *CD = dyn_cast<ConceptDecl>(&ND)) {
+    CD->getTemplateParameters()->print(OS, Ctx, P);
+    OS << "concept";
+  }
+  return Detail;
+}
+
 llvm::Optional<DocumentSymbol> declToSym(ASTContext &Ctx, const NamedDecl &ND) {
   auto &SM = Ctx.getSourceManager();
 
   SourceLocation BeginLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getBeginLoc()));
   SourceLocation EndLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getEndLoc()));
   const auto SymbolRange =
       toHalfOpenFileRange(SM, Ctx.getLangOpts(), {BeginLoc, EndLoc});
   if (!SymbolRange)
     return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
   // FIXME: This is not classifying constructors, destructors and operators
   // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;
   SI.name = printName(Ctx, ND);
   SI.kind = SK;
   SI.deprecated = ND.isDeprecated();
   SI.range = Range{sourceLocToPosition(SM, SymbolRange->getBegin()),
                    sourceLocToPosition(SM, SymbolRange->getEnd())};
+  SI.detail = getSymbolDetail(Ctx, ND);
 
   SourceLocation NameLoc = ND.getLocation();
   SourceLocation FallbackNameLoc;
   if (NameLoc.isMacroID()) {
     if (isSpelledInSource(NameLoc, SM)) {
       // Prefer the spelling loc, but save the expansion loc as a fallback.
       FallbackNameLoc = SM.getExpansionLoc(NameLoc);
       NameLoc = SM.getSpellingLoc(NameLoc);

clang-tools-extra/clangd/test/symbols.test

 # RUN: clangd --index-file=%S/Inputs/symbols.test.yaml -lit-test < %s | FileCheck %s
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"symbol":{"symbolKind":{"valueSet": [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26]}}}},"trace":"off"}}
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"documentSymbol":{"hierarchicalDocumentSymbolSupport":true}},"workspace":{"symbol":{"symbolKind":{"valueSet": [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26]}}}},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
 ---
 {"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"vector"}}
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:    "result": [
 # CHECK-NEXT:      {

 # CHECK-NEXT:    ]
 # CHECK-NEXT:}
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/documentSymbol","params":{"textDocument":{"uri":"test:///main.cpp"}}}
 #      CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:    "result": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "containerName": "",
+# CHECK-NEXT:        "detail": "void ()",
 # CHECK-NEXT:        "kind": 12,
-# CHECK-NEXT:        "location": {
-# CHECK-NEXT:          "range": {
-# CHECK-NEXT:            "end": {
-# CHECK-NEXT:              "character": {{.*}},
-# CHECK-NEXT:              "line": {{.*}}
-# CHECK-NEXT:            },
-# CHECK-NEXT:            "start": {
-# CHECK-NEXT:              "character": {{.*}},
-# CHECK-NEXT:              "line": {{.*}}
-# CHECK-NEXT:            }
-# CHECK-NEXT:          },
-# CHECK-NEXT:          "uri": "file://{{.*}}/main.cpp"
-# CHECK-NEXT:        },
-# CHECK-NEXT:        "name": "foo"
-# CHECK-NEXT:      }
+# CHECK-NEXT:        "name": "foo",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": {{.*}},
+# CHECK-NEXT:            "line": {{.*}}
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": {{.*}},
+# CHECK-NEXT:            "line": {{.*}}
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "selectionRange": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": {{.*}},
+# CHECK-NEXT:            "line": {{.*}}
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": {{.*}},
+# CHECK-NEXT:            "line": {{.*}}
+# CHECK-NEXT:          }
+# CHECK-NEXT:        }
+# CHECK-NEXT:      },
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "containerName": "",
+# CHECK-NEXT:        "detail": "int ()",
 # CHECK-NEXT:        "kind": 12,
-# CHECK-NEXT:        "location": {
-# CHECK-NEXT:          "range": {
-# CHECK-NEXT:            "end": {
-# CHECK-NEXT:              "character": {{.*}},
-# CHECK-NEXT:              "line": {{.*}}
-# CHECK-NEXT:            },
-# CHECK-NEXT:            "start": {
-# CHECK-NEXT:              "character": {{.*}},
-# CHECK-NEXT:              "line": {{.*}}
-# CHECK-NEXT:            }
-# CHECK-NEXT:          },
-# CHECK-NEXT:          "uri": "file://{{.*}}/main.cpp"
-# CHECK-NEXT:        },
-# CHECK-NEXT:        "name": "main"
+# CHECK-NEXT:        "name": "main",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": {{.*}},
+# CHECK-NEXT:            "line": {{.*}}
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": {{.*}},
+# CHECK-NEXT:            "line": {{.*}}
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "selectionRange": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": {{.*}},
+# CHECK-NEXT:            "line": {{.*}}
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": {{.*}},
+# CHECK-NEXT:            "line": {{.*}}
+# CHECK-NEXT:          }
+# CHECK-NEXT:        }
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ]
 # CHECK-NEXT:}
 ---
 {"jsonrpc":"2.0","id":3,"method":"textDocument/documentSymbol","params":{"textDocument":{"uri":"test:///foo.cpp"}}}
 #      CHECK:  "error": {
 # CHECK-NEXT:    "code": -32602,
 # CHECK-NEXT:    "message": "trying to get AST for non-added document"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0"
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}