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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the suggestions! After taking a closer look at boosting factors for other signals, I think I was being too conservative about boosting preferred scopes and penalizing non-preferred ones. I have tuned the parameters to make these more aggressive now according to your suggestion.



================
Comment at: clangd/Quality.cpp:262
+  Opts.UpCost = 1;
+  Opts.DownCost = 1;
+  Opts.AllowDownTraversalFromRoot = false;
----------------
sammccall wrote:
> I thought we concluded down cost was likely to be significantly higher than up cost?
Sorry, forgot to make that change. Made UpCost=1, DownCost=2


================
Comment at: clangd/Quality.cpp:272
+    else if (!S.empty() && Preferred.startswith(S))
+      Param.Cost = Opts.UpCost;
+    else
----------------
sammccall wrote:
> 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`
> As a first approximation, ; // just rely on up-traversals might be OK,
This sounds good to me.


================
Comment at: clangd/Quality.cpp:276
+    if (S == "")
+      Param.Cost += Opts.UpCost;
+    Sources[Path.first] = std::move(Param);
----------------
sammccall wrote:
> 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
> global is the preferred scope: I think the cost has to be 0 in this case, no?
I think we should still apply some penalty here because
  1. All projects can define symbols in the global scope. For example, if I work on a project without namespace, symbols from global namespaces are not necessarily relevant.
  2. A use pattern is that using-namespace is used in place of enclosing namespaces in implementation files. 

Otherwise, done.


================
Comment at: clangd/Quality.cpp:287
+  auto D = Distance->distance(scopeToPath(Scope).first);
+  return std::max(0.6, MaxBoost - D * 0.25);
+}
----------------
sammccall wrote:
> 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))
> ```
Done except for the scope for unreachable paths.

I think 0.1 is too much penalty and would probably make cross-namespace completions not useful. As cross-namespace completion is not enabled by default, it should be safe to give it a higher score (0.4?). 


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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131