Introduce DiagnosticContext, an 'error document' like type. #1323

BillyONeal · 2024-01-10T06:04:48Z

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:

    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.

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

Applying this to the above gives:

    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 ( Make individual print calls thread safe #1290 ) or complex enough that we are not sure they are correct by trying to recover boundaries by reparsing our own error output ( Binary cache: async push_success #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.

Unblocks:

Install files in parallel #1256 (or at least work on it)
Rework and parallelize abi hashing #1225 (though I am still not convinced that this change should happen as 'rework' needs serious justification not present in the PR)
Teach x-ci-verify-versions to check that versions exist in the database #1210 (resolves code review comment about hating multiple return channels for errors and warnings)
Binary cache: async push_success #908

include/vcpkg/base/diagnostics.h

include/vcpkg/base/parse.h

src/vcpkg-test/ci-baseline.cpp

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

…erence-collapsing.

BillyONeal · 2024-01-27T00:03:12Z

@autoantwort What do you think of this direction?

autoantwort · 2024-01-27T08:16:30Z

LGTM.

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

I don't know if this can get annoying if you have to declare a MessageContext object before each function call (Don't know if this would happen in real code).
If this gets annoying maybe something like OptionalWithMessages<T> example_api(int a); could work (the message part is always there even if there is an object)

autoantwort · 2024-01-27T08:19:06Z

Ah MessageContext/DiagnosticContext is like a MessageSink and would directly print to stdout instead of ExpectedL where the caller has to do that.

BillyONeal · 2024-01-29T20:13:32Z

Ah MessageContext/DiagnosticContext is like a MessageSink and would directly print to stdout instead of ExpectedL where the caller has to do that.

That's right, it's like MessageSink but the granularity is 'whole diagnostic' rather than 'individual prints' meaning it works for the synchronization boundary several PRs are looking for here.

ras0219-msft · 2024-02-14T19:18:13Z

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.

I think this proposal could still handle this well by viewing the DiagnosticContext as the localized, user-facing content and then using an ExpectedT<T, std::error_code> return (or other such machine-relevant error type).

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.

I think this interface could handle this via layering. Just as some top of mind thoughts:

Optional<int> foo(Ctx& ctx) {
   WarningsAsErrorsCtx inner_ctx(ctx);
   auto maybe_bar = bar(inner_ctx, 123);
   if (auto b = maybe_bar.get()) { return *b + 1; }
   return nullopt;
}

Or, when alternatives should be attempted:

Optional<int> foo(Ctx& ctx) {
   BufferCtx buffer_ctx(ctx);
   auto maybe_bar = bar(buffer_ctx, 123);
   if (auto b = maybe_bar.get()) { return *b + 1; }
   else if (/* something else */) {
     /* something else worked, drop the messages */
     buffer_ctx.drop();
     /* or perhaps convert them to warnings? */
     buffer_ctx.errors_to_warnings();
     return 5;
   }
   /* something else failed; buffer_ctx will release messages "normally" */
   return nullopt;
}

ras0219-msft · 2024-02-14T19:07:12Z

include/vcpkg/base/diagnostics.h

+        Optional<std::string> m_origin;
+        TextRowCol m_position;


Would it make sense to bundle these (origin + position) into a single Optional? Alternatively: it's a bit unclear to me what the intended states are.

No origin, No position

Empty-string origin, No Position

Origin, No Position

Empty-string origin, Position

Origin, Position

Empty string origin is never intended. Position without origin is never intended either. I'll look to clarify.

ras0219-msft · 2024-02-14T19:08:26Z

include/vcpkg/base/diagnostics.h

+
+    struct DiagnosticLine
+    {
+        template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0>


Since these constructors all have different arities, is this SFINAE needed?

It's needed to make is_constructible_v<DiagnosticLine, DiagKind, int> be false.

(Perfect forwarding is psycho greedy so it basically always needs SFINAE)

What's the concrete consequence of is_constructible_v being true in this case? Issues with wrapping this type inside Expected<>?

Conversely, what value/perf is provided by making this a template versus just taking a LocalizedString?

ras0219-msft · 2024-02-14T19:25:53Z

include/vcpkg/base/diagnostics.h

+        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};


