This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Boost code completion results that are narrowly scoped (local, members)
ClosedPublic

Authored by sammccall on Jun 5 2018, 12:44 AM.

Details

Summary

This signal is considered a relevance rather than a quality signal because it's
dependent on the query (the fact that it's completion, and implicitly the query
context).

This is part of the effort to reduce reliance on Sema priority, so we can have
consistent ranking between Index and Sema results.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Jun 5 2018, 12:44 AM
ioeric added a comment.Jun 5 2018, 1:57 AM

Looks like a great improvement on ranking!

clangd/Quality.cpp
62 ↗(On Diff #149910)

nit: maybe stop when`!DC->isFileContext()`?

66 ↗(On Diff #149910)

DC->isRecord()?

70 ↗(On Diff #149910)

I wonder if this comparison is a common practice. Any reason not to use !=?

71 ↗(On Diff #149910)

Does FileScope indicate that D is defined in the current *translation unit*? If so, I think this is a good signal (boosting symbols in the same TU), but something like TUScope might be a bit clearer?

76 ↗(On Diff #149910)

What's here to be fixed? Do we plan to get rid of this assumption in the future?

86 ↗(On Diff #149910)

nit: does this assume that "smaller" scopes are better? If so, it might make sense to document.

unittests/clangd/QualityTests.cpp
1 ↗(On Diff #149910)

Could you also add a test case for code completion?

sammccall updated this revision to Diff 149951.Jun 5 2018, 5:11 AM
sammccall marked 3 inline comments as done.

Address review comments and sync.

sammccall added inline comments.Jun 5 2018, 5:13 AM
clangd/Quality.cpp
70 ↗(On Diff #149910)

The orderedness in linkage is used e.g. in isExternallyVisible().
I prefer < because while with this particular threshold value != would be equivalent, I'm not actually particularly sure it's the right threshold (vs e.g. ModuleLinkage), and with any other threshold < would be required.

71 ↗(On Diff #149910)

File rather than TU is intended here. AccessibleScope is only an approximate concept (it doesn't need to be precise as it's just a scoring signal).
The idea is "this is a symbol likely to be only used in the same file that declares it", and this is true of e.g. variables in anonymous namespaces, despite the fact that you can technically put them in headers and reference them in the main file.

Added a comment to AccessibleScope to clarify this approximate nature.

86 ↗(On Diff #149910)

There's no connection with "better" here, just that items are assumed to have maximum scope (global) until proven otherwise.

There's no intent here (or with other signals) for merging conflicting results from different sources to do anything clever - it's fine to choose one arbitrarily.

unittests/clangd/QualityTests.cpp
1 ↗(On Diff #149910)

The code completion scoring is tested in SymbolRelevanceSignalsSanity: file scope is boosted compared to default when the query is code-complete, but not when it's generic.

What kind of test do you mean?

ioeric accepted this revision.Jun 5 2018, 6:21 AM

lgtm

clangd/Quality.cpp
70 ↗(On Diff #149910)

sounds good. this would probably deserve a comment though as it's a little confusing...

71 ↗(On Diff #149910)

Cool. Thanks for the explanation!

unittests/clangd/QualityTests.cpp
1 ↗(On Diff #149910)

I was thinking a test case that covers the changes in CodeComplete.cpp e.g. check that Relevance and Quality play well together, and locals/members are boosted? Would that make sense?

This revision is now accepted and ready to land.Jun 5 2018, 6:21 AM
sammccall marked an inline comment as done.Jun 5 2018, 9:28 AM
sammccall added inline comments.
unittests/clangd/QualityTests.cpp
1 ↗(On Diff #149910)

Actually one of the purposes of pulling out the Quality module is to stop writing such tests :-)
They're fragile because ranking depends on many factors, e.g. at the moment you can't construct a completion candidate with a different scope that won't also get a different sema priority, so it's not clear why a test is passing/failing.
And for every signal, you need a test in code complete, and in workspace symbols...

It *would* be useful to have a smoke test in CodeCompletion to make sure we're using those scores. Maybe it would make sense to turn ReferencesAffectRanking or so into that?

This revision was automatically updated to reflect the committed changes.
ioeric added inline comments.Jun 5 2018, 9:38 AM
unittests/clangd/QualityTests.cpp
1 ↗(On Diff #149910)

That makes sense. A smoke test in code completion sounds good.