[clangd] Add a way to enable IncludeCleaner through config

[kbobyrev]

Summary

This is useful for dogfooding the feature and spotting bugs.

[kbobyrev]

Delete debugging leftovers.

[kbobyrev]

Fix the warning range.

[sammccall]

clang-tools-extra/clangd/Config.h

89–90

you've accidentally split this comment from its decl

90

structure, as discussed offline...

"include cleaner" refers to a set of features to do with header hygiene, related but I think we still want to configure them individually.

"Unused headers" is the feature we're trying to configure, so I think it's a key rather than a value. Its value could be a boolean, (and configure other aspects separately), or we could make it an enum so we can choose a policy at the same time.
My suspicion is that the major knob here is strictness: do we warn when a header doesn't *directly* provide any used symbols, or also indirectly?

So I'd probably call this UnusedInclude: Strict

94

don't group with the generic suppression bits

clang-tools-extra/clangd/ConfigFragment.h

213

This is where the documentation for the feature goes :-)

clang-tools-extra/clangd/IncludeCleaner.cpp

215

Source should be Clangd I think (our first?).

Name should be something like "unused-include": this doesn't need to echo the tool name.

216

remove FIXEDME

221

This doesn't handle hitting EOF correctly, and also gets encodings wrong (should be UTF-16 code units, unless we're in another mode).

I think you want:

D.range.end.character = lspLength(Code.drop_front(Inc->HashOffset).take_until([](char C) { return c == '\n' || c == '\r'; }));

You could make this a function in SourceCode.h if you like.
(I'm fairly sure we're not clean when it comes to using \r being a line separator in the rest of the code BTW)

226

No need to explain the ranges thing, this is ubiquitous and documented on the structs.

229

no-op

230

don't do this in the loop

235

D.Tags.push_back(Unneccesary)

This produces nice rendering in vscode

clang-tools-extra/clangd/IncludeCleaner.h

67

nit: llvm::StringRef

clang-tools-extra/clangd/ParsedAST.cpp

380

You're bypassing this logic, which is OK, but you'll need to duplicate it somewhere. (Probably wherever is looking at cfg anyway)

519

You're missing tests for this functionality

524

If/when we support other policies, the issueUnusedIncludesDiagnostics function will need to look at the current policy, so you might want to move this check there already. (We also need to check for suppression, as mentioned above)

525

you're copying all the diags here

I don't mind the temp vector to keep the signature nice, but Result.Diags->insert(Result.Diags->end(), make_move_iterator(NewDiags.begin()), etc)?

[kbobyrev]

Address review comments.

[kbobyrev]

Rebase on top of main, revert unwanted formatting change.

[kbobyrev]

Elaborate a bit more on Strict mode for UnusedHeaders.

[kbobyrev]

[clangd] IncludeCleaner: Implement more complicated rules for considering enums forward declarations "used"

[kbobyrev]

Prevent accidental changes from getting into this patch.

[sammccall]

Mostly dumb nitpicks because this is user-facing magic strings... let's chat before doing another round so I'm not wasting your time.

---

You've done some renaming from UnusedInclude -> UnusedHeaders.

I feel bad because I did say in the last round

"Unused headers" is the feature we're trying to configure

but I wasn't intending to be precise and actually think the old name is better (some discussion below)

clang-tools-extra/clangd/ConfigFragment.h

215

I'm not sure we need a separate block for this. The main cost is an extra line in the config file, and it makes the "include cleaner" name user-facing, so it's an extra concept people have to learn.

Do you think we'll need a lot more than UnusedHeaders/MissingHeaders?

216

s/IncludeCleaner/clangd

225

I wonder whether "UnusedIncludes" would be clearer, especially given the pairing with "MissingHeaders" (consider confusion with pp_file_not_found) vs "MissingIncludes".

Conceptually it's an unused edge rather than node, but mostly i'm just thinking "MissingIncludes" more clearly describes the nature of the fix.

clang-tools-extra/clangd/IncludeCleaner.cpp

