[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D39836#920977, @arphaman wrote:

> In https://reviews.llvm.org/D39836#920587, @sammccall wrote:
>
> > So I probably should have started from the other end, here :-)
> >
> > I'd really like to make the completion retrieval and ranking more flexible. In particular
> >
> > - incorporating results from other sources (indexes: both in-memory and external services).
> > - combining more quality signals like usage count and fuzzy-match-strength in a non-lexicographic-sort way The biggest difficulty in *supporting* unusable functions is maintaining the invariant that all unusable functions are ranked lower than usable ones - all code that deals with ranking has to deal with this special case, e.g. by making score a tuple instead of a single number.
>
>
> That sounds good to me. Please keep in mind that not all clients might want to take advantage of these things as they might have their own fuzz-match logic


Yup! My understanding of the protocol is clients will generally trigger completion after the dot, and then filter client-side *unless* the result set is incomplete.
So fuzzy-match filtering/scoring would only be triggered if we limit the number of results (proposed as optional in https://reviews.llvm.org/D39852) or if completion is explicitly triggered mid-identifier (maybe we should allow turning this off too).

> and external result injection.

Absolutely. To be concrete, i'm thinking about three overlaid:

1. a static generated index that clangd consumes (e.g. generated by index-while-building and located on disk, or a hosted index of google's monorepo)
2. a dynamically generated index of symbols in files that clangd has seen. (Likely to be dirty with respect to 1)
3. context-sensitive completion results as today

1 would definitely be optional.
2 isn't really external... we might want to handle global symbols with 2 and not 3 as now, but that's an implementation detail.

>> If the current approach of "give them a penalty" is enough, knowing that in the future it may lead to e.g. a very widely used but inaccessible protected function being ranked highly, then that seems fine to me too. A wider configuration space means testing is more work, but happy to live with it. What do you think?
>> 
>> (With my user-hat on, configurable is fine, though I do strongly feel they should be off by default, and it seems unlikely many users will change the defaults.)
> 
> I'd be ok with off by default, as long as it's possible to turn it on :)

Sure, I'll update the patch.

For these things that are (I assume) more "user preference" than project- or integration-specific, I wonder whether command-line flags are the right thing. Configuration in `~/.clangd` is a can of worms, but might be the right thing in the long-run.


https://reviews.llvm.org/D39836