[clangd] UnusedIncludes: Strict config now uses the include-cleaner-library implementation.

[hokein]

Summary

And remove the classical clangd-own unused-include implementation.

[kadircet]

clang-tools-extra/clangd/ConfigCompile.cpp

438

i think we should at least be emitting a diagnostics to encourage people for moving back to strict, so what about something like:

if (F.UnusuedIncludes) {
  auto Val = compileEnum....; // only for Strict and None
  if (!Val && **F.UnusedIncludes == "Experiment") {
       diag(Warning, "Experiment is deprecated for UnusedIncludes, use Strict instead.", F.UnusedIncludes.Range);
       Val = Config::IncludesPolicy::Strict;
  }
}

clang-tools-extra/clangd/IncludeCleaner.cpp

769

can you also delete computeUnusedIncludes and its friends (also from the tests)?

[hokein]

Add diagnostic for Experiment flag usage.

clang-tools-extra/clangd/ConfigCompile.cpp

438

I thought it was not worth a diagnostic because this flag was introduced recently, and we have never advertised it to open-source users.

But the flag is in the recent clangd16 release, so it probably justifies the value.

BTW, looks like we forgot to update the release notes for clangd16, https://github.com/llvm/llvm-project/blob/release/16.x/clang-tools-extra/docs/ReleaseNotes.rst#improvements-to-clangd is empty.

clang-tools-extra/clangd/IncludeCleaner.cpp

769

this is in plan, but in a separate patch, https://reviews.llvm.org/D145776

[kadircet]

clang-tools-extra/clangd/ConfigCompile.cpp

438

But the flag is in the recent clangd16 release, so it probably justifies the value.

right.

BTW, looks like we forgot to update the release notes for clangd16, https://github.com/llvm/llvm-project/blob/release/16.x/clang-tools-extra/docs/ReleaseNotes.rst#improvements-to-clangd is empty.

yeah, i've aslo noticed that will take a look. but regarding this feature, I don't think we should be advertising Experiment there, right?

clang-tools-extra/clangd/IncludeCleaner.cpp

769

as mentioned on that patch, i think it's better to land them in single step, to make sure we can revert a single patch if we want the old behavior rather than multiple.

[hokein]

add the removal of the clangd-own implementation.

clang-tools-extra/clangd/Config.h

struct {
     ExternalIndexSpec External;
     bool StandardLibrary = true;
   } Index;
 
   enum class IncludesPolicy {
     /// Diagnose missing and unused includes.
     Strict,
     None,
-    /// The same as Strict, but using the include-cleaner library for
-    /// unused includes.
-    Experiment,
   };
   /// Controls warnings and errors when parsing code.
   struct {
     bool SuppressAll = false;
     llvm::StringSet<> Suppress;
 
     /// Configures what clang-tidy checks to run and options to use with them.
     struct {

clang-tools-extra/clangd/ConfigCompile.cpp

if (!Normalized.empty())
             for (llvm::StringRef N : Normalized)
               C.Diagnostics.Suppress.insert(N);
           });
 
     if (F.UnusedIncludes)
       if (auto Val = compileEnum<Config::IncludesPolicy>("UnusedIncludes",
                                                          **F.UnusedIncludes)
                          .map("Strict", Config::IncludesPolicy::Strict)
-                         .map("Experiment", Config::IncludesPolicy::Experiment)
+                         .map("Experiment",
+                              Config::IncludesPolicy::Strict) // for backward
+                                                              // compatibility
                          .map("None", Config::IncludesPolicy::None)
                          .value())
         Out.Apply.push_back([Val](const Params &, Config &C) {
           C.Diagnostics.UnusedIncludes = *Val;
         });
 
     if (F.AllowStalePreamble) {
       if (auto Val = F.AllowStalePreamble)

clang-tools-extra/clangd/ConfigFragment.h

struct DiagnosticsBlock {
     //
     /// Strict means a header is unused if it does not *directly* provide any
     /// symbol used in the file. Removing it may still break compilation if it
     /// transitively includes headers that are used. This should be fixed by
     /// including those headers directly.
     ///
     /// Valid values are:
     /// - Strict
-    /// - Experiment
     /// - None
     std::optional<Located<std::string>> UnusedIncludes;
 
 
     /// Enable emitting diagnostics using stale preambles.
     std::optional<Located<bool>> AllowStalePreamble;
 
     /// Controls if clangd should analyze missing #include directives.

clang-tools-extra/clangd/IncludeCleaner.cpp

std::vector<Diag> issueIncludeCleanerDiagnostics(ParsedAST &AST,
   if (AST.getLangOpts().ObjC)
     return {};
 
   trace::Span Tracer("IncludeCleaner::issueIncludeCleanerDiagnostics");
 
   const Config &Cfg = Config::current();
   IncludeCleanerFindings Findings;
   if (Cfg.Diagnostics.MissingIncludes == Config::IncludesPolicy::Strict ||
-      Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Experiment) {
+      Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Strict) {
     // will need include-cleaner results, call it once
     Findings = computeIncludeCleanerFindings(AST);
   }
 
   std::vector<Diag> Result = generateUnusedIncludeDiagnostics(
-      AST.tuPath(),
-      Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Strict
-          ? computeUnusedIncludes(AST)
-          : Findings.UnusedIncludes,
-      Code);
+      AST.tuPath(), Findings.UnusedIncludes, Code);
   llvm::move(
       generateMissingIncludeDiagnostics(AST, Findings.MissingIncludes, Code),
       std::back_inserter(Result));
   return Result;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/Preamble.cpp

public:
   }
 
   void BeforeExecute(CompilerInstance &CI) override {
     CanonIncludes.addSystemHeadersMapping(CI.getLangOpts());
     LangOpts = &CI.getLangOpts();
     SourceMgr = &CI.getSourceManager();
     Includes.collect(CI);
     if (Config::current().Diagnostics.UnusedIncludes ==
-                Config::IncludesPolicy::Experiment ||
+                Config::IncludesPolicy::Strict ||
             Config::current().Diagnostics.MissingIncludes ==
             Config::IncludesPolicy::Strict)
       Pragmas.record(CI);
     if (BeforeExecuteCallback)
       BeforeExecuteCallback(CI);
   }
 
   std::unique_ptr<PPCallbacks> createPPCallbacks() override {

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

TEST(IncludeCleaner, IWYUPragmas) {
   TU.AdditionalFiles["behind_keep.h"] = guard("");
   TU.AdditionalFiles["exported.h"] = guard("");
   TU.AdditionalFiles["public.h"] = guard("#include \"private.h\"");
   TU.AdditionalFiles["private.h"] = guard(R"cpp(
     // IWYU pragma: private, include "public.h"
     void foo() {}
   )cpp");
   Config Cfg;
-  Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Experiment;
+  Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict;
   WithContextValue Ctx(Config::Key, std::move(Cfg));
   ParsedAST AST = TU.build();
 
   auto ReferencedFiles = findReferencedFiles(
       findReferencedLocations(AST), AST.getIncludeStructure(),
       AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_EQ(ReferencedFiles.SpelledUmbrellas.size(), 1u);
   EXPECT_EQ(ReferencedFiles.SpelledUmbrellas.begin()->getKey(), "\"public.h\"");