136

maybe worth a comment that we don't bother to handle escaped newlines etc

143

This code is making me a bit dizzy. I get the idea that we're using the precise HashOffset while exploiting the fact that we know the line number to avoid counting lines. But I'm not sure it's worthwhile, we're near the top of the file after all...

What about just

Result.end = result.start = offsetToPosition(code, HashOffset)
Result.end.character += lspLength(Code.drop_front(HashOffset).take_until(...));

210

config is usually passed by TLS rather than explicitly

215

clangd-unused-header -> unused-header

(there's a test, but it's stale)

225

include at least the basename in the message - this also gets listed in e.g. diagnostics view

233

again, we're not removing the header, we're removing the inclusion
"remove #include directive"?

[kbobyrev]

clang-tools-extra/clangd/IncludeCleaner.cpp

215

what do you mean by "stale"?

[kbobyrev]

Resolve review comments.

[kbobyrev]

Use IncludeCleaner to get rid of unused include in IncludeCleaner.cpp 🆒

[sammccall]

Awesome, let's ship it!
Only nits on the text, feel free to ignore any you don't like

clang-tools-extra/clangd/ConfigFragment.h

213

brevity: consider "correct" or "diagnose" instead of "treat and warn users on".
specificity: consider "unnecessary #include directives" instead of "suboptimal include usage".

217

We're glossing over the key idea ("come from") here, and mixing what the Strict policy is about with what the feature is about. I think we can be a bit more precise.

I'd add a second sentence to the first paragraph, elaborating a little:

clangd can warn if a header is `#included` but not used, and suggest removing it.

And then we can define the Strict policy, and mention its limitations.

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.

220

I think at least the "fixme" part isn't suitable for public docs.
We also don't mention what we want to do instead.

I'd suggest:

• move this to the part of the code that generates fixes
• mention that we if it's transitively used, we could suggest replacing it with the needed #includes instead (I think this is actually pretty feasible...)
• while there, maybe also add a FIXME that we should suggest adding IWYU pragma export/keep as an alternative fixes (once we honor that)

clang-tools-extra/clangd/IncludeCleaner.cpp

215

You'd updated the diagnostic name, but not the suppression check. I was going to suggest a test for suppression, and realized you had one, it was also testing the old name though.

LG now!

224

This is totally up to you, but wanted to be explicit with what I meant about basename...

If the path is long, this can be: included header this/is/a/long/path/to/some/header.h is not used
Depending how you feel about having to visually scan past the path to get to the end of the sentence, you might choose to trade precision for brevity: included header header.h is not used.

This is llvm::sys::path::filename(stripHeaderQuotes(Inc->Written), posix), with stripHeaderQuotes to be written.

Again, I don't feel strongly, your call

[kbobyrev]

Resolve the review comments.

clang-tools-extra/clangd/Config.h

struct Config {
   };
   /// Controls background-index behavior.
   struct {
     /// Whether this TU should be indexed.
     BackgroundPolicy Background = BackgroundPolicy::Build;
     ExternalIndexSpec External;
   } Index;
 
   /// Controls warnings and errors when parsing code.
+  enum IncludeCleanerPolicy { UnusedHeaders, None };
   struct {
     bool SuppressAll = false;
     llvm::StringSet<> Suppress;
+    IncludeCleanerPolicy IncludeCleaner = None;
 
     /// Configures what clang-tidy checks to run and options to use with them.
     struct {
       // A comma-seperated list of globs specify which clang-tidy checks to run.
       std::string Checks;
       llvm::StringMap<std::string> CheckOptions;
     } ClangTidy;
   } Diagnostics;

clang-tools-extra/clangd/ConfigCompile.cpp

if (!Normalized.empty())
       Out.Apply.push_back(
           [Normalized(std::move(Normalized))](const Params &, Config &C) {
             if (C.Diagnostics.SuppressAll)
               return;
             for (llvm::StringRef N : Normalized)
               C.Diagnostics.Suppress.insert(N);
           });
 
