[PATCH] D113262: [clangd] Allow IncludeCleaner to replace unused header with transitively used

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This is cool!
I think it will interact with other features (stdlib, exported headers), and so whatever lands second is going to have to eat the complexity of that interaction.
I think both stdlib and exported headers are more critical and more essentially complex, so should probably land first to drive the design.



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:298
+
+llvm::StringSet<> reachableUsedHeaders(
+    const IncludeStructure &IncludeGraph, IncludeStructure::HeaderID Source,
----------------
It seems like this is going to include any files that are transitively reachable through multiple paths and directly reachable through none. These are going to seem like false positives.
Like <stddef.h> if the file uses size_t at all, or something.

Sure, if you want the whole file to be IWYU-clean you should remove <unused.h> and add <stddef.h>, but they're not *really* related.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:311
+      Result.insert(IncludeGraph.getRealPath(NextID));
+    for (const auto &Header : IncludeGraph.IncludeChildren.lookup(NextID))
+      if (!Visited.contains(Header))
----------------
This is going to have problems with private/exported headers.

Consider includes main -> foo -> bar -> private.
Main uses a symbol from private, bar exports private.
You need to suggest replacing the include of foo -> {bar}, not {bar, private}.

However you still want to traverse children of private in case main uses those.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:376
     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.Fixes.back().Edits.back().range.start.line = I.Include->HashLine;
+    D.Fixes.back().Edits.back().range.end.line = I.Include->HashLine + 1;
----------------
We should offer both fixes as alternatives, maybe?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:381
+      DirectIncludes +=
+          llvm::formatv("#include \"{0}\"\n", Replacement.getKey());
+    }
----------------
exactly how to spell an include and where to place it is tricky. See IncludeInserter.h in headers.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:69
 
-std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
+llvm::StringSet<> reachableUsedHeaders(
+    IncludeStructure &IncludeGraph, IncludeStructure::HeaderID Source,
----------------
why are we back to passing around strings?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113262