[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

113

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

200

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

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

200

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

113

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

118

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).

125

Similarly, rename this. I'd suggest DisabledChecks.

[kadircet]

clang-tools-extra/clangd/ClangdServer.cpp

113

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

113

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

118

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

118

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

111

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

113

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...

118

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

200

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

203

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.

209

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)

217

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 ↗

(On Diff #275709)

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

118

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

118

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

118

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

113

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.

209

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

113

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

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.
+std::string getClangTidyBlacklist() {
+  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");
+  return llvm::join_items(", ", FalsePositives);
+}
 } // namespace
 
 ClangdServer::Options ClangdServer::optsForTest() {
   ClangdServer::Options Opts;
   Opts.UpdateDebounce = DebouncePolicy::fixed(/*zero*/ {});
   Opts.StorePreamblesInMemory = true;
   Opts.AsyncThreadsCount = 4; // Consistent!
   Opts.TheiaSemanticHighlighting = true;
   Opts.AsyncPreambleBuilds = true;
   return Opts;
 }
 
 ClangdServer::Options::operator TUScheduler::Options() const {
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)