[clangd] Add --check-range to restrict --check to specific lines

[adamcz]

Summary

This will allow us to add code completion, which is too expensive at
every token, to --check too.

[adamcz]

This has been annoying me for a while ;-)

[sammccall]

Thanks! Just some random nits/ideas.

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

206

you already have the line number as Pos.start.line

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

64

You could also pass in a function_ref<bool(const Position&)> which feels a bit more self-documenting

352

nit: check-lines, which documents the units?

354

seems likely the common case will be a single line, and --check-range=254-254 is a little awkward.
Supporting --check-range=254 seems worthwhile?

921

(I'd omit this if the flag is just ignored and harmless. There should be no danger of expecting it to check something if you didn't provide a filename...)

[adamcz]

addressed review comments + rebase to latest version

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

354

Good idea. Done

921

I'd rather keep it. If you specify --check-lines without --check you're doing something wrong and going into LSP mode is unlikely to be useful. If it's part of the script, you might wait a minute or two until you realize it's not actually working.

[sammccall]

Still LG

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

898–917

nit: consider pulling out a helper function to parse this that returns std:;function

[adamcz]

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

898–917

It gets a bit ugly, since we need a way to report an error. So it's either returning some ErrorOr<std::function> or elog() + exit(1) from a getShouldCheckLines() function.

If you prefer, I can do that, but I feel like this will hurt readability because of the error handling.

clang-tools-extra/clangd/test/check-lines.test

+// RUN: cp %s %t.cpp
+// RUN: not clangd -check=%t.cpp -check-lines=6-14 2>&1 | FileCheck -strict-whitespace %s
+// RUN: not clangd -check=%t.cpp -check-lines=14 2>&1 | FileCheck -strict-whitespace %s
+
+// CHECK: Testing on source file {{.*}}check-lines.test
+// CHECK: internal (cc1) args are: -cc1
+// CHECK: Building preamble...
+// CHECK: Building AST...
+// CHECK: Testing features at each token
+// CHECK: tweak: ExpandAutoType ==> FAIL
+// CHECK: All checks completed, 1 errors
+
+void fun();
+auto x = fun; // This line is tested
+auto y = fun; // This line is not tested

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

bool buildAST() {
     if (Opts.BuildDynamicSymbolIndex) {
       log("Indexing AST...");
       Index.updateMain(File, *AST);
     }
     return true;
   }
 
   // Run AST-based features at each token in the file.
-  void testLocationFeatures() {
+  void testLocationFeatures(
+      llvm::function_ref<bool(const Position &)> ShouldCheckLine) {
     log("Testing features at each token (may be slow in large files)");
-    auto SpelledTokens =
-        AST->getTokens().spelledTokens(AST->getSourceManager().getMainFileID());
+    auto &SM = AST->getSourceManager();
+    auto SpelledTokens = AST->getTokens().spelledTokens(SM.getMainFileID());
     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;
+
       // FIXME: dumping the tokens may leak sensitive code into bug reports.
       // Add an option to turn this off, once we decide how options work.
       vlog("  {0} {1}", Pos, Tok.text(AST->getSourceManager()));
       auto Tree = SelectionTree::createRight(AST->getASTContext(),
                                              AST->getTokens(), Start, End);
       Tweak::Selection Selection(&Index, *AST, Start, End, std::move(Tree));
       for (const auto &T : prepareTweaks(Selection, Opts.TweakFilter)) {
         auto Result = T->apply(Selection);
for (const auto &Tok : SpelledTokens) {
       // FIXME: it'd be nice to include code completion, but it's too slow.
       // Maybe in combination with a line restriction?
     }
   }
 };
 
 } // namespace
 
-bool check(llvm::StringRef File, const ThreadsafeFS &TFS,
-           const ClangdLSPServer::Options &Opts) {
+bool check(llvm::StringRef File,
+           llvm::function_ref<bool(const Position &)> ShouldCheckLine,
+           const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts) {
   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();
+  C.testLocationFeatures(ShouldCheckLine);
 
   log("All checks completed, {0} errors", C.ErrCount);
   return C.ErrCount == 0;
 }
 
 } // namespace clangd
 } // namespace clang

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

 #ifdef __GLIBC__
 #include <malloc.h>
 #endif
 
 namespace clang {
 namespace clangd {
 
 // Implemented in Check.cpp.
-bool check(const llvm::StringRef File, const ThreadsafeFS &TFS,
-           const ClangdLSPServer::Options &Opts);
+bool check(const llvm::StringRef File,
+           llvm::function_ref<bool(const Position &)> ShouldCheckLine,
+           const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts);
 
 namespace {
 
 using llvm::cl::cat;
 using llvm::cl::CommaSeparated;
 using llvm::cl::desc;
 using llvm::cl::Hidden;
 using llvm::cl::init;
opt<Path> CheckFile{
     cat(Misc),
     desc("Parse one file in isolation instead of acting as a language server. "
          "Useful to investigate/reproduce crashes or configuration problems. "
          "With --check=<filename>, attempts to parse a particular file."),
     init(""),
     ValueOptional,
 };
 
+opt<std::string> CheckFileLines{
+    "check-lines",
+    cat(Misc),
+    desc("If specified, limits the range of tokens in -check file on which "
+         "various features are tested. Example --check-lines=3-7 restricts "
+         "testing to lines 3 to 7 (inclusive) or --check-lines=5 to restrict "
+         "to one line. Default is testing entire file."),
+    init(""),
+    ValueOptional,
+};
+
 enum PCHStorageFlag { Disk, Memory };
 opt<PCHStorageFlag> PCHStorage{
     "pch-storage",
     cat(Misc),
     desc("Storing PCHs in memory increases memory usages, but may "
          "improve performance"),
     values(
         clEnumValN(PCHStorageFlag::Disk, "disk", "store PCHs on disk"),
#endif
   };
   if (ForceOffsetEncoding != OffsetEncoding::UnsupportedEncoding)
     Opts.Encoding = ForceOffsetEncoding;
 
   if (CheckFile.getNumOccurrences()) {
     llvm::SmallString<256> Path;
     llvm::sys::fs::real_path(CheckFile, Path, /*expand_tilde=*/true);
     log("Entering check mode (no LSP server)");
-    return check(Path, TFS, Opts)
+    uint32_t Begin = 0, End = std::numeric_limits<uint32_t>::max();
+    if (!CheckFileLines.empty()) {
+      StringRef RangeStr(CheckFileLines);
+      bool ParseError = RangeStr.consumeInteger(0, Begin);
+      if (RangeStr.empty()) {
+        End = Begin;
+      } else {
+        ParseError |= !RangeStr.consume_front("-");
+        ParseError |= RangeStr.consumeInteger(0, End);
+      }
+      if (ParseError || !RangeStr.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)
                ? 0
                : static_cast<int>(ErrorResultCode::CheckFailed);
   }
+  if (!CheckFileLines.empty()) {
+    elog("--check-lines requires --check");
+    return 1;
+  }
 
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
   std::unique_ptr<Transport> TransportLayer;
   if (getenv("CLANGD_AS_XPC_SERVICE")) {
 #if CLANGD_BUILD_XPC
     log("Starting LSP over XPC service");