Skip to content

Commit

Permalink
In several PRs, such as microsoft#908 and microsoft#1210 , it has bec…
Browse files Browse the repository at this point in the history
…ome clear that we need a thread safe channel through which errors and warnings can both be reasonably reported. Now that microsoft#1279 is landed and functionally everything in the codebase uses ExpectedL, we can look at what the new thing that fixes issues is.

Consider the following:

```c++
    ExpectedL<T> example_api(int a);

    ExpectedL<std::unique_ptr<SourceControlFile>> try_load_port_manifest_text(StringView text,
                                                                              StringView control_path,
                                                                              MessageSink& warning_sink);
```

The reason this can't return the warnings through the ExpectedL channel is that we don't want the 'error' state to be engaged when there are merely warnings. Moreover, that these channels are different channels means that situations that might want to return errors and warnings together, as happens when parsing files, means that order relationships between errors and warnings is lost. It is probably a good idea in general to put warnings and errors about the same location next to each other in the output, but that's hard to do with this interface.

Rather than multiplexing everything through the return value, this proposal is to multiplex only the success or failure through the return value, and report any specific error information through an out parameter.

1. Distinguish whether an overall operation succeeded or failed in the return value, but
2. record any errors or warnings via an out parameter.

Applying this to the above gives:

```c++
    Optional<T> example_api(MessageContext& context, int a);

    // unique_ptr is already 'optional'
    std::unique_ptr<SourceControlFile> try_load_port_manifest_text(MessageContext& context,
                                                                   StringView text,
                                                                   StringView control_path);
```

Issues this new mechanism fixes:

