Introduce DiagnosticContext, an 'error document' like type. #1323
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,261 @@ | ||||||
| #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 TextRowCol | ||||||
| { | ||||||
| // '0' indicates uninitialized; '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> | ||||||
|
There was a problem hiding this comment. Since these constructors all have different arities, is this SFINAE needed? There was a problem hiding this comment. It's needed to make There was a problem hiding this comment. (Perfect forwarding is psycho greedy so it basically always needs SFINAE) There was a problem hiding this comment. What's the concrete consequence of Conversely, what value/perf is provided by making this a template versus just taking a |
||||||
| 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)) | ||||||
| { | ||||||
| } | ||||||
|
|
||||||
| template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0> | ||||||
| DiagnosticLine(DiagKind kind, StringView origin, TextRowCol position, MessageLike&& message) | ||||||
|
There was a problem hiding this comment. I have a thought, not yet a suggestion, based on wild prediction of what this will look like in practice: This design requires origin and position to be separately plumbed through every function call layer down to where the DiagnosticLine is created -- typically very close to the leaf function that has the appropriate context. I'm afraid many, many functions will end up looking like the following even though they themselves never create a DiagnosticLine -- they just want to call other functions! Optional<std::string> do_the_thing(
DiagnosticContext& ctx,
Optional<StringView> origin,
TextRowCol position,
/* Now I can start talking about _what I actually care about_ */
int a,
int b);To me, this sounds like a lot of boilerplate that (gut feeling) most layers don't care about. Would it make sense to have a source position "stack-y" side channel as part of the context convention? Optional<std::string> do_the_thing_from_the_file(DiagnosticContext& ctx, Path p, const Filesystem& fs) {
if (auto content = fs.read_contents(ctx, p)) {
DiagnosticFrame with_pos(ctx, p, TextRowCol{});
/* Look ma, short calling conventions! */
return do_the_thing(ctx, content);
}
return nullopt;
}This would necessitate 3 different "reporting" overloads:
There was a problem hiding this comment.
I argue that for functions that are talking about file / position information like this, position information is not 'something they don't care about', it's essential in implementing what that function does. I can see an argument for a type that contains:
and today the type trying to do that is
There was a problem hiding this comment. Seeing that There was a problem hiding this comment. It would be great to add at least a comment outlining the different valid states of this object for future readers. Presumably this should be derivable by looking at all the constructor overloads, but that's much more complex than a single comment block that stands alone. |
||||||
| : m_kind(kind) | ||||||
| , m_origin(origin.to_string()) | ||||||
| , m_position(position) | ||||||
| , m_message(std::forward<MessageLike>(message)) | ||||||
| { | ||||||
| } | ||||||
|
|
||||||
| // Prints this diagnostic to the terminal. | ||||||
| // Not thread safe: The console DiagnosticContext must apply its own synchronization. | ||||||
|
There was a problem hiding this comment. I'm confused by this comment since MessageSink might not be a terminal. Why is console DiagnosticContext mentioned in this comment, when the function takes a MessageSink? |
||||||
| 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; | ||||||
|
|
||||||
| 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; | ||||||
|
Comment on lines
+75
to
+76
There was a problem hiding this comment. Would it make sense to bundle these (origin + position) into a single
There was a problem hiding this comment. Empty string origin is never intended. Position without origin is never intended either. I'll look to clarify. |
||||||
| 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); } | ||||||
|
|
||||||
| 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 terminal. | ||||||
| // Not safe to use in the face of concurrent calls to report() | ||||||
| void print(MessageSink& sink) const; | ||||||
|
There was a problem hiding this comment.
Suggested change
or |
||||||
| // 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; | ||||||
| }; | ||||||
|
|
||||||
| extern DiagnosticContext& console_diagnostic_context; | ||||||
| extern DiagnosticContext& null_diagnostic_context; | ||||||
|
|
||||||
| // 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&&; | ||||||
| }; | ||||||
|
|
||||||
| 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 | ||||||
| }; | ||||||
|
|
||||||
| 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}; | ||||||
BillyONeal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,5 @@ namespace vcpkg | |
| }; | ||
|
|
||
| struct ParseMessage; | ||
| struct ParserBase; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
DiagnosticMessageor simpleMessage? Since it is allowed that the diagnostic line contains multiple linesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word 'message' is heavily used already by the localization infrastructure and I didn't want to step on such names.