[PATCH] D140915: [clangd] Fix getQueryScopes for using-directive with inline namespace

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:674
+    else if (const auto *ND = dyn_cast<NamespaceDecl>(Context)) {
+      if (ND->isInlineNamespace())
+        Scopes.AccessibleScopes.push_back(printQualifiedName(*ND) + "::");
----------------
tom-anders wrote:
> kadircet wrote:
> > since we know that the `Context` is a `NamespaceDecl` it should be safe to use `printQualifiedName` always. any reason for the extra branching here (apart from minimizing the change to behaviour)? if not I think we can get rid of the special casing.
> Unfortunately, this fails CompletionTest.EnclosingScopeComesFirst and CompletionTest.NoDuplicatedQueryScopes, I think because of anonymous namespaces (where printNameSpaceScope would return an empty string, but (printQualifiedName(*ND) + "::" does not). 
i see. taking a closer look at this `getQueryScopes` is used for two things:
- The scopes to query with fuzzyfind requests, hence this should use the same "serialization" as symbolcollector (which only strips anon namespaces today, but initially it were to strip both anon & inline namespaces. it got changed inside clang without clangd tests catching it).
- The shortening of the fully qualified name in `CodeCompletionBuilder`. Not having inline namespaces spelled in the available namespaces implies getting wrong qualifiers (such as the bug you're fixing).

so considering the requirements here:
- when querying index, we actually want to hide inline namespaces (as `ns::hidden::Foo` should be a viable alternative even if only `ns::` is accessible). so we should actually fix `printQualifiedName` to set `SuppressInlineNamespace` in printing policy to restore the old behaviour (and keep using `printNamespaceScope` here).
- inside `CodeCompletionBuilder`, we shouldn't use the same scopes we use during index queries. we should use the visible namespaces while preserving inline namespace information and only ignoring the anonymous namespaces.

hence can we have 2 separate scopes in `CodeCompleteFlow` instead?
One called `QueryScopes`, which has the behavior we have today (fixing printQualifiedName is a separate issues).
Other called `AccessibleScopes`, which has accessible namespaces spelled **with** inline namespaces, so that we can get proper qualification during code-complete.

does that make sense?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140915/new/

https://reviews.llvm.org/D140915