[clangd] Enforce strict unused includes by default

[kadircet]

Summary

Depends on D152685

[hokein]

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

9

nit: this spelling is incorrect, looks like a bug in the tooling spelling logic.

[cjdb]

This patch seems to have broken clangd for a project of mine. Most of my headers are now flagged as not being directly used even though they're the root header for the symbol.

[kadircet]

In D152686#4449481, @cjdb wrote:

This patch seems to have broken clangd for a project of mine. Most of my headers are now flagged as not being directly used even though they're the root header for the symbol.

replied on https://github.com/clangd/clangd/issues/1683

clang-tools-extra/clangd/Config.h

struct {
       // A comma-seperated list of globs specify which clang-tidy checks to run.
       std::string Checks;
       llvm::StringMap<std::string> CheckOptions;
     } ClangTidy;
 
     /// Enable emitting diagnostics using stale preambles.
     bool AllowStalePreamble = false;
 
-    IncludesPolicy UnusedIncludes = IncludesPolicy::None;
+    IncludesPolicy UnusedIncludes = IncludesPolicy::Strict;
     IncludesPolicy MissingIncludes = IncludesPolicy::None;
 
     /// IncludeCleaner will not diagnose usages of these headers matched by
     /// these regexes.
     struct {
       std::vector<std::function<bool(llvm::StringRef)>> IgnoreHeader;
     } Includes;
   } Diagnostics;

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

for (const auto &Case : Cases) {
     Frag.If.PathExclude.emplace_back(Case.PathSpec);
     Frag.Source.Directory = Case.Directory;
     EXPECT_NE(compileAndApply(), Case.ShouldMatch);
     ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   }
 }
 
 TEST_F(ConfigCompileTests, DiagnosticsIncludeCleaner) {
-  // Defaults to None.
+  // Defaults to Strict.
   EXPECT_TRUE(compileAndApply());
-  EXPECT_EQ(Conf.Diagnostics.UnusedIncludes,
-            Config::IncludesPolicy::None);
+  EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, Config::IncludesPolicy::Strict);
 
   Frag = {};
   Frag.Diagnostics.UnusedIncludes.emplace("None");
   EXPECT_TRUE(compileAndApply());
-  EXPECT_EQ(Conf.Diagnostics.UnusedIncludes,
-            Config::IncludesPolicy::None);
+  EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, Config::IncludesPolicy::None);
 
   Frag = {};
   Frag.Diagnostics.UnusedIncludes.emplace("Strict");
   EXPECT_TRUE(compileAndApply());
-  EXPECT_EQ(Conf.Diagnostics.UnusedIncludes,
-            Config::IncludesPolicy::Strict);
+  EXPECT_EQ(Conf.Diagnostics.UnusedIncludes, Config::IncludesPolicy::Strict);
 
   Frag = {};
   EXPECT_TRUE(Conf.Diagnostics.Includes.IgnoreHeader.empty())
       << Conf.Diagnostics.Includes.IgnoreHeader.size();
   Frag.Diagnostics.Includes.IgnoreHeader.push_back(
       Located<std::string>("foo.h"));
   Frag.Diagnostics.Includes.IgnoreHeader.push_back(
       Located<std::string>(".*inc"));

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

 //===--- DiagnosticsTests.cpp ------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "../clang-tidy/ClangTidyOptions.h"
 #include "Annotations.h"
 #include "Config.h"
 #include "Diagnostics.h"
 #include "Feature.h"
 #include "FeatureModule.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "TidyProvider.h"
 #include "index/MemIndex.h"
+#include "index/Ref.h"
+#include "index/Relation.h"
+#include "index/Symbol.h"
 #include "support/Context.h"
 #include "support/Path.h"
+#include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticSema.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/Specifiers.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/TargetSelect.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
-#include <algorithm>
+#include <cstddef>
 #include <memory>
