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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks pretty good! My main concern is latency (and variability of latency) in practice.
Particularly:

- we should avoid paying for fuzzyfind and fetching hundreds of results when we want exact-match
- limit the damage in degenerate cases: should we limit to e.g. 5 index queries per TU?

Actually, now that I think about it - if we can see a forward declaration, can't we do a lookup() instead of a fuzzyFind?
Is the problem that this doesn't generalize to the no-declaration case (where it looks like a typo)?

If we had an operation that looked like lookup() but worked by qname, would we design the feature the same way?
In particular, would we want to batch the requests to the index (lookup takes a set of IDs)? This would affect the code structure a bit. But it would make the feature cost basically O(1) instead of O(errs)...



================
Comment at: clangd/ClangdUnit.cpp:295
 
+  llvm::Optional<IncludeFixer> FixIncludes;
+  auto BuildDir = VFS->getCurrentWorkingDirectory();
----------------
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.


================
Comment at: clangd/ClangdUnit.h:65
 struct ParseInputs {
+  ParseInputs(tooling::CompileCommand CompileCommand,
+              IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
----------------
(nit: can we keep this as a plain struct? Once the number of members get large, initializing them one-by-one probably reads better anyway...)


================
Comment at: clangd/ClangdUnit.h:91
+        IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
+        const SymbolIndex *Index);
 
----------------
Index is a reasonable (if surprising) param here, but I think we need to explicitly enable the include-fixing behavior. Even the very minimal hard-coded list of clang-tidy checks caused issues :-( And this is going to result in new network RPCs in some configs.

I'd suggest keeping the Index param, and wrapping the "use include fixer?" policy along with the clang-tidy options in D55256 as some sort of "diagnostic options" struct. (Though be wary of name clash with the one in Diagnostics.h, sigh).


================
Comment at: clangd/Diagnostics.h:105
+  /// If set, possibly adds fixes for diagnostics using \p Fixer.
+  void setIncludeFixer(const IncludeFixer &Fixer) { FixIncludes = &Fixer; }
+
----------------
This seems like a suspicious dependency.
Can we indirect here e.g. `void contributeFixes(std::function<vector<Fix>(const clang::Diagnostic&)>)`


================
Comment at: clangd/IncludeFixer.cpp:1
+//===--- IncludeFixer.cpp ----------------------------------------*- C++-*-===//
+//
----------------
new copyright notice


================
Comment at: clangd/IncludeFixer.cpp:39
+                                   const clang::Diagnostic &Info) const {
+  if (isIncompleteTypeDiag(Info.getID())) {
+    // Incomplete type diagnostics should have a QualType argument for the
----------------
can you add a trace to this function so we know what the impact on latency is?


================
Comment at: clangd/IncludeFixer.cpp:66
+  vlog("Trying to fix include for incomplete type {0}", IncompleteType);
+  FuzzyFindRequest Req;
+  Req.AnyScope = false;
----------------
limit?


================
Comment at: clangd/IncludeFixer.cpp:74
+  llvm::Optional<Symbol> Matched;
+  Index.fuzzyFind(Req, [&](const Symbol &Sym) {
+    // FIXME: support multiple matched symbols.
----------------
so issuing a bunch of fuzzy finds in sequence is clearly not ideal from a performance standpoint.
Any ideas on what we might do better, or how we can limit the worst case?
(not sure we need to do any better in this patch, but might be worth comments)


================
Comment at: clangd/IncludeFixer.cpp:112
+      vlog("Failed to calculate include insertion for {0} into {1}: {2}", File,
+           Inc, llvm::toString(ToInclude.takeError()));
+    }
----------------
should be no need for toString here


================
Comment at: clangd/IncludeFixer.h:1
+//===- IncludeFixer.h - Add missing includes --------------------*- C++ -*-===//
+//
----------------
new copyright notice


================
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;
+
----------------
nit: incomplete is one word


================
Comment at: unittests/clangd/IncludeFixerTests.cpp:1
+//===-- ClangdUnitTests.cpp - ClangdUnit tests ------------------*- C++ -*-===//
+//
----------------
new copyright notice; wrong filename


================
Comment at: unittests/clangd/IncludeFixerTests.cpp:26
+
+testing::Matcher<const Diag &> WithFix(testing::Matcher<Fix> FixMatcher) {
+  return Field(&Diag::Fixes, ElementsAre(FixMatcher));
----------------
because the helpers and public APIs tested are common, I wonder if we'd be better off creating DiagnosticsTests.cpp and moving the relevant tests from both ClangdUnit and here to there.

(Testing the fixes directly through the IncludeFixer class would make sense in this file, but it's awkward - I think testing through the diagnostics API is actually the right thing)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56903