[clangd] Run code completion on each token coverd by --check-lines

[adamcz]

Summary

In --check mode we do not run code completion because it is too slow,
especially on larger files. With the introducation of --check-lines we
can narrow down the scope and thus we can afford to do code completion.

We vlog() the top completion result, but that's not really the point.
The most value will come from being able to reproduce crashes that occur
during code completion and require preamble build or index (and thus are
more difficult to reproduce with -code-complete-at).

[kadircet]

Thanks, LGTM.

---

A little bit thinking out loud, was there a particular reason to introduce --check-line into ClangdMain.cpp rather than Check.cpp?

It feels like we should have --check related flags in Check.cpp instead. That way both the signature of check stays more reasonable and ClangdMain won't accumulate flag to internal value logic.

clang-tools-extra/clangd/tool/ClangdMain.cpp

66

nit: it is not common to have const on params that are copied (especially when they are just builtins)

[adamcz]

I didn't put much thought into where --check-lines goes. It's an interesting thought.

I think having all the flags in one place is more valuable than trying to split them in some way. We contain all flags in the same place, close to main() and check() gets all it's data from arguments, rather than some side channel. It makes check() easier to re-use and/or test. It also allows us to complain about flag misuse, such as specifying --check-lines without --check, etc.

I'm going to commit as-is, since you LGTMed this, but feel free to continue discussion here and if we decide to move the flags somewhere else I'll happily submit another change.

[adamcz]

review comments

clang-tools-extra/clangd/tool/Check.cpp

if (Opts.BuildDynamicSymbolIndex) {
       log("Indexing AST...");
       Index.updateMain(File, *AST);
     }
     return true;
   }
 
   // Run AST-based features at each token in the file.
   void testLocationFeatures(
-      llvm::function_ref<bool(const Position &)> ShouldCheckLine) {
+      llvm::function_ref<bool(const Position &)> ShouldCheckLine,
+      const bool EnableCodeCompletion) {
     log("Testing features at each token (may be slow in large files)");
     auto &SM = AST->getSourceManager();
     auto SpelledTokens = AST->getTokens().spelledTokens(SM.getMainFileID());
+
+    CodeCompleteOptions CCOpts = Opts.CodeComplete;
+    CCOpts.Index = &Index;
+
     for (const auto &Tok : SpelledTokens) {
       unsigned Start = AST->getSourceManager().getFileOffset(Tok.location());
       unsigned End = Start + Tok.length();
       Position Pos = offsetToPosition(Inputs.Contents, Start);
 
       if (!ShouldCheckLine(Pos))
         continue;
 
for (const auto &Tok : SpelledTokens) {
         }
       }
       unsigned Definitions = locateSymbolAt(*AST, Pos, &Index).size();
       vlog("    definition: {0}", Definitions);
 
       auto Hover = getHover(*AST, Pos, Style, &Index);
       vlog("    hover: {0}", Hover.hasValue());
 
-      // FIXME: it'd be nice to include code completion, but it's too slow.
-      // Maybe in combination with a line restriction?
+      if (EnableCodeCompletion) {
+        Position EndPos = offsetToPosition(Inputs.Contents, End);
+        auto CC = codeComplete(File, EndPos, Preamble.get(), Inputs, CCOpts);
+        vlog("    code completion: {0}",
+             CC.Completions.empty() ? "<empty>" : CC.Completions[0].Name);
+      }
     }
   }
 };
 
 } // namespace
 
 bool check(llvm::StringRef File,
            llvm::function_ref<bool(const Position &)> ShouldCheckLine,
-           const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts) {
+           const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts,
+           bool EnableCodeCompletion) {
   llvm::SmallString<0> FakeFile;
   llvm::Optional<std::string> Contents;
   if (File.empty()) {
     llvm::sys::path::system_temp_directory(false, FakeFile);
     llvm::sys::path::append(FakeFile, "test.cc");
     File = FakeFile;
     Contents = R"cpp(
       #include <stddef.h>
       #include <string>
 
       size_t N = 50;
       auto xxx = std::string(N, 'x');
     )cpp";
   }
   log("Testing on source file {0}", File);
 
   auto ContextProvider = ClangdServer::createConfiguredContextProvider(
       Opts.ConfigProvider, nullptr);
   WithContext Ctx(ContextProvider(""));
   Checker C(File, Opts);
   if (!C.buildCommand(TFS) || !C.buildInvocation(TFS, Contents) ||
       !C.buildAST())
     return false;
-  C.testLocationFeatures(ShouldCheckLine);
+  C.testLocationFeatures(ShouldCheckLine, EnableCodeCompletion);
 
   log("All checks completed, {0} errors", C.ErrCount);
   return C.ErrCount == 0;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/tool/ClangdMain.cpp

 #endif
 
 namespace clang {
 namespace clangd {
 
 // Implemented in Check.cpp.
 bool check(const llvm::StringRef File,
            llvm::function_ref<bool(const Position &)> ShouldCheckLine,
-           const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts);
+           const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts,
+           bool EnableCodeCompletion);
 
 namespace {
 
 using llvm::cl::cat;
 using llvm::cl::CommaSeparated;
 using llvm::cl::desc;
 using llvm::cl::Hidden;
 using llvm::cl::init;
if (!CheckFileLines.empty()) {
         elog("Invalid --check-line specified. Use Begin-End format, e.g. 3-17");
         return 1;
       }
     }
     auto ShouldCheckLine = [&](const Position &Pos) {
       uint32_t Line = Pos.line + 1; // Position::line is 0-based.
       return Line >= Begin && Line <= End;
     };
-    return check(Path, ShouldCheckLine, TFS, Opts)
+    // For now code completion is enabled any time the range is limited via
+    // --check-lines. If it turns out to be to slow, we can introduce a
+    // dedicated flag for that instead.
+    return check(Path, ShouldCheckLine, TFS, Opts,
+                 /*EnableCodeCompletion=*/!CheckFileLines.empty())
                ? 0
                : static_cast<int>(ErrorResultCode::CheckFailed);
   }
   if (!CheckFileLines.empty()) {
     elog("--check-lines requires --check");
     return 1;
   }