+    if (F.IncludeCleaner) {
+      if (auto Val = compileEnum<Config::IncludeCleanerPolicy>(
+                         "IncludeCleaner", **F.IncludeCleaner)
+                         .map("UnusedHeaders",
+                              Config::IncludeCleanerPolicy::UnusedHeaders)
+                         .map("None", Config::IncludeCleanerPolicy::None)
+                         .value())
+        Out.Apply.push_back([Val](const Params &, Config &C) {
+          C.Diagnostics.IncludeCleaner = *Val;
+        });
+    }
+
     compile(std::move(F.ClangTidy));
   }
 
   void compile(Fragment::StyleBlock &&F) {
     if (!F.FullyQualifiedNamespaces.empty()) {
       std::vector<std::string> FullyQualifiedNamespaces;
       for (auto &N : F.FullyQualifiedNamespaces) {
         // Normalize the data by dropping both leading and trailing ::

clang-tools-extra/clangd/ConfigFragment.h

struct DiagnosticsBlock {
     /// - warning categories (e.g. unused-result)
     /// - clang-tidy check names (e.g. bugprone-narrowing-conversions)
     ///
     /// This is a simple filter. Diagnostics can be controlled in other ways
     /// (e.g. by disabling a clang-tidy check, or the -Wunused compile flag).
     /// This often has other advantages, such as skipping some analysis.
     std::vector<Located<std::string>> Suppress;
 
+    /// Valid values are:
+    /// - UnusedHeaders
+    /// - None
+    llvm::Optional<Located<std::string>> IncludeCleaner;
+
     /// Controls how clang-tidy will run over the code base.
     ///
     /// The settings are merged with any settings found in .clang-tidy
     /// configiration files with these ones taking precedence.
     struct ClangTidyBlock {
       std::vector<Located<std::string>> Add;
       /// List of checks to disable.
       /// Takes precedence over Add. To enable all llvm checks except include
       /// order:
       ///   Add: llvm-*
       ///   Remove: llvm-include-onder
       std::vector<Located<std::string>> Remove;
 
       /// A Key-Value pair list of options to pass to clang-tidy checks
       /// These take precedence over options specified in clang-tidy
       /// configuration files. Example:

clang-tools-extra/clangd/ConfigYAML.cpp

Dict.handle("FullyQualifiedNamespaces", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.FullyQualifiedNamespaces = std::move(*Values);
     });
     Dict.parse(N);
   }
 
   void parse(Fragment::DiagnosticsBlock &F, Node &N) {
     DictParser Dict("Diagnostics", this);
+    Dict.handle("IncludeCleaner", [&](Node &N) {
+      F.IncludeCleaner = scalarValue(N, "IncludeCleaner");
+    });
     Dict.handle("Suppress", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.Suppress = std::move(*Values);
     });
     Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
     Dict.parse(N);
   }
 

clang-tools-extra/clangd/IncludeCleaner.h

 
 /// Retrieves headers that are referenced from the main file but not used.
 std::vector<const Inclusion *>
 getUnused(const IncludeStructure &Includes,
           const llvm::DenseSet<IncludeStructure::HeaderID> &ReferencedFiles);
 
 std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
 
+std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST);
+
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_CLEANER_H

clang-tools-extra/clangd/IncludeCleaner.cpp

if (!SM.isWrittenInScratchSpace(Exp.getSpellingLoc()))
       add(Exp.getSpellingLoc());
     if (!SM.isWrittenInScratchSpace(Exp.getExpansionLocStart()))
       add(Exp.getExpansionLocStart());
     if (!SM.isWrittenInScratchSpace(Exp.getExpansionLocEnd()))
       add(Exp.getExpansionLocEnd());
   }
 };
 
 } // namespace
 
 ReferencedLocations findReferencedLocations(ParsedAST &AST) {
   ReferencedLocations Result;
   ReferencedLocationCrawler Crawler(Result);
   Crawler.TraverseAST(AST.getASTContext());
   // FIXME(kirillbobyrev): Handle macros.
   return Result;
 }
 
 llvm::DenseSet<FileID>
 findReferencedFiles(const llvm::DenseSet<SourceLocation> &Locs,
                     const SourceManager &SM) {
   std::vector<SourceLocation> Sorted{Locs.begin(), Locs.end()};
   llvm::sort(Sorted); // Group by FileID.
   ReferencedFiles Result(SM);
std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST) {
   const auto &SM = AST.getSourceManager();
 
   auto Refs = findReferencedLocations(AST);
   auto ReferencedFiles = translateToHeaderIDs(findReferencedFiles(Refs, SM),
                                               AST.getIncludeStructure(), SM);
   return getUnused(AST.getIncludeStructure(), ReferencedFiles);
 }
 
