[clangd] Move clang-tidy check modifications into ClangdServer

[kadircet]

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.

[kadircet]

• Add tests and also disable bugprone-use-after-move

[njames93]

Do you think there should be a hidden command line option to disable disabling blacklisted checks, purely to prevent hindering attempts to fix these problematic checks.

clang-tools-extra/clangd/ClangdServer.cpp

115

Return by StringRef?

[kadircet]

In D83224#2133457, @njames93 wrote:

Do you think there should be a hidden command line option to disable disabling blacklisted checks, purely to prevent hindering attempts to fix these problematic checks.

I would expect those to be fixed and verified with unittests that are using TestTU, so the changes in ClangdServer shouldn't really be relevant to them. And if developer is trying to test something locally, i think it is better for them to just update the blacklist file(they are going to end up doing that anyways and perform possibly multiple compiles), so a flag to change blacklists behaviour might not be that meaningful.

[njames93]

clang-tools-extra/clangd/ClangdServer.cpp

210

Should the ! be removed the branches be swapped? Just looks cleaner imo, WDYT?

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

210

SGTM, will address once we've settled on the idea with Sam.

(and let this be a ping to him :D @sammccall )

[aaron.ballman]

clang-tools-extra/clangd/ClangdServer.cpp

115

How about getDisabledClangTidyChecks() (or literally any other name than blacklist)?

120

I suspect there are more checks that should be added here. For instance, much of modernize- is purely stylistic so it's easy to view as being false positives (like modernize-use-trailing-return-types or whatever it's called).

127

Similarly, rename this. I'd suggest DisabledChecks.

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

115

thanks for bringing this to my attention, i will try to be more conscious next time.

I would prefer allow/deny as disabled might also be offensive in some contexts. Do you know if we already have some settlements around this one in the wider community?

[aaron.ballman]

clang-tools-extra/clangd/ClangdServer.cpp

115

thanks for bringing this to my attention, i will try to be more conscious next time.

No worries!

I would prefer allow/deny as disabled might also be offensive in some contexts. Do you know if we already have some settlements around this one in the wider community?

I don't believe there's any consensus around avoiding use of "disabled" (we use the term in a lot of places, especially when paired with "enabled"), but I'd also be fine with allow/deny terminology instead.

As a minor drive-by comment, the function should also be marked static and placed outside of the anonymous namespace (per our usual coding style).

[njames93]

clang-tools-extra/clangd/ClangdServer.cpp

120

I don't think that's what this list is for. This seems to be purely for checks that don't run properly or crash inside clangd. modernize-use-trailing-return-types is a very noisy check but that's how it is when ran as normal clang-tidy.

[aaron.ballman]

clang-tools-extra/clangd/ClangdServer.cpp

120

Ah, thank you for the explanation. Then how about UnusableWithinClangd or something other than FalsePositives for the name?

[sammccall]

clang-tools-extra/clangd/ClangdServer.cpp

113

nit: "returns a clang-tidy filter string: -check1,-check2"

115

I dislike "disabled" because it's vague: every check that's not enabled is disabled, but only a few of them are mentioned here.

I'd suggest BadlyBehavedTidyChecks or UnusableTidyChecks...

120

The whole list is of checks that aren't usable in clangd, FalsePositives is the specific reason. What's the problem with the name?

210

I'd actually write this explicitly as hasValue() since the nullopt vs empty distinction is critical here

213

this comment no longer belongs here, it's to do with the structure of the various ClangTidyOptionsProvider implementations, which aren't visible here. Fine to just drop it.

219

if you're going to do this on every request, might as well pull out the default set of checks into a function getDefaultTidyChecks() or so.

(So it's just joined once, but more importantly so we're consistent in how we separate the mechanism vs policy)

227

nit: they haven't necessarily enabled checks (e.g. they could have specified -checks="" on the command line).

If the set of checks was configured?

(This is kind of a nit but did confuse me...)

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

1145

Maybe add a comment like "these checks don't work well in clangd, even if configured they shouldn't run"

[aaron.ballman]

clang-tools-extra/clangd/ClangdServer.cpp

120

What's the problem with the name?

If this is a list of checks that should be disabled because they have a ton of false positives, I think the name is fine and the list is incomplete. If this is a list of checks that should be disabled because they're not well-supported in clangd specifically, I think the name is misleading and the list is fine.

[sammccall]

clang-tools-extra/clangd/ClangdServer.cpp

120

If this is a list of checks that should be disabled because they're not well-supported in clangd specifically, I think the name is misleading and the list is fine.

