Index: clangd/ClangdUnit.cpp =================================================================== --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -304,12 +304,13 @@ for (const auto &Inc : Preamble->Includes.MainFileIncludes) Inserter->addExisting(Inc); } - FixIncludes.emplace(MainInput.getFile(), Inserter, *Index, + FixIncludes.emplace(*Clang, MainInput.getFile(), Inserter, *Index, /*IndexRequestLimit=*/5); ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl, const clang::Diagnostic &Info) { return FixIncludes->fix(DiagLevl, Info); }); + Clang->setExternalSemaSource(FixIncludes->typoRecorder()); } // Copy over the includes from the preamble, then combine with the @@ -534,10 +535,10 @@ // dirs. } - return ParsedAST::build( - llvm::make_unique(*Invocation), Preamble, - llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents), PCHs, - std::move(VFS), Inputs.Index ? Inputs.Index : nullptr, Inputs.Opts); + return ParsedAST::build(llvm::make_unique(*Invocation), + Preamble, + llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents), + PCHs, std::move(VFS), Inputs.Index, Inputs.Opts); } SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos, Index: clangd/IncludeFixer.h =================================================================== --- clangd/IncludeFixer.h +++ clangd/IncludeFixer.h @@ -14,6 +14,14 @@ #include "index/Index.h" #include "clang/AST/Type.h" #include "clang/Basic/Diagnostic.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Sema/ExternalSemaSource.h" +#include "clang/Sema/Sema.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/IntrusiveRefCntPtr.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/StringRef.h" #include @@ -25,27 +33,54 @@ /// include headers with the definition. class IncludeFixer { public: - IncludeFixer(llvm::StringRef File, std::shared_ptr Inserter, + IncludeFixer(CompilerInstance &Compiler, llvm::StringRef File, + std::shared_ptr Inserter, const SymbolIndex &Index, unsigned IndexRequestLimit) - : File(File), Inserter(std::move(Inserter)), Index(Index), - IndexRequestLimit(IndexRequestLimit) {} + : Compiler(Compiler), File(File), Inserter(std::move(Inserter)), + Index(Index), IndexRequestLimit(IndexRequestLimit) {} /// Returns include insertions that can potentially recover the diagnostic. std::vector fix(DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) const; + /// Returns an ExternalSemaSource that records failed name lookups in Sema. + /// This allows IncludeFixer to suggest inserting headers that define those + /// names. + llvm::IntrusiveRefCntPtr typoRecorder(); + private: /// Attempts to recover diagnostic caused by an incomplete type \p T. std::vector fixIncompleteType(const Type &T) const; - /// Generates header insertion fixes for \p Sym. - std::vector fixesForSymbol(const Symbol &Sym) const; + /// Generates header insertion fixes for all symbols. Fixes are deduplicated. + std::vector fixesForSymbols(llvm::ArrayRef Syms) const; + + struct UnresolvedName { + 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. + llvm::Optional SS; // The scope qualifier before the typo. + Sema::LookupNameKind LookupKind; // LookupKind of the typo. + }; + + /// Records the last unresolved name (i.e. typo) seen by Sema. + class UnresolvedNameRecorder; + /// Attempts to fix the typo associated with the current diagnostic. We assume + /// a diagnostic is caused by a typo when they have the same source location + /// and the typo is the last typo we've seen during the Sema run. + std::vector fixUnresolvedName(const UnresolvedName &Unresolved) const; + + CompilerInstance &Compiler; std::string File; std::shared_ptr Inserter; const SymbolIndex &Index; const unsigned IndexRequestLimit; // Make at most 5 index requests. mutable unsigned IndexRequestCount = 0; + + // These collect the last typo so that we can associate it with the + // diagnostic. + llvm::Optional LastUnresolvedName; }; } // namespace clangd Index: clangd/IncludeFixer.cpp =================================================================== --- clangd/IncludeFixer.cpp +++ clangd/IncludeFixer.cpp @@ -14,16 +14,47 @@ #include "Trace.h" #include "index/Index.h" #include "clang/AST/Decl.h" +#include "clang/AST/DeclBase.h" +#include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/Type.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticSema.h" +#include "clang/Sema/DeclSpec.h" +#include "clang/Sema/Lookup.h" +#include "clang/Sema/Scope.h" +#include "clang/Sema/Sema.h" +#include "clang/Sema/TypoCorrection.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/None.h" +#include "llvm/ADT/StringSet.h" #include "llvm/Support/Error.h" #include "llvm/Support/FormatVariadic.h" +#include namespace clang { namespace clangd { +namespace { + +// Collects contexts visited during a Sema name lookup. +class VisitedContextCollector : public VisibleDeclConsumer { +public: + void EnteredContext(DeclContext *Ctx) override { Visited.push_back(Ctx); } + + void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx, + bool InBaseClass) override {} + + std::vector takeVisitedContexts() { + return std::move(Visited); + } + +private: + std::vector Visited; +}; + +} // namespace + std::vector IncludeFixer::fix(DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) const { if (IndexRequestCount >= IndexRequestLimit) @@ -42,6 +73,28 @@ return fixIncompleteType(*T); } } + break; + case diag::err_unknown_typename: + case diag::err_unknown_typename_suggest: + case diag::err_typename_nested_not_found: + case diag::err_no_template: + case diag::err_no_template_suggest: + if (LastUnresolvedName) { + // Try to fix typos caused by missing declaraion. + // E.g. + // clang::SourceManager SM; + // ~~~~~~~~~~~~~ + // UnresolvedName + // or + // namespace clang { SourceManager SM; } + // ~~~~~~~~~~~~~ + // UnresolvedName + // We only attempt to recover a diagnostic if it has the same location as + // the last seen typo. + if (DiagLevel >= DiagnosticsEngine::Error && + LastUnresolvedName->Loc == Info.getLocation()) + return fixUnresolvedName(*LastUnresolvedName); + } } return {}; } @@ -74,11 +127,12 @@ if (!Matched || Matched->IncludeHeaders.empty() || !Matched->Definition || Matched->CanonicalDeclaration.FileURI != Matched->Definition.FileURI) return {}; - return fixesForSymbol(*Matched); + return fixesForSymbols({*Matched}); } -std::vector IncludeFixer::fixesForSymbol(const Symbol &Sym) const { - auto Inserted = [&](llvm::StringRef Header) +std::vector +IncludeFixer::fixesForSymbols(llvm::ArrayRef Syms) const { + auto Inserted = [&](const Symbol &Sym, llvm::StringRef Header) -> llvm::Expected> { auto ResolvedDeclaring = toHeaderFile(Sym.CanonicalDeclaration.FileURI, File); @@ -93,21 +147,142 @@ }; std::vector Fixes; - for (const auto &Inc : getRankedIncludes(Sym)) { - if (auto ToInclude = Inserted(Inc)) { - if (ToInclude->second) - if (auto Edit = Inserter->insert(ToInclude->first)) - Fixes.push_back( - Fix{llvm::formatv("Add include {0} for symbol {1}{2}", - ToInclude->first, Sym.Scope, Sym.Name), - {std::move(*Edit)}}); - } else { - vlog("Failed to calculate include insertion for {0} into {1}: {2}", File, - Inc, ToInclude.takeError()); + // Deduplicate fixes by include headers. + llvm::StringSet<> InsertedHeaders; + for (const auto &Sym : Syms) { + for (const auto &Inc : getRankedIncludes(Sym)) { + if (auto ToInclude = Inserted(Sym, Inc)) { + if (ToInclude->second) { + auto I = InsertedHeaders.try_emplace(ToInclude->first); + if (!I.second) + continue; + if (auto Edit = Inserter->insert(ToInclude->first)) + Fixes.push_back( + Fix{llvm::formatv("Add include {0} for symbol {1}{2}", + ToInclude->first, Sym.Scope, Sym.Name), + {std::move(*Edit)}}); + } + } else { + vlog("Failed to calculate include insertion for {0} into {1}: {2}", + File, Inc, ToInclude.takeError()); + } } } return Fixes; } +class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource { +public: + UnresolvedNameRecorder(llvm::Optional &LastUnresolvedName) + : LastUnresolvedName(LastUnresolvedName) {} + + void InitializeSema(Sema &S) override { this->SemaPtr = &S; } + + // Captures the latest typo. + TypoCorrection CorrectTypo(const DeclarationNameInfo &Typo, int LookupKind, + Scope *S, CXXScopeSpec *SS, + CorrectionCandidateCallback &CCC, + DeclContext *MemberContext, bool EnteringContext, + const ObjCObjectPointerType *OPT) override { + assert(S && "Sema must have been set."); + if (SemaPtr->isSFINAEContext()) + return TypoCorrection(); + if (!SemaPtr->SourceMgr.isWrittenInMainFile(Typo.getLoc())) + return clang::TypoCorrection(); + + UnresolvedName Record; + Record.Name = Typo.getAsString(); + Record.Loc = Typo.getBeginLoc(); + assert(S); + Record.S = S; + Record.LookupKind = static_cast(LookupKind); + + // FIXME: support invalid scope before a type name. In the following + // example, namespace "clang::tidy::" hasn't been declared/imported. + // namespace clang { + // void f() { + // tidy::Check c; + // ~~~~ + // // or + // clang::tidy::Check c; + // ~~~~ + // } + // } + // For both cases, the typo and the diagnostic are both on "tidy", and no + // diagnostic is generated for "Check". However, what we want to fix is + // "clang::tidy::Check". + if (SS && SS->isNotEmpty()) { // "::" or "ns::" + if (auto *Nested = SS->getScopeRep()) { + if (Nested->getKind() == NestedNameSpecifier::Global) + Record.SS = ""; + else if (const auto *NS = Nested->getAsNamespace()) + Record.SS = printNamespaceScope(*NS); + else + // We don't fix symbols in scopes that are not top-level e.g. class + // members, as we don't collect includes for them. + return TypoCorrection(); + } + } + + LastUnresolvedName = std::move(Record); + + // Never return a valid correction to try to recorver. Our suggested fixes + // always require a rebuild. + return TypoCorrection(); + } + + llvm::Optional lastUnresolvedName() const { + return LastUnresolvedName; + } + +private: + Sema *SemaPtr = nullptr; + + llvm::Optional &LastUnresolvedName; +}; + +llvm::IntrusiveRefCntPtr IncludeFixer::typoRecorder() { + return new UnresolvedNameRecorder(LastUnresolvedName); +} + +std::vector +IncludeFixer::fixUnresolvedName(const UnresolvedName &Unresolved) const { + std::vector Scopes; + if (Unresolved.SS) { + Scopes.push_back(*Unresolved.SS); + } else { + // No scope qualifier is specified. Collect all accessible scopes in the + // context. + VisitedContextCollector Collector; + Compiler.getSema().LookupVisibleDecls(Unresolved.S, Unresolved.LookupKind, + Collector, + /*IncludeGlobalScope=*/false, + /*LoadExternal=*/false); + + Scopes.push_back(""); + for (const auto *Ctx : Collector.takeVisitedContexts()) + if (isa(Ctx)) + Scopes.push_back(printNamespaceScope(*Ctx)); + } + vlog("Trying to fix typo \"{0}\" in scopes: [{1}]", Unresolved.Name, + llvm::join(Scopes.begin(), Scopes.end(), ", ")); + + FuzzyFindRequest Req; + Req.AnyScope = false; + Req.Query = Unresolved.Name; + Req.Scopes = Scopes; + Req.RestrictForCodeCompletion = true; + Req.Limit = 100; + + SymbolSlab::Builder Matches; + Index.fuzzyFind(Req, [&](const Symbol &Sym) { + if (Sym.Name != Req.Query) + return; + if (!Sym.IncludeHeaders.empty()) + Matches.insert(Sym); + }); + auto Syms = std::move(Matches).build(); + return fixesForSymbols(std::vector(Syms.begin(), Syms.end())); +} } // namespace clangd } // namespace clang Index: clangd/SourceCode.h =================================================================== --- clangd/SourceCode.h +++ clangd/SourceCode.h @@ -47,7 +47,7 @@ /// The returned value is in the range [0, Code.size()]. llvm::Expected positionToOffset(llvm::StringRef Code, Position P, - bool AllowColumnsBeyondLineLength = true); + bool AllowColumnsBeyondLineLength = true); /// Turn an offset in Code into a [line, column] pair. /// The offset must be in range [0, Code.size()]. @@ -110,7 +110,7 @@ // The offset must be in range [0, Code.size()]. // Prefer to use SourceManager if one is available. std::pair offsetToClangLineColumn(llvm::StringRef Code, - size_t Offset); + size_t Offset); /// From "a::b::c", return {"a::b::", "c"}. Scope is empty if there's no /// qualifier. @@ -120,10 +120,10 @@ TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R); std::vector replacementsToEdits(StringRef Code, - const tooling::Replacements &Repls); + const tooling::Replacements &Repls); TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M, - const LangOptions &L); + const LangOptions &L); /// Get the canonical path of \p F. This means: /// @@ -136,7 +136,7 @@ /// component that generate it, so that paths are normalized as much as /// possible. llvm::Optional getCanonicalPath(const FileEntry *F, - const SourceManager &SourceMgr); + const SourceManager &SourceMgr); bool isRangeConsecutive(const Range &Left, const Range &Right); Index: unittests/clangd/DiagnosticsTests.cpp =================================================================== --- unittests/clangd/DiagnosticsTests.cpp +++ unittests/clangd/DiagnosticsTests.cpp @@ -30,6 +30,11 @@ return Field(&Diag::Fixes, ElementsAre(FixMatcher)); } +testing::Matcher WithFix(testing::Matcher FixMatcher1, + testing::Matcher FixMatcher2) { + return Field(&Diag::Fixes, UnorderedElementsAre(FixMatcher1, FixMatcher2)); +} + testing::Matcher WithNote(testing::Matcher NoteMatcher) { return Field(&Diag::Notes, ElementsAre(NoteMatcher)); } @@ -281,6 +286,26 @@ Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty()))); } +struct SymbolWithHeader { + std::string QName; + std::string DeclaringFile; + std::string IncludeHeader; +}; + +std::unique_ptr +buildIndexWithSymbol(llvm::ArrayRef Syms) { + SymbolSlab::Builder Slab; + for (const auto &S : Syms) { + Symbol Sym = cls(S.QName); + Sym.Flags |= Symbol::IndexedForCodeCompletion; + Sym.CanonicalDeclaration.FileURI = S.DeclaringFile.c_str(); + Sym.Definition.FileURI = S.DeclaringFile.c_str(); + Sym.IncludeHeaders.emplace_back(S.IncludeHeader, 1); + Slab.insert(Sym); + } + return MemIndex::build(std::move(Slab).build(), RefSlab()); +} + TEST(IncludeFixerTest, IncompleteType) { Annotations Test(R"cpp( $insert[[]]namespace ns { @@ -293,15 +318,8 @@ } )cpp"); auto TU = TestTU::withCode(Test.code()); - Symbol Sym = cls("ns::X"); - Sym.Flags |= Symbol::IndexedForCodeCompletion; - Sym.CanonicalDeclaration.FileURI = "unittest:///x.h"; - Sym.Definition.FileURI = "unittest:///x.h"; - Sym.IncludeHeaders.emplace_back("\"x.h\"", 1); - - SymbolSlab::Builder Slab; - Slab.insert(Sym); - auto Index = MemIndex::build(std::move(Slab).build(), RefSlab()); + auto Index = buildIndexWithSymbol( + {SymbolWithHeader{"ns::X", "unittest:///x.h", "\"x.h\""}}); TU.ExternalIndex = Index.get(); EXPECT_THAT( @@ -346,6 +364,65 @@ "member access into incomplete type 'ns::X'"))); } +TEST(IncludeFixerTest, Typo) { + Annotations Test(R"cpp( +$insert[[]]namespace ns { +void foo() { + $unqualified[[X]] x; +} +} +void bar() { + ns::$qualified[[X]] x; // ns:: is valid. + ::$global[[Global]] glob; +} + )cpp"); + auto TU = TestTU::withCode(Test.code()); + auto Index = buildIndexWithSymbol( + {SymbolWithHeader{"ns::X", "unittest:///x.h", "\"x.h\""}, + SymbolWithHeader{"Global", "unittest:///global.h", "\"global.h\""}}); + TU.ExternalIndex = Index.get(); + + EXPECT_THAT( + TU.build().getDiagnostics(), + UnorderedElementsAre( + AllOf(Diag(Test.range("unqualified"), "unknown type name 'X'"), + WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n", + "Add include \"x.h\" for symbol ns::X"))), + AllOf(Diag(Test.range("qualified"), + "no type named 'X' in namespace 'ns'"), + WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n", + "Add include \"x.h\" for symbol ns::X"))), + AllOf(Diag(Test.range("global"), + "no type named 'Global' in the global namespace"), + WithFix(Fix(Test.range("insert"), "#include \"global.h\"\n", + "Add include \"global.h\" for symbol Global"))))); +} + +TEST(IncludeFixerTest, MultipleMatchedSymbols) { + Annotations Test(R"cpp( +$insert[[]]namespace na { +namespace nb { +void foo() { + $unqualified[[X]] x; +} +} +} + )cpp"); + auto TU = TestTU::withCode(Test.code()); + auto Index = buildIndexWithSymbol( + {SymbolWithHeader{"na::X", "unittest:///a.h", "\"a.h\""}, + SymbolWithHeader{"na::nb::X", "unittest:///b.h", "\"b.h\""}}); + TU.ExternalIndex = Index.get(); + + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre(AllOf( + Diag(Test.range("unqualified"), "unknown type name 'X'"), + WithFix(Fix(Test.range("insert"), "#include \"a.h\"\n", + "Add include \"a.h\" for symbol na::X"), + Fix(Test.range("insert"), "#include \"b.h\"\n", + "Add include \"b.h\" for symbol na::nb::X"))))); +} + } // namespace } // namespace clangd } // namespace clang