+std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST) {
+  std::vector<Diag> Result;
+  for (const auto *Inc : computeUnusedIncludes(AST)) {
+    Diag D;
+    D.Message = "Included header is unused";
+    D.Name = "clangd-include-cleaner";
+    // FIXME: This range should be the whole line with target #include.
+    D.Range.start.line = Inc->HashLine;
+    D.Range.end.line = Inc->HashLine;
+    D.Fixes.emplace_back();
+    D.Fixes.back().Message = "Remove unused include";
+    D.Fixes.back().Edits.emplace_back();
+    D.Fixes.back().Edits.back().range = D.Range;
+    D.InsideMainFile = true;
+    AST.getSourceManager();
+    D.File = AST.getSourceManager()
+                 .getFileEntryForID(AST.getSourceManager().getMainFileID())
+                 ->getName()
+                 .str();
+    D.Severity = DiagnosticsEngine::Warning;
+    Result.push_back(std::move(D));
+  }
+  return Result;
+}
+
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/ParsedAST.cpp

 #include "AST.h"
 #include "Compiler.h"
 #include "Config.h"
 #include "Diagnostics.h"
 #include "Feature.h"
 #include "FeatureModule.h"
 #include "Headers.h"
 #include "HeuristicResolver.h"
+#include "IncludeCleaner.h"
 #include "IncludeFixer.h"
 #include "Preamble.h"
 #include "SourceCode.h"
 #include "TidyProvider.h"
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
   //    ancestors outside this scope).
   // In practice almost all checks work well without modifications.
   std::vector<std::unique_ptr<tidy::ClangTidyCheck>> CTChecks;
   ast_matchers::MatchFinder CTFinder;
   llvm::Optional<tidy::ClangTidyContext> CTContext;
   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 =
         getTidyOptionsForFile(Inputs.ClangTidyProvider, Filename);
     dlog("ClangTidy configuration for file {0}: {1}", Filename,
          tidy::configurationAsText(ClangTidyOpts));
     tidy::ClangTidyCheckFactories CTFactories;
     for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
       E.instantiate()->addCheckFactories(CTFactories);
     CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
         tidy::ClangTidyGlobalOptions(), ClangTidyOpts));
     CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
     CTContext->setASTContext(&Clang->getASTContext());
     CTContext->setCurrentFile(Filename);
     CTChecks = CTFactories.createChecks(CTContext.getPointer());
     llvm::erase_if(CTChecks, [&](const auto &Check) {
       return !Check->isLanguageVersionSupported(CTContext->getLangOpts());
     });
     Preprocessor *PP = &Clang->getPreprocessor();
     for (const auto &Check : CTChecks) {
       Check->registerPPCallbacks(Clang->getSourceManager(), PP, PP);
       Check->registerMatchers(&CTFinder);
     }
 
