kbobyrev updated this revision to Diff 380665.
kbobyrev marked 16 inline comments as done.
kbobyrev added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111870/new/

https://reviews.llvm.org/D111870

Files:
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1464,6 +1464,45 @@
                   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"
+
+  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";
+  // Off by default.
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+  Config Cfg;
+  Cfg.Diagnostics.IncludeCleaner.UnusedHeaders =
+      Config::UnusedHeadersPolicy::Strict;
+  WithContextValue WithCfg(Config::Key, std::move(Cfg));
+  EXPECT_THAT(
+      *TU.build().getDiagnostics(),
+      UnorderedElementsAre(
+          AllOf(Diag(Test.range("diag"), "included header is not used"),
+                WithTag(DiagnosticTag::Unnecessary), DiagSource(Diag::Clangd),
+                WithFix(Fix(Test.range("fix"), "", "remove unused header")))));
+  Cfg.Diagnostics.SuppressAll = true;
+  WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg));
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+  Cfg.Diagnostics.SuppressAll = false;
+  Cfg.Diagnostics.Suppress = {"clangd-unused-header"};
+  WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -67,6 +67,8 @@
     CheckOptions:
       IgnoreMacros: true
       example-check.ExampleOption: 0
+  IncludeCleaner:
+    UnusedHeaders: Strict
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
@@ -83,6 +85,9 @@
   EXPECT_THAT(Results[3].Diagnostics.ClangTidy.CheckOptions,
               ElementsAre(PairVal("IgnoreMacros", "true"),
                           PairVal("example-check.ExampleOption", "0")));
+  EXPECT_TRUE(Results[3].Diagnostics.IncludeCleaner.UnusedHeaders);
+  EXPECT_EQ("Strict",
+            *Results[3].Diagnostics.IncludeCleaner.UnusedHeaders.getValue());
 }
 
 TEST(ParseYAML, Locations) {
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -244,6 +244,25 @@
   }
 }
 
+TEST_F(ConfigCompileTests, DiagnosticsIncludeCleaner) {
+  // Defaults to None.
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.IncludeCleaner.UnusedHeaders,
+            Config::UnusedHeadersPolicy::None);
+
+  Frag = {};
+  Frag.Diagnostics.IncludeCleaner.UnusedHeaders.emplace("None");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.IncludeCleaner.UnusedHeaders,
+            Config::UnusedHeadersPolicy::None);
+
+  Frag = {};
+  Frag.Diagnostics.IncludeCleaner.UnusedHeaders.emplace("Strict");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.IncludeCleaner.UnusedHeaders,
+            Config::UnusedHeadersPolicy::Strict);
+}
+
 TEST_F(ConfigCompileTests, DiagnosticSuppression) {
   Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move");
   Frag.Diagnostics.Suppress.emplace_back("unreachable-code");
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -18,6 +18,7 @@
 #include "FeatureModule.h"
 #include "Headers.h"
 #include "HeuristicResolver.h"
+#include "IncludeCleaner.h"
 #include "IncludeFixer.h"
 #include "Preamble.h"
 #include "SourceCode.h"
@@ -342,6 +343,7 @@
   llvm::Optional<IncludeFixer> FixIncludes;
   // No need to run clang-tidy or IncludeFixerif we are not going to surface
   // diagnostics.
+  const Config &Cfg = Config::current();
   if (PreserveDiags) {
     trace::Span Tracer("ClangTidyInit");
     tidy::ClangTidyOptions ClangTidyOpts =
@@ -366,7 +368,6 @@
       Check->registerMatchers(&CTFinder);
     }
 
-    const Config &Cfg = Config::current();
     ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
       if (Cfg.Diagnostics.SuppressAll ||
@@ -515,10 +516,18 @@
       Diags->insert(Diags->end(), D.begin(), D.end());
     }
   }