* Errors and warnings can share the same channel and thus be printed together
* The interface between code wanting to report events and the code wanting to consume them is a natural thread synchronization boundary. Other attempts to fix this have been incorrect by synchronizing individual print calls ( microsoft#1290 ) or complex enough that we are not sure they are correct by trying to recover boundaries by reparsing our own error output ( microsoft#908 )
* This shuts down the "error: error:" and similar bugs where it isn't clear who is formatting the overall error message vs. talking about individual components

Known issues that are not fixed by this change:

* This still doesn't make it easy for callers to programmatically handle specific types of errors. Currently, we have some APIs that still use explicit `std::error_code` because they want to do different things for 'file does not exist' vs. 'there was an I/O error'. Given that this condition isn't well served by the ExpectedL mechanism I don't want to wait until we have a better solution to it to proceed.
* Because we aren't making the context parameter the 'success carrier' it's more complex to implement 'warnings as errors' or similar functionality where the caller decides how 'important' something is. I would be in favor of moving all success tests to the context parameter but I'm not proposing that because the other vcpkg maintainers do not like it.
* Contextual information / stack problems aren't solved. However, the context parameter might be extended in the future to help with this.
  • Loading branch information
BillyONeal committed Jan 26, 2024
1 parent 910db8b commit 3138143
Show file tree
Hide file tree
Showing 13 changed files with 394 additions and 52 deletions.
159 changes: 159 additions & 0 deletions include/vcpkg/base/diagnostics.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
#pragma once

#include <vcpkg/base/expected.h>
#include <vcpkg/base/messages.h>
#include <vcpkg/base/optional.h>

#include <mutex>
#include <string>
#include <type_traits>
#include <vector>

namespace vcpkg
{
enum class DiagKind
{
None, // foo.h: localized
Message, // foo.h: message: localized
Error, // foo.h: error: localized
Warning, // foo.h: warning: localized
Note, // foo.h: note: localized
COUNT
};

struct TextPosition
{
// '0' indicates uninitialized; '1' is the first row/column
int row = 0;
int column = 0;
};

struct DiagnosticLine
{
DiagnosticLine(DiagKind kind, LocalizedString&& message)
: m_kind(kind), m_origin(), m_position(), m_message(std::move(message))
{
}

DiagnosticLine(DiagKind kind, StringView origin, LocalizedString&& message)
: m_kind(kind), m_origin(origin.to_string()), m_position(), m_message(std::move(message))
{
}

DiagnosticLine(DiagKind kind, StringView origin, TextPosition position, LocalizedString&& message)
: m_kind(kind), m_origin(origin.to_string()), m_position(position), m_message(std::move(message))
{
}

// Prints this diagnostic to the terminal.
// Not thread safe: The console DiagnosticContext must apply its own synchronization.
void print(MessageSink& sink) const;
// Converts this message into a string
// Prefer print() if possible because it applies color
std::string to_string() const;
void to_string(std::string& target) const;

private:
DiagKind m_kind;
Optional<std::string> m_origin;
TextPosition m_position;
LocalizedString m_message;
};

struct DiagnosticContext
{
// Records a diagnostic. Implementations must make simultaneous calls of report() safe from multiple threads
// and print entire DiagnosticLines as atomic units. Implementations are not required to synchronize with
// other machinery like msg::print and friends.
//
// This serves to make multithreaded code that reports only via this mechanism safe.
virtual void report(const DiagnosticLine& line) = 0;
virtual void report(DiagnosticLine&& line) { report(line); }

protected:
~DiagnosticContext() = default;
};

struct BufferedDiagnosticContext final : DiagnosticContext
{
virtual void report(const DiagnosticLine& line) override;
virtual void report(DiagnosticLine&& line) override;

std::vector<DiagnosticLine> lines;

// Prints all diagnostics to the terminal.
// Not safe to use in the face of concurrent calls to report()
void print(MessageSink& sink) const;
// Converts this message into a string
// Prefer print() if possible because it applies color
// Not safe to use in the face of concurrent calls to report()
std::string to_string() const;
void to_string(std::string& target) const;

private:
std::mutex m_mtx;
};

// If T Ty is an rvalue Optional<U>, typename UnwrapOptional<Ty>::type is the type necessary to forward U
// Otherwise, there is no member UnwrapOptional<Ty>::type
template<class Ty>
struct UnwrapOptional
{
// no member ::type, SFINAEs out when the input type is:
// * not Optional
// * not an rvalue
// * volatile
};

template<class Wrapped>
struct UnwrapOptional<Optional<Wrapped>>
{
// prvalue
using type = Wrapped;
using fwd = Wrapped&&;
};

template<class Wrapped>
struct UnwrapOptional<const Optional<Wrapped>>
{
// const prvalue
using type = Wrapped;
using fwd = const Wrapped&&;
};

template<class Wrapped>
struct UnwrapOptional<Optional<Wrapped>&&>
{
// xvalue
using type = Wrapped&&;
using fwd = Wrapped&&;
};

template<class Wrapped>
struct UnwrapOptional<const Optional<Wrapped>&&>
{
// const xvalue
using type = const Wrapped&&;
using fwd = Wrapped&&;
};

template<class Fn, class... Args>
auto adapt_context_to_expected(Fn functor, Args&&... args)
-> ExpectedL<typename UnwrapOptional<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::type>
{
using Contained = typename UnwrapOptional<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::type;
BufferedDiagnosticContext bdc;
auto maybe_result = functor(bdc, std::forward<Args>(args)...);
if (auto result = maybe_result.get())
{
// N.B.: This may be a move
return ExpectedL<Contained>{
static_cast<
typename UnwrapOptional<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::fwd>(
*result),
expected_left_tag};
}

return ExpectedL<Contained>{LocalizedString::from_raw(bdc.to_string()), expected_right_tag};
}
}
7 changes: 3 additions & 4 deletions include/vcpkg/base/parse.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@

#include <vcpkg/base/fwd/parse.h>

#include <vcpkg/base/diagnostics.h>
#include <vcpkg/base/messages.h>
#include <vcpkg/base/optional.h>
#include <vcpkg/base/stringview.h>
#include <vcpkg/base/unicode.h>

#include <vcpkg/textrowcol.h>

#include <string>

namespace vcpkg
Expand Down Expand Up @@ -40,7 +39,7 @@ namespace vcpkg

struct ParserBase
{
ParserBase(StringView text, StringView origin, TextRowCol init_rowcol = {});
ParserBase(StringView text, StringView origin, TextPosition init_rowcol = {1, 1});

static constexpr bool is_whitespace(char32_t ch) { return ch == ' ' || ch == '\t' || ch == '\r' || ch == '\n'; }
static constexpr bool is_lower_alpha(char32_t ch) { return ch >= 'a' && ch <= 'z'; }
Expand Down Expand Up @@ -96,7 +95,7 @@ namespace vcpkg
Unicode::Utf8Decoder it() const { return m_it; }
char32_t cur() const { return m_it == m_it.end() ? Unicode::end_of_file : *m_it; }
SourceLoc cur_loc() const { return {m_it, m_start_of_line, m_row, m_column}; }
TextRowCol cur_rowcol() const { return {m_row, m_column}; }
TextPosition cur_rowcol() const { return {m_row, m_column}; }
char32_t next();
bool at_eof() const { return m_it == m_it.end(); }

Expand Down
12 changes: 6 additions & 6 deletions include/vcpkg/paragraphparser.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

#include <vcpkg/fwd/paragraphparser.h>

#include <vcpkg/base/diagnostics.h>
#include <vcpkg/base/expected.h>
#include <vcpkg/base/messages.h>
#include <vcpkg/base/stringview.h>

#include <vcpkg/packagespec.h>
#include <vcpkg/textrowcol.h>

#include <map>
#include <memory>
Expand All @@ -16,7 +16,7 @@

namespace vcpkg
{
using Paragraph = std::map<std::string, std::pair<std::string, TextRowCol>, std::less<>>;
using Paragraph = std::map<std::string, std::pair<std::string, TextPosition>, std::less<>>;

struct ParagraphParser
{
Expand All @@ -28,9 +28,9 @@ namespace vcpkg
std::string required_field(StringLiteral fieldname);

std::string optional_field(StringLiteral fieldname);
std::string optional_field(StringLiteral fieldname, TextRowCol& position);
std::string optional_field(StringLiteral fieldname, TextPosition& position);

void add_error(TextRowCol position, msg::MessageT<> error_content);
void add_error(TextPosition position, msg::MessageT<> error_content);

Optional<LocalizedString> error() const;

Expand All @@ -42,8 +42,8 @@ namespace vcpkg

ExpectedL<std::vector<std::string>> parse_default_features_list(const std::string& str,
StringView origin = "<unknown>",
TextRowCol textrowcol = {});
TextPosition position = {1, 1});
ExpectedL<std::vector<ParsedQualifiedSpecifier>> parse_qualified_specifier_list(const std::string& str,
StringView origin = "<unknown>",
TextRowCol textrowcol = {});
TextPosition position = {1, 1});
}
2 changes: 1 addition & 1 deletion include/vcpkg/sourceparagraph.h
Original file line number Diff line number Diff line change
Expand Up @@ -223,5 +223,5 @@ namespace vcpkg
// Exposed for testing
ExpectedL<std::vector<Dependency>> parse_dependencies_list(const std::string& str,
StringView origin,
TextRowCol textrowcol = {});
TextPosition position = {1, 1});
}
17 changes: 0 additions & 17 deletions include/vcpkg/textrowcol.h