Should this handle Optional<XYZ>?

Would this more easily be implemented by using function overloads instead of a trait?

decltype(auto) maybe_result = functor(bdc, std::forward<Args>(args)...); return adapt_result_to_expected(static_cast<decltype(maybe_result)&&>(maybe_result), std::move(bdc)); }

That's handled by the overload on 179.

I don't understand this suggestion. The code block is what this is already doing? The trait is fixing up that the return type of the expected needs to do different things depending on whether the functor returns an lvalue or not.

Function Returns

unique_ptr<T> func() (SFINAE)

Optional<T> func(DiagnosticContext&) (SFINAE, that goes to the other overload)

unique_ptr<T> func(DiagnosticContext&) ExpectedL<unique_ptr<T>>

unique_ptr<T>& func(DiagnosticContext&) ExpectedL<unique_ptr<T>&>

unique_ptr<T>&& func(DiagnosticContext&) ExpectedL<unique_ptr<T>>

Just trying to do ExpectedL<std::invoke_result_t<Fn, BufferedDiagnosticContext&, Args...>> does the wrong thing because it doesn't SFINAE, and will try to form ExpectedL of an xvalue (ExpectedL<unique_ptr<T>&&>)

See test cases: https://github.com/microsoft/vcpkg-tool/pull/1323/files/ddb26ce3f6754c9b569fb32efc7efea63b2ea252#diff-e7cad277e9dea741cb9f0123b2a89306cefbfc47abf27fc5823e101190540075R454-R485

(I would add a comment to this effect but this table is already what the comments on 199, 207, 221, etc. are already saying)

Ah, I think this was inadequate reading on my part. I thought the computation was limited to the ReturnType alias and these lines and so thought we could avoid naming it entirely if we went through a function call (using the overload resolution to calculate ReturnType). However, I now see that the calculation is used both in ReturnType... and the return type :).

ras0219-msft · 2024-02-14T19:36:29Z

include/vcpkg/base/diagnostics.h

+        }
+
+        template<class MessageLike, std::enable_if_t<std::is_convertible_v<MessageLike, LocalizedString>, int> = 0>
+        DiagnosticLine(DiagKind kind, StringView origin, TextRowCol position, MessageLike&& message)


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:

I know this DiagnosticLine is attached to this specific source position

I know this DiagnosticLine is not attached to any source position

I want to inherit the contextual source position

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!
[...]
/* Now I can start talking about _what I actually care about_ */

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:

A DiagnosticContext&

The origin

Tracks a cursor of where the currently being parsed position is

and today the type trying to do that is ParserBase. I'm not a fan of how ParserBase wants and encourages deriving from and making things members when I think a struct containing these 3 members being passed around is a more composable design. I didn't go all the way there here though because

That starts to approach the design where callers are expected to provide additional wrapping context I asked for but you have said over time IRL that you do not like, and

That would require making this PR bigger since it would change more about how ParserBase is used.

Seeing that ParserBase tracks state incorrectly since it accepted a column but set m_start_of_line(m_it) (so the only valid col was 0 or 1) I've become convinced that 2+3 should go together but 1 should not.

ras0219-msft · 2024-02-14T19:46:22Z

include/vcpkg/packagespec.h

@@ -149,6 +150,7 @@ namespace vcpkg

    Optional<std::string> parse_feature_name(ParserBase& parser);
    Optional<std::string> parse_package_name(ParserBase& parser);
+    Optional<ParsedQualifiedSpecifier> parse_qualified_specifier_context(DiagnosticContext& context, StringView input);


I don't think decorating the functions with _context is necessary (and is actively harmful) if this is intended to be a persistent convention. The arity is different than all other overloads, so no worries there.

Suggested change

Optional<ParsedQualifiedSpecifier> parse_qualified_specifier_context(DiagnosticContext& context, StringView input);

Optional<ParsedQualifiedSpecifier> parse_qualified_specifier(DiagnosticContext& context, StringView input);

ras0219-msft · 2024-02-14T19:50:23Z