-  return ParsedAST(Inputs.Version, std::move(Preamble), std::move(Clang),
+  ParsedAST Result(Inputs.Version, std::move(Preamble), std::move(Clang),
                    std::move(Action), std::move(Tokens), std::move(Macros),
                    std::move(Marks), std::move(ParsedDecls), std::move(Diags),
                    std::move(Includes), std::move(CanonIncludes));
+  if (Result.Diags) {
+    auto UnusedHeadersDiags =
+        issueUnusedHeadersDiagnostics(Result, Cfg, Inputs.Contents);
+    Result.Diags->insert(Result.Diags->end(),
+                         make_move_iterator(UnusedHeadersDiags.begin()),
+                         make_move_iterator(UnusedHeadersDiags.end()));
+  }
+  return Result;
 }
 
 ParsedAST::ParsedAST(ParsedAST &&Other) = default;
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -21,6 +21,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_CLEANER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_CLEANER_H
 
+#include "Config.h"
 #include "Headers.h"
 #include "ParsedAST.h"
 #include "clang/Basic/SourceLocation.h"
@@ -63,6 +64,10 @@
 
 std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
 
+std::vector<Diag> issueUnusedHeadersDiagnostics(ParsedAST &AST,
+                                                const Config &Cfg,
+                                                llvm::StringRef Code);
+
 } // namespace clangd
 } // namespace clang
 
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -7,6 +7,9 @@
 //===----------------------------------------------------------------------===//
 
 #include "IncludeCleaner.h"
+#include "Config.h"
+#include "Protocol.h"
+#include "SourceCode.h"
 #include "support/Logger.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
@@ -133,6 +136,31 @@
   }
 };
 
+// Returns the range starting at '#' and ending at EOL.
+clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned Line,
+                                 unsigned HashOffset) {
+  clangd::Range Result;
+  Result.start.line = Line;
+  Result.end.line = Line;
+
+  // Calculate the offset from beginning of the file, EOL to '#'.
+  llvm::StringRef Prefix = Code.drop_back(Code.size() - HashOffset);
+  while (!Prefix.empty() && Prefix.back() != '\n' && Prefix.back() != '\r') {
+    Prefix = Prefix.drop_back();
+  }
+  unsigned HashLineOffset = lspLength(
+      Code.drop_back(Code.size() - HashOffset).drop_front(Prefix.size()));
+
+  // Span the warning until the EOL or EOF.
+  Result.start.character = HashLineOffset;
+  Result.end.character =
+      HashLineOffset +
+      lspLength(Code.drop_front(HashOffset).take_until([](char C) {
+        return C == '\n' || C == '\r';
+      }));
+  return Result;
+}
+
 } // namespace
 
 ReferencedLocations findReferencedLocations(ParsedAST &AST) {
@@ -206,5 +234,39 @@
   return getUnused(AST.getIncludeStructure(), ReferencedFiles);
 }
 
+std::vector<Diag> issueUnusedHeadersDiagnostics(ParsedAST &AST,
+                                                const Config &Cfg,
+                                                llvm::StringRef Code) {
+  if (Cfg.Diagnostics.IncludeCleaner.UnusedHeaders !=
+          Config::UnusedHeadersPolicy::Strict ||
+      Cfg.Diagnostics.SuppressAll ||
+      Cfg.Diagnostics.Suppress.contains("clangd-unused-header"))
+    return {};
+  std::vector<Diag> Result;
+  std::string FileName =
+      AST.getSourceManager()
+          .getFileEntryForID(AST.getSourceManager().getMainFileID())
+          ->getName()
+          .str();
+  for (const auto *Inc : computeUnusedIncludes(AST)) {
+    Diag D;
+    D.Message = "included header is not used";
+    D.Name = "unused-header";
+    D.Source = Diag::DiagSource::Clangd;
+    D.File = FileName;
+    D.Severity = DiagnosticsEngine::Warning;
+    D.Tags.push_back(Unnecessary);
+    D.Range = getDiagnosticRange(Code, Inc->HashLine, Inc->HashOffset);
+    D.Fixes.emplace_back();
+    D.Fixes.back().Message = "remove unused header";
+    D.Fixes.back().Edits.emplace_back();
+    D.Fixes.back().Edits.back().range.start.line = Inc->HashLine;
+    D.Fixes.back().Edits.back().range.end.line = Inc->HashLine + 1;
+    D.InsideMainFile = true;
+    Result.push_back(std::move(D));
+  }
+  return Result;
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Diagnostics.h
===================================================================
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -101,6 +101,7 @@
     Unknown,
     Clang,
     ClangTidy,
+    Clangd,
     ClangdConfig,
   } Source = Unknown;
   /// Elaborate on the problem, usually pointing to a related piece of code.
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -485,6 +485,9 @@
   case Diag::ClangTidy:
     Main.source = "clang-tidy";
     break;
