diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -147,13 +147,9 @@ if (!InvalidFileCount) return llvm::Error::success(); if (InvalidFileCount == 1) - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "File must be saved first: " + - LastInvalidFile); - return llvm::createStringError( - llvm::inconvertibleErrorCode(), - "Files must be saved first: " + LastInvalidFile + " (and " + - llvm::to_string(InvalidFileCount - 1) + " others)"); + return error("File must be saved first: {0}", LastInvalidFile); + return error("Files must be saved first: {0} (and {1} others)", + LastInvalidFile, InvalidFileCount - 1); } } // namespace @@ -284,10 +280,9 @@ } } if (OldestCB) - OldestCB->second(llvm::createStringError( - llvm::inconvertibleErrorCode(), - llvm::formatv("failed to receive a client reply for request ({0})", - OldestCB->first))); + OldestCB->second( + error("failed to receive a client reply for request ({0})", + OldestCB->first)); return ID; } @@ -656,8 +651,7 @@ if (Server->blockUntilIdleForTest(/*TimeoutSeconds=*/60)) Reply(nullptr); else - Reply(llvm::createStringError(llvm::inconvertibleErrorCode(), - "Not idle after a minute")); + Reply(error("Not idle after a minute")); } void ClangdLSPServer::onDocumentDidOpen( @@ -724,9 +718,7 @@ std::string Reason = Response->failureReason ? *Response->failureReason : "unknown reason"; - return Reply(llvm::createStringError( - llvm::inconvertibleErrorCode(), - ("edits were not applied: " + Reason).c_str())); + return Reply(error("edits were not applied: {0}", Reason)); } return Reply(SuccessMessage); }); @@ -747,9 +739,7 @@ Params.tweakArgs) { auto Code = DraftMgr.getDraft(Params.tweakArgs->file.file()); if (!Code) - return Reply(llvm::createStringError( - llvm::inconvertibleErrorCode(), - "trying to apply a code action for a non-added file")); + return Reply(error("trying to apply a code action for a non-added file")); auto Action = [this, ApplyEdit, Reply = std::move(Reply), File = Params.tweakArgs->file, Code = std::move(*Code)]( diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -290,8 +290,7 @@ const auto *PreambleData = IP->Preamble; if (!PreambleData) - return CB(llvm::createStringError(llvm::inconvertibleErrorCode(), - "Failed to parse includes")); + return CB(error("Failed to parse includes")); ParseInputs ParseInput{IP->Command, &TFS, IP->Contents.str()}; ParseInput.Index = Index; diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -333,8 +333,7 @@ return ResolvedInserted.takeError(); auto Spelled = Includes.calculateIncludePath(*ResolvedInserted, FileName); if (!Spelled) - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Header not on include path"); + return error("Header not on include path"); return std::make_pair( std::move(*Spelled), Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted)); diff --git a/clang-tools-extra/clangd/DraftStore.cpp b/clang-tools-extra/clangd/DraftStore.cpp --- a/clang-tools-extra/clangd/DraftStore.cpp +++ b/clang-tools-extra/clangd/DraftStore.cpp @@ -64,9 +64,9 @@ auto EntryIt = Drafts.find(File); if (EntryIt == Drafts.end()) { - return llvm::make_error( - "Trying to do incremental update on non-added document: " + File, - llvm::errc::invalid_argument); + return error(llvm::errc::invalid_argument, + "Trying to do incremental update on non-added document: {0}", + File); } Draft &D = EntryIt->second; std::string Contents = EntryIt->second.Contents; @@ -89,11 +89,9 @@ return EndIndex.takeError(); if (*EndIndex < *StartIndex) - return llvm::make_error( - llvm::formatv( - "Range's end position ({0}) is before start position ({1})", End, - Start), - llvm::errc::invalid_argument); + return error(llvm::errc::invalid_argument, + "Range's end position ({0}) is before start position ({1})", + End, Start); // Since the range length between two LSP positions is dependent on the // contents of the buffer we compute the range length between the start and @@ -106,11 +104,10 @@ lspLength(Contents.substr(*StartIndex, *EndIndex - *StartIndex)); if (Change.rangeLength && ComputedRangeLength != *Change.rangeLength) - return llvm::make_error( - llvm::formatv("Change's rangeLength ({0}) doesn't match the " - "computed range length ({1}).", - *Change.rangeLength, ComputedRangeLength), - llvm::errc::invalid_argument); + return error(llvm::errc::invalid_argument, + "Change's rangeLength ({0}) doesn't match the " + "computed range length ({1}).", + *Change.rangeLength, ComputedRangeLength); std::string NewContents; NewContents.reserve(*StartIndex + Change.text.length() + diff --git a/clang-tools-extra/clangd/JSONTransport.cpp b/clang-tools-extra/clangd/JSONTransport.cpp --- a/clang-tools-extra/clangd/JSONTransport.cpp +++ b/clang-tools-extra/clangd/JSONTransport.cpp @@ -51,12 +51,10 @@ } llvm::Error decodeError(const llvm::json::Object &O) { - std::string Msg = - std::string(O.getString("message").getValueOr("Unspecified error")); + llvm::StringRef Msg = O.getString("message").getValueOr("Unspecified error"); if (auto Code = O.getInteger("code")) - return llvm::make_error(std::move(Msg), ErrorCode(*Code)); - return llvm::make_error(std::move(Msg), - llvm::inconvertibleErrorCode()); + return llvm::make_error(Msg.str(), ErrorCode(*Code)); + return error(Msg.str()); } class JSONTransport : public Transport { diff --git a/clang-tools-extra/clangd/PathMapping.cpp b/clang-tools-extra/clangd/PathMapping.cpp --- a/clang-tools-extra/clangd/PathMapping.cpp +++ b/clang-tools-extra/clangd/PathMapping.cpp @@ -8,6 +8,7 @@ #include "PathMapping.h" #include "Transport.h" #include "URI.h" +#include "support/Logger.h" #include "llvm/ADT/None.h" #include "llvm/ADT/STLExtras.h" #include "llvm/Support/Errno.h" @@ -156,8 +157,7 @@ Converted = "/" + Converted; return Converted; } - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Path not absolute: " + Path); + return error("Path not absolute: {0}", Path); } } // namespace @@ -174,9 +174,7 @@ std::tie(PathPair, Rest) = Rest.split(","); std::tie(ClientPath, ServerPath) = PathPair.split("="); if (ClientPath.empty() || ServerPath.empty()) - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Not a valid path mapping pair: " + - PathPair); + return error("Not a valid path mapping pair: {0}", PathPair); llvm::Expected ParsedClientPath = parsePath(ClientPath); if (!ParsedClientPath) return ParsedClientPath.takeError(); diff --git a/clang-tools-extra/clangd/RIFF.cpp b/clang-tools-extra/clangd/RIFF.cpp --- a/clang-tools-extra/clangd/RIFF.cpp +++ b/clang-tools-extra/clangd/RIFF.cpp @@ -7,35 +7,28 @@ //===----------------------------------------------------------------------===// #include "RIFF.h" +#include "support/Logger.h" #include "llvm/Support/Endian.h" -#include "llvm/Support/Error.h" namespace clang { namespace clangd { namespace riff { -static llvm::Error makeError(const llvm::Twine &Msg) { - return llvm::make_error(Msg, - llvm::inconvertibleErrorCode()); -} - llvm::Expected readChunk(llvm::StringRef &Stream) { if (Stream.size() < 8) - return makeError("incomplete chunk header: " + llvm::Twine(Stream.size()) + - " bytes available"); + return error("incomplete chunk header: {0} bytes available", Stream.size()); Chunk C; std::copy(Stream.begin(), Stream.begin() + 4, C.ID.begin()); Stream = Stream.drop_front(4); uint32_t Len = llvm::support::endian::read32le(Stream.take_front(4).begin()); Stream = Stream.drop_front(4); if (Stream.size() < Len) - return makeError("truncated chunk: want " + llvm::Twine(Len) + ", got " + - llvm::Twine(Stream.size())); + return error("truncated chunk: want {0}, got {1}", Len, Stream.size()); C.Data = Stream.take_front(Len); Stream = Stream.drop_front(Len); if (Len % 2 & !Stream.empty()) { // Skip padding byte. if (Stream.front()) - return makeError("nonzero padding byte"); + return error("nonzero padding byte"); Stream = Stream.drop_front(); } return std::move(C); @@ -57,9 +50,9 @@ if (!RIFF) return RIFF.takeError(); if (RIFF->ID != fourCC("RIFF")) - return makeError("not a RIFF container: root is " + fourCCStr(RIFF->ID)); + return error("not a RIFF container: root is {0}", fourCCStr(RIFF->ID)); if (RIFF->Data.size() < 4) - return makeError("RIFF chunk too short"); + return error("RIFF chunk too short"); File F; std::copy(RIFF->Data.begin(), RIFF->Data.begin() + 4, F.Type.begin()); for (llvm::StringRef Body = RIFF->Data.drop_front(4); !Body.empty();) diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -717,8 +717,7 @@ [&AST, this]() { IdleASTs.put(this, std::move(*AST)); }); // Run the user-provided action. if (!*AST) - return Action(llvm::make_error( - "invalid AST", llvm::errc::invalid_argument)); + return Action(error(llvm::errc::invalid_argument, "invalid AST")); vlog("ASTWorker running {0} on version {2} of {1}", Name, FileName, FileInputs.Version); Action(InputsAndAST{FileInputs, **AST}); diff --git a/clang-tools-extra/clangd/index/Serialization.cpp b/clang-tools-extra/clangd/index/Serialization.cpp --- a/clang-tools-extra/clangd/index/Serialization.cpp +++ b/clang-tools-extra/clangd/index/Serialization.cpp @@ -25,10 +25,6 @@ namespace clang { namespace clangd { namespace { -llvm::Error makeError(const llvm::Twine &Msg) { - return llvm::make_error(Msg, - llvm::inconvertibleErrorCode()); -} // IO PRIMITIVES // We use little-endian 32 bit ints, sometimes with variable-length encoding. @@ -199,7 +195,7 @@ Reader R(Data); size_t UncompressedSize = R.consume32(); if (R.err()) - return makeError("Truncated string table"); + return error("Truncated string table"); llvm::StringRef Uncompressed; llvm::SmallString<1> UncompressedStorage; @@ -218,12 +214,12 @@ for (Reader R(Uncompressed); !R.eof();) { auto Len = R.rest().find(0); if (Len == llvm::StringRef::npos) - return makeError("Bad string table: not null terminated"); + return error("Bad string table: not null terminated"); Table.Strings.push_back(Saver.save(R.consume(Len))); R.consume8(); } if (R.err()) - return makeError("Truncated string table"); + return error("Truncated string table"); return std::move(Table); } @@ -426,24 +422,23 @@ if (!RIFF) return RIFF.takeError(); if (RIFF->Type != riff::fourCC("CdIx")) - return makeError("wrong RIFF filetype: " + riff::fourCCStr(RIFF->Type)); + return error("wrong RIFF filetype: {0}", riff::fourCCStr(RIFF->Type)); llvm::StringMap Chunks; for (const auto &Chunk : RIFF->Chunks) Chunks.try_emplace(llvm::StringRef(Chunk.ID.data(), Chunk.ID.size()), Chunk.Data); if (!Chunks.count("meta")) - return makeError("missing meta chunk"); + return error("missing meta chunk"); Reader Meta(Chunks.lookup("meta")); auto SeenVersion = Meta.consume32(); if (SeenVersion != Version) - return makeError("wrong version: want " + llvm::Twine(Version) + ", got " + - llvm::Twine(SeenVersion)); + return error("wrong version: want {0}, got {1}", Version, SeenVersion); // meta chunk is checked above, as we prefer the "version mismatch" error. for (llvm::StringRef RequiredChunk : {"stri"}) if (!Chunks.count(RequiredChunk)) - return makeError("missing required chunk " + RequiredChunk); + return error("missing required chunk {0}", RequiredChunk); auto Strings = readStringTable(Chunks.lookup("stri")); if (!Strings) @@ -464,7 +459,7 @@ Include = Result.Sources->try_emplace(Include).first->getKey(); } if (SrcsReader.err()) - return makeError("malformed or truncated include uri"); + return error("malformed or truncated include uri"); } if (Chunks.count("symb")) { @@ -473,7 +468,7 @@ while (!SymbolReader.eof()) Symbols.insert(readSymbol(SymbolReader, Strings->Strings)); if (SymbolReader.err()) - return makeError("malformed or truncated symbol"); + return error("malformed or truncated symbol"); Result.Symbols = std::move(Symbols).build(); } if (Chunks.count("refs")) { @@ -485,7 +480,7 @@ Refs.insert(RefsBundle.first, Ref); } if (RefsReader.err()) - return makeError("malformed or truncated refs"); + return error("malformed or truncated refs"); Result.Refs = std::move(Refs).build(); } if (Chunks.count("rela")) { @@ -496,13 +491,13 @@ Relations.insert(Relation); } if (RelationsReader.err()) - return makeError("malformed or truncated relations"); + return error("malformed or truncated relations"); Result.Relations = std::move(Relations).build(); } if (Chunks.count("cmdl")) { Reader CmdReader(Chunks.lookup("cmdl")); if (CmdReader.err()) - return makeError("malformed or truncated commandline section"); + return error("malformed or truncated commandline section"); InternedCompileCommand Cmd = readCompileCommand(CmdReader, Strings->Strings); Result.Cmd.emplace(); @@ -660,8 +655,8 @@ } else if (auto YAMLContents = readYAML(Data)) { return std::move(*YAMLContents); } else { - return makeError("Not a RIFF file and failed to parse as YAML: " + - llvm::toString(YAMLContents.takeError())); + return error("Not a RIFF file and failed to parse as YAML: {0}", + YAMLContents.takeError()); } } diff --git a/clang-tools-extra/clangd/support/Logger.h b/clang-tools-extra/clangd/support/Logger.h --- a/clang-tools-extra/clangd/support/Logger.h +++ b/clang-tools-extra/clangd/support/Logger.h @@ -45,6 +45,8 @@ void log(Logger::Level L, const char *Fmt, Ts &&... Vals) { detail::log(L, llvm::formatv(Fmt, detail::wrap(std::forward(Vals))...)); } + +llvm::Error error(std::error_code, std::string &&); } // namespace detail // Clangd logging functions write to a global logger set by LoggingSession. @@ -67,6 +69,27 @@ template void vlog(const char *Fmt, Ts &&... Vals) { detail::log(Logger::Verbose, Fmt, std::forward(Vals)...); } +// error() constructs an llvm::Error object, using formatv()-style arguments. +// It is not automatically logged! (This function is a little out of place). +// The error simply embeds the message string. +template +llvm::Error error(std::error_code EC, const char *Fmt, Ts &&... Vals) { + return detail::error( + EC, llvm::formatv(Fmt, detail::wrap(std::forward(Vals))...).str()); +} +template llvm::Error error(const char *Fmt, Ts &&... Vals) { + return detail::error( + llvm::inconvertibleErrorCode(), + llvm::formatv(Fmt, detail::wrap(std::forward(Vals))...).str()); +} +// Overload to avoid the formatv complexity for constant strings. +inline llvm::Error error(std::string Msg) { + return detail::error(llvm::inconvertibleErrorCode(), std::move(Msg)); +} +inline llvm::Error error(std::error_code EC, std::string Msg) { + return detail::error(EC, std::move(Msg)); +} + // dlog only logs if --debug was passed, or --debug_only=Basename. // This level would be enabled in a targeted way when debugging. #define dlog(...) \ diff --git a/clang-tools-extra/clangd/support/Logger.cpp b/clang-tools-extra/clangd/support/Logger.cpp --- a/clang-tools-extra/clangd/support/Logger.cpp +++ b/clang-tools-extra/clangd/support/Logger.cpp @@ -9,6 +9,7 @@ #include "support/Logger.h" #include "support/Trace.h" #include "llvm/Support/Chrono.h" +#include "llvm/Support/Error.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/raw_ostream.h" #include @@ -58,5 +59,29 @@ Logs.flush(); } +namespace { +// Like llvm::StringError but with fewer options and no gratuitous copies. +class SimpleStringError : public llvm::ErrorInfo { + std::error_code EC; + std::string Message; + +public: + SimpleStringError(std::error_code EC, std::string &&Message) + : EC(EC), Message(std::move(Message)) {} + void log(llvm::raw_ostream &OS) const override { OS << Message; } + std::string message() const override { return Message; } + std::error_code convertToErrorCode() const override { return EC; } + static char ID; +}; +char SimpleStringError::ID; + +} // namespace + +llvm::Error detail::error(std::error_code EC, std::string &&Msg) { + // We can't create an error object that embeds the formatv_object and thus + // renders lazily to a string, as referenced objects will be out of scope. + return llvm::make_error(EC, std::move(Msg)); +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -62,6 +62,7 @@ IndexActionTests.cpp IndexTests.cpp JSONTransportTests.cpp + LoggerTests.cpp LSPClient.cpp ParsedASTTests.cpp PathMappingTests.cpp diff --git a/clang-tools-extra/clangd/unittests/LoggerTests.cpp b/clang-tools-extra/clangd/unittests/LoggerTests.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clangd/unittests/LoggerTests.cpp @@ -0,0 +1,62 @@ +//===-- LoggerTests.cpp ---------------------------------------------------===// +// +// 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 "support/Logger.h" +#include "llvm/Support/Error.h" +#include "llvm/Support/Errc.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace clang { +namespace clangd { +namespace { + +TEST(ErrorTest, Overloads) { + EXPECT_EQ("foo", llvm::toString(error("foo"))); + // Inconvertible to error code when none is specified. + // Don't actually try to convert, it'll crash. + handleAllErrors(error("foo"), [&](const llvm::ErrorInfoBase &EI) { + EXPECT_EQ(llvm::inconvertibleErrorCode(), EI.convertToErrorCode()); + }); + + EXPECT_EQ("foo 42", llvm::toString(error("foo {0}", 42))); + handleAllErrors(error("foo {0}", 42), [&](const llvm::ErrorInfoBase &EI) { + EXPECT_EQ(llvm::inconvertibleErrorCode(), EI.convertToErrorCode()); + }); + + EXPECT_EQ("foo", llvm::toString(error(llvm::errc::invalid_argument, "foo"))); + EXPECT_EQ(llvm::errc::invalid_argument, + llvm::errorToErrorCode(error(llvm::errc::invalid_argument, "foo"))); + + EXPECT_EQ("foo 42", + llvm::toString(error(llvm::errc::invalid_argument, "foo {0}", 42))); + EXPECT_EQ(llvm::errc::invalid_argument, + llvm::errorToErrorCode( + error(llvm::errc::invalid_argument, "foo {0}", 42))); +} + +TEST(ErrorTest, Lifetimes) { + llvm::Optional Err; + { + // Check the error contains the value when error() was called. + std::string S = "hello, world"; + Err = error("S={0}", llvm::StringRef(S)); + S = "garbage"; + } + EXPECT_EQ("S=hello, world", llvm::toString(std::move(*Err))); +} + +TEST(ErrorTest, ConsumeError) { + llvm::Error Foo = error("foo"); + llvm::Error Bar = error("bar: {0}", std::move(Foo)); + EXPECT_EQ("bar: foo", llvm::toString(std::move(Bar))); + // No assert for unchecked Foo. +} + +} // namespace +} // namespace clangd +} // namespace clang