[clangd] Move clang-tidy check modifications into ClangdServer

Summary:
This enables sharing the logic between standalone clangd and embedders
of it. The new approach should be same performance-wise, as it is only called
once per addDocument call.

This patch also introduces a blacklisting code path for disabling crashy or
high-noise tests, until we figure out a way to make them work with clangd-setup.

The biggest difference is the way we make use of preambles, hence those checks
can't see directives coming from the preamble section of the file. The second
thing is the fact that code might-not be compiling while clangd is trying to
build an AST, hence some checks might choke on those incomplete ASTs.

Reviewers: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, aaron.ballman, cfe-commits

Tags: #clang

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

clang-tools-extra/clangd/ClangdServer.cpp

@@ -40,6 +40,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
@@ -52,6 +53,7 @@
 #include <future>
 #include <memory>
 #include <mutex>
+#include <string>
 #include <type_traits>
 
 namespace clang {
@@ -109,6 +111,41 @@ private:
   ClangdServer::Callbacks *ServerCallbacks;
   bool TheiaSemanticHighlighting;
 };
+
+// Set of clang-tidy checks that don't work well in clangd, either due to
+// crashes or false positives.
+// Returns a clang-tidy filter string: -check1,-check2.
+llvm::StringRef getUnusableTidyChecks() {
+  static const std::string FalsePositives =
+      llvm::join_items(", ",
+                       // Check relies on seeing ifndef/define/endif directives,
+                       // clangd doesn't replay those when using a preamble.
+                       "-llvm-header-guard");
+  static const std::string CrashingChecks =
+      llvm::join_items(", ",
+                       // Check can choke on invalid (intermediate) c++ code,
+                       // which is often the case when clangd tries to build an
+                       // AST.
+                       "-bugprone-use-after-move");
+  static const std::string UnusableTidyChecks =
+      llvm::join_items(", ", FalsePositives, CrashingChecks);
+  return UnusableTidyChecks;
+}
+
+// Returns a clang-tidy options string: check1,check2.
+llvm::StringRef getDefaultTidyChecks() {
+  // These default checks are chosen for:
+  //  - low false-positive rate
+  //  - providing a lot of value
+  //  - being reasonably efficient
+  static const std::string DefaultChecks = llvm::join_items(
+      ",", "readability-misleading-indentation", "readability-deleted-default",
+      "bugprone-integer-division", "bugprone-sizeof-expression",
+      "bugprone-suspicious-missing-comma", "bugprone-unused-raii",
+      "bugprone-unused-return-value", "misc-unused-using-decls",
+      "misc-unused-alias-decls", "misc-definitions-in-headers");
+  return DefaultChecks;
+}
 } // namespace
 
 ClangdServer::Options ClangdServer::optsForTest() {
@@ -200,6 +237,15 @@ void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
   if (GetClangTidyOptions)
     Opts.ClangTidyOpts =
         GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+  if (Opts.ClangTidyOpts.Checks.hasValue()) {
+    // If the set of checks was configured, make sure clangd incompatible ones
+    // are disabled.
+    Opts.ClangTidyOpts.Checks = llvm::join_items(
+        ", ", *Opts.ClangTidyOpts.Checks, getUnusableTidyChecks());
+  } else {
+    // Otherwise provide a nice set of defaults.
+    Opts.ClangTidyOpts.Checks = getDefaultTidyChecks().str();
+  }
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
 
   // Compile command is set asynchronously during update, as it can be slow.

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

@@ -808,23 +808,6 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
         // FIXME: use the FS provided to the function.
         Opts = ClangTidyOptProvider->getOptions(File);
       }
-      if (!Opts.Checks) {
-        // If the user hasn't configured clang-tidy checks at all, including
-        // via .clang-tidy, give them a nice set of checks.
-        // (This should be what the "default" options does, but it isn't...)
-        //
-        // These default checks are chosen for:
-        //  - low false-positive rate
-        //  - providing a lot of value
-        //  - being reasonably efficient
-        Opts.Checks = llvm::join_items(
-            ",", "readability-misleading-indentation",
-            "readability-deleted-default", "bugprone-integer-division",
-            "bugprone-sizeof-expression", "bugprone-suspicious-missing-comma",
-            "bugprone-unused-raii", "bugprone-unused-return-value",
-            "misc-unused-using-decls", "misc-unused-alias-decls",
-            "misc-definitions-in-headers");
-      }
       return Opts;
     };
   }

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

@@ -26,9 +26,11 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -1194,6 +1196,44 @@ TEST(ClangdServerTest, TestStackOverflow) {
 }
 #endif
 
+TEST(ClangdServer, TidyOverrideTest) {
+  struct DiagsCheckingCallback : public ClangdServer::Callbacks {
+  public:
+    void onDiagnosticsReady(PathRef File, llvm::StringRef Version,
+                            std::vector<Diag> Diagnostics) override {
+      std::lock_guard<std::mutex> Lock(Mutex);
+      HadDiagsInLastCallback = !Diagnostics.empty();
+    }
+
+    std::mutex Mutex;
+    bool HadDiagsInLastCallback = false;
+  } DiagConsumer;
+
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags = {"-xc++"};
+  auto Opts = ClangdServer::optsForTest();
+  Opts.GetClangTidyOptions = [](llvm::vfs::FileSystem &, llvm::StringRef) {
+    auto Opts = tidy::ClangTidyOptions::getDefaults();
+    // These checks don't work well in clangd, even if configured they shouldn't
+    // run.
+    Opts.Checks = "bugprone-use-after-move,llvm-header-guard";
+    return Opts;
+  };
+  ClangdServer Server(CDB, FS, Opts, &DiagConsumer);
+  const char *SourceContents = R"cpp(
+    struct Foo { Foo(); Foo(Foo&); Foo(Foo&&); };
+    namespace std { Foo&& move(Foo&); }
+    void foo() {
+      Foo x;
+      Foo y = std::move(x);
+      Foo z = x;
+    })cpp";
+  Server.addDocument(testPath("foo.h"), SourceContents);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang