[clangd] Don't insert extra namespace qualifiers when Sema gets lost.

[sammccall]

Summary

There are cases where Sema can't tell that "foo" in foo::Bar is a
namespace qualifier, like in incomplete macro expansions.

After this patch, if sema reports no specifier but it looks like there's one in
the source code, then we take it into account.

Reworked structure and comments in getQueryScopes to try to fight
creeping complexity - unsure if I succeeded.

I made the test harder (the original version also passes).

[sammccall]

Update comment

[sammccall]

clangd/CodeComplete.cpp

545 ↗

(On Diff #194486)

(AFAICT this lambda was executed the same way on every code path, so I just inlined it)

[ioeric]

Feel free to land this. I'll rebase D60126 on this when it's in.

clangd/CodeComplete.cpp

580 ↗

(On Diff #194486)

nit: Sema Specifier can't be nullptr at this point.

clang-tools-extra/trunk/clangd/CodeComplete.cpp

 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Format.h"
struct SpecifiedScope {
   }
 };
 
 // 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,
+               const CompletionPrefix &HeuristicPrefix,
                const CodeCompleteOptions &Opts) {
-  auto GetAllAccessibleScopes = [](CodeCompletionContext &CCContext) {
-    SpecifiedScope Info;
-    for (auto *Context : CCContext.getVisitedContexts()) {
-      if (isa<TranslationUnitDecl>(Context))
-        Info.AccessibleScopes.push_back(""); // global namespace
-      else if (isa<NamespaceDecl>(Context))
-        Info.AccessibleScopes.push_back(printNamespaceScope(*Context));
-    }
-    return Info;
-  };
-
-  auto SS = CCContext.getCXXScopeSpecifier();
+  SpecifiedScope Scopes;
+      Scopes.AccessibleScopes.push_back(""); // global namespace
+      Scopes.AccessibleScopes.push_back(printNamespaceScope(*Context));
 
-  // Unqualified completion (e.g. "vec^").
-  if (!SS) {
-    std::vector<std::string> Scopes;
+  const CXXScopeSpec *SemaSpecifier =
+      CCContext.getCXXScopeSpecifier().getValueOr(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("::"))
+        Scopes.AccessibleScopes = {""};
+      Scopes.UnresolvedQualifier = SpelledSpecifier;
+      return {Scopes.scopesForIndexQuery(), false};
+    }
+    // The enclosing namespace must be first, it gets a quality boost.
+    std::vector<std::string> EnclosingAtFront;
     std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
-    Scopes.push_back(EnclosingScope);
-    for (auto &S : GetAllAccessibleScopes(CCContext).scopesForIndexQuery()) {
+    EnclosingAtFront.push_back(EnclosingScope);
+    for (auto &S : Scopes.scopesForIndexQuery()) {
       if (EnclosingScope != S)
-        Scopes.push_back(std::move(S));
-    }
-    // Allow AllScopes completion only for there is no explicit scope qualifier.
-    return {Scopes, Opts.AllScopes};
+        EnclosingAtFront.push_back(std::move(S));
     }
-
-  // Qualified completion ("std::vec^"), we have two cases depending on whether
-  // the qualifier can be resolved by Sema.
-  if ((*SS)->isValid()) { // Resolved qualifier.
-    return {GetAllAccessibleScopes(CCContext).scopesForIndexQuery(), false};
+    // Allow AllScopes completion as there is no explicit scope qualifier.
+    return {EnclosingAtFront, Opts.AllScopes};
   }
-
-  // Unresolved qualifier.
-  SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
-  Info.AccessibleScopes.push_back(""); // Make sure global scope is included.
-
-  llvm::StringRef SpelledSpecifier =
-      Lexer::getSourceText(CharSourceRange::getCharRange((*SS)->getRange()),
-                           CCSema.SourceMgr, clang::LangOptions());
+  // Case 3: sema saw and resolved a scope qualifier.
+  if (Specifier && SemaSpecifier->isValid())
+    return {Scopes.scopesForIndexQuery(), false};
+
+  // Case 4: There was a qualifier, and Sema didn't resolve it.
+  Scopes.AccessibleScopes.push_back(""); // Make sure global scope is included.
+  llvm::StringRef SpelledSpecifier = Lexer::getSourceText(
+      CharSourceRange::getCharRange(SemaSpecifier->getRange()),
   if (SpelledSpecifier.consume_front("::"))
-    Info.AccessibleScopes = {""};
-  Info.UnresolvedQualifier = SpelledSpecifier;
+    Scopes.AccessibleScopes = {""};
+  Scopes.UnresolvedQualifier = SpelledSpecifier;
   // Sema excludes the trailing "::".
-  if (!Info.UnresolvedQualifier->empty())
-    *Info.UnresolvedQualifier += "::";
+  if (!Scopes.UnresolvedQualifier->empty())
+    *Scopes.UnresolvedQualifier += "::";
 
-  return {Info.scopesForIndexQuery(), false};
+  return {Scopes.scopesForIndexQuery(), false};
 }
 
 // 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:
private:
   const SymbolIndex *Index;
 }; // SignatureHelpCollector
 
 struct SemaCompleteInput {
   PathRef FileName;
   const tooling::CompileCommand &Command;
   const PreambleData *Preamble;
   llvm::StringRef Contents;
-  Position Pos;
+  size_t Offset;
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS;
 };
 
 // Invokes Sema code completion on a file.
 // If \p Includes is set, it will be updated based on the compiler invocation.
 bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
                       const clang::CodeCompleteOptions &Options,
                       const SemaCompleteInput &Input,
bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
   }
   auto &FrontendOpts = CI->getFrontendOpts();
   FrontendOpts.SkipFunctionBodies = true;
   // Disable typo correction in Sema.
   CI->getLangOpts()->SpellChecking = false;
   // Setup code completion.
   FrontendOpts.CodeCompleteOpts = Options;
   FrontendOpts.CodeCompletionAt.FileName = Input.FileName;
-  auto Offset = positionToOffset(Input.Contents, Input.Pos);
-  if (!Offset) {
-    elog("Code completion position was invalid {0}", Offset.takeError());
-    return false;
-  }
   std::tie(FrontendOpts.CodeCompletionAt.Line,
            FrontendOpts.CodeCompletionAt.Column) =
-      offsetToClangLineColumn(Input.Contents, *Offset);
+      offsetToClangLineColumn(Input.Contents, Input.Offset);
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Input.Contents, Input.FileName);
   // The diagnostic options must be set before creating a CompilerInstance.
   CI->getDiagnosticOpts().IgnoreWarnings = true;
   // We reuse the preamble whether it's valid or not. This is a
   // correctness/performance tradeoff: building without a preamble is slow, and
   // completion is latency-sensitive.
   // However, if we're completing *inside* the preamble section of the draft,
   // overriding the preamble will break sema completion. Fortunately we can just
   // skip all includes in this case; these completions are really simple.
   bool CompletingInPreamble =
       ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0).Size >
-      *Offset;
+      Input.Offset;
   // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise
   // the remapped buffers do not get freed.
   IgnoreDiagnostics DummyDiagsConsumer;
   auto Clang = prepareCompilerInstance(
       std::move(CI),
       (Input.Preamble && !CompletingInPreamble) ? &Input.Preamble->Preamble
                                                 : nullptr,
       std::move(ContentsBuffer), std::move(VFS), DummyDiagsConsumer);
return runAsync<SymbolSlab>([&Index, Req]() {
     Index.fuzzyFind(Req, [&Syms](const Symbol &Sym) { Syms.insert(Sym); });
     return std::move(Syms).build();
   });
 }
 
 // Creates a `FuzzyFindRequest` based on the cached index request from the
 // last completion, if any, and the speculated completion filter text in the
 // source code.
