[clangd] Enforce strict unused includes by default

Depends on D152685 Differential Revision: https://reviews.llvm.org/D152686
llvm · Jun 12, 2023 · ea20f33 · ea20f33
1 parent 031ffc3
commit ea20f33
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 29 deletions.
diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
@@ -108,7 +108,7 @@ struct Config {
     /// 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

diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -246,22 +246,19 @@ TEST_F(ConfigCompileTests, PathSpecMatch) {
 }
 
 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())

diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "../clang-tidy/ClangTidyOptions.h"
 #include "Annotations.h"
 #include "Config.h"
 #include "Diagnostics.h"
@@ -18,18 +19,28 @@
 #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 {
@@ -42,6 +53,7 @@ using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::IsEmpty;
+using ::testing::Not;
 using ::testing::Pair;
 using ::testing::SizeIs;
 using ::testing::UnorderedElementsAre;
@@ -889,7 +901,8 @@ TEST(DiagnosticsTest, RecursivePreamblePragmaOnce) {
     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));
 }
 
@@ -1597,9 +1610,9 @@ TEST(DiagsInHeaders, DiagInTransitiveInclude) {
   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 "
-                                     "required for all declarations")));
+              UnorderedElementsAre(Diag(Main.range(),
+                                        "in included file: a type specifier is "
+                                        "required for all declarations")));
 }
 
 TEST(DiagsInHeaders, DiagInMultipleHeaders) {
@@ -1628,9 +1641,8 @@ TEST(DiagsInHeaders, PreferExpansionLocation) {
       {"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) {
@@ -1646,9 +1658,9 @@ TEST(DiagsInHeaders, PreferExpansionLocationMacros) {
       {"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 "
-                                     "required for all declarations")));
+              UnorderedElementsAre(Diag(Main.range(),
+                                        "in included file: a type specifier is "
+                                        "required for all declarations")));
 }
 
 TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) {
@@ -1675,9 +1687,9 @@ TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) {
       no_type_spec_10;
       #endif)cpp"}};
   EXPECT_THAT(*TU.build().getDiagnostics(),
-              UnorderedElementsAre(
-                  Diag(Main.range(), "in included file: a type specifier is "
-                                     "required for all declarations")));
+              UnorderedElementsAre(Diag(Main.range(),
+                                        "in included file: a type specifier is "
+                                        "required for all declarations")));
 }
 
 TEST(DiagsInHeaders, OnlyErrorOrFatal) {
@@ -1897,8 +1909,6 @@ TEST(DiagnosticsTest, IncludeCleaner) {
   )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.
@@ -1908,11 +1918,11 @@ TEST(DiagnosticsTest, IncludeCleaner) {
   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"),
+               "included header unused.h is not used directly"),
+          withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
+          withFix(Fix(Test.range("fix"), "", "remove #include directive")))));
   auto &Diag = AST.getDiagnostics()->front();
   EXPECT_EQ(getDiagnosticDocURI(Diag.Source, Diag.ID, Diag.Name),
             std::string("https://clangd.llvm.org/guides/include-cleaner"));

diff --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -15,6 +15,7 @@
 #include "AST.h"
 #include "Annotations.h"
 #include "Compiler.h"
+#include "Config.h"
 #include "Diagnostics.h"
 #include "Headers.h"
 #include "ParsedAST.h"
@@ -23,6 +24,7 @@
 #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"
@@ -33,6 +35,7 @@
 #include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <utility>
 
 namespace clang {
 namespace clangd {
@@ -513,6 +516,13 @@ TEST(ParsedASTTest, HeaderGuards) {
 //   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";