[clangd] Refactor speculateCompletionFilter and also extract scope.

[sammccall]

Summary

Intent is to use the heuristically-parsed scope in cases where we get bogus
results from sema, such as in complex macro expansions.
Added a motivating testcase we currently get wrong.

Name changed because we (already) use this for things other than speculation.

[sammccall]

Handle :::: edge-case correctly.
Add test for leading :: case.

[sammccall]

Fix terrible variable name.

[ioeric]

lgtm

clangd/CodeComplete.cpp

1542

Suffix is actually a prefix of Content? This seems a bit confusing...

[sammccall]

clangd/CodeComplete.cpp

1542

Haha, just checking we're all paying attention...

clangd/CodeComplete.h

 //   * primary templates (no specializations)
 // For the other cases, we let Clang do the completion because it does not
 // need any non-local information and it will be much better at following
 // lookup rules. Other symbols still appear in the index for other purposes,
 // like workspace/symbols or textDocument/definition, but are not used for code
 // completion.
 bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx);
 
-/// Retrives a speculative code completion filter text before the cursor.
-/// Exposed for testing only.
-llvm::Expected<llvm::StringRef>
-speculateCompletionFilter(llvm::StringRef Content, Position Pos);
+// Text immediately before the completion point that should be completed.
+// This is heuristically derived from the source code, and is used when:
+//   - semantic analysis fails
+//   - semantic analysis may be slow, and we speculatively query the index
+struct CompletionPrefix {
+  // The unqualified partial name.
+  // If there is none, begin() == end() == completion position.
+  llvm::StringRef Name;
+  // The spelled scope qualifier, such as Foo::.
+  // If there is none, begin() == end() == Name.begin().
+  llvm::StringRef Qualifier;
+};
+// Heuristically parses before Offset to determine what should be completed.
+CompletionPrefix guessCompletionPrefix(llvm::StringRef Content,
+                                       unsigned Offset);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_CODECOMPLETE_H

clangd/CodeComplete.cpp

 #include "SourceCode.h"
 #include "TUScheduler.h"
 #include "Trace.h"
 #include "URI.h"
 #include "index/Index.h"
 #include "index/Symbol.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#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/Sema.h"

 
 // 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 Filter = speculateCompletionFilter(Content, Pos);
-  if (!Filter) {
-    elog("Failed to speculate filter text for code completion at Pos "
-         "{0}:{1}: {2}",
-         Pos.line, Pos.character, Filter.takeError());
-    return None;
+  auto Offset = positionToOffset(Content, Pos);
+  if (!Offset) {
+    elog("No speculative filter: bad offset {0} in {1}", Pos, File);
+    return llvm::None;
   }
-  CachedReq.Query = *Filter;
+  CachedReq.Query = guessCompletionPrefix(Content, *Offset).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.
clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
   // the index can provide results from the preamble.
   // Tell Sema not to deserialize the preamble to look for results.
   Result.LoadExternal = !Index;
   Result.IncludeFixIts = IncludeFixIts;
 
   return Result;
 }
 
