[PATCH] D56903: [clangd] Suggest adding missing includes for incomplete type diagnostics.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Just a bunch of nits, up to you to work out how/whether to address.

One substantive issue around classes defined outside headers - will chat offline.



================
Comment at: clangd/ClangdServer.h:118
+
+    bool EnableIncludeFixer = false;
   };
----------------
Can we call this option `SuggestMissingIncludes` or so? include-fixer is the name of a separate tool.


================
Comment at: clangd/ClangdUnit.cpp:295
 
+  llvm::Optional<IncludeFixer> FixIncludes;
+  auto BuildDir = VFS->getCurrentWorkingDirectory();
----------------
sammccall wrote:
> Probably worth a two-line comment explaining at a high level what this is doing.
> Somewhat redundant with `IncludeFixer.h` but probably worth it anyway since this containing function is a complicated mess.
nit: can you hoist this above the line declaring FixIncludes?
That way (I think) it reads more naturally as "this is what the following block of code does" in a big method, doesn't fold away with the if, etc


================
Comment at: clangd/ClangdUnit.cpp:305
+    }
+    FixIncludes.emplace(MainInput.getFile(), std::move(Inserter), *Index);
+    ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl,
----------------
nit: I'm not sure I agree with the IncludeFixer class taking ownership of the include inserter.
It reads it but never writes to it, so full ownership isn't *needed*. And if we had other features that wanted to compute include insertions, we'd want them to share an inserter.


================
Comment at: clangd/Compiler.h:49
+  // Used to recover from diagnostics (e.g. find missing includes for symbol).
+  llvm::Optional<const SymbolIndex *> Index;
+  ParseOptions Opts;
----------------
isn't an optional pointer just a pointer?


================
Comment at: clangd/IncludeFixer.cpp:39
+                                   const clang::Diagnostic &Info) const {
+  if (IndexRequestCount >= 5)
+    return {}; // Avoid querying index too many times in a single parse.
----------------
consider moving this 5 to a constructor param, so this file can be entirely mechanism and the policy is spelled out at the construction site.
(No need to actually make it configurable, I think)


================
Comment at: clangd/IncludeFixer.cpp:41
+    return {}; // Avoid querying index too many times in a single parse.
+  if (isIncompleteTypeDiag(Info.getID())) {
+    // Incomplete type diagnostics should have a QualType argument for the
----------------
(You could also just write this as just a switch on Info.getID(), I'd find that a little direct/easier to read, up to you)


================
Comment at: clangd/IncludeFixer.cpp:61
+    return {};
+  std::string IncompleteType = printQualifiedName(*TD);
+  if (IncompleteType.empty()) {
----------------
TypeName?


================
Comment at: clangd/IncludeFixer.cpp:63
+  if (IncompleteType.empty()) {
+    vlog("No incomplete type name is found in diagnostic. Ignore.");
+    return {};
----------------
this seems an odd thing to check and way to phrase - there *was* a type in the message, but its name happens to be empty. Am I missing something? (I'd suggest just removing this)


================
Comment at: clangd/IncludeFixer.cpp:80
+  Index.lookup(Req, [&](const Symbol &Sym) {
+    // FIXME: support multiple matched symbols.
+    if (Matched || (Sym.Scope + Sym.Name).str() != IncompleteType)
----------------
fixme is obsolete


================
Comment at: clangd/IncludeFixer.cpp:81
+    // FIXME: support multiple matched symbols.
+    if (Matched || (Sym.Scope + Sym.Name).str() != IncompleteType)
+      return;
----------------
no need to check name


================
Comment at: clangd/IncludeFixer.cpp:86
+
+  if (!Matched || Matched->IncludeHeaders.empty())
+    return {};
----------------
There's a case I'm worried about:

Foo.h declares `class X;`, Foo.cpp defines `class X{}`.
Now Bar.cpp tries to do `sizeof(X)`, so we get an incomplete type diagnostic.
 - suggesting `#include Foo.cpp` is bad - can't include impl file
 - suggesting `#include Foo.h` is also bad - doesn't help
 - providing no suggestion is correct.

I'm not sure the index really has enough structured information to detect this case, but we could check e.g. that definition header == canonical declaration header? Or that basename(IncludeHeaders) == basename(definition header)?

If possible, can you add a test for this case?


================
Comment at: clangd/IncludeFixer.h:39
+  /// Attempts to recover diagnostic caused by an incomplete type \p T.
+  std::vector<Fix> fixInCompleteType(const Type &T) const;
+
----------------
sammccall wrote:
> nit: incomplete is one word
not done - method is still "fixInCompleteType"


================
Comment at: clangd/tool/ClangdMain.cpp:210
 
+static llvm::cl::opt<bool> EnableIncludeFixer(
+    "include-fixer",
----------------
similarly, I'd call this -suggest-missing-includes or just -suggest-includes


================
Comment at: clangd/tool/ClangdMain.cpp:214
+                   "includes using index."),
+    llvm::cl::init(false), llvm::cl::Hidden);
+
----------------
why hidden?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56903