-    const Config &Cfg = Config::current();
     ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
       if (Cfg.Diagnostics.SuppressAll ||
           isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress))
         return DiagnosticsEngine::Ignored;
       if (!CTChecks.empty()) {
         std::string CheckName = CTContext->getCheckName(Info.getID());
         bool IsClangTidyDiag = !CheckName.empty();
         if (IsClangTidyDiag) {
           if (Cfg.Diagnostics.Suppress.contains(CheckName))
             return DiagnosticsEngine::Ignored;
           // Check for suppression comment. Skip the check for diagnostics not
           // in the main file, because we don't want that function to query the
           // source buffer for preamble files. For the same reason, we ask
           // shouldSuppressDiagnostic to avoid I/O.
           // We let suppression comments take precedence over warning-as-error
           // to match clang-tidy's behaviour.
           bool IsInsideMainFile =
if (Preamble)
       Diags->insert(Diags->end(), Preamble->Diags.begin(),
                     Preamble->Diags.end());
     // Finally, add diagnostics coming from the AST.
     {
       std::vector<Diag> D = ASTDiags.take(CTContext.getPointer());
       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 && Cfg.Diagnostics.IncludeCleaner ==
+                          Config::IncludeCleanerPolicy::UnusedHeaders)
+    for (const auto &D : issueUnusedIncludesDiagnostics(Result))
+      Result.Diags->push_back(D);
+  return Result;
 }
 
 ParsedAST::ParsedAST(ParsedAST &&Other) = default;
 
 ParsedAST &ParsedAST::operator=(ParsedAST &&Other) = default;
 
 ParsedAST::~ParsedAST() {
   if (Action) {

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

for (const auto &Case : Cases) {
     Frag = {};
     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.
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.IncludeCleaner,
+            Config::IncludeCleanerPolicy::None);
+
+  Frag = {};
+  Frag.Diagnostics.IncludeCleaner.emplace("None");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.IncludeCleaner,
+            Config::IncludeCleanerPolicy::None);
+
+  Frag = {};
+  Frag.Diagnostics.IncludeCleaner.emplace("UnusedHeaders");
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Diagnostics.IncludeCleaner,
+            Config::IncludeCleanerPolicy::UnusedHeaders);
+}
+
 TEST_F(ConfigCompileTests, DiagnosticSuppression) {
   Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move");
   Frag.Diagnostics.Suppress.emplace_back("unreachable-code");
   Frag.Diagnostics.Suppress.emplace_back("-Wunused-variable");
   Frag.Diagnostics.Suppress.emplace_back("typecheck_bool_condition");
   Frag.Diagnostics.Suppress.emplace_back("err_unexpected_friend");
   Frag.Diagnostics.Suppress.emplace_back("warn_alloca");
   EXPECT_TRUE(compileAndApply());

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

 Index:
   Background: Skip
 ---
 Diagnostics:
   ClangTidy:
     CheckOptions:
       IgnoreMacros: true
       example-check.ExampleOption: 0
+  IncludeCleaner: UnusedHeaders
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
   EXPECT_THAT(Diags.Files, ElementsAre("config.yaml"));
   ASSERT_EQ(Results.size(), 4u);
   EXPECT_FALSE(Results[0].If.HasUnrecognizedCondition);
   EXPECT_THAT(Results[0].If.PathMatch, ElementsAre(Val("abc")));
   EXPECT_THAT(Results[0].CompileFlags.Add, ElementsAre(Val("foo"), Val("bar")));
 
   EXPECT_THAT(Results[1].CompileFlags.Add, ElementsAre(Val("b\naz\n")));
 
   ASSERT_TRUE(Results[2].Index.Background);
   EXPECT_EQ("Skip", *Results[2].Index.Background.getValue());
   EXPECT_THAT(Results[3].Diagnostics.ClangTidy.CheckOptions,
               ElementsAre(PairVal("IgnoreMacros", "true"),
                           PairVal("example-check.ExampleOption", "0")));
+  EXPECT_TRUE(Results[3].Diagnostics.IncludeCleaner);
+  EXPECT_EQ("UnusedHeaders", *Results[3].Diagnostics.IncludeCleaner.getValue());
 }
 
 TEST(ParseYAML, Locations) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
 If:
   PathMatch: [['???bad***regex(((']]
   )yaml");