This adds include-fixer feature into clangd based on D56903. Clangd now captures
diagnostics caused by typos and attach include insertion fixes to potentially
fix the typo.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 27166 Build 27165: arc lint + arc unit
Event Timeline
clangd/IncludeFixer.cpp | ||
---|---|---|
77 | "default" looks a bit scary - could still end up with false positives. | |
206 | a comment that we never return a valid correction to try to recover, our suggested fixes always require a rebuild. | |
212 | 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. | |
230 | limit? | |
236 | why put these into a slab and then build fixes later, rather than build the fixes immediately? | |
244 | Doesn't seem worth special casing this, below code will work fine. | |
249 | the fixes can overlap: distinct symbols may be defined in the same header. At some level we "just want to deduplicate" these. 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. | |
clangd/IncludeFixer.h | ||
34–36 | 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:
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 = ...; } } | |
45 | 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. | |
56 | 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. | |
65 | I think TypoRecorder can be merely declared here, and defined in the CC file? |
clangd/IncludeFixer.cpp | ||
---|---|---|
212 | 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. | |
236 | good point. | |
clangd/IncludeFixer.h | ||
34–36 | Great idea. Thanks! I've done most of the suggested refactoring.
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. |
clangd/IncludeFixer.cpp | ||
---|---|---|
82 | you never seem to clear LastUnresolvedName - I guess you at least want to clear it after using it. | |
167 | 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. | |
174 | SemaPtr? | |
187 | 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? | |
212 | 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. | |
216 | nit: recover | |
221 | unused | |
clangd/IncludeFixer.h | ||
34–36 | Yes, I meant Sema here. I think we can avoid IncludeFixer holding a reference to Sema directly. | |
56 | I don't think this is done: there are still 20+ references to "typo" in this patch. | |
59 | nit: the comment on Loc and LookupKind don't say anything, remove them? | |
59 | nit: why take a parameter here, when it's always *LastUnresolvedName? | |
60 | 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 |
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:
So I'd suggest changing this function to return a *new* ExternalSemaSource that stashes the last typo in this IncludeFixer object directly.
e.g.