[clangd] IncludeCleaner: Don't warn on system headers

[kbobyrev]

Summary

This is a temporary hack to disable diagnostics for system headers. As of right
now, IncludeCleaner does not handle the Standard Library correctly and will
report most system headers as unused because very few symbols are defined in
top-level system headers. This will eventually be fixed, but for now we are
aiming for the most conservative approach with as little false-positive
warnings as possible. After the initial prototype and core functionality is
polished, I will turn back to handling the Standard Library as it requires
custom logic.

[sammccall]

clang-tools-extra/clangd/IncludeCleaner.cpp

294

As discussed offline, I'm not sure this difference in behavior is as temporary or as narrow as we originally planned.

I think the reason we want this hack is:

• umbrella headers need special handling
• standard library headers are umbrella headers
• system headers are likely to be standard library headers

But this is complicated by the fact that non-stdlib headers can be umbrella headers, system headers are likely to be umbrella headers even if not stdlib, stdlib needs different special handling from other umbrella headers, there are other headers (non-self-contained) that need special handling, etc.

Among other things, it's going to be hard to just turn this back on.

For now, I'd suggest extracting a function mayConsiderUnused(const Inclusion&) with the simple logic you have here for now, but we should come up with a more concrete plan soon.

[kbobyrev]

Resolve review comment.

clang-tools-extra/clangd/IncludeCleaner.cpp

for (const syntax::Token &Tok :
     if (!Macro)
       continue;
     auto Loc = Macro->Info->getDefinitionLoc();
     if (Loc.isValid())
       Result.insert(Loc);
   }
 }
 
+// FIXME(kirillbobyrev): We currently do not support the umbrella headers.
+// Standard Library headers are typically umbrella headers, and system headers
+// are likely to be the Standard Library headers. Until we have a good support
+// for umbrella headers and Standard Library headers, don't warn about them.
+bool mayConsiderUnused(const Inclusion *Inc) {
+  return Inc->Written.front() != '<';
+}
+
 } // namespace
 
 ReferencedLocations findReferencedLocations(ParsedAST &AST) {
   ReferencedLocations Result;
   ReferencedLocationCrawler Crawler(Result);
   Crawler.TraverseAST(AST.getASTContext());
   findReferencedMacros(AST, Result);
   return Result;
if (Cfg.Diagnostics.UnusedIncludes != Config::UnusedIncludesPolicy::Strict ||
     return {};
   std::vector<Diag> Result;
   std::string FileName =
       AST.getSourceManager()
           .getFileEntryForID(AST.getSourceManager().getMainFileID())
           ->getName()
           .str();
   for (const auto *Inc : computeUnusedIncludes(AST)) {
+    if (!mayConsiderUnused(Inc))
+      continue;
     Diag D;
     D.Message =
         llvm::formatv("included header {0} is not used",
                       llvm::sys::path::filename(
                           Inc->Written.substr(1, Inc->Written.size() - 2),
                           llvm::sys::path::Style::posix));
     D.Name = "unused-includes";
     D.Source = Diag::DiagSource::Clangd;

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

EXPECT_THAT(*TU.build().getDiagnostics(),
                         WithTag(DiagnosticTag::Unnecessary)),
                   AllOf(Diag(Test.range("deprecated"), "'bar' is deprecated"),
                         WithTag(DiagnosticTag::Deprecated))));
 }
 
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
 $fix[[  $diag[[#include "unused.h"]]
-]]  #include "used.h"
+]]#include "used.h"
 
+  #include <system_header.h>
+
   void foo() {
     used();
   }
   )cpp");
   TestTU TU;
   TU.Code = Test.code().str();
   TU.AdditionalFiles["unused.h"] = R"cpp(
     void unused() {}
   )cpp";
   TU.AdditionalFiles["used.h"] = R"cpp(
     void used() {}
   )cpp";
+  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.ExtraArgs = {"-isystem" + testPath("system")};
   // Off by default.
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
   Config Cfg;
   Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(
       *TU.build().getDiagnostics(),
       UnorderedElementsAre(AllOf(

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

 
 TEST(IncludeCleaner, GetUnusedHeaders) {
   llvm::StringLiteral MainFile = R"cpp(
     #include "a.h"
     #include "b.h"
     #include "dir/c.h"
     #include "dir/unused.h"
     #include "unused.h"
+    #include <system_header.h>
     void foo() {
       a();
       b();
       c();
     })cpp";
   // Build expected ast with symbols coming from headers.
   TestTU TU;
   TU.Filename = "foo.cpp";
   TU.AdditionalFiles["foo.h"] = "void foo();";
   TU.AdditionalFiles["a.h"] = "void a();";
   TU.AdditionalFiles["b.h"] = "void b();";
   TU.AdditionalFiles["dir/c.h"] = "void c();";
   TU.AdditionalFiles["unused.h"] = "void unused();";
   TU.AdditionalFiles["dir/unused.h"] = "void dirUnused();";
-  TU.AdditionalFiles["not_included.h"] = "void notIncluded();";
-  TU.ExtraArgs = {"-I" + testPath("dir")};
+  TU.AdditionalFiles["system/system_header.h"] = "";
+  TU.ExtraArgs.push_back("-I" + testPath("dir"));
+  TU.ExtraArgs.push_back("-isystem" + testPath("system"));
   TU.Code = MainFile.str();
   ParsedAST AST = TU.build();
-  auto UnusedIncludes = computeUnusedIncludes(AST);
-  std::vector<std::string> UnusedHeaders;
-  UnusedHeaders.reserve(UnusedIncludes.size());
-  for (const auto &Include : UnusedIncludes)
-    UnusedHeaders.push_back(Include->Written);
-  EXPECT_THAT(UnusedHeaders,
-              UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
+  std::vector<std::string> UnusedIncludes;
+  for (const auto &Include : computeUnusedIncludes(AST))
+    UnusedIncludes.push_back(Include->Written);
+  EXPECT_THAT(UnusedIncludes,
+              UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\"",
+                                   "<system_header.h>"));
 }
 
 TEST(IncludeCleaner, ScratchBuffer) {
   TestTU TU;
   TU.Filename = "foo.cpp";
   TU.Code = R"cpp(
     #include "macro_spelling_in_scratch_buffer.h"