Resolve several ExpectedT<T> problems #511

BillyONeal · 2022-04-16T04:12:33Z

While forming my response to #483 I ran into several problems trying to use ExpectedT, so I separated out these specific changes which are intended to affect only that type:

Delete the "ErrorHolder" metaprogramming from Expected<T>. That performed 2 functions: 1. avoided storing an extra bool for ExpectedT<..., std::error_code>, and 2. giving ExpectedT a way to print the error case on value_or_exit. 1. was completely unused: there are no uses of ExpectedT<..., std::error_code> in the codebase. 2. should use the same stuff we use to print everything else (the to_string protocol), which this change does.
Make it possible to form ExpectedT<..., not-default-constructible>. This allows callers to avoid creating ExpectedT<..., unique_ptr<...>> as a workaround, as they are doing in some places. Fixing callers to avoid doing that is not in this change though (it is big enough).
Callers of ExpectedT that fell into the default "value was error" were fixed to print the actual error.
A use after free bug in our unit tests where the code wanted C++ references to be assignable was fixed ( https://twitter.com/MalwareMinigun/status/1515171930705317889 )

…d E = std::error_code.

Also cleans up a use-after-free in several platform expression tests where the original author wished references were rebindable.

include/vcpkg/base/expected.h

BillyONeal · 2022-04-22T21:51:29Z

Depends on #520

include/vcpkg/base/expected.h

BillyONeal · 2022-04-24T13:36:42Z

Depends on #521

# Conflicts: # include/vcpkg/paragraphparser.h # locales/messages.en.json # locales/messages.json # src/vcpkg/sourceparagraph.cpp

…expected # Conflicts: # include/vcpkg/base/format.h # include/vcpkg/base/parse.h # include/vcpkg/paragraphparser.h # locales/messages.en.json # locales/messages.json # src/vcpkg-test/json.cpp # src/vcpkg/paragraphs.cpp # src/vcpkg/sourceparagraph.cpp

…rrors from expected.

Currently, Expected<T> has a bug for references, in that it is default constructible, but references shouldn't be default constructible. (This is hidden because we store a pointer internally). In VersionedPackageGraph::require_port_version, an ExpectedS<T&> is default constructed, which is in the "impossible" state of neither having a stored T (since references can't be default constructed), but also not being assigned an error state. This change hoists the only place where the ExpectedS could contain an error into an early-return, allowing a plain pointer to be used, and avoiding constructing the impossible thing. (The bug in Expected that allowed this to happen is fixed in microsoft#511 , but this change was tricky enough I thought extracting it for review alone made sense)

Currently, Expected<T> has a bug for references, in that it is default constructible, but references shouldn't be default constructible. (This is hidden because we store a pointer internally). In VersionedPackageGraph::require_port_version, an ExpectedS<T&> is default constructed, which is in the "impossible" state of neither having a stored T (since references can't be default constructed), but also not being assigned an error state. This change hoists the only place where the ExpectedS could contain an error into an early-return, allowing a plain pointer to be used, and avoiding constructing the impossible thing. (The bug in Expected that allowed this to happen is fixed in microsoft#511 , but this change was tricky enough I thought extracting it for review alone made sense) Reasoning, using "after" line numbers: 1557-1567: This if block could never have stored an error state in the expected, since it is only entered if there is an overlay control file that will be the result. Therefore, we will never add an error to m_errors from here. 1578: get_control_file can return an expected in the error state, so we have to check if it's an error and, if so, add it to m_errors. 1587: At this point, we know p_scfl is not nullptr since both if branches set it on 1556 or 1578.

BillyONeal · 2022-05-25T01:41:37Z

Depends on #557

* Don't attempt to default-construct a reference. Currently, Expected<T> has a bug for references, in that it is default constructible, but references shouldn't be default constructible. (This is hidden because we store a pointer internally). In VersionedPackageGraph::require_port_version, an ExpectedS<T&> is default constructed, which is in the "impossible" state of neither having a stored T (since references can't be default constructed), but also not being assigned an error state. This change hoists the only place where the ExpectedS could contain an error into an early-return, allowing a plain pointer to be used, and avoiding constructing the impossible thing. (The bug in Expected that allowed this to happen is fixed in #511 , but this change was tricky enough I thought extracting it for review alone made sense) Reasoning, using "after" line numbers: 1557-1567: This if block could never have stored an error state in the expected, since it is only entered if there is an overlay control file that will be the result. Therefore, we will never add an error to m_errors from here. 1578: get_control_file can return an expected in the error state, so we have to check if it's an error and, if so, add it to m_errors. 1587: At this point, we know p_scfl is not nullptr since both if branches set it on 1556 or 1578.

# Conflicts: # include/vcpkg/base/messages.h # include/vcpkg/base/parse.h # include/vcpkg/paragraphparser.h # locales/messages.en.json # locales/messages.json # src/vcpkg-test/json.cpp # src/vcpkg/base/git.cpp # src/vcpkg/dependencies.cpp # src/vcpkg/paragraphs.cpp

…ignable.

BillyONeal · 2022-05-26T00:03:05Z

include/vcpkg/base/expected.h

+        {
+            if (!value_is_error)
+            {
+                Checks::unreachable(VCPKG_LINE_INFO);


In a future change I would like to rename value_or_exit to just value, and add a line_info parameter to error(), but that change is so mechanical and cross-cutting that I want it to be its own review.

include/vcpkg/base/expected.h

ras0219-msft

Broadly LGTM, but going to re-review in the morning.

…the metaprogramming.

include/vcpkg/base/expected.h

ras0219-msft · 2022-05-26T15:49:43Z

include/vcpkg/base/expected.h

-        ExpectedT(const T& t, ExpectedLeftTag = {}) : m_t(t) { }
-        template<class U = T, std::enable_if_t<!std::is_reference<U>::value, int> = 0>
-        ExpectedT(T&& t, ExpectedLeftTag = {}) : m_t(std::move(t))
+        template<class ConvToError, std::enable_if_t<std::is_convertible_v<ConvToError, Error>, int> = 0>


Suggested change

template<class ConvToError, std::enable_if_t<std::is_convertible_v<ConvToError, Error>, int> = 0>

template<class ConvToError>

ditto above

include/vcpkg/base/expected.h

ras0219-msft · 2022-05-26T16:14:49Z

include/vcpkg/base/expected.h

        template<class F, class... Args>
-        std::invoke_result_t<F, const_ref_type, Args&&...> then(F f, Args&&... args) const&
+        std::invoke_result_t<F, const T&, Args...> then(F f, Args&&... args) const&


Why is Args&& not required?

Because both Args and Args&& name rvalues.

That is, given:

int returns_prvalue(); int&& returns_xvalue(); int& returns_lvalue(); template<class Arg> void consume(Arg&&);

consume can't tell the difference between consume(returns_prvalue()) and consume(returns_xvalue()), since forwarding references only sense lvalue-or-rvalueness. (In both cases, Arg would deduce to int, giving void consume<int>(int&&). (and consume(returns_lvalue()) would deduce int& giving void consume<int&>(int&)))

Convention when implementing perfect forwarders like this is to not add the meaningless &&s when passing types through, for example http://eel.is/c++draft/func.invoke

See also http://eel.is/c++draft/fig:basic.lval

ras0219-msft · 2022-05-26T16:19:07Z

include/vcpkg/base/expected.h

@@ -105,191 +26,332 @@ namespace vcpkg
    template<class T>
    struct ExpectedHolder


Since you're removing ErrorHolder, would it be "better" to change this to a traits struct as well? e.g.

union { Error m_err; typename ExpectedHolder<T>::type m_t; }; auto get() const noexcept { return ExpectedHolder<T>::get(m_t); }

I did strongly consider doing that, but it didn't seem to reduce much code because it also affects every special member function to need to conditionally insert the T& -> T* transformation.

I think it would be worth doing if we can use if constexpr but I have been paranoid about adding use of standard features that we don't already use for fear of damaging compiler support. But between when I wrote this and now we have affirmed that we only care about our test compilers so maybe I should go ahead and do that?

src/vcpkg-test/platform-expression.cpp

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

* Introduce Expected<void>. * Delete unwrap, which was identical to value_or_exit but lost the VCPKG_LINE_INFO. * Also apply #511 (comment) more completely. * Do expected<Unit> rather than expected<void> * Also remove forward decl.

* Introduce Expected<void>. * Delete unwrap, which was identical to value_or_exit but lost the VCPKG_LINE_INFO. * Also apply #511 (comment) more completely. * Introduce a better type to store system API call failures. * Apply ExpectedApi to windows_create_process. * Apply ExpectedApi to windows_create_windowless_process. * Apply ExpectedApi to windows_create_process_redirect. * Apply ExpectedApi to cmd_execute_and_stream_data. * Apply ExpectedApi to cmd_execute_and_stream_lines. * Apply ExpectedApi to cmd_execute_and_capture_output. * Apply ExpectedApi to cmd_execute. * Apply ExpectedApi to cmd_execute_clean. * Fix MacOS builds and the FIXME in archives.cpp of compress_directory_to_zip * Regenerate messages. * Do expected<Unit> rather than expected<void> * Also remove forward decl. * Fix linux build. * Eliminate SystemApiError. * Delete unnecessary .has_value() calls, and fix unused variable warning. * Also fix POSIX build errors. * Make flatten_out move the input.

BillyONeal added 3 commits April 15, 2022 19:04

Simplify expected by deleting ErrorHolder since we never actually use…

484b5fa

…d E = std::error_code.

Replace bespoke error-to-string functionality with fmt.

d4036ef

Allow not-default-constructible error types in expected.all_of

15af901

Also cleans up a use-after-free in several platform expression tests where the original author wished references were rebindable.

BillyONeal commented Apr 16, 2022

View reviewed changes

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

BillyONeal added 4 commits April 19, 2022 11:24

Merge remote-tracking branch 'origin/main' into cleanup-expected

767c2b4

Guard calls to error() for the expected being in the error state.

9543cca

Merge remote-tracking branch 'origin/main' into cleanup-expected

144f523

Attempt to fix build errors on POSIX

b5a5c33

BillyONeal marked this pull request as draft April 21, 2022 00:53

BillyONeal added 5 commits April 21, 2022 18:42

Use v143 toolset for VS integration tests.

f5489d1

Provide end to end tests defaults.

317dd2b

More expected misuse.

bdc33e6

Fix missing value_is_error assignment.

468de50

:'(

7ef49eb

BillyONeal added the depends:different-pr This PR depends on a different PR which has been filed label Apr 22, 2022

Neumann-A reviewed Apr 22, 2022

View reviewed changes

include/vcpkg/base/expected.h Show resolved Hide resolved

Neumann-A reviewed Apr 22, 2022

View reviewed changes

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

Neumann-A reviewed Apr 22, 2022

View reviewed changes

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

BillyONeal removed the depends:different-pr This PR depends on a different PR which has been filed label Apr 22, 2022

BillyONeal added 4 commits April 22, 2022 15:57

Merge remote-tracking branch 'origin/main' into cleanup-expected

5a92c2a

Localize ParseControlErrorInfo.

4328064

Fix parameters not being forwarded by constructors, and add unit tests.

26ab178

Fix unreferenced formal parameter warning.

b4cdf4d

BillyONeal added the depends:different-pr This PR depends on a different PR which has been filed label Apr 24, 2022

BillyONeal added 4 commits April 24, 2022 13:09

Delete error_as_string.

4ed8458

More tests and cleanup map family.

58746f8

Merge remote-tracking branch 'origin/main' into HEAD

1d5fe22

# Conflicts: # include/vcpkg/paragraphparser.h # locales/messages.en.json # locales/messages.json # src/vcpkg/sourceparagraph.cpp

Fix fuzz tester build.

d1ed66b

BillyONeal added 2 commits May 24, 2022 18:01

Use the to_string protocol rather than the format protocol to print e…

8d74faf

…rrors from expected.

BillyONeal mentioned this pull request May 25, 2022

Don't attempt to default-construct a reference. #557

Merged

BillyONeal removed the depends:different-pr This PR depends on a different PR which has been filed label May 25, 2022

BillyONeal added 2 commits May 25, 2022 16:50

Revert no-longer-necessary test changes now that expected is move-ass…

79b5b65

…ignable.

BillyONeal force-pushed the cleanup-expected branch from 1b21422 to 79b5b65 Compare May 25, 2022 23:59

BillyONeal marked this pull request as ready for review May 26, 2022 00:00

BillyONeal commented May 26, 2022

View reviewed changes

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

ras0219-msft reviewed May 26, 2022

View reviewed changes

Rename S to Error and remove redundant !std::is_same_v<T, S> part of …

b8fafa5

…the metaprogramming.

BillyONeal force-pushed the cleanup-expected branch from b74e8de to b8fafa5 Compare May 26, 2022 06:38

ras0219-msft approved these changes May 26, 2022

View reviewed changes

BillyONeal and others added 3 commits May 26, 2022 11:03

Update src/vcpkg-test/platform-expression.cpp

3ad031c

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

Update include/vcpkg/base/expected.h

78b5fee

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

Make ExpectedHolder more paranoid about cvT

de7fa7d

BillyONeal merged commit dcc7041 into microsoft:main May 26, 2022

BillyONeal deleted the cleanup-expected branch May 26, 2022 21:33

BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request May 27, 2022

Also apply microsoft#511 (comment) more completely.

9c7f386

BillyONeal mentioned this pull request May 27, 2022

Introduce Expected<Unit>. #564

Merged

BillyONeal mentioned this pull request Oct 31, 2023

Apply conditional noexcept to Expected's constructors. #1251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve several ExpectedT<T> problems #511

Resolve several ExpectedT<T> problems #511

BillyONeal commented Apr 16, 2022 •

edited

Loading

BillyONeal commented Apr 22, 2022

BillyONeal commented Apr 24, 2022

BillyONeal commented May 25, 2022

BillyONeal May 26, 2022

ras0219-msft left a comment

ras0219-msft May 26, 2022

ras0219-msft May 26, 2022

BillyONeal May 26, 2022 •

edited

Loading

BillyONeal May 26, 2022

ras0219-msft May 26, 2022

BillyONeal May 26, 2022

	template<class ConvToError, std::enable_if_t<std::is_convertible_v<ConvToError, Error>, int> = 0>
	template<class ConvToError>

		@@ -105,191 +26,332 @@ namespace vcpkg
		template<class T>
		struct ExpectedHolder

Resolve several ExpectedT<T> problems #511

Resolve several ExpectedT<T> problems #511

Conversation

BillyONeal commented Apr 16, 2022 • edited Loading

BillyONeal commented Apr 22, 2022

BillyONeal commented Apr 24, 2022

BillyONeal commented May 25, 2022

BillyONeal May 26, 2022

Choose a reason for hiding this comment

ras0219-msft left a comment

Choose a reason for hiding this comment

ras0219-msft May 26, 2022

Choose a reason for hiding this comment

ras0219-msft May 26, 2022

Choose a reason for hiding this comment

BillyONeal May 26, 2022 • edited Loading

Choose a reason for hiding this comment

BillyONeal May 26, 2022

Choose a reason for hiding this comment

ras0219-msft May 26, 2022

Choose a reason for hiding this comment

BillyONeal May 26, 2022

Choose a reason for hiding this comment

BillyONeal commented Apr 16, 2022 •

edited

Loading

BillyONeal May 26, 2022 •

edited

Loading