-llvm::Expected<llvm::StringRef>
-speculateCompletionFilter(llvm::StringRef Content, Position Pos) {
-  auto Offset = positionToOffset(Content, Pos);
-  if (!Offset)
-    return llvm::make_error<llvm::StringError>(
-        "Failed to convert position to offset in content.",
-        llvm::inconvertibleErrorCode());
-  if (*Offset == 0)
-    return "";
+CompletionPrefix
+guessCompletionPrefix(llvm::StringRef Content, unsigned Offset) {
+  assert(Offset <= Content.size());
+  StringRef Suffix = Content.take_front(Offset);
+  CompletionPrefix Result;
+
+  // Consume the unqualified name. We only handle ASCII characters.
+  // isIdentifierBody will let us match "0invalid", but we don't mind.
+  while (!Suffix.empty() && isIdentifierBody(Suffix.back()))
+    Suffix = Suffix.drop_back();
+  Result.Name = Content.slice(Suffix.size(), Offset);
+
+  // Consume qualifiers.
+  // The inner loop can run 0 times, and so accept "::::". Not worth fixing?
+  while (Suffix.consume_back("::"))
+    while (!Suffix.empty() && isIdentifierBody(Suffix.back()))
+      Suffix = Suffix.drop_back();
+  Result.Qualifier =
+      Content.slice(Suffix.size(), Result.Name.begin() - Content.begin());
 
-  // Start from the character before the cursor.
-  int St = *Offset - 1;
-  // FIXME(ioeric): consider UTF characters?
-  auto IsValidIdentifierChar = [](char C) {
-    return ((C >= 'a' && C <= 'z') || (C >= 'A' && C <= 'Z') ||
-            (C >= '0' && C <= '9') || (C == '_'));
-  };
-  size_t Len = 0;
-  for (; (St >= 0) && IsValidIdentifierChar(Content[St]); --St, ++Len) {
-  }
-  if (Len > 0)
-    St++; // Shift to the first valid character.
-  return Content.substr(St, Len);
+  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) {
   return CodeCompleteFlow(FileName,

unittests/clangd/CodeCompleteTests.cpp

completions(R"cpp(
     };
 
     struct Derived : Base {
       ^
     };
   )cpp");
 }
 
-TEST(SpeculateCompletionFilter, Filters) {
-  Annotations F(R"cpp($bof^
-      $bol^
-      ab$ab^
-      x.ab$dot^
-      x.$dotempty^
-      x::ab$scoped^
-      x::$scopedempty^
-
-  )cpp");
-  auto speculate = [&](StringRef PointName) {
-    auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
-    assert(Filter);
-    return *Filter;
-  };
-  EXPECT_EQ(speculate("bof"), "");
-  EXPECT_EQ(speculate("bol"), "");
-  EXPECT_EQ(speculate("ab"), "ab");
-  EXPECT_EQ(speculate("dot"), "ab");
-  EXPECT_EQ(speculate("dotempty"), "");
-  EXPECT_EQ(speculate("scoped"), "ab");
-  EXPECT_EQ(speculate("scopedempty"), "");
+TEST(GuessCompletionPrefix, Filters) {
+  for (llvm::StringRef Case : {
+    "[[scope::]][[ident]]^",
+    "[[]][[]]^",
+    "\n[[]][[]]^",
+    "[[]][[ab]]^",
+    "x.[[]][[ab]]^",
+    "x.[[]][[]]^",
+    "[[x::]][[ab]]^",
+    "[[x::]][[]]^",
+    "some text [[scope::more::]][[identif]]^ier",
+    "some text [[scope::]][[mor]]^e::identifier",
+  }) {
+    Annotations F(Case);
+    auto Offset = cantFail(positionToOffset(F.code(), F.point()));
+    auto ToStringRef = [&](Range R) {
+      return F.code().slice(cantFail(positionToOffset(F.code(), R.start)),
+                            cantFail(positionToOffset(F.code(), R.end)));
+    };
+    auto WantQualifier = ToStringRef(F.ranges()[0]),
+         WantName = ToStringRef(F.ranges()[1]);
+
+    auto Prefix = guessCompletionPrefix(F.code(), Offset);
+    // Even when components are empty, check their offsets are correct.
+    EXPECT_EQ(WantQualifier, Prefix.Qualifier) << Case;
+    EXPECT_EQ(WantQualifier.begin(), Prefix.Qualifier.begin()) << Case;
+    EXPECT_EQ(WantName, Prefix.Name) << Case;
+    EXPECT_EQ(WantName.begin(), Prefix.Name.begin()) << Case;
+  }
 }
 
 TEST(CompletionTest, EnableSpeculativeIndexRequest) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
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) {
+  clangd::CodeCompleteOptions Opts = {};
+
+  auto Results = completions(R"cpp(
+    namespace ns {}
+    #define M(X) < X
+    M(ns::ABC^
+  )cpp",
+                             {cls("ns::ABCDE")}, Opts);
+  EXPECT_THAT(Results.Completions,
+              UnorderedElementsAre(AllOf(Qualifier(""), Named("ABCDE"))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang