Skip to content

Commit

Permalink
Introduce DiagnosticContext, an 'error document' like type. (#1323)
Browse files Browse the repository at this point in the history
* In several PRs, such as #908 and #1210 , it has become clear that we need a thread safe channel through which errors and warnings can both be reasonably reported. Now that #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 ( #1290 ) or complex enough that we are not sure they are correct by trying to recover boundaries by reparsing our own error output ( #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 authored Dec 7, 2024
1 parent e5ed2e7 commit b4b372c
Show file tree
Hide file tree
Showing 21 changed files with 922 additions and 49 deletions.
273 changes: 273 additions & 0 deletions include/vcpkg/base/diagnostics.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,273 @@
#pragma once

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

#include <memory>
#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 TextRowCol
{
// '0' indicates that line and column information is unknown; '1' is the first row/column
int row = 0;
int column = 0;
};

struct DiagnosticLine
{
template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0>
DiagnosticLine(DiagKind kind, MessageLike&& message)
: m_kind(kind), m_origin(), m_position(), m_message(std::forward<MessageLike>(message))
{
}

template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0>
DiagnosticLine(DiagKind kind, StringView origin, MessageLike&& message)
: m_kind(kind), m_origin(origin.to_string()), m_position(), m_message(std::forward<MessageLike>(message))
{
if (origin.empty())
{
Checks::unreachable(VCPKG_LINE_INFO, "origin must not be empty");
}
}

template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0>
DiagnosticLine(DiagKind kind, StringView origin, TextRowCol position, MessageLike&& message)
: m_kind(kind)
, m_origin(origin.to_string())
, m_position(position)
, m_message(std::forward<MessageLike>(message))
{
if (origin.empty())
{
Checks::unreachable(VCPKG_LINE_INFO, "origin must not be empty");
}
}

// Prints this diagnostic to the supplied sink.
void print_to(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;

LocalizedString to_json_reader_string(const std::string& path, const LocalizedString& type) const;

DiagKind kind() const noexcept { return m_kind; }

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

struct DiagnosticContext
{
virtual void report(const DiagnosticLine& line) = 0;
virtual void report(DiagnosticLine&& line) { report(line); }

void report_error(const LocalizedString& message) { report(DiagnosticLine{DiagKind::Error, message}); }
void report_error(LocalizedString&& message) { report(DiagnosticLine{DiagKind::Error, std::move(message)}); }
template<VCPKG_DECL_MSG_TEMPLATE>
void report_error(VCPKG_DECL_MSG_ARGS)
{
LocalizedString message;
msg::format_to(message, VCPKG_EXPAND_MSG_ARGS);
this->report_error(std::move(message));
}

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 supplied sink.
void print_to(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;

bool any_errors() const noexcept;
};

extern DiagnosticContext& console_diagnostic_context;
extern DiagnosticContext& null_diagnostic_context;

// The following overloads are implementing
// adapt_context_to_expected(Fn functor, Args&&... args)
//
// Given:
// Optional<T> functor(DiagnosticContext&, args...), adapts functor to return ExpectedL<T>
// Optional<T>& functor(DiagnosticContext&, args...), adapts functor to return ExpectedL<T&>
// Optional<T>&& functor(DiagnosticContext&, args...), adapts functor to return ExpectedL<T>
// std::unique_ptr<T> functor(DiagnosticContext&, args...), adapts functor to return ExpectedL<std::unique_ptr<T>>

// If Ty is an Optional<U>, typename AdaptContextUnwrapOptional<Ty>::type is the type necessary to return U, and fwd
// is the type necessary to forward U. Otherwise, there are no members ::type or ::fwd
template<class Ty>
struct AdaptContextUnwrapOptional
{
// no member ::type, SFINAEs out when the input type is:
// * not Optional
// * volatile
};

template<class Wrapped>
struct AdaptContextUnwrapOptional<Optional<Wrapped>>
{
// prvalue, move from the optional into the expected
using type = Wrapped;
using fwd = Wrapped&&;
};

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

template<class Wrapped>
struct AdaptContextUnwrapOptional<Optional<Wrapped>&>
{
// lvalue, return an expected referencing the Wrapped inside the optional
using type = Wrapped&;
using fwd = Wrapped&;
};

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

template<class Wrapped>
struct AdaptContextUnwrapOptional<Optional<Wrapped>&&>
{
// xvalue, move from the optional into the expected
using type = Wrapped;
using fwd = Wrapped&&;
};

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

// The overload for functors that return Optional<T>
template<class Fn, class... Args>
auto adapt_context_to_expected(Fn functor, Args&&... args)
-> ExpectedL<
typename AdaptContextUnwrapOptional<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::type>
{
using Unwrapper = AdaptContextUnwrapOptional<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>;
using ReturnType = ExpectedL<typename Unwrapper::type>;
BufferedDiagnosticContext bdc;
decltype(auto) maybe_result = functor(bdc, std::forward<Args>(args)...);
if (auto result = maybe_result.get())
{
// N.B.: This may be a move
return ReturnType{static_cast<typename Unwrapper::fwd>(*result), expected_left_tag};
}

return ReturnType{LocalizedString::from_raw(bdc.to_string()), expected_right_tag};
}

// If Ty is a std::unique_ptr<U>, typename AdaptContextDetectUniquePtr<Ty>::type is the type necessary to return
template<class Ty>
struct AdaptContextDetectUniquePtr
{
// no member ::type, SFINAEs out when the input type is:
// * not unique_ptr
// * volatile
};

template<class Wrapped, class Deleter>
struct AdaptContextDetectUniquePtr<std::unique_ptr<Wrapped, Deleter>>
{
// prvalue, move into the Expected
using type = std::unique_ptr<Wrapped, Deleter>;
};

template<class Wrapped, class Deleter>
struct AdaptContextDetectUniquePtr<const std::unique_ptr<Wrapped, Deleter>>
{
// const prvalue (not valid, can't be moved from)
// no members
};

template<class Wrapped, class Deleter>
struct AdaptContextDetectUniquePtr<std::unique_ptr<Wrapped, Deleter>&>
{
// lvalue, reference the unique_ptr itself
using type = std::unique_ptr<Wrapped, Deleter>&;
};

template<class Wrapped, class Deleter>
struct AdaptContextDetectUniquePtr<const std::unique_ptr<Wrapped, Deleter>&>
{
// const lvalue
using type = const std::unique_ptr<Wrapped, Deleter>&;
};

template<class Wrapped, class Deleter>
struct AdaptContextDetectUniquePtr<std::unique_ptr<Wrapped, Deleter>&&>
{
// xvalue, move into the Expected
using type = std::unique_ptr<Wrapped, Deleter>;
};

template<class Wrapped, class Deleter>
struct AdaptContextDetectUniquePtr<const std::unique_ptr<Wrapped, Deleter>&&>
{
// const xvalue (not valid, can't be moved from)
// no members
};

// The overload for functors that return std::unique_ptr<T>
template<class Fn, class... Args>
auto adapt_context_to_expected(Fn functor, Args&&... args)
-> ExpectedL<
typename AdaptContextDetectUniquePtr<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::type>
{
using ReturnType = ExpectedL<
typename AdaptContextDetectUniquePtr<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::type>;
BufferedDiagnosticContext bdc;
decltype(auto) maybe_result = functor(bdc, std::forward<Args>(args)...);
if (maybe_result)
{
return ReturnType{static_cast<decltype(maybe_result)&&>(maybe_result), expected_left_tag};
}

return ReturnType{LocalizedString::from_raw(bdc.to_string()), expected_right_tag};
}
}
5 changes: 2 additions & 3 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 @@ -44,7 +43,7 @@ namespace vcpkg

struct ParserBase
{
ParserBase(StringView text, Optional<StringView> origin, TextRowCol init_rowcol = {});
ParserBase(StringView text, Optional<StringView> origin, TextRowCol init_rowcol);

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
2 changes: 1 addition & 1 deletion include/vcpkg/paragraphparser.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

#include <vcpkg/fwd/paragraphparser.h>

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

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

#include <map>
#include <memory>
Expand Down
17 changes: 0 additions & 17 deletions include/vcpkg/textrowcol.h

This file was deleted.

13 changes: 6 additions & 7 deletions src/vcpkg-test/manifests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1326,29 +1326,28 @@ TEST_CASE ("license error messages", "[manifests][license]")
ParseMessages messages;
parse_spdx_license_expression("", messages);
CHECK(messages.error.value_or_exit(VCPKG_LINE_INFO) ==
LocalizedString::from_raw(R"(<license string>:1:1: error: SPDX license expression was empty.
LocalizedString::from_raw(R"(<license string>: error: SPDX license expression was empty.
on expression:
^)"));

parse_spdx_license_expression("MIT ()", messages);
CHECK(messages.error.value_or_exit(VCPKG_LINE_INFO) ==
LocalizedString::from_raw(
R"(<license string>:1:5: error: Expected a compound or the end of the string, found a parenthesis.
R"(<license string>: error: Expected a compound or the end of the string, found a parenthesis.
on expression: MIT ()
^)"));

parse_spdx_license_expression("MIT +", messages);
CHECK(
messages.error.value_or_exit(VCPKG_LINE_INFO) ==
LocalizedString::from_raw(
R"(<license string>:1:5: error: SPDX license expression contains an extra '+'. These are only allowed directly after a license identifier.
R"(<license string>: error: SPDX license expression contains an extra '+'. These are only allowed directly after a license identifier.
on expression: MIT +
^)"));

parse_spdx_license_expression("MIT AND", messages);
CHECK(
messages.error.value_or_exit(VCPKG_LINE_INFO) ==
LocalizedString::from_raw(R"(<license string>:1:8: error: Expected a license name, found the end of the string.
CHECK(messages.error.value_or_exit(VCPKG_LINE_INFO) ==
LocalizedString::from_raw(R"(<license string>: error: Expected a license name, found the end of the string.
on expression: MIT AND
^)"));

Expand All @@ -1357,7 +1356,7 @@ TEST_CASE ("license error messages", "[manifests][license]")
REQUIRE(messages.warnings.size() == 1);
CHECK(
test_format_parse_warning(messages.warnings[0]) ==
R"(<license string>:1:9: warning: Unknown license identifier 'unknownlicense'. Known values are listed at https://spdx.org/licenses/
R"(<license string>: warning: Unknown license identifier 'unknownlicense'. Known values are listed at https://spdx.org/licenses/
on expression: MIT AND unknownlicense
^)");
}
Expand Down
Loading

0 comments on commit b4b372c

Please sign in to comment.