+  case Diag::Clangd:
+    Main.source = "clangd";
+    break;
   case Diag::ClangdConfig:
     Main.source = "clangd-config";
     break;
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -118,10 +118,19 @@
       if (auto Values = scalarValues(N))
         F.Suppress = std::move(*Values);
     });
+    Dict.handle("IncludeCleaner", [&](Node &N) { parse(F.IncludeCleaner, N); });
     Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
     Dict.parse(N);
   }
 
+  void parse(Fragment::DiagnosticsBlock::IncludeCleanerBlock &F, Node &N) {
+    DictParser Dict("IncludeCleaner", this);
+    Dict.handle("UnusedHeaders", [&](Node &N) {
+      F.UnusedHeaders = scalarValue(N, "UnusedIncludes");
+    });
+    Dict.parse(N);
+  }
+
   void parse(Fragment::DiagnosticsBlock::ClangTidyBlock &F, Node &N) {
     DictParser Dict("ClangTidy", this);
     Dict.handle("Add", [&](Node &N) {
@@ -218,7 +227,7 @@
     // If Key is seen twice, Parse runs only once and an error is reported.
     void handle(llvm::StringLiteral Key, std::function<void(Node &)> Parse) {
       for (const auto &Entry : Keys) {
-        (void) Entry;
+        (void)Entry;
         assert(Entry.first != Key && "duplicate key handler");
       }
       Keys.emplace_back(Key, std::move(Parse));
Index: clang-tools-extra/clangd/ConfigFragment.h
===================================================================
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -210,6 +210,19 @@
     /// This often has other advantages, such as skipping some analysis.
     std::vector<Located<std::string>> Suppress;
 
+    /// Controls how clangd will treat and warn users on suboptimal include
+    /// usage.
+    struct IncludeCleanerBlock {
+      /// When set to Strict, IncludeCleaner will warn about all headers that
+      /// are not used in the main file but are directly included from it.
+      ///
+      /// Valid values are:
+      /// - Strict
+      /// - None
+      llvm::Optional<Located<std::string>> UnusedHeaders;
+    };
+    IncludeCleanerBlock IncludeCleaner;
+
     /// Controls how clang-tidy will run over the code base.
     ///
     /// The settings are merged with any settings found in .clang-tidy
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -414,6 +414,7 @@
               C.Diagnostics.Suppress.insert(N);
           });
 
+    compile(std::move(F.IncludeCleaner));
     compile(std::move(F.ClangTidy));
   }
 
@@ -458,6 +459,18 @@
     CurSpec += Str;
   }
 
+  void compile(Fragment::DiagnosticsBlock::IncludeCleanerBlock &&F) {
+    if (F.UnusedHeaders)
+      if (auto Val = compileEnum<Config::UnusedHeadersPolicy>("IncludeCleaner",
+                                                              **F.UnusedHeaders)
+                         .map("Strict", Config::UnusedHeadersPolicy::Strict)
+                         .map("None", Config::UnusedHeadersPolicy::None)
+                         .value())
+        Out.Apply.push_back([Val](const Params &, Config &C) {
+          C.Diagnostics.IncludeCleaner.UnusedHeaders = *Val;
+        });
+  }
+
   void compile(Fragment::DiagnosticsBlock::ClangTidyBlock &&F) {
     std::string Checks;
     for (auto &CheckGlob : F.Add)
Index: clang-tools-extra/clangd/Config.h
===================================================================
--- clang-tools-extra/clangd/Config.h
+++ clang-tools-extra/clangd/Config.h
@@ -86,6 +86,7 @@
     ExternalIndexSpec External;
   } Index;
 
+  enum UnusedHeadersPolicy { Strict, None };
   /// Controls warnings and errors when parsing code.
   struct {
     bool SuppressAll = false;
@@ -97,6 +98,10 @@
       std::string Checks;
       llvm::StringMap<std::string> CheckOptions;
     } ClangTidy;
+
+    struct {
+      UnusedHeadersPolicy UnusedHeaders = None;
+    } IncludeCleaner;
   } Diagnostics;
 
   /// Style of the codebase.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to