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

[tom-anders]

Summary

For example, in the following code

#include <string>

using namespace std::string_literals;

int main() {
    strin^ // Completes `string` instead of `std::string`
}

The using declaration would make completion drop the std namespace, even though it shouldn't.

printNamespaceScope() skips inline namespaces, so to fix this use
printQualifiedName() instead

See https://github.com/clangd/clangd/issues/1451

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

700

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.

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

700

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).

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

700

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?

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

700

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.

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

700

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).

[tom-anders]

Address review comment: Introduce new AccessibleScopes variable

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

700

Ok, think I got it - To make this a bit less confusing, I first renamed SpecifiedScope::AccessibleScopes to SpecifiedScope::QueryScopes (that's the number under which it is (and was) stored in CodeCompleteFlow anyway) and then added a back a new field AccessibleScopes, that keeps the inline namespaces. This is then stored in CodeCompleteFlow:AccessibleScopes and passed to the CodeCompletionBuilder instead of QueryScopes.

This now still passes all existing tests and I verified in my editor that it also fixes the original issue.

I couldn't figure out how to write a regression test for this now though :(

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

648

could you actually change the comments above to showcase an inline namespace and keep this as AccessibleScopes.

Then introduce QueryScopes with a comment saying that This is an overestimate of AccessibleScopes, e.g. it ignores inline namespaces, to fetch more relevant symbols from index.

656

`Scopes that are accessible from current context. Used for dropping unnecessary namespecifiers.

657

s/scopesForCodeCompletion/scopesForQualificaiton

692–693

can we have a dedicated struct for return type here? especially the "enclosing namespace first" logic seem to be getting out of control. so what about updating SpecificedScope to have two new fields std::optional<std::string> EnclosingNamespace; and a bool AllowAllScopes;? that way we can just call getQueryScopes/getAccessibleScopes on needed places and make sure they're putting the EnclosingNamespace (if it exists) in front.

700

I couldn't figure out how to write a regression test for this now though :(

We've got a bunch of tests in unittests/CodeCompleteTests.cpp, something like:

TEST(CompletionTest, QualificationWithInlineNamespace) {
  auto Results = completions(R"cpp(
    namespace a { inline namespace b {} }
    using namespace a::b;
    void f() { Foo^ }
  )cpp",
                             {cls("a::Foo")});
  EXPECT_THAT(Results.Completions,
              UnorderedElementsAre(AllOf(qualifier("a::"), named("Foo"))));
}

should fail without your patch, and pass afterwards.

722–723

note that this is actually going to skip inline namespaces (and you're using that in the returned set)

1665–1666

we should be setting AccessibleScopes too

[tom-anders]

Address review comments, add regression test

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

700

That works, thanks!

722–723

Hm I should probably fix this and add another regression test for this..?

1665–1666

Ah thanks, that's what caused the one failing test. I just copied over QueryScopes here for now, looks like this doesn't need any special handling for inline namespaces, does it?

1726

Should I add a reserve here for QueryScopes and AccessibleScopes? Would make this a bit more complicated for a small performance boost.

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

722–723

yeah, let's just leave a fixme.
i also think we should be setting it in all cases, i can't think of a reason for only setting it in a single case. i believe current scope should always get a boost whenever it's part of query scopes.

1665–1666

looks like this doesn't need any special handling for inline namespaces, does it?

well it probably does, but there isn't much we can do. as this code path happens with only heuristic parsing of the main-file code.

1721

i was actually suggesting to put this logic inside SpecifiedScope::scopesForIndexQuery any reason for only including it in this code path?

1731

AccessibleScopes doesn't need any particular ordering. we can use scopesForQualification as-is.

[tom-anders]

Move EnclosingNamespace logic to scopesForIndexQuery, add FIXME

clang-tools-extra/clangd/CodeComplete.cpp

1721

Ah I just misunderstood, moved the logic to scopesForIndexQuery now.

1731

Might as well just make it a std::set instead of std::vector?

[kadircet]

LGTM, thanks!

clang-tools-extra/clangd/CodeComplete.cpp

675

nit: you can drop the braces

1731

let's leave it as-is for now. i feel like this part can benefit from a bigger refactoring going forward and thinking about vectors is easier then having a bunch of references or sets.

[tom-anders]

Thanks for reviewing!

clang-tools-extra/clangd/CodeComplete.cpp

 // the candidate bundle is going to be returned.
 //
 // Many fields are the same for all candidates in a bundle (e.g. name), and are
 // computed from the first candidate, in the constructor.
 // Others vary per candidate, so add() must be called for remaining candidates.
 struct CodeCompletionBuilder {
   CodeCompletionBuilder(ASTContext *ASTCtx, const CompletionCandidate &C,
                         CodeCompletionString *SemaCCS,
-                        llvm::ArrayRef<std::string> QueryScopes,
+                        llvm::ArrayRef<std::string> AccessibleScopes,
                         const IncludeInserter &Includes,
                         llvm::StringRef FileName,
                         CodeCompletionContext::Kind ContextKind,
                         const CodeCompleteOptions &Opts,
                         bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
       : ASTCtx(ASTCtx),
         EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
         IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
if (C.IndexResult) {
       if (Completion.Name.empty())
         Completion.Name = std::string(C.IndexResult->Name);
       if (Completion.FilterText.empty())
         Completion.FilterText = Completion.Name;
       // If the completion was visible to Sema, no qualifier is needed. This
       // avoids unneeded qualifiers in cases like with `using ns::X`.
       if (Completion.RequiredQualifier.empty() && !C.SemaResult) {
         llvm::StringRef ShortestQualifier = C.IndexResult->Scope;
-        for (llvm::StringRef Scope : QueryScopes) {
+        for (llvm::StringRef Scope : AccessibleScopes) {
           llvm::StringRef Qualifier = C.IndexResult->Scope;
           if (Qualifier.consume_front(Scope) &&
               Qualifier.size() < ShortestQualifier.size())
             ShortestQualifier = Qualifier;
         }
         Completion.RequiredQualifier = std::string(ShortestQualifier);
       }
     }
struct SpecifiedScope {
   //
   //   "::vec"                                      => {""}
   //   "using namespace std; ::vec^"                => {"", "std::"}
   //   "namespace ns {using namespace std;} ns::^"  => {"ns::", "std::"}
   //   "std::vec^"                                  => {""}  // "std" unresolved
   //
   // Examples of unqualified completion:
   //
-  //   "vec^"                                       => {""}
-  //   "using namespace std; vec^"                  => {"", "std::"}
-  //   "using namespace std; namespace ns { vec^ }" => {"ns::", "std::", ""}
+  //   "vec^"                                        => {""}
+  //   "using namespace std; vec^"                   => {"", "std::"}
+  //   "namespace ns {inline namespace ni { struct Foo {}}}
+  //    using namespace ns::ni; Fo^ "                => {"", "ns::ni::"}
+  //   "using namespace std; namespace ns { vec^ }"  => {"ns::", "std::", ""}
   //
   // "" for global namespace, "ns::" for normal namespace.
   std::vector<std::string> AccessibleScopes;
+  // This is an overestimate of AccessibleScopes, e.g. it ignores inline
+  // namespaces, to fetch more relevant symbols from index.
+  std::vector<std::string> QueryScopes;
   // The full scope qualifier as typed by the user (without the leading "::").
   // Set if the qualifier is not fully resolved by Sema.
   llvm::Optional<std::string> UnresolvedQualifier;
 
-  // Construct scopes being queried in indexes. The results are deduplicated.
-  // This method format the scopes to match the index request representation.
-  std::vector<std::string> scopesForIndexQuery() {
+  std::optional<std::string> EnclosingNamespace;
+
+  bool AllowAllScopes = false;
+
+  // Scopes that are accessible from current context. Used for dropping
+  // unnecessary namespecifiers.
+  std::vector<std::string> scopesForQualification() {
     std::set<std::string> Results;
     for (llvm::StringRef AS : AccessibleScopes)
       Results.insert(
           (AS + (UnresolvedQualifier ? *UnresolvedQualifier : "")).str());
     return {Results.begin(), Results.end()};
   }
+
+  // Construct scopes being queried in indexes. The results are deduplicated.
+  // This method formats the scopes to match the index request representation.
+  std::vector<std::string> scopesForIndexQuery() {
+    // The enclosing namespace must be first, it gets a quality boost.
+    std::vector<std::string> EnclosingAtFront;
+    if (EnclosingNamespace.has_value()) {
+      EnclosingAtFront.push_back(*EnclosingNamespace);
+    }
+    std::set<std::string> Deduplicated;
+    for (llvm::StringRef S : QueryScopes)
+      if (S != EnclosingNamespace)
+        Deduplicated.insert((S + UnresolvedQualifier.value_or("")).str());
+
+    EnclosingAtFront.reserve(EnclosingAtFront.size() + Deduplicated.size());
+    llvm::copy(Deduplicated, std::back_inserter(EnclosingAtFront));
+
+    return EnclosingAtFront;
+  }
 };
 
 // Get all scopes that will be queried in indexes and whether symbols from
 // any scope is allowed. The first scope in the list is the preferred scope
 // (e.g. enclosing namespace).
-std::pair<std::vector<std::string>, bool>
-getQueryScopes(CodeCompletionContext &CCContext, const Sema &CCSema,
+SpecifiedScope getQueryScopes(CodeCompletionContext &CCContext,
+                              const Sema &CCSema,
                               const CompletionPrefix &HeuristicPrefix,
                               const CodeCompleteOptions &Opts) {
   SpecifiedScope Scopes;
   for (auto *Context : CCContext.getVisitedContexts()) {
-    if (isa<TranslationUnitDecl>(Context))
-      Scopes.AccessibleScopes.push_back(""); // global namespace
-    else if (isa<NamespaceDecl>(Context))
-      Scopes.AccessibleScopes.push_back(printNamespaceScope(*Context));
+    if (isa<TranslationUnitDecl>(Context)) {
+      Scopes.QueryScopes.push_back("");
+      Scopes.AccessibleScopes.push_back("");
+    } else if (const auto *ND = dyn_cast<NamespaceDecl>(Context)) {
+      Scopes.QueryScopes.push_back(printNamespaceScope(*Context));
+      Scopes.AccessibleScopes.push_back(printQualifiedName(*ND) + "::");
+    }
   }
 
   const CXXScopeSpec *SemaSpecifier =
       CCContext.getCXXScopeSpecifier().value_or(nullptr);
   // Case 1: unqualified completion.
   if (!SemaSpecifier) {
     // Case 2 (exception): sema saw no qualifier, but there appears to be one!
     // This can happen e.g. in incomplete macro expansions. Use heuristics.
     if (!HeuristicPrefix.Qualifier.empty()) {
       vlog("Sema said no scope specifier, but we saw {0} in the source code",
            HeuristicPrefix.Qualifier);
       StringRef SpelledSpecifier = HeuristicPrefix.Qualifier;
-      if (SpelledSpecifier.consume_front("::"))
+      if (SpelledSpecifier.consume_front("::")) {
         Scopes.AccessibleScopes = {""};
-      Scopes.UnresolvedQualifier = std::string(SpelledSpecifier);
-      return {Scopes.scopesForIndexQuery(), false};
+        Scopes.QueryScopes = {""};
       }
-    // The enclosing namespace must be first, it gets a quality boost.
-    std::vector<std::string> EnclosingAtFront;
-    std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
-    EnclosingAtFront.push_back(EnclosingScope);
-    for (auto &S : Scopes.scopesForIndexQuery()) {
-      if (EnclosingScope != S)
-        EnclosingAtFront.push_back(std::move(S));
+      Scopes.UnresolvedQualifier = std::string(SpelledSpecifier);
+      return Scopes;
     }
+    /// FIXME: When the enclosing namespace contains an inline namespace,
+    /// it's dropped here. This leads to a behavior similar to
+    /// https://github.com/clangd/clangd/issues/1451
+    Scopes.EnclosingNamespace = printNamespaceScope(*CCSema.CurContext);
     // Allow AllScopes completion as there is no explicit scope qualifier.
-    return {EnclosingAtFront, Opts.AllScopes};
+    Scopes.AllowAllScopes = Opts.AllScopes;
+    return Scopes;
   }
   // Case 3: sema saw and resolved a scope qualifier.
   if (SemaSpecifier && SemaSpecifier->isValid())
-    return {Scopes.scopesForIndexQuery(), false};
+    return Scopes;
 
   // Case 4: There was a qualifier, and Sema didn't resolve it.
-  Scopes.AccessibleScopes.push_back(""); // Make sure global scope is included.
+  Scopes.QueryScopes.push_back(""); // Make sure global scope is included.
   llvm::StringRef SpelledSpecifier = Lexer::getSourceText(
       CharSourceRange::getCharRange(SemaSpecifier->getRange()),
       CCSema.SourceMgr, clang::LangOptions());
-  if (SpelledSpecifier.consume_front("::"))
-    Scopes.AccessibleScopes = {""};
+  if (SpelledSpecifier.consume_front("::")) 
+      Scopes.QueryScopes = {""};
   Scopes.UnresolvedQualifier = std::string(SpelledSpecifier);
   // Sema excludes the trailing "::".
   if (!Scopes.UnresolvedQualifier->empty())
     *Scopes.UnresolvedQualifier += "::";
 
-  return {Scopes.scopesForIndexQuery(), false};
+  Scopes.AccessibleScopes = Scopes.QueryScopes;
+
+  return Scopes;
 }
 
 // Should we perform index-based completion in a context of the specified kind?
 // FIXME: consider allowing completion, but restricting the result types.
 bool contextAllowsIndex(enum CodeCompletionContext::Kind K) {
   switch (K) {
   case CodeCompletionContext::CCC_TopLevel:
   case CodeCompletionContext::CCC_ObjCInterface:
class CodeCompleteFlow {
   tok::TokenKind NextTokenKind = tok::eof;
   // Counters for logging.
   int NSema = 0, NIndex = 0, NSemaAndIndex = 0, NIdent = 0;
   bool Incomplete = false; // Would more be available with a higher limit?
   CompletionPrefix HeuristicPrefix;
   llvm::Optional<FuzzyMatcher> Filter; // Initialized once Sema runs.
   Range ReplacedRange;
   std::vector<std::string> QueryScopes; // Initialized once Sema runs.
+  std::vector<std::string> AccessibleScopes; // Initialized once Sema runs.
   // Initialized once QueryScopes is initialized, if there are scopes.
   llvm::Optional<ScopeDistance> ScopeProximity;
   llvm::Optional<OpaqueType> PreferredType; // Initialized once Sema runs.
   // Whether to query symbols from any scope. Initialized once Sema runs.
   bool AllScopes = false;
   llvm::StringSet<> ContextWords;
   // Include-insertion and proximity scoring rely on the include structure.
   // This is available after Sema has run.
for (const auto &IDAndCount : Identifiers) {
       if (ID.References > 0)
         IdentifierResults.push_back(std::move(ID));
     }
 
     // Simplified version of getQueryScopes():
     //  - accessible scopes are determined heuristically.
     //  - all-scopes query if no qualifier was typed (and it's allowed).
     SpecifiedScope Scopes;
-    Scopes.AccessibleScopes = visibleNamespaces(
+    Scopes.QueryScopes = visibleNamespaces(
         Content.take_front(Offset), format::getFormattingLangOpts(Style));
-    for (std::string &S : Scopes.AccessibleScopes)
+    for (std::string &S : Scopes.QueryScopes)
       if (!S.empty())
         S.append("::"); // visibleNamespaces doesn't include trailing ::.
     if (HeuristicPrefix.Qualifier.empty())
       AllScopes = Opts.AllScopes;
     else if (HeuristicPrefix.Qualifier.startswith("::")) {
-      Scopes.AccessibleScopes = {""};
+      Scopes.QueryScopes = {""};
       Scopes.UnresolvedQualifier =
           std::string(HeuristicPrefix.Qualifier.drop_front(2));
     } else
       Scopes.UnresolvedQualifier = std::string(HeuristicPrefix.Qualifier);
     // First scope is the (modified) enclosing scope.
     QueryScopes = Scopes.scopesForIndexQuery();
+    AccessibleScopes = QueryScopes;
     ScopeProximity.emplace(QueryScopes);
 
     SymbolSlab IndexResults = Opts.Index ? queryIndex() : SymbolSlab();
 
     CodeCompleteResult Output = toCodeCompleteResult(mergeResults(
         /*SemaResults=*/{}, IndexResults, IdentifierResults));
     Output.RanParser = false;
     logResults(Output, Tracer);
CodeCompleteResult runWithSema() {
     } else {
       const auto &Pos = sourceLocToPosition(
           Recorder->CCSema->getSourceManager(),
           Recorder->CCSema->getPreprocessor().getCodeCompletionLoc());
       ReplacedRange.start = ReplacedRange.end = Pos;
     }
     Filter = FuzzyMatcher(
         Recorder->CCSema->getPreprocessor().getCodeCompletionFilter());
-    std::tie(QueryScopes, AllScopes) = getQueryScopes(
+    auto SpecifiedScopes = getQueryScopes(
         Recorder->CCContext, *Recorder->CCSema, HeuristicPrefix, Opts);
+
+    QueryScopes = SpecifiedScopes.scopesForIndexQuery();
+    AccessibleScopes = SpecifiedScopes.scopesForQualification();
+    AllScopes = SpecifiedScopes.AllowAllScopes;
     if (!QueryScopes.empty())
       ScopeProximity.emplace(QueryScopes);
     PreferredType =
         OpaqueType::fromType(Recorder->CCSema->getASTContext(),
                              Recorder->CCContext.getPreferredType());
     // Sema provides the needed context to query the index.
     // FIXME: in addition to querying for extra/overlapping symbols, we should
     //        explicitly request symbols corresponding to Sema results.
     //        We can use their signals even if the index can't suggest them.
     // We must copy index results to preserve them, but there are at most Limit.
     auto IndexResults = (Opts.Index && allowIndex(Recorder->CCContext))
                             ? queryIndex()
                             : SymbolSlab();
     trace::Span Tracer("Populate CodeCompleteResult");
     // Merge Sema and Index results, score them, and pick the winners.
     auto Top =
private:
   CodeCompletion toCodeCompletion(const CompletionCandidate::Bundle &Bundle) {
     llvm::Optional<CodeCompletionBuilder> Builder;
     for (const auto &Item : Bundle) {
       CodeCompletionString *SemaCCS =
           Item.SemaResult ? Recorder->codeCompletionString(*Item.SemaResult)
                           : nullptr;
       if (!Builder)
         Builder.emplace(Recorder ? &Recorder->CCSema->getASTContext() : nullptr,
-                        Item, SemaCCS, QueryScopes, *Inserter, FileName,
+                        Item, SemaCCS, AccessibleScopes, *Inserter, FileName,
                         CCContextKind, Opts, IsUsingDeclaration, NextTokenKind);
       else
         Builder->add(Item, SemaCCS);
     }
     return Builder->build();
   }
 };
 

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

TEST(CompletionTest, Qualifiers) {
   // Hidden members are not shown.
   EXPECT_THAT(Results.Completions,
               Not(Contains(AllOf(qualifier("Foo::"), named("foo")))));
   // Private members are not shown.
   EXPECT_THAT(Results.Completions,
               Not(Contains(AllOf(qualifier(""), named("foo")))));
 }
 
+// https://github.com/clangd/clangd/issues/1451
+TEST(CompletionTest, QualificationWithInlineNamespace) {
+  auto Results = completions(R"cpp(
+    namespace a { inline namespace b {} }
+    using namespace a::b;
+    void f() { Foo^ }
+  )cpp",
+                             {cls("a::Foo")});
+  EXPECT_THAT(Results.Completions,
+              UnorderedElementsAre(AllOf(qualifier("a::"), named("Foo"))));
+}
+
 TEST(CompletionTest, InjectedTypename) {
   // These are suppressed when accessed as a member...
   EXPECT_THAT(completions("struct X{}; void foo(){ X().^ }").Completions,
               Not(has("X")));
   EXPECT_THAT(completions("struct X{ void foo(){ this->^ } };").Completions,
               Not(has("X")));
   // ...but accessible in other, more useful cases.
   EXPECT_THAT(completions("struct X{ void foo(){ ^ } };").Completions,