This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Initial supoprt for cross-namespace global code completion.
ClosedPublic

Authored by ioeric on Sep 21 2018, 9:27 AM.

Details

Summary

When no scope qualifier is specified, allow completing index symbols
from any scope and insert proper automatically. This is still experimental and
hidden behind a flag.

Things missing:

  • Scope proximity based scoring.
  • FuzzyFind supports weighted scopes.

Event Timeline

ioeric created this revision.Sep 21 2018, 9:27 AM

This is really cool!
From reading the code this behaves exactly how I'd expect.
Ranking will be the real test.

Main comment is I'd like to tweak the interface on FuzzyFindRequest, rest is basically nits.

clangd/CodeComplete.cpp
367

FWIW, I find "C.IndexResult" clearer than "Sym" here, the "index" part is relevant

374

Consider inverting this comment, I found it a bit hard to unpack:

// If the completion was visible to Sema, no qualifier is needed.
// This avoids unneeded qualifiers in cases like with `using ns::X`.
377

I'm not sure this FIXME is important enough to ever get fixed, consider removing it.

clangd/CodeComplete.h
105

This comment should also mention that this applies in contexts without explicit scope qualifiers.

106

nit: I'd remove the parenthetical, it's a little ambiguous whether it applies to the clause inside the "not" or outside it.

108

what about just calling this AllScopes?

clangd/index/Index.h
430–433

I'm not a big fan of this magic value, it seems too easy to forget to handle.
Especially since we have a lot of existing code that touches this interface, and we may forget to check some of it.

I suggest making this a separate boolean field AnyScope, with a comment that scopes explicitly listed will be ranked higher.
We can probably also remove the "If this is empty" special case from Scopes now too, as the old behavior can be achieved by setting Scopes = {}; AnyScope = true;

432

May not want a heavyweight API with explicit weights...

I think what we really need here is the ability to weight clang::clangd:: > clang:: > `` when you're in the scope of namespace clangd.

We could just say something like "the primary scope should come first" and do some FileDistance-like penalizing of the others...

clangd/index/dex/Dex.cpp
168

wrap in a createBoost(..., 0.1) or so?

clangd/tool/ClangdMain.cpp
139

It's a little confusing to give these different internal vs external names - do we have a strong reason not to call this "all-scopes-completion" or so?

142

I'm not sure whether putting "experimental" in every flag description is worthwhile - maybe hidden is enough?

unittests/clangd/CodeCompleteTests.cpp
2043

also verify that na::Clangd^ only gets one result?

2059

can you add the scope matcher on this line for clarity?

2073

maybe add a comment reiterating what this is testing: "Although Clangd1 is from another namespace, Sema tells us it's in-scope and needs no qualifier."

ioeric updated this revision to Diff 167135.Sep 26 2018, 7:48 AM
ioeric marked 14 inline comments as done.
  • address review comments
clangd/index/Index.h
430–433

sounds good.

We can probably also remove the "If this is empty" special case from Scopes now too, as the old behavior can be achieved by setting Scopes = {}; AnyScope = true;

I think this is a good idea. Unfortunately, there seem to be many tests that rely on the old behavior. I'll add a FIXME and do it in followup patch.

432

Changed the wording of FIXME.

I think what we really need here is the ability to weight clang::clangd:: > clang:: > `` when you're in the scope of namespace clangd.

It's unclear what this would mean for scopes from using-namespace directives. But we can revisit when we get there.

kbobyrev added inline comments.
clangd/index/dex/Dex.cpp
171

Probably also check !ScopeIterators.empty(): otherwise the latency might increase for queries without any scope/any scope known to Dex.

unittests/clangd/DexTests.cpp
557

Probably also add test which ensures that wildcard symbols are downranked in the presence of Req.Scopes (ProximityPathsBoosting test is probably the closest in this sense)? Isn't too important, but nice-to-have in the presence of ScopesProximity plans (judging from the FIXME in FuzzyFindRequest).

sammccall accepted this revision.Sep 27 2018, 9:50 AM
sammccall added inline comments.
clangd/CodeComplete.cpp
1251

consider splitting out AllScopes as a separate variable, and assign std::tie(QueryScopes, AllScopes) = getQueryScopes(...), for readability

clangd/index/Index.h
432

Yeah, my hypothesis is that it doesn't matter much, as long as the using-namespaces are ranked lower than the enclosing namespaces, and above "any" namespaces.

clangd/index/dex/Dex.cpp
171

if there are no other scopes, the "naive" query tree will end up looking like and(..., or(boost(true)).

The or() will already be optimized away today.
I'd suggest you just remove the boost() here if req.scopes is empty (or better: make the boost 1) and we make "generic optimizations" take care of eliminating boosts with value 1, and dropping true when it's an argument to and()

This revision is now accepted and ready to land.Sep 27 2018, 9:50 AM
ioeric updated this revision to Diff 167356.Sep 27 2018, 11:45 AM
ioeric marked 2 inline comments as done.
  • address review comments
This revision was automatically updated to reflect the committed changes.