[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:
> > 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.
sorry for the long and confusing answer :D

I was talking about `CodeCompleteFlow` class specifically, inside `CodeComplete.cpp`. Currently it only has `QueryScopes`, derived from the visible contexts reported by Sema. Unfortunately it loses some granularity to fetch more symbols from index hence it should not be used when picking the required qualifier.
My suggestion is to add a new field in `CodeCompleteFlow` called `AccessibleScope`, which is derived at the same stage as `QueryScopes`, while preserving inline namespaces, and used when creating `CodeCompletionBuilder` in `CodeCompleteFlow::toCodeCompletion` (instead of `QueryScopes`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140915