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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/AST.cpp:77
+  for (const auto *Ctx = &DC; Ctx != nullptr; Ctx = Ctx->getParent())
+    if (const auto *NS = dyn_cast<NamespaceDecl>(Ctx))
+      if (!NS->isAnonymousNamespace() && !NS->isInlineNamespace())
----------------
It would be nice to have the invariant `splitQualifiedName(printQualifiedName(D)) == printScope(D.getContext())` for Decl D.

That's why I was suggesting splitting `NamedDecl::printQualifiedName`.

Given the limited functionality here it should probably be called printNamespaceScope, lest someone try to reuse it to get a symbol's scope.


================
Comment at: clangd/AST.h:51
 
+/// Returns the first enclosing namespace scope starting from \p DC.
+std::string printScope(const DeclContext &DC);
----------------
next to printQualifiedName?


================
Comment at: clangd/CodeComplete.cpp:562
     SpecifiedScope Info;
     for (auto *Context : CCContext.getVisitedContexts()) {
       if (isa<TranslationUnitDecl>(Context))
----------------
do you also want to use printScope here?


================
Comment at: clangd/Quality.cpp:250
+  SmallVector<StringRef, 4> Split;
+  Scope.split(Split, "::", /*MaxSplit=*/-1, /*KeppEmpty=*/false);
+  return {llvm::join(Split, "/"), Split.size()};
----------------
kepp -> keep


================
Comment at: clangd/Quality.cpp:251
+  Scope.split(Split, "::", /*MaxSplit=*/-1, /*KeppEmpty=*/false);
+  return {llvm::join(Split, "/"), Split.size()};
+}
----------------
might be clearer to explicitly prepend "/" rather than relying on normalization to do it?


================
Comment at: clangd/Quality.cpp:261
+  StringMap<SourceParams> Sources;
+  Opts.UpCost = 1;
+  Opts.DownCost = 1;
----------------
My intuition is that mostly the ranking should look like:
 1. results from queried namespaces
 2. results from nearby namespaces
 3. results from unrelated namespaces

That would require Source.Cost < (UpCost, DownCost), which isn't possible when they're 1.
WDYT?


================
Comment at: clangd/Quality.cpp:262
+  Opts.UpCost = 1;
+  Opts.DownCost = 1;
+  Opts.AllowDownTraversalFromRoot = false;
----------------
I thought we concluded down cost was likely to be significantly higher than up cost?


================
Comment at: clangd/Quality.cpp:267
+    SourceParams Param;
+    // Make sure cost of "" is not from traversals from other query scopes.
+    Param.MaxUpTraversals = std::max(Path.second - 1, 0);
----------------
This seems a little focused on the implementation rather than the intent...
maybe "The global namespace is not 'near' its children"?


================
Comment at: clangd/Quality.cpp:272
+    else if (!S.empty() && Preferred.startswith(S))
+      Param.Cost = Opts.UpCost;
+    else
----------------
This seems like an odd rule, what's the intuition?
 - if the preferred scope is a::b::c:: then we're considering a::b:: and a:: to be equal, rather than preferring the former
 - you're defining the cost in terms of UpCost, but it's not clear why - is this just being used as a scale?
 - for the direct parent of the preferred scope, this is a no-op. So this only does anything when the preferred scope is at least 3 deep

As a first approximation, `; // just rely on up-traversals` might be OK,
otherwise, I guess you're trying to boost these compared to just relying on up-traversals, so I'd expect this to look something like `Cost = UpCost * (len(preferred) - len(S)) / 2`


================
Comment at: clangd/Quality.cpp:274
+    else
+      Param.Cost = 2 * Opts.UpCost;
+    if (S == "")
----------------
Again, if this just means "parents of preferred are 1, others are 2", I'm not sure that sounds right.
A using-directive is explicit in the user's code, I'm not sure it should rank lower?

Happy enough to try this out as it is though, maybe just drop the references to UpCost.


================
Comment at: clangd/Quality.cpp:276
+    if (S == "")
+      Param.Cost += Opts.UpCost;
+    Sources[Path.first] = std::move(Param);
----------------
Why is this +=?

Seems like there's three cases here:
 - global is the preferred scope: I think the cost has to be 0 in this case, no?
 - global is another acceptable scope: some large fixed cost seems appropriate
 - global is not listed: no source to worry about


================
Comment at: clangd/Quality.cpp:287
+  auto D = Distance->distance(scopeToPath(Scope).first);
+  return std::max(0.6, MaxBoost - D * 0.25);
+}
----------------
0.6 seems too high for AnyScope matches, to me. And you're not distinguishing anyScope matches from bad path matches.

I think the linear scale is too flat: if you're in clang::clangd::, then clangd symbols will only get a 13% boost over clang ones.

Given the current UpCost == DownCost == 1, I'd consider something like
```
if (D == FileDistance::Unreachable)
  return 0.1;
return max(0.5, MaxBoost * pow(0.6, D))
```


================
Comment at: clangd/Quality.cpp:382
+    // always in the accessible scope.
+    Score *= SemaSaysInScope ? QueryScopeProximity::MaxBoost
+                             : scopeBoost(*ScopeProximityMatch, SymbolScope);
----------------
hmm, if you have no scopes in the query I think you're boosting the sema results and not the index ones. Intended?


================
Comment at: clangd/Quality.h:83
+/// Proximity between symbol scope and query scopes.
+class QueryScopeProximity {
+  public:
----------------
This is mechanism, not signals, and makes this header harder to understand.

Can you move the string-translation/wrapping part to FileDistance.h (returning Distance) and the boost part to Quality.cpp?


================
Comment at: clangd/Quality.h:84
+class QueryScopeProximity {
+  public:
+    constexpr static float MaxBoost = 2.0;
----------------
clang-format


================
Comment at: clangd/Quality.h:93
+  private:
+    llvm::Optional<FileDistance> Distance;
+    FileDistanceOptions Opts;
----------------
why is this optional? does FileDistance not work with an empty set of roots?
If the idea is to avoid downranking everything if there are no scopes, QueryScopeProximity itself is already optional.


================
Comment at: clangd/Quality.h:94
+    llvm::Optional<FileDistance> Distance;
+    FileDistanceOptions Opts;
+};
----------------
unused


================
Comment at: clangd/Quality.h:106
   URIDistance *FileProximityMatch = nullptr;
-  /// This is used to calculate proximity between the index symbol and the
+  /// These are used to calculate proximity between the index symbol and the
   /// query.
----------------
you've now got scope-proximity and file-proximity info interleaved. These are analogous signals, but separate, so probably clearest to group each one.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131