[clangd] Refactor speculateCompletionFilter and also extract scope.

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.

Reviewers: ioeric

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D60500

llvm-svn: 358074

clang-tools-extra/clangd/CodeComplete.cpp

@@ -37,6 +37,7 @@
 #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"
@@ -1110,14 +1111,12 @@ 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;
 }
 
@@ -1537,29 +1536,26 @@ clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
   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 Rest = Content.take_front(Offset);
+  CompletionPrefix Result;
 
-  // 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);
+  // Consume the unqualified name. We only handle ASCII characters.
+  // isIdentifierBody will let us match "0invalid", but we don't mind.
+  while (!Rest.empty() && isIdentifierBody(Rest.back()))
+    Rest = Rest.drop_back();
+  Result.Name = Content.slice(Rest.size(), Offset);
+
+  // Consume qualifiers.
+  while (Rest.consume_back("::") && !Rest.endswith(":")) // reject ::::
+    while (!Rest.empty() && isIdentifierBody(Rest.back()))
+      Rest = Rest.drop_back();
+  Result.Qualifier =
+      Content.slice(Rest.size(), Result.Name.begin() - Content.begin());
+
+  return Result;
 }
 
 CodeCompleteResult

clang-tools-extra/clangd/CodeComplete.h

@@ -254,10 +254,21 @@ SignatureHelp signatureHelp(PathRef FileName,
 // 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

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

@@ -32,7 +32,6 @@ namespace {
 using ::llvm::Failed;
 using ::testing::AllOf;
 using ::testing::Contains;
-using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::HasSubstr;
@@ -1915,28 +1914,37 @@ TEST(CompletionTest, OverridesNonIdentName) {
   )cpp");
 }
 
-TEST(SpeculateCompletionFilter, Filters) {
-  Annotations F(R"cpp($bof^
-      $bol^
-      ab$ab^
-      x.ab$dot^
-      x.$dotempty^
-      x::ab$scoped^
-      x::$scopedempty^
+TEST(GuessCompletionPrefix, Filters) {
+  for (llvm::StringRef Case : {
+    "[[scope::]][[ident]]^",
+    "[[]][[]]^",
+    "\n[[]][[]]^",
+    "[[]][[ab]]^",
+    "x.[[]][[ab]]^",
+    "x.[[]][[]]^",
+    "[[x::]][[ab]]^",
+    "[[x::]][[]]^",
+    "[[::x::]][[ab]]^",
+    "some text [[scope::more::]][[identif]]^ier",
+    "some text [[scope::]][[mor]]^e::identifier",
+    "weird case foo::[[::bar::]][[baz]]^",
+  }) {
+    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]);
 
-  )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"), "");
+    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) {
@@ -2366,6 +2374,23 @@ TEST(CompletionTest, NestedScopeIsUnresolved) {
               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