In several PRs, such as microsoft#908 and microsoft#1210 , it has bec…

…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.

include/vcpkg/base/diagnostics.h

@@ -0,0 +1,158 @@
+#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
+    };
+
+    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};
+    }
+}

include/vcpkg/base/parse.h

@@ -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 <memory>
 #include <string>
 
@@ -75,7 +74,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'; }
@@ -131,7 +130,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(); }
 

include/vcpkg/paragraphparser.h

@@ -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>
@@ -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
     {
@@ -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;
 
@@ -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});
 }

include/vcpkg/sourceparagraph.h

@@ -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});
 }

src/vcpkg-test/messages.cpp

@@ -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>
@@ -99,3 +100,84 @@ 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);
+    }
+} // unnamed namespace
+
+TEST_CASE ("adapt DiagnosticContext to ExpectedL", "[diagnostics]")
+{
+    // adapt_context_to_expected(returns_int); // should not compile
+    // adapt_context_to_expected(returns_unique_ptr); // should not compile
+
+    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"));
+    }
+}

src/vcpkg-test/paragraph.cpp

@@ -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));
     }
@@ -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));
     }

src/vcpkg-test/util.cpp

@@ -63,7 +63,7 @@ namespace vcpkg::Test
             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));