From dd66277c36af442460c60dc993b41b831ee28973 Mon Sep 17 00:00:00 2001 From: Eric Liu Date: Mon, 28 Jan 2019 14:01:55 +0000 Subject: [PATCH] [clangd] Suggest adding missing includes for incomplete type diagnostics. Summary: This enables clangd to intercept compiler diagnostics and attach fixes (e.g. by querying index). This patch adds missing includes for incomplete types e.g. member access into class with only forward declaration. This would allow adding missing includes for user-typed symbol names that are missing declarations (e.g. typos) in the future. Reviewers: sammccall Reviewed By: sammccall Subscribers: mgorny, ilya-biryukov, javed.absar, MaskRay, jkorous, mgrang, arphaman, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D56903 llvm-svn: 352361 --- clang-tools-extra/clangd/CMakeLists.txt | 1 + clang-tools-extra/clangd/ClangdServer.cpp | 19 +- clang-tools-extra/clangd/ClangdServer.h | 6 + clang-tools-extra/clangd/ClangdUnit.cpp | 44 ++- clang-tools-extra/clangd/ClangdUnit.h | 5 +- clang-tools-extra/clangd/CodeComplete.cpp | 64 +--- clang-tools-extra/clangd/Compiler.h | 11 +- clang-tools-extra/clangd/Diagnostics.cpp | 5 + clang-tools-extra/clangd/Diagnostics.h | 6 + clang-tools-extra/clangd/Headers.cpp | 35 ++ clang-tools-extra/clangd/Headers.h | 11 + clang-tools-extra/clangd/IncludeFixer.cpp | 113 +++++++ clang-tools-extra/clangd/IncludeFixer.h | 54 ++++ clang-tools-extra/clangd/SourceCode.cpp | 13 + clang-tools-extra/clangd/SourceCode.h | 7 + clang-tools-extra/clangd/tool/ClangdMain.cpp | 7 + clang-tools-extra/unittests/clangd/CMakeLists.txt | 1 + .../unittests/clangd/ClangdUnitTests.cpp | 257 --------------- .../unittests/clangd/CodeCompleteTests.cpp | 48 +-- .../unittests/clangd/DiagnosticsTests.cpp | 351 +++++++++++++++++++++ .../unittests/clangd/FileIndexTests.cpp | 2 +- .../unittests/clangd/TUSchedulerTests.cpp | 10 +- clang-tools-extra/unittests/clangd/TestIndex.cpp | 54 ++++ clang-tools-extra/unittests/clangd/TestIndex.h | 13 + clang-tools-extra/unittests/clangd/TestTU.cpp | 7 +- clang-tools-extra/unittests/clangd/TestTU.h | 2 + 26 files changed, 763 insertions(+), 383 deletions(-) create mode 100644 clang-tools-extra/clangd/IncludeFixer.cpp create mode 100644 clang-tools-extra/clangd/IncludeFixer.h create mode 100644 clang-tools-extra/unittests/clangd/DiagnosticsTests.cpp diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt index 92f6200..b58f8a8 100644 --- a/clang-tools-extra/clangd/CMakeLists.txt +++ b/clang-tools-extra/clangd/CMakeLists.txt @@ -40,6 +40,7 @@ add_clang_library(clangDaemon FuzzyMatch.cpp GlobalCompilationDatabase.cpp Headers.cpp + IncludeFixer.cpp JSONTransport.cpp Logger.cpp Protocol.cpp diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 5f49421..9b9eb10 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -7,6 +7,7 @@ //===-------------------------------------------------------------------===// #include "ClangdServer.h" +#include "ClangdUnit.h" #include "CodeComplete.h" #include "FindSymbols.h" #include "Headers.h" @@ -106,6 +107,7 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex) : nullptr), ClangTidyOptProvider(Opts.ClangTidyOptProvider), + SuggestMissingIncludes(Opts.SuggestMissingIncludes), WorkspaceRoot(Opts.WorkspaceRoot), PCHs(std::make_shared()), // Pass a callback into `WorkScheduler` to extract symbols from a newly @@ -141,17 +143,22 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents, WantDiagnostics WantDiags) { - tidy::ClangTidyOptions Options = tidy::ClangTidyOptions::getDefaults(); + ParseOptions Opts; + Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults(); if (ClangTidyOptProvider) - Options = ClangTidyOptProvider->getOptions(File); + Opts.ClangTidyOpts = ClangTidyOptProvider->getOptions(File); + Opts.SuggestMissingIncludes = SuggestMissingIncludes; // FIXME: some build systems like Bazel will take time to preparing // environment to build the file, it would be nice if we could emit a // "PreparingBuild" status to inform users, it is non-trivial given the // current implementation. - WorkScheduler.update(File, ParseInputs{getCompileCommand(File), - FSProvider.getFileSystem(), - Contents.str(), Options}, - WantDiags); + ParseInputs Inputs; + Inputs.CompileCommand = getCompileCommand(File); + Inputs.FS = FSProvider.getFileSystem(); + Inputs.Contents = Contents; + Inputs.Opts = std::move(Opts); + Inputs.Index = Index; + WorkScheduler.update(File, Inputs, WantDiags); } void ClangdServer::removeDocument(PathRef File) { WorkScheduler.remove(File); } diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index dc9581b..0be1602 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -114,6 +114,8 @@ public: /// Time to wait after a new file version before computing diagnostics. std::chrono::steady_clock::duration UpdateDebounce = std::chrono::milliseconds(500); + + bool SuggestMissingIncludes = false; }; // Sensible default options for use in tests. // Features like indexing must be enabled if desired. @@ -268,6 +270,10 @@ private: // The provider used to provide a clang-tidy option for a specific file. tidy::ClangTidyOptionsProvider *ClangTidyOptProvider = nullptr; + // If this is true, suggest include insertion fixes for diagnostic errors that + // can be caused by missing includes (e.g. member access in incomplete type). + bool SuggestMissingIncludes = false; + // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex) llvm::StringMap> CachedCompletionFuzzyFindRequestByFile; diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp index 551d179..5c79859 100644 --- a/clang-tools-extra/clangd/ClangdUnit.cpp +++ b/clang-tools-extra/clangd/ClangdUnit.cpp @@ -11,9 +11,12 @@ #include "../clang-tidy/ClangTidyModuleRegistry.h" #include "Compiler.h" #include "Diagnostics.h" +#include "Headers.h" +#include "IncludeFixer.h" #include "Logger.h" #include "SourceCode.h" #include "Trace.h" +#include "index/Index.h" #include "clang/AST/ASTContext.h" #include "clang/Basic/LangOptions.h" #include "clang/Frontend/CompilerInstance.h" @@ -30,10 +33,12 @@ #include "clang/Serialization/ASTWriter.h" #include "clang/Tooling/CompilationDatabase.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/raw_ostream.h" #include +#include namespace clang { namespace clangd { @@ -231,7 +236,7 @@ ParsedAST::build(std::unique_ptr CI, std::unique_ptr Buffer, std::shared_ptr PCHs, llvm::IntrusiveRefCntPtr VFS, - const tidy::ClangTidyOptions &ClangTidyOpts) { + const SymbolIndex *Index, const ParseOptions &Opts) { assert(CI); // Command-line parsing sets DisableFree to true by default, but we don't want // to leak memory in clangd. @@ -240,9 +245,11 @@ ParsedAST::build(std::unique_ptr CI, Preamble ? &Preamble->Preamble : nullptr; StoreDiags ASTDiags; + std::string Content = Buffer->getBuffer(); + auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH, std::move(Buffer), - std::move(PCHs), std::move(VFS), ASTDiags); + std::move(PCHs), VFS, ASTDiags); if (!Clang) return None; @@ -266,12 +273,12 @@ ParsedAST::build(std::unique_ptr CI, { trace::Span Tracer("ClangTidyInit"); vlog("ClangTidy configuration for file {0}: {1}", MainInput.getFile(), - tidy::configurationAsText(ClangTidyOpts)); + tidy::configurationAsText(Opts.ClangTidyOpts)); tidy::ClangTidyCheckFactories CTFactories; for (const auto &E : tidy::ClangTidyModuleRegistry::entries()) E.instantiate()->addCheckFactories(CTFactories); CTContext.emplace(llvm::make_unique( - tidy::ClangTidyGlobalOptions(), ClangTidyOpts)); + tidy::ClangTidyGlobalOptions(), Opts.ClangTidyOpts)); CTContext->setDiagnosticsEngine(&Clang->getDiagnostics()); CTContext->setASTContext(&Clang->getASTContext()); CTContext->setCurrentFile(MainInput.getFile()); @@ -284,6 +291,27 @@ ParsedAST::build(std::unique_ptr CI, } } + // Add IncludeFixer which can recorver diagnostics caused by missing includes + // (e.g. incomplete type) and attach include insertion fixes to diagnostics. + llvm::Optional FixIncludes; + auto BuildDir = VFS->getCurrentWorkingDirectory(); + if (Opts.SuggestMissingIncludes && Index && !BuildDir.getError()) { + auto Style = getFormatStyleForFile(MainInput.getFile(), Content, VFS.get()); + auto Inserter = std::make_shared( + MainInput.getFile(), Content, Style, BuildDir.get(), + Clang->getPreprocessor().getHeaderSearchInfo()); + if (Preamble) { + for (const auto &Inc : Preamble->Includes.MainFileIncludes) + Inserter->addExisting(Inc); + } + FixIncludes.emplace(MainInput.getFile(), Inserter, *Index, + /*IndexRequestLimit=*/5); + ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl, + const clang::Diagnostic &Info) { + return FixIncludes->fix(DiagLevl, Info); + }); + } + // Copy over the includes from the preamble, then combine with the // non-preamble includes below. auto Includes = Preamble ? Preamble->Includes : IncludeStructure{}; @@ -506,10 +534,10 @@ buildAST(PathRef FileName, std::unique_ptr Invocation, // dirs. } - return ParsedAST::build(llvm::make_unique(*Invocation), - Preamble, - llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents), - PCHs, std::move(VFS), Inputs.ClangTidyOpts); + return ParsedAST::build( + llvm::make_unique(*Invocation), Preamble, + llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents), PCHs, + std::move(VFS), Inputs.Index ? Inputs.Index : nullptr, Inputs.Opts); } SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos, diff --git a/clang-tools-extra/clangd/ClangdUnit.h b/clang-tools-extra/clangd/ClangdUnit.h index 23adb15..f4883fd 100644 --- a/clang-tools-extra/clangd/ClangdUnit.h +++ b/clang-tools-extra/clangd/ClangdUnit.h @@ -16,6 +16,7 @@ #include "Headers.h" #include "Path.h" #include "Protocol.h" +#include "index/Index.h" #include "clang/Frontend/FrontendAction.h" #include "clang/Frontend/PrecompiledPreamble.h" #include "clang/Lex/Preprocessor.h" @@ -70,8 +71,8 @@ public: std::shared_ptr Preamble, std::unique_ptr Buffer, std::shared_ptr PCHs, - IntrusiveRefCntPtr VFS, - const tidy::ClangTidyOptions &ClangTidyOpts); + IntrusiveRefCntPtr VFS, const SymbolIndex *Index, + const ParseOptions &Opts); ParsedAST(ParsedAST &&Other); ParsedAST &operator=(ParsedAST &&Other); diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index 66100d8c..83dcfb7 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -177,28 +177,6 @@ std::string getOptionalParameters(const CodeCompletionString &CCS, return Result; } -/// Creates a `HeaderFile` from \p Header which can be either a URI or a literal -/// include. -static llvm::Expected toHeaderFile(llvm::StringRef Header, - llvm::StringRef HintPath) { - if (isLiteralInclude(Header)) - return HeaderFile{Header.str(), /*Verbatim=*/true}; - auto U = URI::parse(Header); - if (!U) - return U.takeError(); - - auto IncludePath = URI::includeSpelling(*U); - if (!IncludePath) - return IncludePath.takeError(); - if (!IncludePath->empty()) - return HeaderFile{std::move(*IncludePath), /*Verbatim=*/true}; - - auto Resolved = URI::resolve(*U, HintPath); - if (!Resolved) - return Resolved.takeError(); - return HeaderFile{std::move(*Resolved), /*Verbatim=*/false}; -} - /// A code completion result, in clang-native form. /// It may be promoted to a CompletionItem if it's among the top-ranked results. struct CompletionCandidate { @@ -1019,11 +997,12 @@ bool semaCodeComplete(std::unique_ptr Consumer, llvm::IntrusiveRefCntPtr VFS = Input.VFS; if (Input.Preamble && Input.Preamble->StatCache) VFS = Input.Preamble->StatCache->getConsumingFS(std::move(VFS)); - ParseInputs PInput; - PInput.CompileCommand = Input.Command; - PInput.FS = VFS; - PInput.Contents = Input.Contents; - auto CI = buildCompilerInvocation(PInput); + ParseInputs ParseInput; + ParseInput.CompileCommand = Input.Command; + ParseInput.FS = VFS; + ParseInput.Contents = Input.Contents; + ParseInput.Opts = ParseOptions(); + auto CI = buildCompilerInvocation(ParseInput); if (!CI) { elog("Couldn't create CompilerInvocation"); return false; @@ -1143,24 +1122,6 @@ speculativeFuzzyFindRequestForCompletion(FuzzyFindRequest CachedReq, return CachedReq; } -// Returns the most popular include header for \p Sym. If two headers are -// equally popular, prefer the shorter one. Returns empty string if \p Sym has -// no include header. -llvm::SmallVector getRankedIncludes(const Symbol &Sym) { - auto Includes = Sym.IncludeHeaders; - // Sort in descending order by reference count and header length. - llvm::sort(Includes, [](const Symbol::IncludeHeaderWithReferences &LHS, - const Symbol::IncludeHeaderWithReferences &RHS) { - if (LHS.References == RHS.References) - return LHS.IncludeHeader.size() < RHS.IncludeHeader.size(); - return LHS.References > RHS.References; - }); - llvm::SmallVector Headers; - for (const auto &Include : Includes) - Headers.push_back(Include.IncludeHeader); - return Headers; -} - // Runs Sema-based (AST) and Index-based completion, returns merged results. // // There are a few tricky considerations: @@ -1241,19 +1202,12 @@ public: CodeCompleteResult Output; auto RecorderOwner = llvm::make_unique(Opts, [&]() { assert(Recorder && "Recorder is not set"); - auto Style = - format::getStyle(format::DefaultFormatStyle, SemaCCInput.FileName, - format::DefaultFallbackStyle, SemaCCInput.Contents, - SemaCCInput.VFS.get()); - if (!Style) { - log("getStyle() failed for file {0}: {1}. Fallback is LLVM style.", - SemaCCInput.FileName, Style.takeError()); - Style = format::getLLVMStyle(); - } + auto Style = getFormatStyleForFile( + SemaCCInput.FileName, SemaCCInput.Contents, SemaCCInput.VFS.get()); // If preprocessor was run, inclusions from preprocessor callback should // already be added to Includes. Inserter.emplace( - SemaCCInput.FileName, SemaCCInput.Contents, *Style, + SemaCCInput.FileName, SemaCCInput.Contents, Style, SemaCCInput.Command.Directory, Recorder->CCSema->getPreprocessor().getHeaderSearchInfo()); for (const auto &Inc : Includes.MainFileIncludes) diff --git a/clang-tools-extra/clangd/Compiler.h b/clang-tools-extra/clangd/Compiler.h index 13b499c..9466d8e 100644 --- a/clang-tools-extra/clangd/Compiler.h +++ b/clang-tools-extra/clangd/Compiler.h @@ -16,6 +16,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H #include "../clang-tidy/ClangTidyOptions.h" +#include "index/Index.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/PrecompiledPreamble.h" @@ -33,12 +34,20 @@ public: const clang::Diagnostic &Info) override; }; +// Options to run clang e.g. when parsing AST. +struct ParseOptions { + tidy::ClangTidyOptions ClangTidyOpts; + bool SuggestMissingIncludes = false; +}; + /// Information required to run clang, e.g. to parse AST or do code completion. struct ParseInputs { tooling::CompileCommand CompileCommand; IntrusiveRefCntPtr FS; std::string Contents; - tidy::ClangTidyOptions ClangTidyOpts; + // Used to recover from diagnostics (e.g. find missing includes for symbol). + const SymbolIndex *Index = nullptr; + ParseOptions Opts; }; /// Builds compiler invocation that could be used to build AST or preamble. diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 3c22676..f370caa 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -374,6 +374,11 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, if (!Info.getFixItHints().empty()) AddFix(true /* try to invent a message instead of repeating the diag */); + if (Fixer) { + auto ExtraFixes = Fixer(DiagLevel, Info); + LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(), + ExtraFixes.end()); + } } else { // Handle a note to an existing diagnostic. if (!LastDiag) { diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h index 0866c3b..9130ad5 100644 --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -99,9 +99,15 @@ public: void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) override; + using DiagFixer = std::function(DiagnosticsEngine::Level, + const clang::Diagnostic &)>; + /// If set, possibly adds fixes for diagnostics using \p Fixer. + void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; } + private: void flushLastDiag(); + DiagFixer Fixer = nullptr; std::vector Output; llvm::Optional LangOpts; llvm::Optional LastDiag; diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp index c4af420..a5759a4 100644 --- a/clang-tools-extra/clangd/Headers.cpp +++ b/clang-tools-extra/clangd/Headers.cpp @@ -73,6 +73,41 @@ bool HeaderFile::valid() const { (!Verbatim && llvm::sys::path::is_absolute(File)); } +llvm::Expected toHeaderFile(llvm::StringRef Header, + llvm::StringRef HintPath) { + if (isLiteralInclude(Header)) + return HeaderFile{Header.str(), /*Verbatim=*/true}; + auto U = URI::parse(Header); + if (!U) + return U.takeError(); + + auto IncludePath = URI::includeSpelling(*U); + if (!IncludePath) + return IncludePath.takeError(); + if (!IncludePath->empty()) + return HeaderFile{std::move(*IncludePath), /*Verbatim=*/true}; + + auto Resolved = URI::resolve(*U, HintPath); + if (!Resolved) + return Resolved.takeError(); + return HeaderFile{std::move(*Resolved), /*Verbatim=*/false}; +} + +llvm::SmallVector getRankedIncludes(const Symbol &Sym) { + auto Includes = Sym.IncludeHeaders; + // Sort in descending order by reference count and header length. + llvm::sort(Includes, [](const Symbol::IncludeHeaderWithReferences &LHS, + const Symbol::IncludeHeaderWithReferences &RHS) { + if (LHS.References == RHS.References) + return LHS.IncludeHeader.size() < RHS.IncludeHeader.size(); + return LHS.References > RHS.References; + }); + llvm::SmallVector Headers; + for (const auto &Include : Includes) + Headers.push_back(Include.IncludeHeader); + return Headers; +} + std::unique_ptr collectIncludeStructureCallback(const SourceManager &SM, IncludeStructure *Out) { diff --git a/clang-tools-extra/clangd/Headers.h b/clang-tools-extra/clangd/Headers.h index 5358c34..e29f7f6 100644 --- a/clang-tools-extra/clangd/Headers.h +++ b/clang-tools-extra/clangd/Headers.h @@ -12,10 +12,12 @@ #include "Path.h" #include "Protocol.h" #include "SourceCode.h" +#include "index/Index.h" #include "clang/Format/Format.h" #include "clang/Lex/HeaderSearch.h" #include "clang/Lex/PPCallbacks.h" #include "clang/Tooling/Inclusions/HeaderIncludes.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/StringRef.h" #include "llvm/ADT/StringSet.h" #include "llvm/Support/Error.h" @@ -37,6 +39,15 @@ struct HeaderFile { bool valid() const; }; +/// Creates a `HeaderFile` from \p Header which can be either a URI or a literal +/// include. +llvm::Expected toHeaderFile(llvm::StringRef Header, + llvm::StringRef HintPath); + +// Returns include headers for \p Sym sorted by popularity. If two headers are +// equally popular, prefer the shorter one. +llvm::SmallVector getRankedIncludes(const Symbol &Sym); + // An #include directive that we found in the main file. struct Inclusion { Range R; // Inclusion range. diff --git a/clang-tools-extra/clangd/IncludeFixer.cpp b/clang-tools-extra/clangd/IncludeFixer.cpp new file mode 100644 index 0000000..0f49888 --- /dev/null +++ b/clang-tools-extra/clangd/IncludeFixer.cpp @@ -0,0 +1,113 @@ +//===--- IncludeFixer.cpp ----------------------------------------*- C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "IncludeFixer.h" +#include "AST.h" +#include "Diagnostics.h" +#include "Logger.h" +#include "SourceCode.h" +#include "Trace.h" +#include "index/Index.h" +#include "clang/AST/Decl.h" +#include "clang/AST/Type.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/DiagnosticSema.h" +#include "llvm/ADT/None.h" +#include "llvm/Support/Error.h" +#include "llvm/Support/FormatVariadic.h" + +namespace clang { +namespace clangd { + +std::vector IncludeFixer::fix(DiagnosticsEngine::Level DiagLevel, + const clang::Diagnostic &Info) const { + if (IndexRequestCount >= IndexRequestLimit) + return {}; // Avoid querying index too many times in a single parse. + switch (Info.getID()) { + case diag::err_incomplete_type: + case diag::err_incomplete_member_access: + case diag::err_incomplete_base_class: + // Incomplete type diagnostics should have a QualType argument for the + // incomplete type. + for (unsigned Idx = 0; Idx < Info.getNumArgs(); ++Idx) { + if (Info.getArgKind(Idx) == DiagnosticsEngine::ak_qualtype) { + auto QT = QualType::getFromOpaquePtr((void *)Info.getRawArg(Idx)); + if (const Type *T = QT.getTypePtrOrNull()) + if (T->isIncompleteType()) + return fixIncompleteType(*T); + } + } + } + return {}; +} + +std::vector IncludeFixer::fixIncompleteType(const Type &T) const { + // Only handle incomplete TagDecl type. + const TagDecl *TD = T.getAsTagDecl(); + if (!TD) + return {}; + std::string TypeName = printQualifiedName(*TD); + trace::Span Tracer("Fix include for incomplete type"); + SPAN_ATTACH(Tracer, "type", TypeName); + vlog("Trying to fix include for incomplete type {0}", TypeName); + + auto ID = getSymbolID(TD); + if (!ID) + return {}; + ++IndexRequestCount; + // FIXME: consider batching the requests for all diagnostics. + // FIXME: consider caching the lookup results. + LookupRequest Req; + Req.IDs.insert(*ID); + llvm::Optional Matched; + Index.lookup(Req, [&](const Symbol &Sym) { + if (Matched) + return; + Matched = Sym; + }); + + if (!Matched || Matched->IncludeHeaders.empty() || !Matched->Definition || + Matched->CanonicalDeclaration.FileURI != Matched->Definition.FileURI) + return {}; + return fixesForSymbol(*Matched); +} + +std::vector IncludeFixer::fixesForSymbol(const Symbol &Sym) const { + auto Inserted = [&](llvm::StringRef Header) + -> llvm::Expected> { + auto ResolvedDeclaring = + toHeaderFile(Sym.CanonicalDeclaration.FileURI, File); + if (!ResolvedDeclaring) + return ResolvedDeclaring.takeError(); + auto ResolvedInserted = toHeaderFile(Header, File); + if (!ResolvedInserted) + return ResolvedInserted.takeError(); + return std::make_pair( + Inserter->calculateIncludePath(*ResolvedDeclaring, *ResolvedInserted), + Inserter->shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted)); + }; + + 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()); + } + } + return Fixes; +} + +} // namespace clangd +} // namespace clang diff --git a/clang-tools-extra/clangd/IncludeFixer.h b/clang-tools-extra/clangd/IncludeFixer.h new file mode 100644 index 0000000..740710c --- /dev/null +++ b/clang-tools-extra/clangd/IncludeFixer.h @@ -0,0 +1,54 @@ +//===--- IncludeFixer.h ------------------------------------------*- C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_FIXER_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_FIXER_H + +#include "Diagnostics.h" +#include "Headers.h" +#include "index/Index.h" +#include "clang/AST/Type.h" +#include "clang/Basic/Diagnostic.h" +#include "llvm/ADT/StringRef.h" +#include + +namespace clang { +namespace clangd { + +/// Attempts to recover from error diagnostics by suggesting include insertion +/// fixes. For example, member access into incomplete type can be fixes by +/// include headers with the definition. +class IncludeFixer { +public: + IncludeFixer(llvm::StringRef File, std::shared_ptr Inserter, + const SymbolIndex &Index, unsigned IndexRequestLimit) + : 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; + +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; + + std::string File; + std::shared_ptr Inserter; + const SymbolIndex &Index; + const unsigned IndexRequestLimit; // Make at most 5 index requests. + mutable unsigned IndexRequestCount = 0; +}; + +} // namespace clangd +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_FIXER_H diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp index ede6435..1d8ceb2 100644 --- a/clang-tools-extra/clangd/SourceCode.cpp +++ b/clang-tools-extra/clangd/SourceCode.cpp @@ -248,5 +248,18 @@ llvm::Optional digestFile(const SourceManager &SM, FileID FID) { return digest(Content); } +format::FormatStyle getFormatStyleForFile(llvm::StringRef File, + llvm::StringRef Content, + llvm::vfs::FileSystem *FS) { + auto Style = format::getStyle(format::DefaultFormatStyle, File, + format::DefaultFallbackStyle, Content, FS); + if (!Style) { + log("getStyle() failed for file {0}: {1}. Fallback is LLVM style.", File, + Style.takeError()); + Style = format::getLLVMStyle(); + } + return *Style; +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h index 7c8499d..2bbfd33 100644 --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -16,7 +16,9 @@ #include "clang/Basic/Diagnostic.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" +#include "clang/Format/Format.h" #include "clang/Tooling/Core/Replacement.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/SHA1.h" namespace clang { @@ -91,6 +93,11 @@ llvm::Optional getCanonicalPath(const FileEntry *F, const SourceManager &SourceMgr); bool isRangeConsecutive(const Range &Left, const Range &Right); + +format::FormatStyle getFormatStyleForFile(llvm::StringRef File, + llvm::StringRef Content, + llvm::vfs::FileSystem *FS); + } // namespace clangd } // namespace clang #endif diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index c969a5b..e83ac2f 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -207,6 +207,12 @@ static llvm::cl::opt ClangTidyChecks( ".clang-tidy files)"), llvm::cl::init(""), llvm::cl::Hidden); +static llvm::cl::opt SuggestMissingIncludes( + "suggest-missing-includes", + llvm::cl::desc("Attempts to fix diagnostic errors caused by missing " + "includes using index."), + llvm::cl::init(false)); + namespace { /// \brief Supports a test URI scheme with relaxed constraints for lit tests. @@ -442,6 +448,7 @@ int main(int argc, char *argv[]) { /* Default */ tidy::ClangTidyOptions::getDefaults(), /* Override */ OverrideClangTidyOptions, FSProvider.getFileSystem()); Opts.ClangTidyOptProvider = &ClangTidyOptProvider; + Opts.SuggestMissingIncludes = SuggestMissingIncludes; ClangdLSPServer LSPServer( *TransportLayer, FSProvider, CCOpts, CompileCommandsDirPath, /*UseDirBasedCDB=*/CompileArgsFrom == FilesystemCompileArgs, Opts); diff --git a/clang-tools-extra/unittests/clangd/CMakeLists.txt b/clang-tools-extra/unittests/clangd/CMakeLists.txt index 73655fe..13aca89 100644 --- a/clang-tools-extra/unittests/clangd/CMakeLists.txt +++ b/clang-tools-extra/unittests/clangd/CMakeLists.txt @@ -18,6 +18,7 @@ add_extra_unittest(ClangdTests CodeCompletionStringsTests.cpp ContextTests.cpp DexTests.cpp + DiagnosticsTests.cpp DraftStoreTests.cpp ExpectedTypeTest.cpp FileDistanceTests.cpp diff --git a/clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp b/clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp index b5e83a0..dd3fc6d 100644 --- a/clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp +++ b/clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp @@ -19,263 +19,6 @@ namespace clangd { namespace { using testing::ElementsAre; -using testing::Field; -using testing::IsEmpty; -using testing::Pair; -using testing::UnorderedElementsAre; - -testing::Matcher WithFix(testing::Matcher FixMatcher) { - return Field(&Diag::Fixes, ElementsAre(FixMatcher)); -} - -testing::Matcher WithNote(testing::Matcher NoteMatcher) { - return Field(&Diag::Notes, ElementsAre(NoteMatcher)); -} - -MATCHER_P2(Diag, Range, Message, - "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") { - return arg.Range == Range && arg.Message == Message; -} - -MATCHER_P3(Fix, Range, Replacement, Message, - "Fix " + llvm::to_string(Range) + " => " + - testing::PrintToString(Replacement) + " = [" + Message + "]") { - return arg.Message == Message && arg.Edits.size() == 1 && - arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement; -} - -MATCHER_P(EqualToLSPDiag, LSPDiag, - "LSP diagnostic " + llvm::to_string(LSPDiag)) { - return std::tie(arg.range, arg.severity, arg.message) == - std::tie(LSPDiag.range, LSPDiag.severity, LSPDiag.message); -} - -MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) { - if (arg.Message != Fix.Message) - return false; - if (arg.Edits.size() != Fix.Edits.size()) - return false; - for (std::size_t I = 0; I < arg.Edits.size(); ++I) { - if (arg.Edits[I].range != Fix.Edits[I].range || - arg.Edits[I].newText != Fix.Edits[I].newText) - return false; - } - return true; -} - -// Helper function to make tests shorter. -Position pos(int line, int character) { - Position Res; - Res.line = line; - Res.character = character; - return Res; -} - -TEST(DiagnosticsTest, DiagnosticRanges) { - // Check we report correct ranges, including various edge-cases. - Annotations Test(R"cpp( - namespace test{}; - void $decl[[foo]](); - int main() { - $typo[[go\ -o]](); - foo()$semicolon[[]]//with comments - $unk[[unknown]](); - double $type[[bar]] = "foo"; - struct Foo { int x; }; Foo a; - a.$nomember[[y]]; - test::$nomembernamespace[[test]]; - } - )cpp"); - EXPECT_THAT( - TestTU::withCode(Test.code()).build().getDiagnostics(), - ElementsAre( - // This range spans lines. - AllOf(Diag(Test.range("typo"), - "use of undeclared identifier 'goo'; did you mean 'foo'?"), - WithFix( - Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")), - // This is a pretty normal range. - WithNote(Diag(Test.range("decl"), "'foo' declared here"))), - // This range is zero-width and insertion. Therefore make sure we are - // not expanding it into other tokens. Since we are not going to - // replace those. - AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"), - WithFix(Fix(Test.range("semicolon"), ";", "insert ';'"))), - // This range isn't provided by clang, we expand to the token. - Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"), - Diag(Test.range("type"), - "cannot initialize a variable of type 'double' with an lvalue " - "of type 'const char [4]'"), - Diag(Test.range("nomember"), "no member named 'y' in 'Foo'"), - Diag(Test.range("nomembernamespace"), - "no member named 'test' in namespace 'test'"))); -} - -TEST(DiagnosticsTest, FlagsMatter) { - Annotations Test("[[void]] main() {}"); - auto TU = TestTU::withCode(Test.code()); - EXPECT_THAT(TU.build().getDiagnostics(), - ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"), - WithFix(Fix(Test.range(), "int", - "change 'void' to 'int'"))))); - // Same code built as C gets different diagnostics. - TU.Filename = "Plain.c"; - EXPECT_THAT( - TU.build().getDiagnostics(), - ElementsAre(AllOf( - Diag(Test.range(), "return type of 'main' is not 'int'"), - WithFix(Fix(Test.range(), "int", "change return type to 'int'"))))); -} - -TEST(DiagnosticsTest, ClangTidy) { - Annotations Test(R"cpp( - #include $deprecated[["assert.h"]] - - #define $macrodef[[SQUARE]](X) (X)*(X) - int main() { - return $doubled[[sizeof]](sizeof(int)); - int y = 4; - return SQUARE($macroarg[[++]]y); - } - )cpp"); - auto TU = TestTU::withCode(Test.code()); - TU.HeaderFilename = "assert.h"; // Suppress "not found" error. - TU.ClangTidyChecks = - "-*, bugprone-sizeof-expression, bugprone-macro-repeated-side-effects, " - "modernize-deprecated-headers"; - EXPECT_THAT( - TU.build().getDiagnostics(), - UnorderedElementsAre( - AllOf(Diag(Test.range("deprecated"), - "inclusion of deprecated C++ header 'assert.h'; consider " - "using 'cassert' instead [modernize-deprecated-headers]"), - WithFix(Fix(Test.range("deprecated"), "", - "change '\"assert.h\"' to ''"))), - Diag(Test.range("doubled"), - "suspicious usage of 'sizeof(sizeof(...))' " - "[bugprone-sizeof-expression]"), - AllOf( - Diag(Test.range("macroarg"), - "side effects in the 1st macro argument 'X' are repeated in " - "macro expansion [bugprone-macro-repeated-side-effects]"), - WithNote(Diag(Test.range("macrodef"), - "macro 'SQUARE' defined here " - "[bugprone-macro-repeated-side-effects]"))), - Diag(Test.range("macroarg"), - "multiple unsequenced modifications to 'y'"))); -} - -TEST(DiagnosticsTest, Preprocessor) { - // This looks like a preamble, but there's an #else in the middle! - // Check that: - // - the #else doesn't generate diagnostics (we had this bug) - // - we get diagnostics from the taken branch - // - we get no diagnostics from the not taken branch - Annotations Test(R"cpp( - #ifndef FOO - #define FOO - int a = [[b]]; - #else - int x = y; - #endif - )cpp"); - EXPECT_THAT( - TestTU::withCode(Test.code()).build().getDiagnostics(), - ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'"))); -} - -TEST(DiagnosticsTest, InsideMacros) { - Annotations Test(R"cpp( - #define TEN 10 - #define RET(x) return x + 10 - - int* foo() { - RET($foo[[0]]); - } - int* bar() { - return $bar[[TEN]]; - } - )cpp"); - EXPECT_THAT(TestTU::withCode(Test.code()).build().getDiagnostics(), - ElementsAre(Diag(Test.range("foo"), - "cannot initialize return object of type " - "'int *' with an rvalue of type 'int'"), - Diag(Test.range("bar"), - "cannot initialize return object of type " - "'int *' with an rvalue of type 'int'"))); -} - -TEST(DiagnosticsTest, ToLSP) { - clangd::Diag D; - D.Message = "something terrible happened"; - D.Range = {pos(1, 2), pos(3, 4)}; - D.InsideMainFile = true; - D.Severity = DiagnosticsEngine::Error; - D.File = "foo/bar/main.cpp"; - - clangd::Note NoteInMain; - NoteInMain.Message = "declared somewhere in the main file"; - NoteInMain.Range = {pos(5, 6), pos(7, 8)}; - NoteInMain.Severity = DiagnosticsEngine::Remark; - NoteInMain.File = "../foo/bar/main.cpp"; - NoteInMain.InsideMainFile = true; - D.Notes.push_back(NoteInMain); - - clangd::Note NoteInHeader; - NoteInHeader.Message = "declared somewhere in the header file"; - NoteInHeader.Range = {pos(9, 10), pos(11, 12)}; - NoteInHeader.Severity = DiagnosticsEngine::Note; - NoteInHeader.File = "../foo/baz/header.h"; - NoteInHeader.InsideMainFile = false; - D.Notes.push_back(NoteInHeader); - - clangd::Fix F; - F.Message = "do something"; - D.Fixes.push_back(F); - - auto MatchingLSP = [](const DiagBase &D, StringRef Message) { - clangd::Diagnostic Res; - Res.range = D.Range; - Res.severity = getSeverity(D.Severity); - Res.message = Message; - return Res; - }; - - // Diagnostics should turn into these: - clangd::Diagnostic MainLSP = MatchingLSP(D, R"(Something terrible happened - -main.cpp:6:7: remark: declared somewhere in the main file - -../foo/baz/header.h:10:11: -note: declared somewhere in the header file)"); - - clangd::Diagnostic NoteInMainLSP = - MatchingLSP(NoteInMain, R"(Declared somewhere in the main file - -main.cpp:2:3: error: something terrible happened)"); - - // Transform dianostics and check the results. - std::vector>> LSPDiags; - toLSPDiags(D, -#ifdef _WIN32 - URIForFile::canonicalize("c:\\path\\to\\foo\\bar\\main.cpp", - /*TUPath=*/""), -#else - URIForFile::canonicalize("/path/to/foo/bar/main.cpp", /*TUPath=*/""), -#endif - ClangdDiagnosticOptions(), - [&](clangd::Diagnostic LSPDiag, ArrayRef Fixes) { - LSPDiags.push_back( - {std::move(LSPDiag), - std::vector(Fixes.begin(), Fixes.end())}); - }); - - EXPECT_THAT( - LSPDiags, - ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))), - Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty()))); -} TEST(ClangdUnitTest, GetBeginningOfIdentifier) { std::string Preamble = R"cpp( diff --git a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp index 16d5eef..ba539fc 100644 --- a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp +++ b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp @@ -16,6 +16,7 @@ #include "SourceCode.h" #include "SyncAPI.h" #include "TestFS.h" +#include "TestIndex.h" #include "index/MemIndex.h" #include "clang/Sema/CodeCompleteConsumer.h" #include "llvm/Support/Error.h" @@ -137,53 +138,6 @@ CodeCompleteResult completions(llvm::StringRef Text, FilePath); } -std::string replace(llvm::StringRef Haystack, llvm::StringRef Needle, - llvm::StringRef Repl) { - std::string Result; - llvm::raw_string_ostream OS(Result); - std::pair Split; - for (Split = Haystack.split(Needle); !Split.second.empty(); - Split = Split.first.split(Needle)) - OS << Split.first << Repl; - Result += Split.first; - OS.flush(); - return Result; -} - -// Helpers to produce fake index symbols for memIndex() or completions(). -// USRFormat is a regex replacement string for the unqualified part of the USR. -Symbol sym(llvm::StringRef QName, index::SymbolKind Kind, - llvm::StringRef USRFormat) { - Symbol Sym; - std::string USR = "c:"; // We synthesize a few simple cases of USRs by hand! - size_t Pos = QName.rfind("::"); - if (Pos == llvm::StringRef::npos) { - Sym.Name = QName; - Sym.Scope = ""; - } else { - Sym.Name = QName.substr(Pos + 2); - Sym.Scope = QName.substr(0, Pos + 2); - USR += "@N@" + replace(QName.substr(0, Pos), "::", "@N@"); // ns:: -> @N@ns - } - USR += llvm::Regex("^.*$").sub(USRFormat, Sym.Name); // e.g. func -> @F@func# - Sym.ID = SymbolID(USR); - Sym.SymInfo.Kind = Kind; - Sym.Flags |= Symbol::IndexedForCodeCompletion; - Sym.Origin = SymbolOrigin::Static; - return Sym; -} -Symbol func(llvm::StringRef Name) { // Assumes the function has no args. - return sym(Name, index::SymbolKind::Function, "@F@\\0#"); // no args -} -Symbol cls(llvm::StringRef Name) { - return sym(Name, index::SymbolKind::Class, "@S@\\0"); -} -Symbol var(llvm::StringRef Name) { - return sym(Name, index::SymbolKind::Variable, "@\\0"); -} -Symbol ns(llvm::StringRef Name) { - return sym(Name, index::SymbolKind::Namespace, "@N@\\0"); -} Symbol withReferences(int N, Symbol S) { S.References = N; return S; diff --git a/clang-tools-extra/unittests/clangd/DiagnosticsTests.cpp b/clang-tools-extra/unittests/clangd/DiagnosticsTests.cpp new file mode 100644 index 0000000..ea1952e --- /dev/null +++ b/clang-tools-extra/unittests/clangd/DiagnosticsTests.cpp @@ -0,0 +1,351 @@ +//===--- DiagnosticsTests.cpp ------------------------------------*- C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "Annotations.h" +#include "ClangdUnit.h" +#include "SourceCode.h" +#include "TestIndex.h" +#include "TestTU.h" +#include "index/MemIndex.h" +#include "llvm/Support/ScopedPrinter.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace clang { +namespace clangd { +namespace { + +using testing::ElementsAre; +using testing::Field; +using testing::IsEmpty; +using testing::Pair; +using testing::UnorderedElementsAre; + +testing::Matcher WithFix(testing::Matcher FixMatcher) { + return Field(&Diag::Fixes, ElementsAre(FixMatcher)); +} + +testing::Matcher WithNote(testing::Matcher NoteMatcher) { + return Field(&Diag::Notes, ElementsAre(NoteMatcher)); +} + +MATCHER_P2(Diag, Range, Message, + "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") { + return arg.Range == Range && arg.Message == Message; +} + +MATCHER_P3(Fix, Range, Replacement, Message, + "Fix " + llvm::to_string(Range) + " => " + + testing::PrintToString(Replacement) + " = [" + Message + "]") { + return arg.Message == Message && arg.Edits.size() == 1 && + arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement; +} + +MATCHER_P(EqualToLSPDiag, LSPDiag, + "LSP diagnostic " + llvm::to_string(LSPDiag)) { + return std::tie(arg.range, arg.severity, arg.message) == + std::tie(LSPDiag.range, LSPDiag.severity, LSPDiag.message); +} + +MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) { + if (arg.Message != Fix.Message) + return false; + if (arg.Edits.size() != Fix.Edits.size()) + return false; + for (std::size_t I = 0; I < arg.Edits.size(); ++I) { + if (arg.Edits[I].range != Fix.Edits[I].range || + arg.Edits[I].newText != Fix.Edits[I].newText) + return false; + } + return true; +} + + +// Helper function to make tests shorter. +Position pos(int line, int character) { + Position Res; + Res.line = line; + Res.character = character; + return Res; +} + +TEST(DiagnosticsTest, DiagnosticRanges) { + // Check we report correct ranges, including various edge-cases. + Annotations Test(R"cpp( + namespace test{}; + void $decl[[foo]](); + int main() { + $typo[[go\ +o]](); + foo()$semicolon[[]]//with comments + $unk[[unknown]](); + double $type[[bar]] = "foo"; + struct Foo { int x; }; Foo a; + a.$nomember[[y]]; + test::$nomembernamespace[[test]]; + } + )cpp"); + EXPECT_THAT( + TestTU::withCode(Test.code()).build().getDiagnostics(), + ElementsAre( + // This range spans lines. + AllOf(Diag(Test.range("typo"), + "use of undeclared identifier 'goo'; did you mean 'foo'?"), + WithFix( + Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")), + // This is a pretty normal range. + WithNote(Diag(Test.range("decl"), "'foo' declared here"))), + // This range is zero-width and insertion. Therefore make sure we are + // not expanding it into other tokens. Since we are not going to + // replace those. + AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"), + WithFix(Fix(Test.range("semicolon"), ";", "insert ';'"))), + // This range isn't provided by clang, we expand to the token. + Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"), + Diag(Test.range("type"), + "cannot initialize a variable of type 'double' with an lvalue " + "of type 'const char [4]'"), + Diag(Test.range("nomember"), "no member named 'y' in 'Foo'"), + Diag(Test.range("nomembernamespace"), + "no member named 'test' in namespace 'test'"))); +} + +TEST(DiagnosticsTest, FlagsMatter) { + Annotations Test("[[void]] main() {}"); + auto TU = TestTU::withCode(Test.code()); + EXPECT_THAT(TU.build().getDiagnostics(), + ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"), + WithFix(Fix(Test.range(), "int", + "change 'void' to 'int'"))))); + // Same code built as C gets different diagnostics. + TU.Filename = "Plain.c"; + EXPECT_THAT( + TU.build().getDiagnostics(), + ElementsAre(AllOf( + Diag(Test.range(), "return type of 'main' is not 'int'"), + WithFix(Fix(Test.range(), "int", "change return type to 'int'"))))); +} + +TEST(DiagnosticsTest, ClangTidy) { + Annotations Test(R"cpp( + #include $deprecated[["assert.h"]] + + #define $macrodef[[SQUARE]](X) (X)*(X) + int main() { + return $doubled[[sizeof]](sizeof(int)); + int y = 4; + return SQUARE($macroarg[[++]]y); + } + )cpp"); + auto TU = TestTU::withCode(Test.code()); + TU.HeaderFilename = "assert.h"; // Suppress "not found" error. + TU.ClangTidyChecks = + "-*, bugprone-sizeof-expression, bugprone-macro-repeated-side-effects, " + "modernize-deprecated-headers"; + EXPECT_THAT( + TU.build().getDiagnostics(), + UnorderedElementsAre( + AllOf(Diag(Test.range("deprecated"), + "inclusion of deprecated C++ header 'assert.h'; consider " + "using 'cassert' instead [modernize-deprecated-headers]"), + WithFix(Fix(Test.range("deprecated"), "", + "change '\"assert.h\"' to ''"))), + Diag(Test.range("doubled"), + "suspicious usage of 'sizeof(sizeof(...))' " + "[bugprone-sizeof-expression]"), + AllOf( + Diag(Test.range("macroarg"), + "side effects in the 1st macro argument 'X' are repeated in " + "macro expansion [bugprone-macro-repeated-side-effects]"), + WithNote(Diag(Test.range("macrodef"), + "macro 'SQUARE' defined here " + "[bugprone-macro-repeated-side-effects]"))), + Diag(Test.range("macroarg"), + "multiple unsequenced modifications to 'y'"))); +} + +TEST(DiagnosticsTest, Preprocessor) { + // This looks like a preamble, but there's an #else in the middle! + // Check that: + // - the #else doesn't generate diagnostics (we had this bug) + // - we get diagnostics from the taken branch + // - we get no diagnostics from the not taken branch + Annotations Test(R"cpp( + #ifndef FOO + #define FOO + int a = [[b]]; + #else + int x = y; + #endif + )cpp"); + EXPECT_THAT( + TestTU::withCode(Test.code()).build().getDiagnostics(), + ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'"))); +} + +TEST(DiagnosticsTest, InsideMacros) { + Annotations Test(R"cpp( + #define TEN 10 + #define RET(x) return x + 10 + + int* foo() { + RET($foo[[0]]); + } + int* bar() { + return $bar[[TEN]]; + } + )cpp"); + EXPECT_THAT(TestTU::withCode(Test.code()).build().getDiagnostics(), + ElementsAre(Diag(Test.range("foo"), + "cannot initialize return object of type " + "'int *' with an rvalue of type 'int'"), + Diag(Test.range("bar"), + "cannot initialize return object of type " + "'int *' with an rvalue of type 'int'"))); +} + +TEST(DiagnosticsTest, ToLSP) { + clangd::Diag D; + D.Message = "something terrible happened"; + D.Range = {pos(1, 2), pos(3, 4)}; + D.InsideMainFile = true; + D.Severity = DiagnosticsEngine::Error; + D.File = "foo/bar/main.cpp"; + + clangd::Note NoteInMain; + NoteInMain.Message = "declared somewhere in the main file"; + NoteInMain.Range = {pos(5, 6), pos(7, 8)}; + NoteInMain.Severity = DiagnosticsEngine::Remark; + NoteInMain.File = "../foo/bar/main.cpp"; + NoteInMain.InsideMainFile = true; + D.Notes.push_back(NoteInMain); + + clangd::Note NoteInHeader; + NoteInHeader.Message = "declared somewhere in the header file"; + NoteInHeader.Range = {pos(9, 10), pos(11, 12)}; + NoteInHeader.Severity = DiagnosticsEngine::Note; + NoteInHeader.File = "../foo/baz/header.h"; + NoteInHeader.InsideMainFile = false; + D.Notes.push_back(NoteInHeader); + + clangd::Fix F; + F.Message = "do something"; + D.Fixes.push_back(F); + + auto MatchingLSP = [](const DiagBase &D, StringRef Message) { + clangd::Diagnostic Res; + Res.range = D.Range; + Res.severity = getSeverity(D.Severity); + Res.message = Message; + return Res; + }; + + // Diagnostics should turn into these: + clangd::Diagnostic MainLSP = MatchingLSP(D, R"(Something terrible happened + +main.cpp:6:7: remark: declared somewhere in the main file + +../foo/baz/header.h:10:11: +note: declared somewhere in the header file)"); + + clangd::Diagnostic NoteInMainLSP = + MatchingLSP(NoteInMain, R"(Declared somewhere in the main file + +main.cpp:2:3: error: something terrible happened)"); + + // Transform dianostics and check the results. + std::vector>> LSPDiags; + toLSPDiags(D, +#ifdef _WIN32 + URIForFile::canonicalize("c:\\path\\to\\foo\\bar\\main.cpp", + /*TUPath=*/""), +#else + URIForFile::canonicalize("/path/to/foo/bar/main.cpp", /*TUPath=*/""), +#endif + ClangdDiagnosticOptions(), + [&](clangd::Diagnostic LSPDiag, ArrayRef Fixes) { + LSPDiags.push_back( + {std::move(LSPDiag), + std::vector(Fixes.begin(), Fixes.end())}); + }); + + EXPECT_THAT( + LSPDiags, + ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))), + Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty()))); +} + +TEST(IncludeFixerTest, IncompleteType) { + Annotations Test(R"cpp( +$insert[[]]namespace ns { + class X; +} +class Y : $base[[public ns::X]] {}; +int main() { + ns::X *x; + x$access[[->]]f(); +} + )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()); + TU.ExternalIndex = Index.get(); + + EXPECT_THAT( + TU.build().getDiagnostics(), + UnorderedElementsAre( + AllOf(Diag(Test.range("base"), "base class has incomplete type"), + WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n", + "Add include \"x.h\" for symbol ns::X"))), + AllOf(Diag(Test.range("access"), + "member access into incomplete type 'ns::X'"), + WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n", + "Add include \"x.h\" for symbol ns::X"))))); +} + +TEST(IncludeFixerTest, NoSuggestIncludeWhenNoDefinitionInHeader) { + Annotations Test(R"cpp( +$insert[[]]namespace ns { + class X; +} +class Y : $base[[public ns::X]] {}; +int main() { + ns::X *x; + x$access[[->]]f(); +} + )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.cc"; + Sym.IncludeHeaders.emplace_back("\"x.h\"", 1); + + SymbolSlab::Builder Slab; + Slab.insert(Sym); + auto Index = MemIndex::build(std::move(Slab).build(), RefSlab()); + TU.ExternalIndex = Index.get(); + + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Test.range("base"), "base class has incomplete type"), + Diag(Test.range("access"), + "member access into incomplete type 'ns::X'"))); +} + +} // namespace +} // namespace clangd +} // namespace clang + diff --git a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp index b0c21c0..898dd64 100644 --- a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp +++ b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp @@ -364,7 +364,7 @@ TEST(FileIndexTest, ReferencesInMainFileWithPreamble) { ParsedAST::build(createInvocationFromCommandLine(Cmd), PreambleData, llvm::MemoryBuffer::getMemBufferCopy(Main.code()), std::make_shared(), PI.FS, - tidy::ClangTidyOptions::getDefaults()); + /*Index=*/nullptr, ParseOptions()); ASSERT_TRUE(AST); FileIndex Index; Index.updateMain(MainFile, *AST); diff --git a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp index 79ee779..e5b42328 100644 --- a/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp +++ b/clang-tools-extra/unittests/clangd/TUSchedulerTests.cpp @@ -26,7 +26,6 @@ using ::testing::AllOf; using ::testing::AnyOf; using ::testing::Each; using ::testing::ElementsAre; -using ::testing::Pair; using ::testing::Pointee; using ::testing::UnorderedElementsAre; @@ -37,9 +36,12 @@ MATCHER_P2(TUState, State, ActionName, "") { class TUSchedulerTests : public ::testing::Test { protected: ParseInputs getInputs(PathRef File, std::string Contents) { - return ParseInputs{*CDB.getCompileCommand(File), - buildTestFS(Files, Timestamps), std::move(Contents), - tidy::ClangTidyOptions::getDefaults()}; + ParseInputs Inputs; + Inputs.CompileCommand = *CDB.getCompileCommand(File); + Inputs.FS = buildTestFS(Files, Timestamps); + Inputs.Contents = std::move(Contents); + Inputs.Opts = ParseOptions(); + return Inputs; } void updateWithCallback(TUScheduler &S, PathRef File, diff --git a/clang-tools-extra/unittests/clangd/TestIndex.cpp b/clang-tools-extra/unittests/clangd/TestIndex.cpp index a909193..877bb75 100644 --- a/clang-tools-extra/unittests/clangd/TestIndex.cpp +++ b/clang-tools-extra/unittests/clangd/TestIndex.cpp @@ -7,6 +7,8 @@ //===----------------------------------------------------------------------===// #include "TestIndex.h" +#include "clang/Index/IndexSymbol.h" +#include "llvm/Support/Regex.h" namespace clang { namespace clangd { @@ -25,6 +27,58 @@ Symbol symbol(llvm::StringRef QName) { return Sym; } +static std::string replace(llvm::StringRef Haystack, llvm::StringRef Needle, + llvm::StringRef Repl) { + std::string Result; + llvm::raw_string_ostream OS(Result); + std::pair Split; + for (Split = Haystack.split(Needle); !Split.second.empty(); + Split = Split.first.split(Needle)) + OS << Split.first << Repl; + Result += Split.first; + OS.flush(); + return Result; +} + +// Helpers to produce fake index symbols for memIndex() or completions(). +// USRFormat is a regex replacement string for the unqualified part of the USR. +Symbol sym(llvm::StringRef QName, index::SymbolKind Kind, + llvm::StringRef USRFormat) { + Symbol Sym; + std::string USR = "c:"; // We synthesize a few simple cases of USRs by hand! + size_t Pos = QName.rfind("::"); + if (Pos == llvm::StringRef::npos) { + Sym.Name = QName; + Sym.Scope = ""; + } else { + Sym.Name = QName.substr(Pos + 2); + Sym.Scope = QName.substr(0, Pos + 2); + USR += "@N@" + replace(QName.substr(0, Pos), "::", "@N@"); // ns:: -> @N@ns + } + USR += llvm::Regex("^.*$").sub(USRFormat, Sym.Name); // e.g. func -> @F@func# + Sym.ID = SymbolID(USR); + Sym.SymInfo.Kind = Kind; + Sym.Flags |= Symbol::IndexedForCodeCompletion; + Sym.Origin = SymbolOrigin::Static; + return Sym; +} + +Symbol func(llvm::StringRef Name) { // Assumes the function has no args. + return sym(Name, index::SymbolKind::Function, "@F@\\0#"); // no args +} + +Symbol cls(llvm::StringRef Name) { + return sym(Name, index::SymbolKind::Class, "@S@\\0"); +} + +Symbol var(llvm::StringRef Name) { + return sym(Name, index::SymbolKind::Variable, "@\\0"); +} + +Symbol ns(llvm::StringRef Name) { + return sym(Name, index::SymbolKind::Namespace, "@N@\\0"); +} + SymbolSlab generateSymbols(std::vector QualifiedNames) { SymbolSlab::Builder Slab; for (llvm::StringRef QName : QualifiedNames) diff --git a/clang-tools-extra/unittests/clangd/TestIndex.h b/clang-tools-extra/unittests/clangd/TestIndex.h index 6fef48d..01de089 100644 --- a/clang-tools-extra/unittests/clangd/TestIndex.h +++ b/clang-tools-extra/unittests/clangd/TestIndex.h @@ -18,6 +18,19 @@ namespace clangd { // Creates Symbol instance and sets SymbolID to given QualifiedName. Symbol symbol(llvm::StringRef QName); +// Helpers to produce fake index symbols with proper SymbolID. +// USRFormat is a regex replacement string for the unqualified part of the USR. +Symbol sym(llvm::StringRef QName, index::SymbolKind Kind, + llvm::StringRef USRFormat); +// Creats a function symbol assuming no function arg. +Symbol func(llvm::StringRef Name); +// Creates a class symbol. +Symbol cls(llvm::StringRef Name); +// Creates a variable symbol. +Symbol var(llvm::StringRef Name); +// Creates a namespace symbol. +Symbol ns(llvm::StringRef Name); + // Create a slab of symbols with the given qualified names as IDs and names. SymbolSlab generateSymbols(std::vector QualifiedNames); diff --git a/clang-tools-extra/unittests/clangd/TestTU.cpp b/clang-tools-extra/unittests/clangd/TestTU.cpp index 067d3f3..8c61bb8 100644 --- a/clang-tools-extra/unittests/clangd/TestTU.cpp +++ b/clang-tools-extra/unittests/clangd/TestTU.cpp @@ -35,8 +35,11 @@ ParsedAST TestTU::build() const { Inputs.CompileCommand.Directory = testRoot(); Inputs.Contents = Code; Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}}); - Inputs.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults(); - Inputs.ClangTidyOpts.Checks = ClangTidyChecks; + Inputs.Opts = ParseOptions(); + Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks; + Inputs.Index = ExternalIndex; + if (Inputs.Index) + Inputs.Opts.SuggestMissingIncludes = true; auto PCHs = std::make_shared(); auto CI = buildCompilerInvocation(Inputs); assert(CI && "Failed to build compilation invocation."); diff --git a/clang-tools-extra/unittests/clangd/TestTU.h b/clang-tools-extra/unittests/clangd/TestTU.h index 7b84e4b..beed124 100644 --- a/clang-tools-extra/unittests/clangd/TestTU.h +++ b/clang-tools-extra/unittests/clangd/TestTU.h @@ -49,6 +49,8 @@ struct TestTU { std::vector ExtraArgs; llvm::Optional ClangTidyChecks; + // Index to use when building AST. + const SymbolIndex *ExternalIndex = nullptr; ParsedAST build() const; SymbolSlab headerSymbols() const; -- 2.7.4