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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 6 01:08:17 PST 2019


sammccall added inline comments.


================
Comment at: clangd/IncludeFixer.cpp:209
+  std::vector<std::string> Scopes;
+  if (Typo.SS) {
+    Scopes.push_back(*Typo.SS);
----------------
ioeric wrote:
> sammccall wrote:
> > 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.
> Because not all typos will result in diagnostics. Computing scopes could be expensive, so we would only want to do that when necessary. For example, sema can perform typo corrections for both `x` and `y` in `x::y`, while only one diagnostic (for either `x` or `y`) would be generated.
If the goal is to make the scope computation lazy, `CorrectTypo` can store them as a `std::function<std::vector<string>()>` - that way we can still group the related code and dependencies together.

This function can capture the Sema instance by pointer - this deserves a comment that sema must be alive to access the scopes.


================
Comment at: clangd/IncludeFixer.cpp:82
+  case diag::err_no_template_suggest:
+    if (LastUnresolvedName) {
+      // Try to fix typos caused by missing declaraion.
----------------
you never seem to clear LastUnresolvedName - I guess you at least want to clear it after using it.
It would be more robust to clear after every diagnostic - would this work?


================
Comment at: clangd/IncludeFixer.cpp:161
+            Fixes.push_back(
+                Fix{llvm::formatv("Add include {0} for symbol {1}{2}",
+                                  ToInclude->first, Sym.Scope, Sym.Name),
----------------
note that if you have symbols in different scopes from the same header, you're only generating one fix and it will only mention one of the symbols.

It's fine to decide this case doesn't matter, but it's worth adding a comment.
(Alternatively, using a set of pair<inserted-header, qualified-name> to track dupes would avoid this)


================
Comment at: clangd/IncludeFixer.cpp:186
+                             const ObjCObjectPointerType *OPT) override {
+    assert(S && "Sema must have been set.");
+    if (SemaPtr->isSFINAEContext())
----------------
SemaPtr?


================
Comment at: clangd/IncludeFixer.cpp:199
+
+    // FIXME: support invalid scope before a type name. In the following
+    // example, namespace "clang::tidy::" hasn't been declared/imported.
----------------
The FIXME makes sense, but it's not clear to me what the code below it does or how it relates to the comment.

maybe separate with a blank line and then "Extract the typed scope" or so?


================
Comment at: clangd/IncludeFixer.cpp:228
+
+    // Never return a valid correction to try to recorver. Our suggested fixes
+    // always require a rebuild.
----------------
nit: recover


================
Comment at: clangd/IncludeFixer.cpp:233
+
+  llvm::Optional<UnresolvedName> lastUnresolvedName() const {
+    return LastUnresolvedName;
----------------
unused


================
Comment at: clangd/IncludeFixer.h:60
+    std::string Name;                // The typo identifier e.g. "X" in ns::X.
+    SourceLocation Loc;              // Location of the typo.
+    Scope *S;                        // Scope in which the typo is found.
----------------
nit: the comment on Loc and LookupKind don't say anything, remove them?


================
Comment at: clangd/IncludeFixer.h:61
+    SourceLocation Loc;              // Location of the typo.
+    Scope *S;                        // Scope in which the typo is found.
+    llvm::Optional<std::string> SS;  // The scope qualifier before the typo.
----------------
please use real names for these fields (EnclosingScope, SpecifiedScope?)

Maybe it would help documenting here to have a more complete example e.g.

`namespace ns { int Y = foo::x }`

Name is x
Loc is points at x (or f?)
EnclosingScope is ns::
SpecifiedScope is either ns::foo:: or foo:: (depending how 'foo' was resolved)


================
Comment at: clangd/IncludeFixer.h:72
+  /// and the typo is the last typo we've seen during the Sema run.
+  std::vector<Fix> fixUnresolvedName(const UnresolvedName &Unresolved) const;
+
----------------
nit: why take a parameter here, when it's always *LastUnresolvedName?


================
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,
----------------
ioeric wrote:
> sammccall wrote:
> > 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 = ...;
> >     }
> >   }
> >   ```
> Great idea. Thanks!
> 
> I've done most of the suggested refactoring.
> 
> > the fact that fixTypo() needs to access the typo recorder doesn't seem necessary.
> Do you mean access to *Sema*? The `IncludeFixer` still needs sema because we don't want to run Lookup for each typo sema encounters. We only want to do that for typos that make it to diagnostics. See my response to the other comment about this for details.
Yes, I meant Sema here. I think we can avoid IncludeFixer holding a reference to Sema directly.


================
Comment at: clangd/IncludeFixer.h:59
 
+  struct TypoRecord {
+    std::string Typo;   // The typo identifier e.g. "X" in ns::X.
----------------
sammccall wrote:
> 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.
I don't think this is done: there are still 20+ references to "typo" in this patch.
I think they should all be changed, apart from the implementation of `CorrectTypo()` and a comment explaining the use of this mechanism.
Happy to discuss if you don't think this is a good idea, though!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57021





More information about the cfe-commits mailing list