include/vcpkg/paragraphparser.h

@@ -40,10 +40,19 @@ namespace vcpkg
        std::vector<LocalizedString> errors;
    };

+    Optional<std::vector<std::string>> parse_default_features_list_context(DiagnosticContext& context,
+                                                                           const std::string& str,
+                                                                           StringView origin = "<unknown>",


I think as part of this effort I'd like to see this "<unknown>" source get standardized and localized. For example, maybe this should be represented by nonzero position + empty string origin. Then, the at the rendering layer, all empty-string origins can be replaced by a consistent (localized!) "unknown".

I have a different take: <unknown> must not exist in the first place, and anywhere it can happen is a bug. But this PR is big enough without trying to solve for that problem...

<unknown> must not exist in the first place

And now it doesn't #1346

ras0219-msft · 2024-02-14T19:54:08Z

src/vcpkg-test/cgroup-parser.cpp

@@ -72,7 +72,7 @@ TEST_CASE ("parse proc/pid/stat file", "[cgroup-parser]")
        std::string contents =
            R"(4281 (cpptools-srv) S 4099 1676 1676 0 -1 1077936384 51165 303 472 0 81 25 0 0 20 0 10 0 829158 4924583936 39830 18446744073709551615 4194304 14147733 140725993620736 0 0 0 0 16781312 16386 0 0 0 17 1 0 0 5 0 0 16247120 16519160 29999104 140725993622792 140725993622920 140725993622920 140725993627556 0)";

-        auto maybe_stat = try_parse_process_stat_file({contents, "test"});
+        auto maybe_stat = try_parse_process_stat_file(console_diagnostic_context, {contents, "test"});


I believe it should be possible to create a context emitter that interoperates with Catch2 to only display messages on failure.

(I know there's an INFO() macro, but I believe it's stack based)

I don't know of a good way of doing this. The problem with INFO() et al. is that they destroy contextual information about why something happened.

I could see changing these to null_diagnostic_context if you wish?

ras0219-msft · 2024-02-14T19:55:26Z

src/vcpkg-test/manifests.cpp

+    BufferedDiagnosticContext bdc;
+    auto result = parse_spdx_license_expression(bdc, license).has_value();
+    auto any_errors =
+        Util::any_of(bdc.lines, [](const DiagnosticLine& line) { return line.kind() == DiagKind::Error; });


This seems common enough to warrant a member function.

ras0219-msft · 2024-02-14T20:01:26Z

src/vcpkg/base/diagnostics.cpp

+            &Empty, &MessagePrefix, &ErrorPrefix, &WarningPrefix, &NotePrefix};
+
+        const auto diag_index = static_cast<unsigned int>(kind);
+        if (diag_index >= static_cast<unsigned int>(DiagKind::COUNT))


Suggested change

if (diag_index >= static_cast<unsigned int>(DiagKind::COUNT))

if (diag_index >= std::size(prefixes))

(and or the static assert)

ras0219-msft · 2024-02-14T20:08:19Z

src/vcpkg/base/diagnostics.cpp

+        std::mutex mtx;
+        virtual void report(const DiagnosticLine& line) override
+        {
+            std::lock_guard<std::mutex> lck(mtx);


I think that if we are going to do synchronization here, this should be a batch interface.

However, I'm also not totally sure synchronization is appropriate at this layer. This synchronization implies that it's usually correct to interleave lines coming from multiple threads. I suspect that's almost never correct, which means additional synchronization must be added above this, making this pure overhead.

I'd honestly be more in favor of some system which crashes the process in debug mode if you try to call this function concurrently.

I think that if we are going to do synchronization here, this should be a batch interface.

There is no reason a batch interface could not be added. I did not add one because this PR was already big enough, and the addition of such a member doesn't change the structure of that already here. None of the APIs here desired a batch interface, so I didn't add one.

This synchronization implies that it's usually correct to interleave lines coming from multiple threads. I suspect that's almost never correct

I believe it is almost always correct. (In fact, this belief is half the reason to open this can of worms in the first place, and that nothing here needed one added argues for this)

It was correct for all users of ParserBase herein.

I'd honestly be more in favor of some system which crashes the process in debug mode if you try to call this function concurrently.

Debug mode functionally never happens for us, so that would be insufficient. I'm also unaware of practical ways to write an assert like that that would be better than an uncontended lock acquisition. You'd need something like a CAS to say "this thread is touching the thing", and a store afterwards to say that it is no longer touching the thing, which is exactly what an uncontended mutex lock/unlock is.

BillyONeal · 2024-02-17T03:28:29Z

That's right, it's like MessageSink but the granularity is 'whole diagnostic' rather than 'individual prints' meaning it works for the synchronization boundary several PRs are looking for here.

After in person discussion with @ras0219-msft , he is unhappy with binary caching or similar displaying anything while port builds are happening, so this isn't as natural a synchronization barrier as I thought. I still think this will be helpful in implementing what those other PRs wanted to do but it won't do it entirely.

# Conflicts: # include/vcpkg/base/parse.h # include/vcpkg/paragraphparser.h # src/vcpkg/base/parse.cpp # src/vcpkg/packagespec.cpp # src/vcpkg/paragraphs.cpp # src/vcpkg/sourceparagraph.cpp

# Conflicts: # src/vcpkg/binarycaching.cpp # src/vcpkg/commands.package-info.cpp

The 'vcpkg install' blobs at the end aren't helpful. ```console PS D:\vcpkg-tool\out\build\Win-x64-Debug-WithArtifacts> .\vcpkg.exe install this-is-super-not-a-port#port error: expected eof on expression: this-is-super-not-a-port#port ^ vcpkg install <port name> <port name>... vcpkg install zlib zlib:x64-windows curl boost ```

# Conflicts: # azure-pipelines/end-to-end-tests-dir/cli.ps1 # include/vcpkg/base/parse.h # include/vcpkg/input.h # include/vcpkg/packagespec.h # include/vcpkg/paragraphparser.h # include/vcpkg/sourceparagraph.h # src/vcpkg/base/parse.cpp # src/vcpkg/binarycaching.cpp # src/vcpkg/binaryparagraph.cpp # src/vcpkg/commands.build-external.cpp # src/vcpkg/commands.build.cpp # src/vcpkg/commands.check-support.cpp # src/vcpkg/commands.depend-info.cpp # src/vcpkg/commands.export.cpp # src/vcpkg/commands.install.cpp # src/vcpkg/commands.package-info.cpp # src/vcpkg/commands.remove.cpp # src/vcpkg/commands.set-installed.cpp # src/vcpkg/commands.upgrade.cpp # src/vcpkg/input.cpp # src/vcpkg/packagespec.cpp # src/vcpkg/paragraphs.cpp # src/vcpkg/sourceparagraph.cpp

autoantwort · 2024-04-20T18:05:34Z

After in person discussion with @ras0219-msft , he is unhappy with binary caching or similar displaying anything while port builds are happening, so this isn't as natural a synchronization barrier as I thought. I still think this will be helpful in implementing what those other PRs wanted to do but it won't do it entirely.

I thought this was the reason for this PR. Because of robert's concerns BGMessageSink was introduced to prevent interleaved output. But we don't know when a message begins and ends with MessageSinks which would be resolved by the "error document" wrapping the message in an object

BillyONeal · 2024-04-22T21:19:48Z

I thought this was the reason for this PR. Because of robert's concerns BGMessageSink was introduced to prevent interleaved output. But we don't know when a message begins and ends with MessageSinks which would be resolved by the "error document" wrapping the message in an object

I think that's still true. The effect of the discussion there was that implementers of the interface
don't need to make it safe to submit from arbitrary threads. In your case, a context gets made for the BG thread and only the BG thread ever writes to it, so the interface doesn't need to allow interleaved access from threads.

…eculative parsing. Note that TextRowCol was downgraded to int because ParserBase::ParserBase assumes start_of_line == the content.

Thomas1664 · 2024-04-28T23:11:04Z

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 ( Make individual print calls thread safe #1290 )

@BillyONeal I don't think the approach in #1290 is wrong because that is exactly what's needed. Without that PR, you can't even print single messages from multiple threads. We need both PRs!

If I understand this PR correctly, the approach here is to collect all errors and print them when the operation is finished. I think this is more complicated than it needs to be for simple cases like #1256.

autoantwort

I thing we need documentation when one should use msg::println_error, MessageSink::println_error or DiagnosticContext::report_error

autoantwort · 2024-04-30T13:50:32Z

include/vcpkg/base/diagnostics.h

+        std::vector<DiagnosticLine> lines;
+
+        // Prints all diagnostics to the terminal.
+        void print(MessageSink& sink) const;


Suggested change

void print(MessageSink& sink) const;

void print_to(MessageSink& sink) const;

or print_diagnostics_to makes it more clear what this does.

autoantwort · 2024-04-30T13:54:07Z

src/vcpkg/base/diagnostics.cpp

+
+        buf.append(m_message.data());
+        buf.push_back('\n');
+        sink.print(Color::none, buf);


Is there a specific reason why only the error is printed in red and not the whole error message as it is now?

We've gotten feedback in the past that users find our error messages annoying and/or difficult to read because their terminal is bleeding. Coloring only the 'error' or 'warning' part is consistent with what compilers do:

autoantwort · 2024-04-30T13:57:13Z

include/vcpkg/base/diagnostics.h

+        int column = 0;
+    };
+
+    struct DiagnosticLine


Maybe DiagnosticMessage or simple Message? Since it is allowed that the diagnostic line contains multiple lines

The word 'message' is heavily used already by the localization infrastructure and I didn't want to step on such names.

BillyONeal · 2024-05-01T01:10:14Z

I thing we need documentation when one should use msg::println_error, MessageSink::println_error or DiagnosticContext::report_error

msg:: vs DiagnosticContext: Unfortunately I think that one will remain at 'it depends' for a long time. Ideally over time I expect more things will choose the latter but we don't want to set a standard requiring people to upgrade swaths of code in order to do anything.

MessageSink -> DiagnosticContext SoonTM

BillyONeal added the depends:different-pr This PR depends on a different PR which has been filed label Jan 10, 2024

BillyONeal commented Jan 10, 2024

View reviewed changes

include/vcpkg/base/diagnostics.h Outdated Show resolved Hide resolved

BillyONeal commented Jan 10, 2024

View reviewed changes

include/vcpkg/base/diagnostics.h Outdated Show resolved Hide resolved

BillyONeal commented Jan 10, 2024

View reviewed changes

include/vcpkg/base/parse.h Outdated Show resolved Hide resolved

BillyONeal commented Jan 10, 2024

View reviewed changes

src/vcpkg-test/ci-baseline.cpp Outdated Show resolved Hide resolved

BillyONeal marked this pull request as draft January 10, 2024 19:59

BillyONeal removed the depends:different-pr This PR depends on a different PR which has been filed label Jan 26, 2024

BillyONeal added 11 commits January 26, 2024 13:47

Add color printing.

0223881

Add unique_ptr handling.

94722bb

Forward in more places.

ba58259

More tests and bugfixes.

bbd39f2

More testing!

1549d60

Tests, bugfixes, comments. Simplify the unique_ptr adapter to use ref…

164a8dc

…erence-collapsing.

First crack at applying DiagnosticContext to ParserBase.

fbcf1e7

Fix unit test failures.

9396f8a

Revert the TextRowCol rename

6e3f487

Minor self CR feedback.

9fdf0e2

BillyONeal force-pushed the error-document branch from ca6d48d to 9fdf0e2 Compare January 26, 2024 21:54

BillyONeal added 3 commits January 26, 2024 13:56

Remove unused WarningsTreatedAsErrors

04c46ec

Avoid -Wunneeded-internal-declaration

e0287e6

Avoid -Wmissing-prototypes

ddb26ce

BillyONeal marked this pull request as ready for review January 26, 2024 23:50

BillyONeal requested a review from ras0219-msft January 27, 2024 00:02

ras0219-msft reviewed Feb 14, 2024

View reviewed changes

BillyONeal added 2 commits February 16, 2024 20:34

"<unknown>" is not an origin

58eb07e

Merge branch 'no-unknown' into error-document

0d3005b

# Conflicts: # include/vcpkg/base/parse.h # include/vcpkg/paragraphparser.h # src/vcpkg/base/parse.cpp # src/vcpkg/packagespec.cpp # src/vcpkg/paragraphs.cpp # src/vcpkg/sourceparagraph.cpp

BillyONeal marked this pull request as draft February 17, 2024 05:15

BillyONeal added 10 commits February 20, 2024 18:16

Merge remote-tracking branch 'origin/main' into no-unknown

bc2ac20

<command> is not an origin either.

bd20e7a

Merge branch 'no-unknown' into error-document

a02e4d4

# Conflicts: # src/vcpkg/binarycaching.cpp # src/vcpkg/commands.package-info.cpp

Add more comments for adapt_context_to_expected.

9c22b23

Drop the _context convention.

8d49cf0

Merge branch 'no-unknown' into error-document

2c43b40

Block empty origins.

b49ad4f

Remove implication that report() is locked.

0a92366

Other code review fixes.

9a11f96

BillyONeal marked this pull request as ready for review February 22, 2024 03:08

BillyONeal requested a review from ras0219-msft February 22, 2024 03:08

BillyONeal added 2 commits February 21, 2024 19:34

clang-format

de35a73

BillyONeal force-pushed the error-document branch from f69db32 to b28aadc Compare April 20, 2024 02:16

BillyONeal added 5 commits April 22, 2024 14:32

Merge remote-tracking branch 'origin/main' into error-document

24ac5bf

Correctly track row information and avoid emitting diagnostics for sp…

f7c13ab

…eculative parsing. Note that TextRowCol was downgraded to int because ParserBase::ParserBase assumes start_of_line == the content.

Dedupe CLI e2e tests.

87a6c0e

Fix posix build

1616bbd

Downgrade clang-format to 16

3159407

autoantwort mentioned this pull request Apr 30, 2024

Make individual print calls thread safe #1290

Open

autoantwort reviewed Apr 30, 2024

View reviewed changes

Function	Returns
`unique_ptr<T> func()`	(SFINAE)
`Optional<T> func(DiagnosticContext&)`	(SFINAE, that goes to the other overload)
`unique_ptr<T> func(DiagnosticContext&)`	`ExpectedL<unique_ptr<T>>`
`unique_ptr<T>& func(DiagnosticContext&)`	`ExpectedL<unique_ptr<T>&>`
`unique_ptr<T>&& func(DiagnosticContext&)`	`ExpectedL<unique_ptr<T>>`

	Optional<ParsedQualifiedSpecifier> parse_qualified_specifier_context(DiagnosticContext& context, StringView input);
	Optional<ParsedQualifiedSpecifier> parse_qualified_specifier(DiagnosticContext& context, StringView input);

	if (diag_index >= static_cast<unsigned int>(DiagKind::COUNT))
	if (diag_index >= std::size(prefixes))

	void print(MessageSink& sink) const;
	void print_to(MessageSink& sink) const;

Introduce DiagnosticContext, an 'error document' like type. #1323

Are you sure you want to change the base?

Introduce DiagnosticContext, an 'error document' like type. #1323

Conversation

BillyONeal commented Jan 10, 2024 • edited

BillyONeal commented Jan 27, 2024

autoantwort commented Jan 27, 2024

autoantwort commented Jan 27, 2024

BillyONeal commented Jan 29, 2024

ras0219-msft commented Feb 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ras0219-msft May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal Feb 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Feb 17, 2024

autoantwort commented Apr 20, 2024

BillyONeal commented Apr 22, 2024

Thomas1664 commented Apr 28, 2024

autoantwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented May 1, 2024 • edited

BillyONeal commented Jan 10, 2024 •

edited

ras0219-msft commented Feb 14, 2024 •

edited

ras0219-msft May 1, 2024 •

edited

BillyONeal Feb 15, 2024 •

edited

BillyONeal commented May 1, 2024 •

edited