This file was deleted.

104 changes: 104 additions & 0 deletions src/vcpkg-test/messages.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <vcpkg-test/util.h>

#include <vcpkg/base/diagnostics.h>
#include <vcpkg/base/setup-messages.h>

#include <vcpkg/commands.z-generate-message-map.h>
Expand Down Expand Up @@ -99,3 +100,106 @@ TEST_CASE ("generate message get_format_arg_mismatches", "[messages]")
CHECK(res.arguments_without_comment == std::vector<StringView>{"go", "ho"});
CHECK(res.comments_without_argument == std::vector<StringView>{"blah"});
}

namespace
{
struct OnlyMoveOnce
{
bool& m_moved;

explicit OnlyMoveOnce(bool& moved) : m_moved(moved) { }
OnlyMoveOnce(const OnlyMoveOnce&) = delete;
OnlyMoveOnce(OnlyMoveOnce&& other) : m_moved(other.m_moved)
{
REQUIRE(!m_moved);
m_moved = true;
}

OnlyMoveOnce& operator=(const OnlyMoveOnce&) = delete;
OnlyMoveOnce& operator=(OnlyMoveOnce&&) = delete;
};

int returns_int(DiagnosticContext&) { return 42; }
std::unique_ptr<int> returns_unique_ptr(DiagnosticContext&) { return std::unique_ptr<int>{new int{42}}; }
Optional<int> returns_optional_prvalue(DiagnosticContext&, int val) { return val; }
const Optional<int> returns_optional_const_prvalue(DiagnosticContext&, int val) { return val; }
Optional<int>&& returns_optional_xvalue(DiagnosticContext&, Optional<int>&& val) { return std::move(val); }
const Optional<int>&& returns_optional_const_xvalue(DiagnosticContext&, Optional<int>&& val)
{
return std::move(val);
}
Optional<int> returns_optional_prvalue_fail(DiagnosticContext& context)
{
context.report(DiagnosticLine{DiagKind::Error, LocalizedString::from_raw("something bad happened")});
return nullopt;
}
const Optional<int> returns_optional_const_prvalue_fail(DiagnosticContext& context)
{
context.report(DiagnosticLine{DiagKind::Error, LocalizedString::from_raw("something bad happened")});
return nullopt;
}
Optional<int>&& returns_optional_xvalue_fail(DiagnosticContext& context, Optional<int>&& val)
{
val.clear();
context.report(DiagnosticLine{DiagKind::Error, LocalizedString::from_raw("something bad happened")});
return std::move(val);
}

const Optional<int>&& returns_optional_const_xvalue_fail(DiagnosticContext& context, Optional<int>&& val)
{
val.clear();
context.report(DiagnosticLine{DiagKind::Error, LocalizedString::from_raw("something bad happened")});
return std::move(val);
}

template<class Void, class Test, class... Args>
constexpr bool adapt_context_to_expected_invocable_with_impl = false;

template<class Test, class... Args>
constexpr bool adapt_context_to_expected_invocable_with_impl<
std::void_t<decltype(adapt_context_to_expected(std::declval<Test>(), std::declval<Args>()...))>,
Test,
Args...> = true;

template<class Test, class... Args>
constexpr bool adapt_context_to_expected_invocable_with =
adapt_context_to_expected_invocable_with_impl<void, Test, Args...>;
} // unnamed namespace

TEST_CASE ("adapt DiagnosticContext to ExpectedL", "[diagnostics]")
{
// adapt_context_to_expected(returns_int); // should not compile
static_assert(!adapt_context_to_expected_invocable_with<decltype(returns_int)>,
"Callable needs to return optional");
// adapt_context_to_expected(returns_unique_ptr); // should not compile
static_assert(!adapt_context_to_expected_invocable_with<decltype(returns_unique_ptr)>,
"Callable needs to return optional");

static_assert(adapt_context_to_expected_invocable_with<decltype(returns_optional_prvalue), int>,
"adapt_context_to_expected_invocable_with needs to succeed with a value that should "
"work");

// test that the type of ExpectedL is determined correctly
static_assert(std::is_same_v<ExpectedL<int>, decltype(adapt_context_to_expected(returns_optional_prvalue, 42))>,
"boom");
static_assert(
std::is_same_v<ExpectedL<int>, decltype(adapt_context_to_expected(returns_optional_const_prvalue, 42))>,
"boom");
static_assert(
std::is_same_v<ExpectedL<int&&>,
decltype(adapt_context_to_expected(returns_optional_xvalue, std::declval<Optional<int>>()))>,
"boom");
static_assert(std::is_same_v<ExpectedL<const int&&>,
decltype(adapt_context_to_expected(returns_optional_const_xvalue,
std::declval<Optional<int>>()))>,
"boom");
{
auto adapted = adapt_context_to_expected(returns_optional_prvalue, 42);
REQUIRE(adapted.value_or_exit(VCPKG_LINE_INFO) == 42);
}
{
auto adapted = adapt_context_to_expected(returns_optional_prvalue_fail);
REQUIRE(!adapted.has_value());
REQUIRE(adapted.error() == LocalizedString::from_raw("error: something bad happened"));
}
}
4 changes: 2 additions & 2 deletions src/vcpkg-test/paragraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace
{
pghs.emplace_back();
for (auto&& kv : p)
pghs.back().emplace(kv.first, std::make_pair(kv.second, vcpkg::TextRowCol{}));
pghs.back().emplace(kv.first, std::make_pair(kv.second, vcpkg::TextPosition{}));
}
return vcpkg::SourceControlFile::parse_control_file("", std::move(pghs));
}
Expand All @@ -24,7 +24,7 @@ namespace
{
Paragraph pgh;
for (auto&& kv : v)
pgh.emplace(kv.first, std::make_pair(kv.second, vcpkg::TextRowCol{}));
pgh.emplace(kv.first, std::make_pair(kv.second, vcpkg::TextPosition{}));

return vcpkg::BinaryParagraph("test", std::move(pgh));
}
Expand Down
Loading

0 comments on commit 3138143

Please sign in to comment.