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

[Tom Praschan via Phabricator via cfe-commits]

tom-anders 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) + "::");
----------------
kadircet wrote:
> 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?
tbh I'm a bit confused - I understand your requirements, but am not sure I understand your proposed solution. Can you expand a bit further? Looking at the code, there are already both `QueryScopes` and `AccessibleScopes` variables/fields in various classes, I'm not really sure at which places you want to make changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140915