-llvm::Optional<FuzzyFindRequest>
-speculativeFuzzyFindRequestForCompletion(FuzzyFindRequest CachedReq,
-                                         PathRef File, llvm::StringRef Content,
-                                         Position Pos) {
-  auto Offset = positionToOffset(Content, Pos);
-  if (!Offset) {
-    elog("No speculative filter: bad offset {0} in {1}", Pos, File);
-    return llvm::None;
-  }
-  CachedReq.Query = guessCompletionPrefix(Content, *Offset).Name;
+FuzzyFindRequest speculativeFuzzyFindRequestForCompletion(
+    FuzzyFindRequest CachedReq, const CompletionPrefix &HeuristicPrefix) {
+  CachedReq.Query = HeuristicPrefix.Name;
   return CachedReq;
 }
 
 // Runs Sema-based (AST) and Index-based completion, returns merged results.
 //
 // There are a few tricky considerations:
 //   - the AST provides information needed for the index query (e.g. which
 //     namespaces to search in). So Sema must start first.
class CodeCompleteFlow {
   IncludeStructure Includes;           // Complete once the compiler runs.
   SpeculativeFuzzyFind *SpecFuzzyFind; // Can be nullptr.
   const CodeCompleteOptions &Opts;
 
   // Sema takes ownership of Recorder. Recorder is valid until Sema cleanup.
   CompletionRecorder *Recorder = nullptr;
   int NSema = 0, NIndex = 0, NBoth = 0; // Counters for logging.
   bool Incomplete = false;       // Would more be available with a higher limit?
+  CompletionPrefix HeuristicPrefix;
   llvm::Optional<FuzzyMatcher> Filter;  // Initialized once Sema runs.
   std::vector<std::string> QueryScopes; // 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;
   // Include-insertion and proximity scoring rely on the include structure.
public:
   CodeCompleteFlow(PathRef FileName, const IncludeStructure &Includes,
                    SpeculativeFuzzyFind *SpecFuzzyFind,
                    const CodeCompleteOptions &Opts)
       : FileName(FileName), Includes(Includes), SpecFuzzyFind(SpecFuzzyFind),
         Opts(Opts) {}
 
   CodeCompleteResult run(const SemaCompleteInput &SemaCCInput) && {
     trace::Span Tracer("CodeCompleteFlow");
+    HeuristicPrefix =
+        guessCompletionPrefix(SemaCCInput.Contents, SemaCCInput.Offset);
     if (Opts.Index && SpecFuzzyFind && SpecFuzzyFind->CachedReq.hasValue()) {
       assert(!SpecFuzzyFind->Result.valid());
-      if ((SpecReq = speculativeFuzzyFindRequestForCompletion(
-               *SpecFuzzyFind->CachedReq, SemaCCInput.FileName,
-               SemaCCInput.Contents, SemaCCInput.Pos)))
-        SpecFuzzyFind->Result = startAsyncFuzzyFind(*Opts.Index, *SpecReq);
+      SpecReq = speculativeFuzzyFindRequestForCompletion(
+          *SpecFuzzyFind->CachedReq, HeuristicPrefix);
     }
 
     // We run Sema code completion first. It builds an AST and calculates:
     //   - completion results based on the AST.
     //   - partial identifier and context. We need these for the index query.
     CodeCompleteResult Output;
     auto RecorderOwner = llvm::make_unique<CompletionRecorder>(Opts, [&]() {
       assert(Recorder && "Recorder is not set");
CodeCompleteResult runWithSema() {
     } else {
       const auto &Pos = sourceLocToPosition(
           Recorder->CCSema->getSourceManager(),
           Recorder->CCSema->getPreprocessor().getCodeCompletionLoc());
       TextEditRange.start = TextEditRange.end = Pos;
     }
     Filter = FuzzyMatcher(
         Recorder->CCSema->getPreprocessor().getCodeCompletionFilter());
-    std::tie(QueryScopes, AllScopes) =
-        getQueryScopes(Recorder->CCContext, *Recorder->CCSema, Opts);
+    std::tie(QueryScopes, AllScopes) = getQueryScopes(
+        Recorder->CCContext, *Recorder->CCSema, HeuristicPrefix, Opts);
     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.
guessCompletionPrefix(llvm::StringRef Content, unsigned Offset) {
   return Result;
 }
 
 CodeCompleteResult
 codeComplete(PathRef FileName, const tooling::CompileCommand &Command,
              const PreambleData *Preamble, llvm::StringRef Contents,
              Position Pos, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
              CodeCompleteOptions Opts, SpeculativeFuzzyFind *SpecFuzzyFind) {
+  auto Offset = positionToOffset(Contents, Pos);
+  if (!Offset) {
+    elog("Code completion position was invalid {0}", Offset.takeError());
+    return CodeCompleteResult();
+  }
   return CodeCompleteFlow(FileName,
                           Preamble ? Preamble->Includes : IncludeStructure(),
                           SpecFuzzyFind, Opts)
-      .run({FileName, Command, Preamble, Contents, Pos, VFS});
+      .run({FileName, Command, Preamble, Contents, *Offset, VFS});
 }
 
 SignatureHelp signatureHelp(PathRef FileName,
                             const tooling::CompileCommand &Command,
                             const PreambleData *Preamble,
                             llvm::StringRef Contents, Position Pos,
                             llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
                             const SymbolIndex *Index) {
+  auto Offset = positionToOffset(Contents, Pos);
+  if (!Offset) {
+    elog("Code completion position was invalid {0}", Offset.takeError());
+    return SignatureHelp();
+  }
   SignatureHelp Result;
   clang::CodeCompleteOptions Options;
   Options.IncludeGlobals = false;
   Options.IncludeMacros = false;
   Options.IncludeCodePatterns = false;
   Options.IncludeBriefComments = false;
   IncludeStructure PreambleInclusions; // Unused for signatureHelp
   semaCodeComplete(
       llvm::make_unique<SignatureHelpCollector>(Options, Index, Result),
-      Options, {FileName, Command, Preamble, Contents, Pos, std::move(VFS)});
+      Options,
+      {FileName, Command, Preamble, Contents, *Offset, std::move(VFS)});
   return Result;
 }
 
 bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx) {
   auto InTopLevelScope = [](const NamedDecl &ND) {
     switch (ND.getDeclContext()->getDeclKind()) {
     case Decl::TranslationUnit:
     case Decl::Namespace:

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

auto Results = completions(R"cpp(
     void f() { b::c::X^ }
     }
   )cpp",
                              {cls("a::b::c::XYZ")}, Opts);
   EXPECT_THAT(Results.Completions,
               UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ"))));
 }
 
-// Regression test: clang parser gets confused here and doesn't report the ns::
-// prefix - naive behavior is to insert it again.
-// However we can recognize this from the source code.
-// Test disabled until we can make it pass.
-TEST(CompletionTest, DISABLED_NamespaceDoubleInsertion) {
+// Clang parser gets confused here and doesn't report the ns:: prefix.
+// Naive behavior is to insert it again. We examine the source and recover.
+TEST(CompletionTest, NamespaceDoubleInsertion) {
   clangd::CodeCompleteOptions Opts = {};
 
   auto Results = completions(R"cpp(
+    namespace foo {
     namespace ns {}
     #define M(X) < X
     M(ns::ABC^
+    }
   )cpp",
-                             {cls("ns::ABCDE")}, Opts);
+                             {cls("foo::ns::ABCDE")}, Opts);
   EXPECT_THAT(Results.Completions,
               UnorderedElementsAre(AllOf(Qualifier(""), Named("ABCDE"))));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang