[PATCH] D57021: [clangd] Suggest adding missing includes for typos (like include-fixer).

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/IncludeFixer.cpp:74
+    break;
+  default:
+    assert(RecordTypo);
----------------
"default" looks a bit scary - could still end up with false positives.
Can we cover a few of the most common cases (even if we know they're not exhaustive) to start with?


================
Comment at: clangd/IncludeFixer.cpp:203
+  LastTypo = std::move(Record);
+
+  return TypoCorrection();
----------------
a comment that we never return a valid correction to try to recover, our suggested fixes always require a rebuild.


================
Comment at: clangd/IncludeFixer.cpp:209
+  std::vector<std::string> Scopes;
+  if (Typo.SS) {
+    Scopes.push_back(*Typo.SS);
----------------
why do we do this here rather than compute the scopes/name already in CorrectTypo()?

Passing around more of these AST objects and doing the computations later seems a bit more fragile.


================
Comment at: clangd/IncludeFixer.cpp:227
+
+  FuzzyFindRequest Req;
+  Req.AnyScope = false;
----------------
limit?


================
Comment at: clangd/IncludeFixer.cpp:233
+
+  SymbolSlab::Builder Matches;
+  Index.fuzzyFind(Req, [&](const Symbol &Sym) {
----------------
why put these into a slab and then build fixes later, rather than build the fixes immediately?


================
Comment at: clangd/IncludeFixer.cpp:241
+  auto Syms = std::move(Matches).build();
+  if (Syms.empty())
+    return {};
----------------
Doesn't seem worth special casing this, below code will work fine.


================
Comment at: clangd/IncludeFixer.cpp:246
+    auto Fixes = fixesForSymbol(Sym);
+    Results.insert(Results.end(), Fixes.begin(), Fixes.end());
+  }
----------------
the fixes can overlap: distinct symbols may be defined in the same header.
Common case: function overloads.

At some level we "just want to deduplicate" these.
It's slightly complicated because of the tricky way we calculate the edits.
Maybe the best thing is to pass in all the symbols to fixesForSymbol() instead of just one.

One thing to keep in mind is a future where we want to pull in results across namespaces: the fixes for "include 'a.h' for x::Foo" and "include 'a.h' for y::Foo" need to be distinct because of the different added qualifiers.
So it'd be good to end up with an explicit e.g. map<header_to_include, Fix> to deduplicate, then we can just change the key to pair<header_to_include, qualifier_to_insert>


================
Comment at: clangd/IncludeFixer.h:35
 public:
-  IncludeFixer(llvm::StringRef File, std::shared_ptr<IncludeInserter> Inserter,
+  IncludeFixer(CompilerInstance &Compiler, llvm::StringRef File,
+               std::shared_ptr<IncludeInserter> Inserter,
----------------
Having to inject the whole compiler into the include fixer doesn't seem right. Apart from the huge scope, we're also going to register parts of the include fixer with the compiler.

A few observations:
 - all we actually need here is Sema and the SM (available through Sema)
 - it's only the typo recorder that needs access to Sema
 - the typo recorder gets fed the Sema instance (`ExternalSemaSource::InitializeSema`)
 - the fact that `fixTypo()` needs to access the typo recorder doesn't seem necessary

So I'd suggest changing this function to return a *new* ExternalSemaSource that stashes the last typo in this IncludeFixer object directly.

e.g.

```
llvm::IntrusiveRefCntPtr<ExternalSemaSource> typoRecorder() {
  return new TypoRecorder(LastTypo);
}
private:
  Optional<TypoRecord> LastTypo;
  class TypoRecorder : public ExternalSemaSource {
    Sema *S;
    Optional<TypoRecord> &Out;
    InitializeSema(Sema& S) { this->S = &S; }
    CorrectTypo(...) {
      assert(S);
      ...
      Out = ...;
    }
  }
  ```


================
Comment at: clangd/IncludeFixer.h:46
 
+  /// Returns an ExternalSemaSource that records typos seen in Sema. It must be
+  /// used in the same Sema run as the IncludeFixer.
----------------
This would be a reasonable place to document what the feature is and how it works :-)

e.g.
```
/// Returns an ExternalSemaSource that records failed name lookups in Sema.
/// This allows IncludeFixer to suggest inserting headers that define those names.
```


================
Comment at: clangd/IncludeFixer.h:59
 
+  struct TypoRecord {
+    std::string Typo;   // The typo identifier e.g. "X" in ns::X.
----------------
So an annoying naming thing: I find the use of "typo" to describe this feature really confusing. There's no actual typo, and conflating Sema's typo-correction fixes (which allow the parse to recover) and our generated fixes (which requires changing the source and rebuilding) breaks my brain a bit.

I'd suggest calling this "unresolved name" throughout, except when specifically referring to the mechanism by which we observe unresolved names.
e.g. `TypoRecorder` -> `UnresolvedNameRecorder` etc.


================
Comment at: clangd/IncludeFixer.h:68
+  /// Records the last typo seen by Sema.
+  class TypoRecorder : public ExternalSemaSource {
+  public:
----------------
I think TypoRecorder can be merely declared here, and defined in the CC file?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57021