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,13 @@ #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/DenseMap.h" +#include "llvm/ADT/IntrusiveRefCntPtr.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/StringRef.h" #include @@ -25,15 +32,23 @@ /// 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), + RecordTypo(new TypoRecorder(Compiler)) {} /// 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 typos seen in Sema. It must be + /// used in the same Sema run as the IncludeFixer. + llvm::IntrusiveRefCntPtr typoRecorder() { + return RecordTypo; + } + private: /// Attempts to recover diagnostic caused by an incomplete type \p T. std::vector fixIncompleteType(const Type &T) const; @@ -41,11 +56,49 @@ /// Generates header insertion fixes for \p Sym. std::vector fixesForSymbol(const Symbol &Sym) const; + struct TypoRecord { + std::string Typo; // 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 typo seen by Sema. + class TypoRecorder : public ExternalSemaSource { + public: + TypoRecorder(CompilerInstance &Compiler) : Compiler(Compiler) {} + + // 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; + + llvm::Optional lastTypo() const { return LastTypo; } + + private: + CompilerInstance &Compiler; + + llvm::Optional LastTypo; + }; + + /// 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 fixTypo(const TypoRecord &Typo) 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; + + // This collects the last typo so that we can associate it with the + // diagnostic. + llvm::IntrusiveRefCntPtr RecordTypo; }; } // namespace clangd Index: clangd/IncludeFixer.cpp =================================================================== --- clangd/IncludeFixer.cpp +++ clangd/IncludeFixer.cpp @@ -14,16 +14,44 @@ #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/None.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 +70,25 @@ return fixIncompleteType(*T); } } + break; + default: + assert(RecordTypo); + if (auto LastTypo = RecordTypo->lastTypo()) { + // Try to fix typos caused by missing declaraion. + // E.g. + // clang::SourceManager SM; + // ~~~~~~~~~~~~~ + // Typo + // or + // namespace clang { SourceManager SM; } + // ~~~~~~~~~~~~~ + // Typo + // We only attempt to recover a diagnostic if it has the same location as + // the last seen typo. + if (DiagLevel >= DiagnosticsEngine::Error && + LastTypo->Loc == Info.getLocation()) + return fixTypo(*LastTypo); + } } return {}; } @@ -109,5 +156,97 @@ return Fixes; } +TypoCorrection IncludeFixer::TypoRecorder::CorrectTypo( + const DeclarationNameInfo &Typo, int LookupKind, Scope *S, CXXScopeSpec *SS, + CorrectionCandidateCallback &CCC, DeclContext *MemberContext, + bool EnteringContext, const ObjCObjectPointerType *OPT) { + if (Compiler.getSema().isSFINAEContext()) + return TypoCorrection(); + if (!Compiler.getSourceManager().isWrittenInMainFile(Typo.getLoc())) + return clang::TypoCorrection(); + + TypoRecord Record; + Record.Typo = 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(); + } + } + + LastTypo = std::move(Record); + + return TypoCorrection(); +} + +std::vector IncludeFixer::fixTypo(const TypoRecord &Typo) const { + std::vector Scopes; + if (Typo.SS) { + Scopes.push_back(*Typo.SS); + } else { + // No scope qualifier is specified. Collect all accessible scopes in the + // context. + VisitedContextCollector Collector; + Compiler.getSema().LookupVisibleDecls(Typo.S, Typo.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}]", Typo.Typo, + llvm::join(Scopes.begin(), Scopes.end(), ", ")); + + FuzzyFindRequest Req; + Req.AnyScope = false; + Req.Query = Typo.Typo; + Req.Scopes = Scopes; + Req.RestrictForCodeCompletion = true; + + 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(); + if (Syms.empty()) + return {}; + std::vector Results; + for (const auto &Sym : Syms) { + auto Fixes = fixesForSymbol(Sym); + Results.insert(Results.end(), Fixes.begin(), Fixes.end()); + } + return Results; +} + } // namespace clangd } // namespace clang Index: clangd/SourceCode.h =================================================================== --- clangd/SourceCode.h +++ clangd/SourceCode.h @@ -46,7 +46,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()]. @@ -64,7 +64,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. @@ -74,10 +74,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: /// @@ -90,7 +90,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)); } @@ -280,6 +285,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 { @@ -292,15 +317,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( @@ -345,6 +363,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