[PATCH] D143496: [clangd] Add support for missing includes analysis.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Config.h:91
 
+  enum class MissingIncludesPolicy {
+    /// Diagnose missing includes.
----------------
rather than duplicating, what about renaming `UnusedIncludesPolicy` to `IncludesPolicy` and use it for both `UnusedIncludes` and `MissingIncludes` options below?


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:224
 
-    /// Controls how clangd will correct "unnecessary #include directives.
+    /// Controls how clangd will correct "unnecessary" #include directives.
     /// clangd can warn if a header is `#include`d but not used, and suggest
----------------
nit: i'd rather drop the quotes completely to match your description of MissingIncludes below.


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:238
 
+    /// Controls how clangd handles missing #include directives.
+    /// clangd can warn if a header for a symbol is not `#include`d (missing),
----------------



================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:239-240
+    /// Controls how clangd handles missing #include directives.
+    /// clangd can warn if a header for a symbol is not `#include`d (missing),
+    /// and suggest adding it.
+    ///
----------------



================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:242
+    ///
+    /// Strict means a header is missing if it is not *directly #include'd.
+    /// The file might still compile if the header is included transitively.
----------------



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:516
+
+  std::vector<include_cleaner::SymbolReference> Macros =
+      collectMacroReferences(AST);
----------------



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:547
+convertIncludes(const SourceManager &SM,
+                std::vector<Inclusion> MainFileIncludes) {
+  include_cleaner::Includes Includes;
----------------
you can just pass an `llvm::ArrayRef<Inclusion>` to prevent a copy


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:550
+  for (const Inclusion &Inc : MainFileIncludes) {
+    llvm::ErrorOr<const FileEntry *> ResolvedOrError =
+        SM.getFileManager().getFile(Inc.Resolved);
----------------
you can re-write this as:
```
include_cleaner::Include TransformedInc;
TransformedInc.Spelled = Inc.Written.trim("\"<>");
TransformedInc.HashLocation = SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset); // we should actually convert this from a SourceLocation to offset in include_cleaner::Include as well
TransformedInc.Line = Inc.HashLine;
TransformedInc.Angled = WrittenRef.starts_with("<");
if(auto FE = SM.getFileManager().getFile(Inc.Resolved))
  TransformedInc.Resolved = *FE;
Includes.add(std::move(TransformedInc));
```


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:570
+computeMissingIncludes(ParsedAST &AST) {
+  std::vector<include_cleaner::SymbolReference> Macros =
+      collectMacroReferences(AST);
----------------
nit: `auto Macros = ..`


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:572
+      collectMacroReferences(AST);
+  std::vector<Inclusion> MainFileIncludes =
+      AST.getIncludeStructure().MainFileIncludes;
----------------
no need for copying to vector here, `const auto& MainFileIncludes = ...`


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:577
+      convertIncludes(AST.getSourceManager(), MainFileIncludes);
+  std::string FileName =
+      AST.getSourceManager()
----------------
you can use `AST.tuPath()`


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:582
+          .str();
+  if (FileName.find("foo") != std::string::npos) {
+    vlog("Include cleaner includes: {0}", IncludeCleanerIncludes.all().size());
----------------
looks like debugging artifact?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:593
+          llvm::ArrayRef<include_cleaner::Header> Providers) {
+        bool Satisfied = false;
+        for (const include_cleaner::Header &H : Providers) {
----------------
nit: you can check whether `Ref.RT` is `Explicit` at the top, and bail out early.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:597
+              H.physical() == MainFile) {
+            Satisfied = true;
+          }
----------------
nit: you can just `break` after satisfying the include (same below)


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:605
+            Ref.RT == include_cleaner::RefType::Explicit) {
+          std::string SpelledHeader = include_cleaner::spellHeader(
+              Providers.front(), AST.getPreprocessor().getHeaderSearchInfo(),
----------------
clangd has some header spelling customizations. so we should actually be doing this through `URI::includeSpelling(URI::create(getCanonicalPath(Providers.front().physical(), SM)))` first, and fall back to `spellHeader` if it fails for physical header providers to make sure we're consistent.

this is used by $EMPLOYER$'s integration to always spell includes relative to depot root, rather than certain include search paths.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:609
+          std::pair<FileID, unsigned int> FileIDAndOffset =
+              AST.getSourceManager().getDecomposedLoc(Ref.RefLocation);
+          Missing.push_back({SpelledHeader, FileIDAndOffset.second});
----------------
`RefLocation` is not necessarily spelled token location, e.g. it might be pointing at a macro expansion.

you can make use of `TokenBuffer` inside `ParsedAST` to go from this expanded location to a token spelled inside the main file, e.g. `Tokens.spelledForExpanded(Tokens.expandedTokens(Ref.RefLocation));` and use the full spelled token range afterwards for diagnostic location.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:661
+    D.Fixes.back().Message = "#include " + Missing.first;
+    D.Fixes.back().Edits.emplace_back();
+    D.Fixes.back().Edits.back().newText = "#include " + Missing.first + "\n";
----------------
we've got some tooling library to generate these edits, see https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Tooling/Inclusions/HeaderIncludes.h#L76. that way they'll be placed in "correct" position among the existing includes. you can then use [replacementToEdit](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/SourceCode.h#L148) to convert into a clangd edit.


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:695
+    auto MissingHeadersDiags =
+        issueMissingIncludesDiagnostics(Result, Inputs.Contents);
     Result.Diags->insert(Result.Diags->end(),
----------------
instead of introducing a new function here and duplicating logic what about something like:
```
auto IncludeDiags = issueIncludeDiagnostics(...);
```

in IncludeCleaner.cpp:
```
struct IncludeCleanerFindings {
   std::vector<Inclusion*> Unused;
   llvm::StringMap<std::vector<Range>> MissingIncludes; // Map from missing header spelling to ranges of references.
};

IncludeCleanerFindings computeIncludeCleanerFindings(AST) {
   auto Incs = convertIncludes();
   walkUsed(...., {
        // Update Missing includes, mark relevant main file includes as used.
   });
   // Create the unused includes set
   return Findings;
}


std::vector<Diag> issueIncludeDiagnostics(...) {
  std::vector<Inclusion*> UnusedIncludes;
  llvm::StringMap<std::vector</*Location of reference*/clangd::Range>> MissingIncludes;
  if (UnusedIncludePolicy is Strict) {
     UnusedIncludes = computeUnusedIncludes(...);
  } else if(UnusedIncludesPolicy is Experiment || MissingIncludesPolicy is Strict) {
     auto Findings = computeIncludeCleanerFindings(AST);
     if(UnusedIncludesPolicy is Experiment) {
           UnusedIncludes = std::move(Findings.Unused);
     }
     if(MissingIncludesPolicy is Strict) {
            MissingIncludes = std::move(Findings.Missing);
     }
  }
  for(auto &Inc: UnusedIncludes) {
       ... // create unused include diag.
  }
  for(auto &Missing: MissingIncludes) {
       // create missing include diag.
  }
  return Diags;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143496