+#include <string>
+#include <utility>
+#include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::_;
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::IsEmpty;
+using ::testing::Not;
 using ::testing::Pair;
 using ::testing::SizeIs;
 using ::testing::UnorderedElementsAre;
 
 ::testing::Matcher<const Diag &> withFix(::testing::Matcher<Fix> FixMatcher) {
   return Field(&Diag::Fixes, ElementsAre(FixMatcher));
 }
 

 // Recursive main-file include with #pragma once guard is OK.
 TEST(DiagnosticsTest, RecursivePreamblePragmaOnce) {
   auto TU = TestTU::withCode(R"cpp(
     #pragma once
     #include "foo.h"
     int symbol;
   )cpp");
   TU.Filename = "foo.h";
-  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+  EXPECT_THAT(*TU.build().getDiagnostics(),
+              Not(Contains(diagName("pp_including_mainfile_in_preamble"))));
   EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1));
 }
 
 // Recursive main-file include with #ifndef guard should be OK.
 // However, it's not yet recognized (incomplete at end of preamble).
 TEST(DiagnosticsTest, RecursivePreambleIfndefGuard) {
   auto TU = TestTU::withCode(R"cpp(
     #ifndef FOO

 TEST(DiagsInHeaders, DiagInTransitiveInclude) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", "#include \"b.h\""},
                         {"b.h", "no_type_spec; // error-ok"}};
   EXPECT_THAT(*TU.build().getDiagnostics(),
-              UnorderedElementsAre(
-                  Diag(Main.range(), "in included file: a type specifier is "
+              UnorderedElementsAre(Diag(Main.range(),
+                                        "in included file: a type specifier is "
                                         "required for all declarations")));
 }
 
 TEST(DiagsInHeaders, DiagInMultipleHeaders) {
   Annotations Main(R"cpp(
     #include $a[["a.h"]]
     #include $b[["b.h"]]
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
Annotations Main(R"cpp(
     #include [["a.h"]]
     #include "b.h"
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {
       {"a.h", "#include \"b.h\"\n"},
       {"b.h", "#ifndef X\n#define X\nno_type_spec; // error-ok\n#endif"}};
   EXPECT_THAT(*TU.build().getDiagnostics(),
-              UnorderedElementsAre(Diag(Main.range(),
-                                        "in included file: a type specifier is "
-                                        "required for all declarations")));
+              Contains(Diag(Main.range(), "in included file: a type specifier "
+                                          "is required for all declarations")));
 }
 
 TEST(DiagsInHeaders, PreferExpansionLocationMacros) {
   Annotations Main(R"cpp(
     #define X
     #include "a.h"
     #undef X
     #include [["b.h"]]
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {
       {"a.h", "#include \"c.h\"\n"},
       {"b.h", "#include \"c.h\"\n"},
       {"c.h", "#ifndef X\n#define X\nno_type_spec; // error-ok\n#endif"}};
   EXPECT_THAT(*TU.build().getDiagnostics(),
-              UnorderedElementsAre(
-                  Diag(Main.range(), "in included file: a type specifier is "
+              UnorderedElementsAre(Diag(Main.range(),
+                                        "in included file: a type specifier is "
                                         "required for all declarations")));
 }
 
 TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     #include "b.h"
     void foo() {})cpp");
   TestTU TU = TestTU::withCode(Main.code());
TU.AdditionalFiles = {{"a.h", "#include \"c.h\"\n"},
       no_type_spec_5;
       no_type_spec_6;
       no_type_spec_7;
       no_type_spec_8;
       no_type_spec_9;
       no_type_spec_10;
       #endif)cpp"}};
   EXPECT_THAT(*TU.build().getDiagnostics(),
-              UnorderedElementsAre(
-                  Diag(Main.range(), "in included file: a type specifier is "
+              UnorderedElementsAre(Diag(Main.range(),
+                                        "in included file: a type specifier is "
                                         "required for all declarations")));
 }
 
 TEST(DiagsInHeaders, OnlyErrorOrFatal) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   Annotations Header(R"cpp(
     [[no_type_spec]]; // error-ok
TU.AdditionalFiles["used.h"] = R"cpp(
     void used() {}
   )cpp";
   TU.AdditionalFiles["ignore.h"] = R"cpp(
     #pragma once
     void ignore() {}
   )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::IncludesPolicy::Strict;
   // Set filtering.
   Cfg.Diagnostics.Includes.IgnoreHeader.emplace_back(
       [](llvm::StringRef Header) { return Header.endswith("ignore.h"); });
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
   auto AST = TU.build();
   EXPECT_THAT(
       *AST.getDiagnostics(),
-      UnorderedElementsAre(
-          AllOf(Diag(Test.range("diag"),
-                     "included header unused.h is not used directly"),
-                withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
-                withFix(Fix(Test.range("fix"), "", "remove #include directive")))));
+      Contains(AllOf(
+          Diag(Test.range("diag"),
   auto &Diag = AST.getDiagnostics()->front();
   EXPECT_EQ(getDiagnosticDocURI(Diag.Source, Diag.ID, Diag.Name),
             std::string("https://clangd.llvm.org/guides/include-cleaner"));
   Cfg.Diagnostics.SuppressAll = true;
   WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
   Cfg.Diagnostics.SuppressAll = false;
   Cfg.Diagnostics.Suppress = {"unused-includes"};

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

 // in ParsedAST and Preamble, via the TestTU helper.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../../clang-tidy/ClangTidyCheck.h"
 #include "AST.h"
 #include "Annotations.h"
 #include "Compiler.h"
+#include "Config.h"
 #include "Diagnostics.h"
 #include "Headers.h"
 #include "ParsedAST.h"
 #include "Preamble.h"
 #include "SourceCode.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "TidyProvider.h"
+#include "support/Context.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <utility>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;

 // Note: the semicolons (empty statements) are significant!
 // - they force the preamble to end and the body to begin. Directives can have
 //   different effects in the preamble vs main file (which we try to hide).
 // - if the preamble would otherwise cover the whole file, a trailing semicolon
 //   forces their sizes to be different. This is significant because the file
 //   size is part of the lookup key for HeaderFileInfo, and we don't want to
 //   rely on the preamble's HFI being looked up when parsing the main file.
 TEST(ParsedASTTest, HeaderGuardsSelfInclude) {
+  // Disable include cleaner diagnostics to prevent them from interfering with
+  // other diagnostics.
+  Config Cfg;
+  Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::None;
+  Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::None;
+  WithContextValue Ctx(Config::Key, std::move(Cfg));
+
   TestTU TU;
   TU.ImplicitHeaderGuard = false;
   TU.Filename = "self.h";
 
   TU.Code = R"cpp(
     #include "self.h" // error-ok
     ;
   )cpp";