[PATCH] D42073: [clangd] Use accessible scopes to query indexes for global code completion.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I think this mostly does the right thing, but I find some of the code structure/description confusing.
I might have made misguided comments, happy to sit down together and work this out :-)



================
Comment at: clangd/CodeComplete.cpp:316
+
+/// \brief Scopes being queried in indexes for the qualified-id code completion
+/// (e.g. "ns::ab^").
----------------
This comment is a bit confusing.
If this struct is trying to model "what we're going to  query the index for" then it's not needed - just use vector<string>.
If it's trying to model the context of the identifier we're completing, then this comment is inaccurate and should be something like `The namespace context of the partial identifier we're trying to complete.`
Then add something like `We use this when querying the index for more results.`


================
Comment at: clangd/CodeComplete.cpp:316
+
+/// \brief Scopes being queried in indexes for the qualified-id code completion
+/// (e.g. "ns::ab^").
----------------
sammccall wrote:
> This comment is a bit confusing.
> If this struct is trying to model "what we're going to  query the index for" then it's not needed - just use vector<string>.
> If it's trying to model the context of the identifier we're completing, then this comment is inaccurate and should be something like `The namespace context of the partial identifier we're trying to complete.`
> Then add something like `We use this when querying the index for more results.`
nit: avoid \brief where possible - the first sentence is used by default.


================
Comment at: clangd/CodeComplete.cpp:319
+//
+// FIXME: we might want to make Sema code completion smarter on handling
+// unresolved qualified-id completion.
----------------
it's not totally clear to me whether you're talking about *our* sema-based results, or the sema infrastructure we're using, or it's a mistake and you mean our index code completion.

Maybe just:
  // FIXME: When we resolve part of a scope chain (e.g. "known::unknown::ident"), we should
  // expand the known part rather than treating the whole thing as unknown.
I think this should go in the implementation, rather than the type comment.


================
Comment at: clangd/CodeComplete.cpp:327-328
+// way is to return an unresolved qualifier "ns1::ns2" with all scopes that are
+// accessible in "foo".
+struct QualifiedScopes {
+  // All scopes that are accessible in the completion scope qualifier.
----------------
Hmm, you're renamed this from `SpecifiedScope` to `QualifiedScope`. I don't understand this name, can you explain what it means?
(SpecifiedScope refers to the scope that the *user* specified when typing)


================
Comment at: clangd/CodeComplete.cpp:329
+struct QualifiedScopes {
+  // All scopes that are accessible in the completion scope qualifier.
+  //
----------------
This is a bit hard to parse, and doesn't seem to describe the "unresolved by sema" case in a meaningful way.

What about:
  The scopes we should look in, determined by Sema.
  If the qualifier was fully resolved, we should look for completions in these scopes.
  If there is an unresolved part of the qualifier, it should be resolved within these scopes.
That way we describe what we aim to do (which is pretty simple), and we can document deviations with FIXMEs.


================
Comment at: clangd/CodeComplete.cpp:334
+  //   scopes (namespace) in the resolved qualifier;
+  //   * unresolved by Sema, global namespace "::" is the only accessible scope.
+  //
----------------
for this case: why not "the containing namespace and all accessible namespaces"? You've implemented that below, and it seems like the right thing to do here.



================
Comment at: clangd/CodeComplete.cpp:340
+  //   "namespace ns {using namespace std;} ns::^" => {"ns", "std"}
+  //   "std::vec^" => {"::"}  // unresolved "std::"
+  std::vector<std::string> AccessibleScopes;
----------------
you seem to have a comment-in-a-comment here.
nit: please line up the => so it forms a table.

I'd like to see an example `"namespace ns { vec^ }" => {"ns", ""}
(I know it's not what the code currently does, but IMO it should!)


================
Comment at: clangd/CodeComplete.cpp:342
+  std::vector<std::string> AccessibleScopes;
+  // The scope qualifier that is not resolved in Sema, it is the user-written
+  // qualifiers.
----------------
This is ambiguous, does it mean `The suffix of the user-writter qualifiers that Sema didn't resolve` or `The full scope qualifier as typed by the user`?
(I'm hoping the former, but not sure).


================
Comment at: clangd/CodeComplete.cpp:346
+
+  std::vector<std::string> forIndex() {
+    std::vector<std::string> Results;
----------------
this needs a new name. Previously it described one scope, and was being formatted to match the index representation.
Now it contains logic to determine the index query, so maybe `scopesForIndexQuery()`?


================
Comment at: clangd/CodeComplete.cpp:348
+    std::vector<std::string> Results;
+    for (llvm::StringRef VS : AccessibleScopes) {
+      std::string QueryScope =
----------------
what is "VS"?


================
Comment at: clangd/CodeComplete.cpp:350
+      std::string QueryScope =
+          (normalizeScope(VS) +
+           (UnresolvedQualifier
----------------
It seems like you have stored the scopes in an unknown format, and call "normalizeScope" defensively? Seems cleaner if you ensure both the scopes and unknown are in the form:
  - global = ""
  - top-level = "::foo"
  - nested = "::foo::bar"

Then this code can produce similarly-formatted output with just:

  Results.push_back(VS);
  if (UnresolvedQualifier)
    Results.back() += *UnresolvedQualifier;

We need to trim leading `::` before sending to the index, but I think that's because we got the index API wrong. I'll send a patch to fix it.


================
Comment at: clangd/CodeComplete.cpp:845
     // If the user typed a scope, e.g. a::b::xxx(), restrict to that scope.
-    // FIXME(ioeric): add scopes based on using directives and enclosing ns.
-    if (auto SS = Recorder.CCContext.getCXXScopeSpecifier())
-      Req.Scopes = {getSpecifiedScope(*Recorder.CCSema, **SS).forIndex()};
-    else
-      // Unless the user typed a ns qualifier, complete in global scope only.
-      // FIXME: once we know what namespaces are in scope (D42073), use those.
+    if (auto SS = Recorder.CCContext.getCXXScopeSpecifier()) {
+      Req.Scopes =
----------------
This seems like too much fiddly logic inlined here.

Can't `getQualifiedScopes(CodeCompletionContext, Sema) -> QualifiedScopes` handle both cases here?


================
Comment at: clangd/CodeComplete.cpp:849
+              .forIndex();
+    } else {
+      // Unqualified code completion. Use the containing namespace and all
----------------
Please use a `QualifiedScopes` object to handle this case too.
It's simple: the unresolved part is "", and the list of accessible scopes is the same one you're computing here.


================
Comment at: clangd/CodeComplete.cpp:863
+        if (isa<TranslationUnitDecl>(Context))
+          Req.Scopes.push_back(normalizeScope("::")); // global namespace
+        else if (const auto*NS = dyn_cast<NamespaceDecl>(Context))
----------------
the global scope is spelled `""` in both conventions we use (the current one, and the one we should be using instead :-)).


================
Comment at: clangd/CodeComplete.cpp:868
+    }
+    log(Ctx, "Query scopes: [");
+    for (auto &R : Req.Scopes)
----------------
log doesn't work that way - you need to combine into a single message :-)

I think since we don't have verbosity levels, it's best to remove this - it's much more fine-grained than anything else we log, so it will be spammy.
(If we do want to log something, we should summarize the whole query.)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42073