Skip to content

Commit b4b372c

Browse files
authored
Introduce DiagnosticContext, an 'error document' like type. (microsoft#1323)
* In several PRs, such as microsoft#908 and microsoft#1210 , it has become 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.
1 parent e5ed2e7 commit b4b372c

21 files changed

+922
-49
lines changed

include/vcpkg/base/diagnostics.h

+273
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,273 @@
1+
#pragma once
2+
3+
#include <vcpkg/base/expected.h>
4+
#include <vcpkg/base/messages.h>
5+
#include <vcpkg/base/optional.h>
6+
7+
#include <memory>
8+
#include <string>
9+
#include <type_traits>
10+
#include <vector>
11+
12+
namespace vcpkg
13+
{
14+
enum class DiagKind
15+
{
16+
None, // foo.h: localized
17+
Message, // foo.h: message: localized
18+
Error, // foo.h: error: localized
19+
Warning, // foo.h: warning: localized
20+
Note, // foo.h: note: localized
21+
COUNT
22+
};
23+
24+
struct TextRowCol
25+
{
26+
// '0' indicates that line and column information is unknown; '1' is the first row/column
27+
int row = 0;
28+
int column = 0;
29+
};
30+
31+
struct DiagnosticLine
32+
{
33+
template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0>
34+
DiagnosticLine(DiagKind kind, MessageLike&& message)
35+
: m_kind(kind), m_origin(), m_position(), m_message(std::forward<MessageLike>(message))
36+
{
37+
}
38+
39+
template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0>
40+
DiagnosticLine(DiagKind kind, StringView origin, MessageLike&& message)
41+
: m_kind(kind), m_origin(origin.to_string()), m_position(), m_message(std::forward<MessageLike>(message))
42+
{
43+
if (origin.empty())
44+
{
45+
Checks::unreachable(VCPKG_LINE_INFO, "origin must not be empty");
46+
}
47+
}
48+
49+
template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0>
50+
DiagnosticLine(DiagKind kind, StringView origin, TextRowCol position, MessageLike&& message)
51+
: m_kind(kind)
52+
, m_origin(origin.to_string())
53+
, m_position(position)
54+
, m_message(std::forward<MessageLike>(message))
55+
{
56+
if (origin.empty())
57+
{
58+
Checks::unreachable(VCPKG_LINE_INFO, "origin must not be empty");
59+
}
60+
}
61+
62+
// Prints this diagnostic to the supplied sink.
63+
void print_to(MessageSink& sink) const;
64+
// Converts this message into a string
65+
// Prefer print() if possible because it applies color
66+
std::string to_string() const;
67+
void to_string(std::string& target) const;
68+
69+
LocalizedString to_json_reader_string(const std::string& path, const LocalizedString& type) const;
70+
71+
DiagKind kind() const noexcept { return m_kind; }
72+
73+
private:
74+
DiagKind m_kind;
75+
Optional<std::string> m_origin;
76+
TextRowCol m_position;
77+
LocalizedString m_message;
78+
};
79+
80+
struct DiagnosticContext
81+
{
82+
virtual void report(const DiagnosticLine& line) = 0;
83+
virtual void report(DiagnosticLine&& line) { report(line); }
84+
85+
void report_error(const LocalizedString& message) { report(DiagnosticLine{DiagKind::Error, message}); }
86+
void report_error(LocalizedString&& message) { report(DiagnosticLine{DiagKind::Error, std::move(message)}); }
87+
template<VCPKG_DECL_MSG_TEMPLATE>
88+
void report_error(VCPKG_DECL_MSG_ARGS)
89+
{
90+
LocalizedString message;
91+
msg::format_to(message, VCPKG_EXPAND_MSG_ARGS);
92+
this->report_error(std::move(message));
93+
}
94+
95+
protected:
96+
~DiagnosticContext() = default;
97+
};
98+
99+
struct BufferedDiagnosticContext final : DiagnosticContext
100+
{
101+
virtual void report(const DiagnosticLine& line) override;
102+
virtual void report(DiagnosticLine&& line) override;
103+
104+
std::vector<DiagnosticLine> lines;
105+
106+
// Prints all diagnostics to the supplied sink.
107+
void print_to(MessageSink& sink) const;
108+
// Converts this message into a string
109+
// Prefer print() if possible because it applies color
110+
std::string to_string() const;
111+
void to_string(std::string& target) const;
112+
113+
bool any_errors() const noexcept;
114+
};
115+
116+
extern DiagnosticContext& console_diagnostic_context;
117+
extern DiagnosticContext& null_diagnostic_context;
118+
119+
// The following overloads are implementing
120+
// adapt_context_to_expected(Fn functor, Args&&... args)
121+
//
122+
// Given:
123+
// Optional<T> functor(DiagnosticContext&, args...), adapts functor to return ExpectedL<T>
124+
// Optional<T>& functor(DiagnosticContext&, args...), adapts functor to return ExpectedL<T&>
125+
// Optional<T>&& functor(DiagnosticContext&, args...), adapts functor to return ExpectedL<T>
126+
// std::unique_ptr<T> functor(DiagnosticContext&, args...), adapts functor to return ExpectedL<std::unique_ptr<T>>
127+
128+
// If Ty is an Optional<U>, typename AdaptContextUnwrapOptional<Ty>::type is the type necessary to return U, and fwd
129+
// is the type necessary to forward U. Otherwise, there are no members ::type or ::fwd
130+
template<class Ty>
131+
struct AdaptContextUnwrapOptional
132+
{
133+
// no member ::type, SFINAEs out when the input type is:
134+
// * not Optional
135+
// * volatile
136+
};
137+
138+
template<class Wrapped>
139+
struct AdaptContextUnwrapOptional<Optional<Wrapped>>
140+
{
141+
// prvalue, move from the optional into the expected
142+
using type = Wrapped;
143+
using fwd = Wrapped&&;
144+
};
145+
146+
template<class Wrapped>
147+
struct AdaptContextUnwrapOptional<const Optional<Wrapped>>
148+
{
149+
// const prvalue
150+
using type = Wrapped;
151+
using fwd = const Wrapped&&;
152+
};
153+
154+
template<class Wrapped>
155+
struct AdaptContextUnwrapOptional<Optional<Wrapped>&>
156+
{
157+
// lvalue, return an expected referencing the Wrapped inside the optional
158+
using type = Wrapped&;
159+
using fwd = Wrapped&;
160+
};
161+
162+
template<class Wrapped>
163+
struct AdaptContextUnwrapOptional<const Optional<Wrapped>&>
164+
{
165+
// const lvalue
166+
using type = const Wrapped&;
167+
using fwd = const Wrapped&;
168+
};
169+
170+
template<class Wrapped>
171+
struct AdaptContextUnwrapOptional<Optional<Wrapped>&&>
172+
{
173+
// xvalue, move from the optional into the expected
174+
using type = Wrapped;
175+
using fwd = Wrapped&&;
176+
};
177+
178+
template<class Wrapped>
179+
struct AdaptContextUnwrapOptional<const Optional<Wrapped>&&>
180+
{
181+
// const xvalue
182+
using type = Wrapped;
183+
using fwd = const Wrapped&&;
184+
};
185+
186+
// The overload for functors that return Optional<T>
187+
template<class Fn, class... Args>
188+
auto adapt_context_to_expected(Fn functor, Args&&... args)
189+
-> ExpectedL<
190+
typename AdaptContextUnwrapOptional<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::type>
191+
{
192+
using Unwrapper = AdaptContextUnwrapOptional<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>;
193+
using ReturnType = ExpectedL<typename Unwrapper::type>;
194+
BufferedDiagnosticContext bdc;
195+
decltype(auto) maybe_result = functor(bdc, std::forward<Args>(args)...);
196+
if (auto result = maybe_result.get())
197+
{
198+
// N.B.: This may be a move
199+
return ReturnType{static_cast<typename Unwrapper::fwd>(*result), expected_left_tag};
200+
}
201+
202+
return ReturnType{LocalizedString::from_raw(bdc.to_string()), expected_right_tag};
203+
}
204+
205+
// If Ty is a std::unique_ptr<U>, typename AdaptContextDetectUniquePtr<Ty>::type is the type necessary to return
206+
template<class Ty>
207+
struct AdaptContextDetectUniquePtr
208+
{
209+
// no member ::type, SFINAEs out when the input type is:
210+
// * not unique_ptr
211+
// * volatile
212+
};
213+
214+
template<class Wrapped, class Deleter>
215+
struct AdaptContextDetectUniquePtr<std::unique_ptr<Wrapped, Deleter>>
216+
{
217+
// prvalue, move into the Expected
218+
using type = std::unique_ptr<Wrapped, Deleter>;
219+
};
220+
221+
template<class Wrapped, class Deleter>
222+
struct AdaptContextDetectUniquePtr<const std::unique_ptr<Wrapped, Deleter>>
223+
{
224+
// const prvalue (not valid, can't be moved from)
225+
// no members
226+
};
227+
228+
template<class Wrapped, class Deleter>
229+
struct AdaptContextDetectUniquePtr<std::unique_ptr<Wrapped, Deleter>&>
230+
{
231+
// lvalue, reference the unique_ptr itself
232+
using type = std::unique_ptr<Wrapped, Deleter>&;
233+
};
234+
235+
template<class Wrapped, class Deleter>
236+
struct AdaptContextDetectUniquePtr<const std::unique_ptr<Wrapped, Deleter>&>
237+
{
238+
// const lvalue
239+
using type = const std::unique_ptr<Wrapped, Deleter>&;
240+
};
241+
242+
template<class Wrapped, class Deleter>
243+
struct AdaptContextDetectUniquePtr<std::unique_ptr<Wrapped, Deleter>&&>
244+
{
245+
// xvalue, move into the Expected
246+
using type = std::unique_ptr<Wrapped, Deleter>;
247+
};
248+
249+
template<class Wrapped, class Deleter>
250+
struct AdaptContextDetectUniquePtr<const std::unique_ptr<Wrapped, Deleter>&&>
251+
{
252+
// const xvalue (not valid, can't be moved from)
253+
// no members
254+
};
255+
256+
// The overload for functors that return std::unique_ptr<T>
257+
template<class Fn, class... Args>
258+
auto adapt_context_to_expected(Fn functor, Args&&... args)
259+
-> ExpectedL<
260+
typename AdaptContextDetectUniquePtr<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::type>
261+
{
262+
using ReturnType = ExpectedL<
263+
typename AdaptContextDetectUniquePtr<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>>::type>;
264+
BufferedDiagnosticContext bdc;
265+
decltype(auto) maybe_result = functor(bdc, std::forward<Args>(args)...);
266+
if (maybe_result)
267+
{
268+
return ReturnType{static_cast<decltype(maybe_result)&&>(maybe_result), expected_left_tag};
269+
}
270+
271+
return ReturnType{LocalizedString::from_raw(bdc.to_string()), expected_right_tag};
272+
}
273+
}

include/vcpkg/base/parse.h

+2-3
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,12 @@
22

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

5+
#include <vcpkg/base/diagnostics.h>
56
#include <vcpkg/base/messages.h>
67
#include <vcpkg/base/optional.h>
78
#include <vcpkg/base/stringview.h>
89
#include <vcpkg/base/unicode.h>
910

10-
#include <vcpkg/textrowcol.h>
11-
1211
#include <string>
1312

1413
namespace vcpkg
@@ -44,7 +43,7 @@ namespace vcpkg
4443

4544
struct ParserBase
4645
{
47-
ParserBase(StringView text, Optional<StringView> origin, TextRowCol init_rowcol = {});
46+
ParserBase(StringView text, Optional<StringView> origin, TextRowCol init_rowcol);
4847

4948
static constexpr bool is_whitespace(char32_t ch) { return ch == ' ' || ch == '\t' || ch == '\r' || ch == '\n'; }
5049
static constexpr bool is_lower_alpha(char32_t ch) { return ch >= 'a' && ch <= 'z'; }

include/vcpkg/paragraphparser.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,13 @@
22

33
#include <vcpkg/fwd/paragraphparser.h>
44

5+
#include <vcpkg/base/diagnostics.h>
56
#include <vcpkg/base/expected.h>
67
#include <vcpkg/base/messages.h>
78
#include <vcpkg/base/optional.h>
89
#include <vcpkg/base/stringview.h>
910

1011
#include <vcpkg/packagespec.h>
11-
#include <vcpkg/textrowcol.h>
1212

1313
#include <map>
1414
#include <memory>

include/vcpkg/textrowcol.h

-17
This file was deleted.

src/vcpkg-test/manifests.cpp

+6-7
Original file line numberDiff line numberDiff line change
@@ -1326,29 +1326,28 @@ TEST_CASE ("license error messages", "[manifests][license]")
13261326
ParseMessages messages;
13271327
parse_spdx_license_expression("", messages);
13281328
CHECK(messages.error.value_or_exit(VCPKG_LINE_INFO) ==
1329-
LocalizedString::from_raw(R"(<license string>:1:1: error: SPDX license expression was empty.
1329+
LocalizedString::from_raw(R"(<license string>: error: SPDX license expression was empty.
13301330
on expression:
13311331
^)"));
13321332

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

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

13481348
parse_spdx_license_expression("MIT AND", messages);
1349-
CHECK(
1350-
messages.error.value_or_exit(VCPKG_LINE_INFO) ==
1351-
LocalizedString::from_raw(R"(<license string>:1:8: error: Expected a license name, found the end of the string.
1349+
CHECK(messages.error.value_or_exit(VCPKG_LINE_INFO) ==
1350+
LocalizedString::from_raw(R"(<license string>: error: Expected a license name, found the end of the string.
13521351
on expression: MIT AND
13531352
^)"));
13541353

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

0 commit comments

Comments
 (0)