[PATCH] D53131: [clangd] Support scope proximity in code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/CodeComplete.cpp:558
+    if (const auto *NS = dyn_cast<NamespaceDecl>(Ctx))
+      return NS->getQualifiedNameAsString() + "::";
+  return llvm::None;
----------------
sammccall wrote:
> does this do the right thing if it's anonymous or inline, or a parent is?
> 
> For indexing, we call a function in AST.h, we should probably do something similar.
> 
> The catch is that function relies on NamedDecl::printQualifiedName, maybe we need to factor out the relevant part so we can call it on a DeclContext.
Good catch.

`NamedDecl::printQualifiedName` doesn't skip the `(anonymous)` part if the decl itself is anonymous, even with `SuppressUnwrittenScope` set. So I think we should explicitly filter those out here and then call `printQualifiedName`.


================
Comment at: clangd/CodeComplete.cpp:559
+      return NS->getQualifiedNameAsString() + "::";
+  return llvm::None;
+}
----------------
sammccall wrote:
> shouldn't this be ""? That's a definite scope, not a failure to find one.
> (It's not a *namespace* scope, but I'm not sure why that's important)
Make sense. 

I think we might still want to special-case the global scope (not here, maybe in proximity scoring) because:
- if any scope is specified, it's probably more desireable than the global scope.
- there are patterns like below in cpp files (instead of using enclosing namespace):
```
using namespace clang;
using namespace clang::clangd;
```

Currently, if the enclosing scope is "", then we would treat it the same as the scopes from using-namespace directives.


================
Comment at: clangd/Quality.cpp:246
 
+float QueryScopeProximity::proximity(llvm::StringRef Scope) const {
+  if (QueryScopes.empty())
----------------
sammccall wrote:
> If you're going to decide these numbers directly...
> 
> a) I think you should just return a multiplier here, rather than a 0-1 score
> b) we should more aggressively downrank non-preferred symbols: currently by only ~1/3 vs symbols from a preferred scopes
> 
> e.g. I'd suggest returning 1, 2, 1.5, 1, 1, 1.5, 0.3 or similar
> a) I think you should just return a multiplier here, rather than a 0-1 score.
I tried this. Unfortunately, allowing <1 multiplier would make the default value (for sema proximity) tricky. And [0,1] proximity score seems to be less confusing as it's consistent with the file proximity scale. If [0,1] isn't enough, we could still increase the upper bound?

> b) we should more aggressively downrank non-preferred symbols: currently by only ~1/3 vs symbols from a preferred scopes
It's ~1/3 only for the global scope vs non-preferred symbols, which seems reasonable to me. I worry penalizing non-preferred scopes too much can make cross-namespace completion less useable.


================
Comment at: clangd/Quality.h:92
+  private:
+    std::vector<std::string> QueryScopes;
+};
----------------
sammccall wrote:
> hmm, can/should we use FileProximity for this, and just transform the strings?
> 
> This isn't going to cache for symbols sharing a namespace, isn't going to handle "symbol is in a child namespace of an included namespace", and some other combinations.
It seems to me that it'd be less trivial to associate scopes with up/down traverses. Currently, the query scopes contain all enclosing namespaces, so the distance seems less important here. In general, I think the characteristics of scope proximity (e.g. all enclosing namespaces are already in query scopes) allow us to get away with something more trivial than file proximity? 

> This isn't going to cache for symbols sharing a namespace.
This seems to be less a concern if we are not calculating up/down distance.
> isn't going to handle "symbol is in a child namespace of an included namespace"
This can be covered by `SymbolScope.startswith(QueryScope)`? It might not work well for `using-namespace`s, but it's unclear how important that is.

Still happy to consider FileDistance-approach as I might be missing something.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131