This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Index UsingDecls
ClosedPublic

Authored by kadircet on Feb 18 2019, 2:18 AM.

Details

Summary

D58340 enables indexing of USRs, this makes sure test in clangd are
aligned with the change

Event Timeline

kadircet created this revision.Feb 18 2019, 2:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2019, 2:18 AM
kadircet updated this revision to Diff 187209.Feb 18 2019, 3:04 AM
  • Update USR to include "UD"

Could we also add a test for code completion to make sure we return the new decls there?

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

namespace abc {
int foo(int);
int foo(double);
};

namespace std {
using abc::foo;
}

void f() {
  std::^
}

For context: @hokein mentioned problems in the clangd's code completion if we would index these symbols.
This patch in itself does not hurt much, users of the indexing API can decide how to deal with UsingDecl on their own, clangd is just one of the clients.

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

That's true, but we're not sure how much this would hurt in practice. Currently we don't show any results from dynamic index for std::strcmp, which is arguably worse than showing an extra completion item for the using.

For context: @hokein mentioned problems in the clangd's code completion if we would index these symbols.
This patch in itself does not hurt much, users of the indexing API can decide how to deal with UsingDecl on their own, clangd is just one of the clients.

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

That's true, but we're not sure how much this would hurt in practice. Currently we don't show any results from dynamic index for std::strcmp, which is arguably worse than showing an extra completion item for the using.

std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior? Is it possible for us to extend Sema to support it?

If we decide to provide using-decl results from index, I think we should make sure the code completion information (e.g. signature) is correct.

std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior?

Yeah, I think it is. There's an explicit code path that takes the target decls of a using. Arguably, that's good if you to show signatures of the methods.

Is it possible for us to extend Sema to support it?

We could, but then we'd loose the signatures of the targets functions, which is sad :-(

If we decide to provide using-decl results from index, I think we should make sure the code completion information (e.g. signature) is correct.

The problem is that using-decls have multiple signatures. They can introduce more than one name into the scope, so the question is which one should we pick and how should we store them.
In any case, it feels like any solution we can come up with would require storing using declarations in the index in one form or the other, so this patch definitely makes sense: it gives us hooks we can use to handle usings in clangd.

kadircet updated this revision to Diff 187367.Feb 19 2019, 7:24 AM
  • Add tests for code completion
ilya-biryukov accepted this revision.Feb 19 2019, 7:42 AM

LGTM from my side, with a few NITs.
But let's wait for an LGTM from Haojian too, to make sure his concerns are addressed.

unittests/clangd/SymbolCollectorTests.cpp
1126

That's definitely too much setup for such a simple test.
I thought it's possible to wire up a real index in the completion tests, but it seems that's not the case. So let's not bother to run an actual completion here, ignore my previous comment about adding a test.

1163

a typo NIT: s/does not/do not
also, maybe use "type info" or "signature" instead of "target info"?

This revision is now accepted and ready to land.Feb 19 2019, 7:42 AM
kadircet updated this revision to Diff 187376.Feb 19 2019, 8:04 AM
kadircet marked an inline comment as done.
  • Revert last change

Thanks for the explanation.

std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior?

Yeah, I think it is. There's an explicit code path that takes the target decls of a using. Arguably, that's good if you to show signatures of the methods.

Is it possible for us to extend Sema to support it?

We could, but then we'd loose the signatures of the targets functions, which is sad :-(

If we decide to provide using-decl results from index, I think we should make sure the code completion information (e.g. signature) is correct.

The problem is that using-decls have multiple signatures. They can introduce more than one name into the scope, so the question is which one should we pick and how should we store them.

I think for most cases, the using-decl has only one shadow decl, anyway it is out scope of this patch.

In any case, it feels like any solution we can come up with would require storing using declarations in the index in one form or the other, so this patch definitely makes sense: it gives us hooks we can use to handle usings in clangd.

unittests/clangd/SymbolCollectorTests.cpp
15

These includes are not needed.

58

here as well.

1126

I thought it's possible to wire up a real index in the completion tests, but it seems that's not the case. So let's not bother to run an actual completion here, ignore my previous comment about adding a test.

Adding completions to SymbolCollectorTest is overweight, but I think this is possible (and worthy) to add one to CodeCompleteTest without too much effort. We have TU.index() to build a real index.

I understand the problem better now, we are missing some decls from sema because we avoid deserialization in preamble, I think we should document it somewhere, can't think a better place, maybe at the completion test?

kadircet updated this revision to Diff 188170.Feb 25 2019, 7:25 AM
kadircet marked 2 inline comments as done.
  • Address comments
kadircet marked an inline comment as done.Feb 25 2019, 7:27 AM
hokein accepted this revision.Feb 25 2019, 7:33 AM
This revision was automatically updated to reflect the committed changes.