Index: clangd/CMakeLists.txt =================================================================== --- clangd/CMakeLists.txt +++ clangd/CMakeLists.txt @@ -11,6 +11,7 @@ CompileArgsCache.cpp Compiler.cpp Context.cpp + Diagnostics.cpp DraftStore.cpp FuzzyMatch.cpp GlobalCompilationDatabase.cpp Index: clangd/ClangdLSPServer.h =================================================================== --- clangd/ClangdLSPServer.h +++ clangd/ClangdLSPServer.h @@ -50,9 +50,7 @@ private: // Implement DiagnosticsConsumer. - virtual void - onDiagnosticsReady(PathRef File, - Tagged> Diagnostics) override; + void onDiagnosticsReady(PathRef File, Tagged Diagnostics) override; // Implement ProtocolCallbacks. void onInitialize(InitializeParams &Params) override; Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ clangd/ClangdLSPServer.cpp @@ -8,6 +8,7 @@ //===---------------------------------------------------------------------===// #include "ClangdLSPServer.h" +#include "Diagnostics.h" #include "JSONRPCDispatcher.h" #include "SourceCode.h" #include "URI.h" @@ -304,6 +305,12 @@ } void ClangdLSPServer::onCodeAction(CodeActionParams &Params) { + auto GetFixitMessage = [](llvm::StringRef Message) -> llvm::StringRef { + // VSCode does not really print messages with newlines nicely. + // And we put notes into diagnostic messages, which are not useful for FixIt + // messages shown to the users. + return Message.take_until([](char C) { return C == '\n'; }); + }; // We provide a code action for each diagnostic at the requested location // which has FixIts available. auto Code = Server.getDocument(Params.textDocument.uri.file()); @@ -318,7 +325,8 @@ WorkspaceEdit WE; WE.changes = {{Params.textDocument.uri.uri(), std::move(Edits)}}; Commands.push_back(json::obj{ - {"title", llvm::formatv("Apply FixIt {0}", D.message)}, + {"title", + llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))}, {"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND}, {"arguments", {WE}}, }); @@ -441,23 +449,22 @@ return FixItsIter->second; } -void ClangdLSPServer::onDiagnosticsReady( - PathRef File, Tagged> Diagnostics) { +void ClangdLSPServer::onDiagnosticsReady(PathRef File, + Tagged Diagnostics) { json::ary DiagnosticsJSON; DiagnosticToReplacementMap LocalFixIts; // Temporary storage - for (auto &DiagWithFixes : Diagnostics.Value) { - auto Diag = DiagWithFixes.Diag; + toLSPDiags(Diagnostics.Value, [&](DiagWithFixIts D) { DiagnosticsJSON.push_back(json::obj{ - {"range", Diag.range}, - {"severity", Diag.severity}, - {"message", Diag.message}, + {"range", D.Diag.range}, + {"severity", D.Diag.severity}, + {"message", D.Diag.message}, }); - // We convert to Replacements to become independent of the SourceManager. - auto &FixItsForDiagnostic = LocalFixIts[Diag]; - std::copy(DiagWithFixes.FixIts.begin(), DiagWithFixes.FixIts.end(), + + auto &FixItsForDiagnostic = LocalFixIts[D.Diag]; + std::copy(D.FixIts.begin(), D.FixIts.end(), std::back_inserter(FixItsForDiagnostic)); - } + }); // Cache FixIts { Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -70,9 +70,8 @@ virtual ~DiagnosticsConsumer() = default; /// Called by ClangdServer when \p Diagnostics for \p File are ready. - virtual void - onDiagnosticsReady(PathRef File, - Tagged> Diagnostics) = 0; + virtual void onDiagnosticsReady(PathRef File, + Tagged Diagnostics) = 0; }; class FileSystemProvider { Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -527,7 +527,7 @@ VFSTag Tag = std::move(TaggedFS.Tag); auto Callback = [this, Version, FileStr, - Tag](std::vector Diags) { + Tag](DiagList Diags) { // We need to serialize access to resulting diagnostics to avoid calling // `onDiagnosticsReady` in the wrong order. std::lock_guard DiagsLock(DiagnosticsMutex); Index: clangd/ClangdUnit.h =================================================================== --- clangd/ClangdUnit.h +++ clangd/ClangdUnit.h @@ -10,6 +10,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H +#include "Diagnostics.h" #include "Function.h" #include "Path.h" #include "Protocol.h" @@ -39,24 +40,17 @@ namespace clangd { -/// A diagnostic with its FixIts. -struct DiagWithFixIts { - clangd::Diagnostic Diag; - llvm::SmallVector FixIts; -}; - using InclusionLocations = std::vector>; // Stores Preamble and associated data. struct PreambleData { PreambleData(PrecompiledPreamble Preamble, std::vector TopLevelDeclIDs, - std::vector Diags, - InclusionLocations IncLocations); + DiagList Diags, InclusionLocations IncLocations); PrecompiledPreamble Preamble; std::vector TopLevelDeclIDs; - std::vector Diags; + DiagList Diags; InclusionLocations IncLocations; }; @@ -96,7 +90,7 @@ /// this call might be expensive. ArrayRef getTopLevelDecls(); - const std::vector &getDiagnostics() const; + const DiagList &getDiagnostics() const; /// Returns the esitmated size of the AST and the accessory structures, in /// bytes. Does not include the size of the preamble. @@ -107,8 +101,8 @@ ParsedAST(std::shared_ptr Preamble, std::unique_ptr Clang, std::unique_ptr Action, - std::vector TopLevelDecls, - std::vector Diags, InclusionLocations IncLocations); + std::vector TopLevelDecls, DiagList Diags, + InclusionLocations IncLocations); private: void ensurePreambleDeclsDeserialized(); @@ -125,7 +119,7 @@ std::unique_ptr Action; // Data, stored after parsing. - std::vector Diags; + DiagList Diags; std::vector TopLevelDecls; bool PreambleDeclsDeserialized; InclusionLocations IncLocations; @@ -143,7 +137,7 @@ /// Rebuild the AST and the preamble. /// Returns a list of diagnostics or llvm::None, if an error occured. - llvm::Optional> rebuild(ParseInputs &&Inputs); + llvm::Optional rebuild(ParseInputs &&Inputs); /// Returns the last built preamble. const std::shared_ptr &getPreamble() const; /// Returns the last built AST. Index: clangd/ClangdUnit.cpp =================================================================== --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -9,7 +9,9 @@ #include "ClangdUnit.h" #include "Compiler.h" +#include "Diagnostics.h" #include "Logger.h" +#include "SourceCode.h" #include "Trace.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/CompilerInvocation.h" @@ -81,22 +83,6 @@ std::vector TopLevelDecls; }; -// Converts a half-open clang source range to an LSP range. -// Note that clang also uses closed source ranges, which this can't handle! -Range toRange(CharSourceRange R, const SourceManager &M) { - // Clang is 1-based, LSP uses 0-based indexes. - Position Begin; - Begin.line = static_cast(M.getSpellingLineNumber(R.getBegin())) - 1; - Begin.character = - static_cast(M.getSpellingColumnNumber(R.getBegin())) - 1; - - Position End; - End.line = static_cast(M.getSpellingLineNumber(R.getEnd())) - 1; - End.character = static_cast(M.getSpellingColumnNumber(R.getEnd())) - 1; - - return {Begin, End}; -} - class InclusionLocationsCollector : public PPCallbacks { public: InclusionLocationsCollector(SourceManager &SourceMgr, @@ -115,7 +101,7 @@ if (SourceMgr.isInMainFile(SR.getBegin())) { // Only inclusion directives in the main file make sense. The user cannot // select directives not in the main file. - IncLocations.emplace_back(toRange(FilenameRange, SourceMgr), + IncLocations.emplace_back(halfOpenToRange(FilenameRange, SourceMgr), File->tryGetRealPathName()); } } @@ -170,113 +156,6 @@ SourceManager *SourceMgr = nullptr; }; -/// Convert from clang diagnostic level to LSP severity. -static int getSeverity(DiagnosticsEngine::Level L) { - switch (L) { - case DiagnosticsEngine::Remark: - return 4; - case DiagnosticsEngine::Note: - return 3; - case DiagnosticsEngine::Warning: - return 2; - case DiagnosticsEngine::Fatal: - case DiagnosticsEngine::Error: - return 1; - case DiagnosticsEngine::Ignored: - return 0; - } - llvm_unreachable("Unknown diagnostic level!"); -} - -// Checks whether a location is within a half-open range. -// Note that clang also uses closed source ranges, which this can't handle! -bool locationInRange(SourceLocation L, CharSourceRange R, - const SourceManager &M) { - assert(R.isCharRange()); - if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) || - M.getFileID(R.getBegin()) != M.getFileID(L)) - return false; - return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd()); -} - -// Clang diags have a location (shown as ^) and 0 or more ranges (~~~~). -// LSP needs a single range. -Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) { - auto &M = D.getSourceManager(); - auto Loc = M.getFileLoc(D.getLocation()); - // Accept the first range that contains the location. - for (const auto &CR : D.getRanges()) { - auto R = Lexer::makeFileCharRange(CR, M, L); - if (locationInRange(Loc, R, M)) - return toRange(R, M); - } - // The range may be given as a fixit hint instead. - for (const auto &F : D.getFixItHints()) { - auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L); - if (locationInRange(Loc, R, M)) - return toRange(R, M); - } - // If no suitable range is found, just use the token at the location. - auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L); - if (!R.isValid()) // Fall back to location only, let the editor deal with it. - R = CharSourceRange::getCharRange(Loc); - return toRange(R, M); -} - -TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M, - const LangOptions &L) { - TextEdit Result; - Result.range = toRange(Lexer::makeFileCharRange(FixIt.RemoveRange, M, L), M); - Result.newText = FixIt.CodeToInsert; - return Result; -} - -llvm::Optional toClangdDiag(const clang::Diagnostic &D, - DiagnosticsEngine::Level Level, - const LangOptions &LangOpts) { - if (!D.hasSourceManager() || !D.getLocation().isValid() || - !D.getSourceManager().isInMainFile(D.getLocation())) { - IgnoreDiagnostics::log(Level, D); - return llvm::None; - } - - SmallString<64> Message; - D.FormatDiagnostic(Message); - - DiagWithFixIts Result; - Result.Diag.range = diagnosticRange(D, LangOpts); - Result.Diag.severity = getSeverity(Level); - Result.Diag.message = Message.str(); - for (const FixItHint &Fix : D.getFixItHints()) - Result.FixIts.push_back(toTextEdit(Fix, D.getSourceManager(), LangOpts)); - return std::move(Result); -} - -class StoreDiagsConsumer : public DiagnosticConsumer { -public: - StoreDiagsConsumer(std::vector &Output) : Output(Output) {} - - // Track language options in case we need to expand token ranges. - void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override { - LangOpts = Opts; - } - - void EndSourceFile() override { LangOpts = llvm::None; } - - void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, - const clang::Diagnostic &Info) override { - DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info); - - if (LangOpts) - if (auto D = toClangdDiag(Info, DiagLevel, *LangOpts)) - Output.push_back(std::move(*D)); - } - -private: - std::vector &Output; - llvm::Optional LangOpts; -}; - } // namespace void clangd::dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) { @@ -289,8 +168,8 @@ std::unique_ptr Buffer, std::shared_ptr PCHs, IntrusiveRefCntPtr VFS) { - std::vector ASTDiags; - StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags); + DiagList ASTDiags; + StoreDiagsConsumer UnitDiagsConsumer(ASTDiags); const PrecompiledPreamble *PreamblePCH = Preamble ? &Preamble->Preamble : nullptr; @@ -328,6 +207,8 @@ // UnitDiagsConsumer is local, we can not store it in CompilerInstance that // has a longer lifetime. Clang->getDiagnostics().setClient(new IgnoreDiagnostics); + // CompilerInstance won't run this callback, do it directly. + UnitDiagsConsumer.EndSourceFile(); std::vector ParsedDecls = Action->takeTopLevelDecls(); return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action), @@ -399,16 +280,12 @@ return TopLevelDecls; } -const std::vector &ParsedAST::getDiagnostics() const { - return Diags; -} +const DiagList &ParsedAST::getDiagnostics() const { return Diags; } std::size_t ParsedAST::getUsedBytes() const { auto &AST = getASTContext(); - // FIXME(ibiryukov): we do not account for the dynamically allocated part of - // SmallVector inside each Diag. return AST.getASTAllocatedMemory() + AST.getSideTableAllocatedMemory() + - ::getUsedBytes(TopLevelDecls) + ::getUsedBytes(Diags); + ::getUsedBytes(TopLevelDecls) + Diags.getUsedBytes(); } const InclusionLocations &ParsedAST::getInclusionLocations() const { @@ -417,8 +294,7 @@ PreambleData::PreambleData(PrecompiledPreamble Preamble, std::vector TopLevelDeclIDs, - std::vector Diags, - InclusionLocations IncLocations) + DiagList Diags, InclusionLocations IncLocations) : Preamble(std::move(Preamble)), TopLevelDeclIDs(std::move(TopLevelDeclIDs)), Diags(std::move(Diags)), IncLocations(std::move(IncLocations)) {} @@ -426,8 +302,7 @@ ParsedAST::ParsedAST(std::shared_ptr Preamble, std::unique_ptr Clang, std::unique_ptr Action, - std::vector TopLevelDecls, - std::vector Diags, + std::vector TopLevelDecls, DiagList Diags, InclusionLocations IncLocations) : Preamble(std::move(Preamble)), Clang(std::move(Clang)), Action(std::move(Action)), Diags(std::move(Diags)), @@ -445,8 +320,7 @@ log("Created CppFile for " + FileName); } -llvm::Optional> -CppFile::rebuild(ParseInputs &&Inputs) { +llvm::Optional CppFile::rebuild(ParseInputs &&Inputs) { log("Rebuilding file " + FileName + " with command [" + Inputs.CompileCommand.Directory + "] " + llvm::join(Inputs.CompileCommand.CommandLine, " ")); @@ -500,14 +374,12 @@ std::move(ContentsBuffer), PCHs, Inputs.FS); } - std::vector Diagnostics; + DiagList Diagnostics; if (NewAST) { // Collect diagnostics from both the preamble and the AST. if (NewPreamble) - Diagnostics.insert(Diagnostics.begin(), NewPreamble->Diags.begin(), - NewPreamble->Diags.end()); - Diagnostics.insert(Diagnostics.end(), NewAST->getDiagnostics().begin(), - NewAST->getDiagnostics().end()); + Diagnostics.insert(NewPreamble->Diags); + Diagnostics.insert(NewAST->getDiagnostics()); } if (ASTCallback && NewAST) { trace::Span Tracer("Running ASTCallback"); @@ -557,7 +429,7 @@ trace::Span Tracer("Preamble"); SPAN_ATTACH(Tracer, "File", FileName); - std::vector PreambleDiags; + DiagList PreambleDiags; StoreDiagsConsumer PreambleDiagnosticsConsumer(/*ref*/ PreambleDiags); IntrusiveRefCntPtr PreambleDiagsEngine = CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(), Index: clangd/Diagnostics.h =================================================================== --- /dev/null +++ clangd/Diagnostics.h @@ -0,0 +1,156 @@ +//===--- Diagnostics.h ------------------------------------------*- C++-*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===---------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H + +#include "Path.h" +#include "Protocol.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/LangOptions.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/StringSet.h" +#include +#include + +namespace clang { +namespace clangd { + +/// A single diagnostic stored in DiagList. StringRefs in the diag are owned by +/// DiagList. +struct PersistentDiag { + llvm::StringRef Message; + // Intended to be used only in error messages. + // May be relative, absolute or even artifically constructed. + llvm::StringRef File; + clangd::Range Range; + DiagnosticsEngine::Level Severity; + llvm::SmallVector FixIts; + // Since File is only descriptive, we store a separate flag to distinguish + // diags from the main file. + bool InsideMainFile = false; +}; + +class DiagWithNotes { +public: + DiagWithNotes(llvm::ArrayRef DiagAndNotes); + + PersistentDiag main() const; + llvm::ArrayRef notes() const; + llvm::ArrayRef all() const; + +private: + // First item is the diagnostic, the rest are notes for it. + llvm::ArrayRef DiagAndNotes; +}; + +class DiagNotesIterator; + +/// A list of diagnostics with notes attached to them. Owns strings referenced +/// by fields of PersistentDiag. +class DiagList { +public: + DiagList() = default; + + DiagList(const DiagList &Other); + DiagList &operator=(const DiagList &Other); + DiagList(DiagList &&) = default; + DiagList &operator=(DiagList &&) = default; + + // Add \p Diag to DiagList and intern all strings used inside it. If \p Diag + // is a note it will be attached to the latest added non-note diagnostic. + void add(PersistentDiag &Diag); + // Add all diagnostics from \p Other. + void insert(const DiagList &Other); + + std::size_t getUsedBytes() const; + + DiagNotesIterator begin() const; + DiagNotesIterator end() const; + + /// A raw list of all stored diagnostics and notes. Prefer to iterate over + /// grouped values using begin() and end(). + llvm::ArrayRef rawDiags() const; + +private: + /// Stores concatenated list of [diag, note1, note2, ...], where each note + /// corresponds to diag. + std::vector RawDiags; + /// Owns strins used in Message and Filename fields of PersistentDiag. + llvm::StringSet<> Intern; +}; + +class DiagNotesIterator { +private: + friend class DiagList; + DiagNotesIterator(llvm::ArrayRef RawDiags); + +public: + DiagWithNotes operator*() const; + + DiagNotesIterator &operator++(); + DiagNotesIterator operator++(int); + + friend bool operator==(const DiagNotesIterator &LHS, + const DiagNotesIterator &RHS); + friend bool operator!=(const DiagNotesIterator &LHS, + const DiagNotesIterator &RHS); + +private: + const PersistentDiag *Diag; + const PersistentDiag *NotesEnd; + const PersistentDiag *End; +}; + +/// An LSP diagnostic with FixIts. +struct DiagWithFixIts { + clangd::Diagnostic Diag; + llvm::SmallVector FixIts; +}; + +/// Conventional conversion to LSP diagnostics. Formats the error message of +/// each diagnostic to include all its notes. Notes inside main file are also +/// provided as separate diagnostics with their corresponding fixits. +/// Notes outside main file do not have a corresponding LSP diagnostic, but can +/// still be included as part of their main diagnostic's message. +void toLSPDiags(const DiagList &Diags, + llvm::function_ref OutFn); +void toLSPDiags(const DiagWithNotes &D, + llvm::function_ref OutFn); + +class StoreDiagsConsumer : public DiagnosticConsumer { +public: + StoreDiagsConsumer(DiagList &Output); + + void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override; + void EndSourceFile() override; + void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, + const clang::Diagnostic &Info) override; + +private: + bool shouldIgnore(DiagnosticsEngine::Level DiagLevel, + const clang::Diagnostic &Info); + + void flushLastDiag(); + void addToOutput(const StoredDiagnostic &D); + + DiagList &Output; + llvm::Optional LangOpts; + /// Either empty or contains a diagnostic as a first element and the rest are + /// the first element's notes. + std::vector LastDiagAndNotes; + /// Is any diag in LastDiagAndNotes in the main file? + bool LastDiagMentionsMainFile = false; +}; + +} // namespace clangd +} // namespace clang + +#endif Index: clangd/Diagnostics.cpp =================================================================== --- /dev/null +++ clangd/Diagnostics.cpp @@ -0,0 +1,348 @@ +//===--- Diagnostics.cpp ----------------------------------------*- C++-*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===---------------------------------------------------------------------===// + +#include "Diagnostics.h" +#include "Compiler.h" +#include "SourceCode.h" +#include "clang/Basic/SourceManager.h" +#include "clang/Lex/Lexer.h" +#include "llvm/Support/Capacity.h" +#include "llvm/Support/Path.h" +#include + +namespace clang { +namespace clangd { + +namespace { +/// Convert from clang diagnostic level to LSP severity. +int getSeverity(DiagnosticsEngine::Level L) { + switch (L) { + case DiagnosticsEngine::Remark: + return 4; + case DiagnosticsEngine::Note: + return 3; + case DiagnosticsEngine::Warning: + return 2; + case DiagnosticsEngine::Fatal: + case DiagnosticsEngine::Error: + return 1; + case DiagnosticsEngine::Ignored: + return 0; + } + llvm_unreachable("Unknown diagnostic level!"); +} + +// Checks whether a location is within a half-open range. +// Note that clang also uses closed source ranges, which this can't handle! +bool locationInRange(SourceLocation L, CharSourceRange R, + const SourceManager &M) { + assert(R.isCharRange()); + if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) || + M.getFileID(R.getBegin()) != M.getFileID(L)) + return false; + return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd()); +} + +// Clang diags have a location (shown as ^) and 0 or more ranges (~~~~). +// LSP needs a single range. +Range diagnosticRange(const StoredDiagnostic &D, const LangOptions &L) { + auto &M = D.getLocation().getManager(); + auto Loc = M.getFileLoc(D.getLocation()); + // Accept the first range that contains the location. + for (const auto &CR : D.getRanges()) { + auto R = Lexer::makeFileCharRange(CR, M, L); + if (locationInRange(Loc, R, M)) + return halfOpenToRange(R, M); + } + // The range may be given as a fixit hint instead. + for (const auto &F : D.getFixIts()) { + auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L); + if (locationInRange(Loc, R, M)) + return halfOpenToRange(R, M); + } + // If no suitable range is found, just use the token at the location. + auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L); + if (!R.isValid()) // Fall back to location only, let the editor deal with it. + R = CharSourceRange::getCharRange(Loc); + return halfOpenToRange(R, M); +} + +TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M, + const LangOptions &L) { + TextEdit Result; + Result.range = + halfOpenToRange(Lexer::makeFileCharRange(FixIt.RemoveRange, M, L), M); + Result.newText = FixIt.CodeToInsert; + return Result; +} + +bool isInsdieMainFile(const clang::Diagnostic &D) { + if (!D.hasSourceManager()) + return false; + + auto Loc = D.getLocation(); + return Loc.isValid() && D.getSourceManager().isInMainFile(Loc); +} + +bool isNote(DiagnosticsEngine::Level L) { + return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark; +} + +const PersistentDiag *findNotesEnd(llvm::ArrayRef RawDiags) { + if (RawDiags.empty()) + return RawDiags.end(); + + assert(!isNote(RawDiags.front().Severity)); + return std::find_if( + RawDiags.begin() + 1, RawDiags.end(), + [](const PersistentDiag &D) { return !isNote(D.Severity); }); +} + +llvm::StringRef diagLeveltoString(DiagnosticsEngine::Level Lvl) { + switch (Lvl) { + case DiagnosticsEngine::Ignored: + return "ignored"; + case DiagnosticsEngine::Note: + return "note"; + case DiagnosticsEngine::Remark: + return "remark"; + case DiagnosticsEngine::Warning: + return "warning"; + case DiagnosticsEngine::Error: + return "error"; + case DiagnosticsEngine::Fatal: + return "fatal error"; + } + llvm_unreachable("unhandled DiagnosticsEngine::Level"); +} + +void printDiag(llvm::raw_string_ostream &OS, const PersistentDiag &D) { + if (D.InsideMainFile) { + // Paths to main files are often taken from compile_command.json, where they + // are typically absolute. To reduce noise we print only basename for them, + // it should not be confusing and saves space. + OS << llvm::sys::path::filename(D.File) << ":"; + } else { + OS << D.File << ":"; + } + // Note +1 to line and character. clangd::Range is zero-based, but when + // printing for users we want one-based indexes. + auto Pos = D.Range.start; + OS << (Pos.line + 1) << ":" << (Pos.character + 1) << ":"; + // The non-main-file paths are often too long, putting them on a separate + // line improves readability. + if (D.InsideMainFile) + OS << " "; + else + OS << "\n"; + OS << diagLeveltoString(D.Severity) << ": " << D.Message; +} + +std::string presentMainMessage(const DiagWithNotes &D) { + std::string Result; + llvm::raw_string_ostream OS(Result); + OS << D.main().Message; + for (auto &Note : D.notes()) { + OS << "\n\n"; + printDiag(OS, Note); + } + OS.flush(); + return Result; +} + +std::string presentNoteMessage(const PersistentDiag &Main, + const PersistentDiag &Note) { + std::string Result; + llvm::raw_string_ostream OS(Result); + OS << Note.Message; + OS << "\n\n"; + printDiag(OS, Main); + OS.flush(); + return Result; +} +} // namespace + +DiagWithNotes::DiagWithNotes(llvm::ArrayRef DiagAndNotes) + : DiagAndNotes(DiagAndNotes) { + assert(!DiagAndNotes.empty()); + assert(!isNote(DiagAndNotes.front().Severity)); + assert( + std::all_of(DiagAndNotes.begin() + 1, DiagAndNotes.end(), + [](const PersistentDiag &D) { return isNote(D.Severity); })); +} + +PersistentDiag DiagWithNotes::main() const { return DiagAndNotes.front(); } + +llvm::ArrayRef DiagWithNotes::notes() const { + return DiagAndNotes.drop_front(); +} + +llvm::ArrayRef DiagWithNotes::all() const { + return DiagAndNotes; +} + +void toLSPDiags(const DiagWithNotes &D, + llvm::function_ref OutFn) { + auto PreBuild = [](const PersistentDiag &D) { + DiagWithFixIts Res; + Res.Diag.range = D.Range; + Res.Diag.severity = getSeverity(D.Severity); + Res.FixIts = D.FixIts; + return Res; + }; + + DiagWithFixIts Main = PreBuild(D.main()); + Main.Diag.message = presentMainMessage(D); + OutFn(std::move(Main)); + + for (auto &Note : D.notes()) { + DiagWithFixIts Res = PreBuild(Note); + Res.Diag.message = presentNoteMessage(D.main(), Note); + OutFn(std::move(Res)); + } +} + +DiagList::DiagList(const DiagList &Other) { insert(Other); } + +DiagList &DiagList::operator=(const DiagList &Other) { + Intern.clear(); + std::vector().swap(RawDiags); + + insert(Other); + return *this; +} + +void DiagList::add(PersistentDiag &Diag) { + assert((!isNote(Diag.Severity) || !RawDiags.empty()) && + "Adding a note without prior diagnostics"); + + Diag.Message = Intern.insert(Diag.Message).first->getKey(); + Diag.File = Intern.insert(Diag.File).first->getKey(); + + RawDiags.push_back(Diag); +} + +void DiagList::insert(const DiagList &Other) { + for (auto D : Other.RawDiags) + add(D); +} + +std::size_t DiagList::getUsedBytes() const { + // FIXME(ibiryukov): we do not account for strings inside Intern and FixIts + // inside Intern. + return llvm::capacity_in_bytes(RawDiags); +} + +DiagNotesIterator DiagList::begin() const { + return DiagNotesIterator(RawDiags); +} + +DiagNotesIterator DiagList::end() const { + return DiagNotesIterator( + llvm::makeArrayRef(RawDiags.data() + RawDiags.size(), 0)); +} + +llvm::ArrayRef DiagList::rawDiags() const { return RawDiags; } + +void toLSPDiags(const DiagList &Diags, + llvm::function_ref OutFn) { + for (auto D : Diags) + toLSPDiags(D, OutFn); +} + +DiagNotesIterator::DiagNotesIterator(llvm::ArrayRef RawDiags) + : Diag(RawDiags.begin()), End(RawDiags.end()) { + NotesEnd = findNotesEnd(RawDiags); +} + +DiagWithNotes DiagNotesIterator::operator*() const { + assert(Diag != End); + return DiagWithNotes(llvm::makeArrayRef(Diag, NotesEnd)); +} + +DiagNotesIterator &DiagNotesIterator::operator++() { + Diag = NotesEnd; + NotesEnd = findNotesEnd(llvm::makeArrayRef(Diag, End)); + return *this; +} + +DiagNotesIterator DiagNotesIterator::operator++(int) { + auto Copy = *this; + ++(*this); + return Copy; +} + +bool operator==(const DiagNotesIterator &LHS, const DiagNotesIterator &RHS) { + assert(LHS.End == RHS.End); + return LHS.Diag == RHS.Diag; +} + +bool operator!=(const DiagNotesIterator &LHS, const DiagNotesIterator &RHS) { + assert(LHS.End == RHS.End); + return !(LHS == RHS); +} + +StoreDiagsConsumer::StoreDiagsConsumer(DiagList &Output) : Output(Output) {} + +void StoreDiagsConsumer::BeginSourceFile(const LangOptions &Opts, + const Preprocessor *) { + LangOpts = Opts; +} + +void StoreDiagsConsumer::EndSourceFile() { + flushLastDiag(); + LangOpts = llvm::None; +} + +void StoreDiagsConsumer::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, + const clang::Diagnostic &Info) { + DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info); + + if (!LangOpts) { + IgnoreDiagnostics::log(DiagLevel, Info); + return; + } + if (!isNote(DiagLevel)) + flushLastDiag(); + + LastDiagAndNotes.push_back(StoredDiagnostic(DiagLevel, Info)); + LastDiagMentionsMainFile = LastDiagMentionsMainFile || isInsdieMainFile(Info); +} + +void StoreDiagsConsumer::flushLastDiag() { + if (LastDiagMentionsMainFile) { + for (const StoredDiagnostic &D : LastDiagAndNotes) + addToOutput(D); + } + LastDiagAndNotes.clear(); + LastDiagMentionsMainFile = false; +} + +void StoreDiagsConsumer::addToOutput(const StoredDiagnostic &D) { + assert(LangOpts); + assert(D.getLocation().isValid()); + assert(D.getLocation().hasManager()); + + FullSourceLoc Loc = D.getLocation(); + auto &SourceMgr = Loc.getManager(); + + PersistentDiag Result; + Result.File = SourceMgr.getFilename(Loc.getFileLoc()); + Result.Message = D.getMessage(); + Result.Range = diagnosticRange(D, *LangOpts); + Result.Severity = D.getLevel(); + Result.InsideMainFile = SourceMgr.isInMainFile(Loc); + for (const FixItHint &Fix : D.getFixIts()) + Result.FixIts.push_back(toTextEdit(Fix, SourceMgr, *LangOpts)); + + Output.add(Result); +} + +} // namespace clangd +} // namespace clang Index: clangd/SourceCode.h =================================================================== --- clangd/SourceCode.h +++ clangd/SourceCode.h @@ -30,6 +30,10 @@ /// Turn a SourceLocation into a [line, column] pair. Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc); +// Converts a half-open clang source range to an LSP range. +// Note that clang also uses closed source ranges, which this can't handle! +Range halfOpenToRange(CharSourceRange R, const SourceManager &M); + } // namespace clangd } // namespace clang #endif Index: clangd/SourceCode.cpp =================================================================== --- clangd/SourceCode.cpp +++ clangd/SourceCode.cpp @@ -48,5 +48,19 @@ return P; } +Range halfOpenToRange(CharSourceRange R, const SourceManager &M) { + // Clang is 1-based, LSP uses 0-based indexes. + Position Begin; + Begin.line = static_cast(M.getSpellingLineNumber(R.getBegin())) - 1; + Begin.character = + static_cast(M.getSpellingColumnNumber(R.getBegin())) - 1; + + Position End; + End.line = static_cast(M.getSpellingLineNumber(R.getEnd())) - 1; + End.character = static_cast(M.getSpellingColumnNumber(R.getEnd())) - 1; + + return {Begin, End}; +} + } // namespace clangd } // namespace clang Index: clangd/TUScheduler.h =================================================================== --- clangd/TUScheduler.h +++ clangd/TUScheduler.h @@ -63,7 +63,7 @@ /// \p File was not part of it before. /// FIXME(ibiryukov): remove the callback from this function. void update(PathRef File, ParseInputs Inputs, WantDiagnostics WD, - UniqueFunction)> OnUpdated); + UniqueFunction OnUpdated); /// Remove \p File from the list of tracked files and schedule removal of its /// resources. Index: clangd/TUScheduler.cpp =================================================================== --- clangd/TUScheduler.cpp +++ clangd/TUScheduler.cpp @@ -85,7 +85,7 @@ ~ASTWorker(); void update(ParseInputs Inputs, WantDiagnostics, - UniqueFunction)> OnUpdated); + UniqueFunction OnUpdated); void runWithAST(llvm::StringRef Name, UniqueFunction)> Action); bool blockUntilIdle(Deadline Timeout) const; @@ -135,8 +135,8 @@ // Result of getUsedBytes() after the last rebuild or read of AST. std::size_t LastASTSize; /* GUARDED_BY(Mutex) */ // Set to true to signal run() to finish processing. - bool Done; /* GUARDED_BY(Mutex) */ - std::deque Requests; /* GUARDED_BY(Mutex) */ + bool Done; /* GUARDED_BY(Mutex) */ + std::deque Requests; /* GUARDED_BY(Mutex) */ mutable std::condition_variable RequestsCV; }; @@ -210,9 +210,8 @@ #endif } -void ASTWorker::update( - ParseInputs Inputs, WantDiagnostics WantDiags, - UniqueFunction)> OnUpdated) { +void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags, + UniqueFunction OnUpdated) { auto Task = [=](decltype(OnUpdated) OnUpdated) mutable { FileInputs = Inputs; auto Diags = AST.rebuild(std::move(Inputs)); @@ -447,9 +446,9 @@ return true; } -void TUScheduler::update( - PathRef File, ParseInputs Inputs, WantDiagnostics WantDiags, - UniqueFunction)> OnUpdated) { +void TUScheduler::update(PathRef File, ParseInputs Inputs, + WantDiagnostics WantDiags, + UniqueFunction OnUpdated) { std::unique_ptr &FD = Files[File]; if (!FD) { // Create a new worker to process the AST-related tasks. Index: test/clangd/diagnostics.test =================================================================== --- test/clangd/diagnostics.test +++ test/clangd/diagnostics.test @@ -6,7 +6,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT: "diagnostics": [ # CHECK-NEXT: { -# CHECK-NEXT: "message": "return type of 'main' is not 'int'", +# CHECK-NEXT: "message": "return type of 'main' is not 'int'\n\nfoo.c:1:1: note: change return type to 'int'", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 4, @@ -20,7 +20,7 @@ # CHECK-NEXT: "severity": 2 # CHECK-NEXT: }, # CHECK-NEXT: { -# CHECK-NEXT: "message": "change return type to 'int'", +# CHECK-NEXT: "message": "change return type to 'int'\n\nfoo.c:1:1: warning: return type of 'main' is not 'int'", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 4, Index: test/clangd/execute-command.test =================================================================== --- test/clangd/execute-command.test +++ test/clangd/execute-command.test @@ -6,7 +6,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT: "diagnostics": [ # CHECK-NEXT: { -# CHECK-NEXT: "message": "using the result of an assignment as a condition without parentheses", +# CHECK-NEXT: "message": "using the result of an assignment as a condition without parentheses\n\nfoo.c:1:35: note: place parentheses around the assignment to silence this warning\n\nfoo.c:1:35: note: use '==' to turn this assignment into an equality comparison", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 37, @@ -20,7 +20,7 @@ # CHECK-NEXT: "severity": 2 # CHECK-NEXT: }, # CHECK-NEXT: { -# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning", +# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 35, @@ -34,7 +34,7 @@ # CHECK-NEXT: "severity": 3 # CHECK-NEXT: }, # CHECK-NEXT: { -# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison", +# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 35, Index: test/clangd/extra-flags.test =================================================================== --- test/clangd/extra-flags.test +++ test/clangd/extra-flags.test @@ -6,7 +6,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT: "diagnostics": [ # CHECK-NEXT: { -# CHECK-NEXT: "message": "variable 'i' is uninitialized when used here", +# CHECK-NEXT: "message": "variable 'i' is uninitialized when used here\n\nfoo.c:1:19: note: initialize the variable 'i' to silence this warning", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 28, @@ -20,7 +20,7 @@ # CHECK-NEXT: "severity": 2 # CHECK-NEXT: }, # CHECK-NEXT: { -# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning", +# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning\n\nfoo.c:1:28: warning: variable 'i' is uninitialized when used here", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 19, @@ -42,7 +42,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT: "diagnostics": [ # CHECK-NEXT: { -# CHECK-NEXT: "message": "variable 'i' is uninitialized when used here", +# CHECK-NEXT: "message": "variable 'i' is uninitialized when used here\n\nfoo.c:1:19: note: initialize the variable 'i' to silence this warning", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 28, @@ -56,7 +56,7 @@ # CHECK-NEXT: "severity": 2 # CHECK-NEXT: }, # CHECK-NEXT: { -# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning", +# CHECK-NEXT: "message": "initialize the variable 'i' to silence this warning\n\nfoo.c:1:28: warning: variable 'i' is uninitialized when used here", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 19, Index: test/clangd/fixits.test =================================================================== --- test/clangd/fixits.test +++ test/clangd/fixits.test @@ -6,7 +6,7 @@ # CHECK-NEXT: "params": { # CHECK-NEXT: "diagnostics": [ # CHECK-NEXT: { -# CHECK-NEXT: "message": "using the result of an assignment as a condition without parentheses", +# CHECK-NEXT: "message": "using the result of an assignment as a condition without parentheses\n\nfoo.c:1:35: note: place parentheses around the assignment to silence this warning\n\nfoo.c:1:35: note: use '==' to turn this assignment into an equality comparison", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 37, @@ -20,7 +20,7 @@ # CHECK-NEXT: "severity": 2 # CHECK-NEXT: }, # CHECK-NEXT: { -# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning", +# CHECK-NEXT: "message": "place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 35, @@ -34,7 +34,7 @@ # CHECK-NEXT: "severity": 3 # CHECK-NEXT: }, # CHECK-NEXT: { -# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison", +# CHECK-NEXT: "message": "use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { # CHECK-NEXT: "character": 35, @@ -51,7 +51,7 @@ # CHECK-NEXT: "uri": "file://{{.*}}/foo.c" # CHECK-NEXT: } --- -{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}} +{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses\n\nfoo.c:1:35: note: place parentheses around the assignment to silence this warning\n\nfoo.c:1:35: note: use '==' to turn this assignment into an equality comparison"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"}]}}} # CHECK: "id": 2, # CHECK-NEXT: "jsonrpc": "2.0", # CHECK-NEXT: "result": [ @@ -120,7 +120,7 @@ # CHECK-NEXT: } # CHECK-NEXT: ] --- -{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}} +{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses\n\nfoo.c:1:35: note: place parentheses around the assignment to silence this warning\n\nfoo.c:1:35: note: use '==' to turn this assignment into an equality comparison"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison\n\nfoo.c:1:33: warning: using the result of an assignment as a condition without parentheses"}]}}} # Make sure unused "code" and "source" fields ignored gracefully # CHECK: "id": 3, # CHECK-NEXT: "jsonrpc": "2.0", Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -42,12 +42,12 @@ namespace { -static bool diagsContainErrors(ArrayRef Diagnostics) { - for (const auto &DiagAndFixIts : Diagnostics) { +static bool diagsContainErrors(const DiagList &Diagnostics) { + for (auto DiagAndNotes : Diagnostics) { // FIXME: severities returned by clangd should have a descriptive // diagnostic severity enum - const int ErrorSeverity = 1; - if (DiagAndFixIts.Diag.severity == ErrorSeverity) + if (DiagAndNotes.main().Severity == DiagnosticsEngine::Error || + DiagAndNotes.main().Severity == DiagnosticsEngine::Fatal) return true; } return false; @@ -55,9 +55,7 @@ class ErrorCheckingDiagConsumer : public DiagnosticsConsumer { public: - void - onDiagnosticsReady(PathRef File, - Tagged> Diagnostics) override { + void onDiagnosticsReady(PathRef File, Tagged Diagnostics) override { bool HadError = diagsContainErrors(Diagnostics.Value); std::lock_guard Lock(Mutex); @@ -82,9 +80,7 @@ /// least one error. class MultipleErrorCheckingDiagConsumer : public DiagnosticsConsumer { public: - void - onDiagnosticsReady(PathRef File, - Tagged> Diagnostics) override { + void onDiagnosticsReady(PathRef File, Tagged Diagnostics) override { bool HadError = diagsContainErrors(Diagnostics.Value); std::lock_guard Lock(Mutex); @@ -635,9 +631,8 @@ public: TestDiagConsumer() : Stats(FilesCount, FileStat()) {} - void onDiagnosticsReady( - PathRef File, - Tagged> Diagnostics) override { + void onDiagnosticsReady(PathRef File, + Tagged Diagnostics) override { StringRef FileIndexStr = llvm::sys::path::stem(File); ASSERT_TRUE(FileIndexStr.consume_front("Foo")); @@ -907,8 +902,7 @@ NoConcurrentAccessDiagConsumer(std::promise StartSecondReparse) : StartSecondReparse(std::move(StartSecondReparse)) {} - void onDiagnosticsReady(PathRef, - Tagged>) override { + void onDiagnosticsReady(PathRef, Tagged) override { ++Count; std::unique_lock Lock(Mutex, std::try_to_lock_t()); ASSERT_TRUE(Lock.owns_lock()) Index: unittests/clangd/ClangdUnitTests.cpp =================================================================== --- unittests/clangd/ClangdUnitTests.cpp +++ unittests/clangd/ClangdUnitTests.cpp @@ -7,8 +7,8 @@ // //===----------------------------------------------------------------------===// -#include "ClangdUnit.h" #include "Annotations.h" +#include "ClangdUnit.h" #include "TestFS.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/PCHContainerOperations.h" @@ -20,9 +20,11 @@ namespace clang { namespace clangd { using namespace llvm; -void PrintTo(const DiagWithFixIts &D, std::ostream *O) { +void PrintTo(const PersistentDiag &D, std::ostream *O) { llvm::raw_os_ostream OS(*O); - OS << D.Diag; + if (!D.InsideMainFile) + OS << "[in " << D.File << "] "; + OS << D.Message; if (!D.FixIts.empty()) { OS << " {"; const char *Sep = ""; @@ -38,8 +40,8 @@ using testing::ElementsAre; // FIXME: this is duplicated with FileIndexTests. Share it. -ParsedAST build(StringRef Code, std::vector Flags = {}) { - std::vector Cmd = {"clang", "main.cpp"}; +ParsedAST build(StringRef Code, std::vector Flags = {}) { + std::vector Cmd = {"clang", "main.cpp"}; Cmd.insert(Cmd.begin() + 1, Flags.begin(), Flags.end()); auto CI = createInvocationFromCommandLine(Cmd); auto Buf = MemoryBuffer::getMemBuffer(Code); @@ -50,16 +52,20 @@ return std::move(*AST); } +DiagList buildDiags(llvm::StringRef Code, + std::vector Flags = {}) { + return build(Code, std::move(Flags)).getDiagnostics(); +} + MATCHER_P2(Diag, Range, Message, "Diagnostic at " + llvm::to_string(Range) + " = [" + Message + "]") { - return arg.Diag.range == Range && arg.Diag.message == Message && - arg.FixIts.empty(); + return arg.Range == Range && arg.Message == Message && arg.FixIts.empty(); } MATCHER_P3(Fix, Range, Replacement, Message, "Fix " + llvm::to_string(Range) + " => " + testing::PrintToString(Replacement) + " = [" + Message + "]") { - return arg.Diag.range == Range && arg.Diag.message == Message && + return arg.Range == Range && arg.Message == Message && arg.FixIts.size() == 1 && arg.FixIts[0].range == Range && arg.FixIts[0].newText == Replacement; } @@ -75,31 +81,29 @@ $unk[[unknown]](); } )cpp"); - llvm::errs() << Test.code(); - EXPECT_THAT( - build(Test.code()).getDiagnostics(), - ElementsAre( - // This range spans lines. - Fix(Test.range("typo"), "foo", - "use of undeclared identifier 'goo'; did you mean 'foo'?"), - // This is a pretty normal range. - Diag(Test.range("decl"), "'foo' declared here"), - // This range is zero-width, and at the end of a line. - Fix(Test.range("semicolon"), ";", - "expected ';' after expression"), - // This range isn't provided by clang, we expand to the token. - Diag(Test.range("unk"), - "use of undeclared identifier 'unknown'"))); + llvm::errs() << Test.code(); + EXPECT_THAT( + buildDiags(Test.code()).rawDiags(), + ElementsAre( + // This range spans lines. + Fix(Test.range("typo"), "foo", + "use of undeclared identifier 'goo'; did you mean 'foo'?"), + // This is a pretty normal range. + Diag(Test.range("decl"), "'foo' declared here"), + // This range is zero-width, and at the end of a line. + Fix(Test.range("semicolon"), ";", "expected ';' after expression"), + // This range isn't provided by clang, we expand to the token. + Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"))); } TEST(DiagnosticsTest, FlagsMatter) { Annotations Test("[[void]] main() {}"); EXPECT_THAT( - build(Test.code()).getDiagnostics(), + buildDiags(Test.code()).rawDiags(), ElementsAre(Fix(Test.range(), "int", "'main' must return 'int'"))); // Same code built as C gets different diagnostics. EXPECT_THAT( - build(Test.code(), {"-x", "c"}).getDiagnostics(), + buildDiags(Test.code(), {"-x", "c"}).rawDiags(), ElementsAre( // FIXME: ideally this would be one diagnostic with a named FixIt. Diag(Test.range(), "return type of 'main' is not 'int'"), @@ -121,7 +125,7 @@ #endif )cpp"); EXPECT_THAT( - build(Test.code()).getDiagnostics(), + buildDiags(Test.code()).rawDiags(), ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'"))); } Index: unittests/clangd/CodeCompleteTests.cpp =================================================================== --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -56,13 +56,13 @@ using ::testing::Contains; using ::testing::Each; using ::testing::ElementsAre; +using ::testing::Field; using ::testing::Not; using ::testing::UnorderedElementsAre; -using ::testing::Field; class IgnoreDiagnostics : public DiagnosticsConsumer { - void onDiagnosticsReady( - PathRef File, Tagged> Diagnostics) override {} + void onDiagnosticsReady(PathRef File, Tagged Diagnostics) override { + } }; // GMock helpers for matching completion items. @@ -321,11 +321,11 @@ }; // We used to test every combination of options, but that got too slow (2^N). auto Flags = { - &clangd::CodeCompleteOptions::IncludeMacros, - &clangd::CodeCompleteOptions::IncludeBriefComments, - &clangd::CodeCompleteOptions::EnableSnippets, - &clangd::CodeCompleteOptions::IncludeCodePatterns, - &clangd::CodeCompleteOptions::IncludeIneligibleResults, + &clangd::CodeCompleteOptions::IncludeMacros, + &clangd::CodeCompleteOptions::IncludeBriefComments, + &clangd::CodeCompleteOptions::EnableSnippets, + &clangd::CodeCompleteOptions::IncludeCodePatterns, + &clangd::CodeCompleteOptions::IncludeIneligibleResults, }; // Test default options. Test({}); @@ -551,7 +551,6 @@ auto WithoutIndex = runCodeComplete(Server, File, Test.point(), Opts).Value; EXPECT_THAT(WithoutIndex.items, UnorderedElementsAre(Named("local"), Named("preamble"))); - } TEST(CompletionTest, DynamicIndexMultiFile) { Index: unittests/clangd/TUSchedulerTests.cpp =================================================================== --- unittests/clangd/TUSchedulerTests.cpp +++ unittests/clangd/TUSchedulerTests.cpp @@ -21,7 +21,7 @@ using ::testing::Pair; using ::testing::Pointee; -void ignoreUpdate(llvm::Optional>) {} +void ignoreUpdate(llvm::Optional) {} void ignoreError(llvm::Error Err) { handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {}); } @@ -102,20 +102,20 @@ /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero()); auto Path = testPath("foo.cpp"); S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes, - [&](std::vector) { Ready.wait(); }); + [&](DiagList) { Ready.wait(); }); S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes, - [&](std::vector Diags) { ++CallbackCount; }); + [&](DiagList Diags) { ++CallbackCount; }); S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto, - [&](std::vector Diags) { + [&](DiagList Diags) { ADD_FAILURE() << "auto should have been cancelled by auto"; }); S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No, - [&](std::vector Diags) { + [&](DiagList Diags) { ADD_FAILURE() << "no diags should not be called back"; }); S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto, - [&](std::vector Diags) { ++CallbackCount; }); + [&](DiagList Diags) { ++CallbackCount; }); Ready.notify(); } EXPECT_EQ(2, CallbackCount); @@ -131,15 +131,15 @@ // FIXME: we could probably use timeouts lower than 1 second here. auto Path = testPath("foo.cpp"); S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto, - [&](std::vector Diags) { + [&](DiagList Diags) { ADD_FAILURE() << "auto should have been debounced and canceled"; }); std::this_thread::sleep_for(std::chrono::milliseconds(200)); S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto, - [&](std::vector Diags) { ++CallbackCount; }); + [&](DiagList Diags) { ++CallbackCount; }); std::this_thread::sleep_for(std::chrono::seconds(2)); S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto, - [&](std::vector Diags) { ++CallbackCount; }); + [&](DiagList Diags) { ++CallbackCount; }); } EXPECT_EQ(2, CallbackCount); } @@ -189,15 +189,14 @@ { WithContextValue WithNonce(NonceKey, ++Nonce); - S.update(File, Inputs, WantDiagnostics::Auto, - [Nonce, &Mut, &TotalUpdates]( - llvm::Optional> Diags) { - EXPECT_THAT(Context::current().get(NonceKey), - Pointee(Nonce)); - - std::lock_guard Lock(Mut); - ++TotalUpdates; - }); + S.update( + File, Inputs, WantDiagnostics::Auto, + [Nonce, &Mut, &TotalUpdates](llvm::Optional Diags) { + EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce)); + + std::lock_guard Lock(Mut); + ++TotalUpdates; + }); } { Index: unittests/clangd/XRefsTests.cpp =================================================================== --- unittests/clangd/XRefsTests.cpp +++ unittests/clangd/XRefsTests.cpp @@ -41,8 +41,8 @@ using testing::UnorderedElementsAreArray; class IgnoreDiagnostics : public DiagnosticsConsumer { - void onDiagnosticsReady( - PathRef File, Tagged> Diagnostics) override {} + void onDiagnosticsReady(PathRef File, Tagged Diagnostics) override { + } }; // FIXME: this is duplicated with FileIndexTests. Share it.