[PATCH] D111870: [clangd] Add a way to enable IncludeCleaner through config

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Config.h:89
 
   /// Controls warnings and errors when parsing code.
+  enum IncludeCleanerPolicy { UnusedHeaders, None };
----------------
you've accidentally split this comment from its decl


================
Comment at: clang-tools-extra/clangd/Config.h:90
   /// Controls warnings and errors when parsing code.
+  enum IncludeCleanerPolicy { UnusedHeaders, None };
   struct {
----------------
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` 


================
Comment at: clang-tools-extra/clangd/Config.h:94
     llvm::StringSet<> Suppress;
+    IncludeCleanerPolicy IncludeCleaner = None;
 
----------------
don't group with the generic suppression bits


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:213
 
+    /// Valid values are:
+    /// - UnusedHeaders
----------------
This is where the documentation for the feature goes :-)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:215
+    D.Message = "Included header is unused";
+    D.Name = "clangd-include-cleaner";
+    // FIXME: This range should be the whole line with target #include.
----------------
Source should be `Clangd` I think (our first?).

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


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:216
+    D.Name = "clangd-include-cleaner";
+    // FIXME: This range should be the whole line with target #include.
+    D.Range.start.line = Inc->HashLine;
----------------
remove FIXEDME


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:221
+    D.Range.end.character =
+        Code.drop_front(Inc->HashOffset).find_first_of("\n\1A");
+    D.Fixes.emplace_back();
----------------
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)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:226
+    D.Fixes.back().Edits.back().range.start.line = Inc->HashLine;
+    // LSP uses [start; end) ranges: this will span the deletion range to EOL.
+    D.Fixes.back().Edits.back().range.end.line = Inc->HashLine + 1;
----------------
No need to explain the ranges thing, this is ubiquitous and documented on the structs.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:229
+    D.InsideMainFile = true;
+    AST.getSourceManager();
+    D.File = AST.getSourceManager()
----------------
no-op


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:230
+    AST.getSourceManager();
+    D.File = AST.getSourceManager()
+                 .getFileEntryForID(AST.getSourceManager().getMainFileID())
----------------
don't do this in the loop


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:235
+    D.Severity = DiagnosticsEngine::Warning;
+    Result.push_back(std::move(D));
+  }
----------------
D.Tags.push_back(Unneccesary)

This produces nice rendering in vscode


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:67
+std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
+                                                 StringRef Code);
+
----------------
nit: llvm::StringRef


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:380
         if (IsClangTidyDiag) {
           if (Cfg.Diagnostics.Suppress.contains(CheckName))
             return DiagnosticsEngine::Ignored;
----------------
You're bypassing this logic, which is OK, but you'll need to duplicate it somewhere. (Probably wherever is looking at cfg anyway)


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:519
   }
-  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),
----------------
You're missing tests for this functionality


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:524
+  if (Result.Diags && Cfg.Diagnostics.IncludeCleaner ==
+                          Config::IncludeCleanerPolicy::UnusedHeaders)
+    for (const auto &D :
----------------
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)


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:525
+                          Config::IncludeCleanerPolicy::UnusedHeaders)
+    for (const auto &D :
+         issueUnusedIncludesDiagnostics(Result, Inputs.Contents))
----------------
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111870