Fair enough - I think there's enough context to not need to repeat "in clangd" here though. This code is in clangd, in a function with a comment indicating that it's for checks that don't work well specifically in clangd. (Maybe s/not suitable to be run through clangd/don't work well in clangd/ though).

[aaron.ballman]

clang-tools-extra/clangd/ClangdServer.cpp

120

Fair enough - I think there's enough context to not need to repeat "in clangd" here though. This code is in clangd, in a function with a comment indicating that it's for checks that don't work well specifically in clangd. (Maybe s/not suitable to be run through clangd/don't work well in clangd/ though).

That makes sense to me.

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

115

As a minor drive-by comment, the function should also be marked static and placed outside of the anonymous namespace (per our usual coding style).

That's what we do when working in llvm and clang, but convention around clangd is putting implementation details in anon namespaces. I would rather not stray from it here.

As for the general naming issues, i went with UnusableTidyChecks.

219

SG. Just as a note, we were already doing this at every request, but indirectly through GetClangTidyOptions()

[kadircet]

• Address comments

[aaron.ballman]

clang-tools-extra/clangd/ClangdServer.cpp

115

That's what we do when working in llvm and clang, but convention around clangd is putting implementation details in anon namespaces. I would rather not stray from it here.

It's unfortunate that clangd decided to stray from the community standards like this, but thank you for letting me know -- I agree it should match the local style.

clang-tools-extra/clangd/ClangdServer.cpp

 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/ArrayRef.h"
 #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"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <future>
 #include <memory>
 #include <mutex>
+#include <string>
 #include <type_traits>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Update the FileIndex with new ASTs and plumb the diagnostics responses.
 struct UpdateIndexCallbacks : public ParsingCallbacks {
if (ServerCallbacks)
       ServerCallbacks->onFileUpdated(File, Status);
   }
 
 private:
   FileIndex *FIndex;
   ClangdServer::Callbacks *ServerCallbacks;
   bool TheiaSemanticHighlighting;
 };
+
+// Set of clang-tidy checks that are not suitable to be run through clangd,
+// either due to crashes or false positives.
+const char *getClangTidyBlacklist() {
+  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 BlackList =
+      llvm::join_items(", ", FalsePositives, CrashingChecks);
+  return BlackList.c_str();
+}
 } // namespace
 
 ClangdServer::Options ClangdServer::optsForTest() {
   ClangdServer::Options Opts;
   Opts.UpdateDebounce = DebouncePolicy::fixed(/*zero*/ {});
   Opts.StorePreamblesInMemory = true;
   Opts.AsyncThreadsCount = 4; // Consistent!
   Opts.TheiaSemanticHighlighting = true;
void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                llvm::StringRef Version,
                                WantDiagnostics WantDiags, bool ForceRebuild) {
   ParseOptions Opts;
   Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults();
   // FIXME: call tidy options builder on the worker thread, it can do IO.
   if (GetClangTidyOptions)
     Opts.ClangTidyOpts =
         GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File);
+  if (!Opts.ClangTidyOpts.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.ClangTidyOpts.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");
+  } else {
+    // If user has enabled some checks, make sure clangd incompatible ones are
+    // disabled.
+    Opts.ClangTidyOpts.Checks = llvm::join_items(
+        ", ", *Opts.ClangTidyOpts.Checks, getClangTidyBlacklist());
+  }
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
 
   // Compile command is set asynchronously during update, as it can be slow.
   ParseInputs Inputs;
   Inputs.TFS = &TFS;
   Inputs.Contents = std::string(Contents);
   Inputs.Version = Version.str();
   Inputs.ForceRebuild = ForceRebuild;

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

Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &,
                                    llvm::StringRef File) {
       // This function must be thread-safe and tidy option providers are not.
       tidy::ClangTidyOptions Opts;
       {
         std::lock_guard<std::mutex> Lock(ClangTidyOptMu);
         // 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;
     };
   }
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
 
   Opts.TweakFilter = [&](const Tweak &T) {
     if (T.hidden() && !HiddenFeatures)

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

 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/None.h"
 #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 "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <algorithm>
 #include <chrono>
 #include <iostream>
 #include <random>
 #include <string>
 #include <thread>
TEST_F(ClangdVFSTest, TestStackOverflow) {
   Server.addDocument(FooCpp, SourceContents);
   ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics";
   // check that we got a constexpr depth error, and not crashed by stack
   // overflow
   EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
 }
 #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();
+    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