Make a stand-in for `std::expected` available in the code base #3916

scottschurr · 2021-08-26T21:04:56Z

Make a Stand-in For `std::expected` Available in the Code Base

std::expected is a proposed type for the standard library. The current paper for std::expected is: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p0323r10.html

The interface of std::expected looks great to me. It's like a std::optional that tells you why something came back empty. I'd like to be able to use it in the code base without waiting for us to adopt C++23.

Context of Change

Boost does not directly provide an interface like std::expected. However Boost has something similar called boost::outcome_v2::result. I've coded a fairly thin wrapper around boost::outcome_v2::result that provides the characteristics of std::expected that I particularly like. I've named that wrapper ripple::Expected.

That wrapper, and unit tests for it, are in the first commit.

My initial impression was that Expected would be immediately usable in significant portions of the code base. So I integrated Expected into our code base. To my surprise I found relatively few places where I could directly drop in Expected as a replacement for returning a std::pair where one of the pair members was the reason for the failure. But I did find a few. You can find that integration in the second commit.

Reviewers should consider two things:

Given that Expected is not essential to the code base, should we still incorporate it? I think we should. But you may not agree
If it is worth incorporating Expected into the code base, do you see any coding errors or places for improvement?

For what it's worth, the std::expected paper is on track to go into C++23 and has passed the Library Evolution working group. But there's some risk because it has not yet survived the Library (wording) working group.

Type of Change

New feature (non-breaking change which adds functionality)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)

This is an internals-only change. It does not need to be mentioned in the release notes.

Before / After

From an interface perspective, an implementation returning an Expected changes something sorta like this ...

std::pair<std::optional<AcctRootRd>, NotTEC >
makeAcctRoot(std::shared_ptr<STLedgerEntry const> slePtr)
{
    if (NotTEC const ter = validateAcctRootSle(slePtr); !isTesSuccess(ter))
        return {nullopt, ter}

    return {AcctRootRd(std::move(slePtr), tesSUCCESS};
}

... to this ...

Expected<AcctRootRd, NotTEC>
makeAcctRoot(std::shared_ptr<STLedgerEntry const> slePtr)
{
    if (NotTEC const ter = validateAcctRootSle(slePtr); !isTesSuccess(ter))
        return Unexpected(ter);

    return AcctRootRd(std::move(slePtr));
}

At the call site this ...

    auto const acctRoot = makeAcctRoot(ctx.view.read(keylet::account(id)));
    if (!isTesSuccess(acctRoot.second))
        return acctRoot.second;

    std::uint32_t const uFlagsIn = {acctRoot.first->flags()};

... becomes this ...

    auto const acctRoot = makeAcctRoot(ctx.view.read(keylet::account(id)));
    if (!acctRoot)
        return acctRoot.error();

    std::uint32_t const uFlagsIn = {acctRoot->flags()};

The examples have not been run through a compiler, so there may be bugs.

Test Plan

Unit tests for Expected are included. The integration also builds confidence that the Expected wrapper is behaving as expected.

Future Tasks

Presuming std::expected makes it into C++23 it will make sense to remove ripple::Expected from the code base and replace its usage with std::expected. There shouldn't be significant usage changes to std::expected, since it has passed through Library Evolution. So the replacement should be straight forward at that time.

seelabs

I took a look at the wrapper and that looks great!

The code is simple - this isn't an overly complex wrapper over outcome.
Making this class "pointer-like" makes a lot of sense, and I do think that's a nice improvement over raw outcome.

Yeah, I think it's time to add a class like this and clean up some of the existing interfaces. I'm on board - expect a thumbs up from me.

I'm going to play with this code a little before I give an official thumbs up, but I don't expect I'll want any changes.

Great job on this!

seelabs

I think this looks great, but I'd suggest we don't change the interface to the checkSign functions in this PR. Other than that I think this is good to go.

seelabs · 2021-08-27T14:01:57Z

src/ripple/protocol/STTx.h

@@ -131,7 +132,7 @@ class STTx final : public STObject, public CountedObject<STTx>
        @return `true` if valid signature. If invalid, the error message string.
    */
    enum class RequireFullyCanonicalSig : bool { no, yes };
-    std::pair<bool, std::string>
+    Expected<bool, std::string>


I don't love this interface:

The "success" variant can only ever be true.

Types like Expected<bool,...> and optional<bool> have both an "outer bool" and an "inner bool", which make them error prone.

My suggestion is to punt on changing this interface in this PR. Let's focus on Expected itself, and use deserialize as a good test-case interface to change. While we should change this one too, I think I'd rather do it in a different PR.

I think I want an Expected<void,std::string> here. But of course the current Expected interface can't handle a void success type. It looks like the standard version does. Maybe we should add support for our wrapper?

Yeah, I think Expected<void, T> is a good solution to this interface. And, yes, it looks like std::expected supports this model. Thanks for the great suggestion!

A quick experiment suggests that a partial specialization of Expected should do the trick and add very little additional code. If that experiment continues to be successful I'll add a FOLD commit with those changes and some additional unit tests. If the experiment fails I'll report back the reason for the failure.

seelabs · 2021-08-27T14:02:09Z

src/ripple/protocol/STTx.h

@@ -150,10 +151,10 @@ class STTx final : public STObject, public CountedObject<STTx>
        std::string const& escapedMetaData) const;

 private:
-    std::pair<bool, std::string>
+    Expected<bool, std::string>


See comment above.

seelabs · 2021-08-27T14:02:24Z

src/ripple/protocol/STTx.h

    checkSingleSign(RequireFullyCanonicalSig requireCanonicalSig) const;

-    std::pair<bool, std::string>
+    Expected<bool, std::string>


See comment above.

HowardHinnant

This looks good to me. I am agnostic concerning what commit initial integration happens in.

I initially thought we should try to implement the proposed spec. But after looking into it I changed my mind.

The proposed spec isn't yet mature enough, and
The implementation is a bigger project than we want to take on and maintain.

scottschurr · 2021-08-30T16:02:09Z

I pushed a commit with a partial specialization for Expected<void, E>. To me it looks like it should serve the purpose. Let me know what you think. Thanks.

seelabs

Thanks for coding up the void specialization. I had one comment about removing the true_type constructor and replacing it with a default constructor. Other than that it looks great.

seelabs · 2021-08-30T17:56:36Z

src/ripple/basics/Expected.h

+    // We'd like to avoid a default constructor.  That means someone needs
+    // explicitly say they intent to return success.  Seems like
+    // std::true_type is a good way to do that...
+    constexpr Expected(std::true_type) : Base(boost::outcome_v2::success())


It looks like the proposed standard expected has a default constructor for a void return (ref: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0323r9.html#expected.object.ctor). I think a default constructor makes sense in this case, and given that the standard has it, I'd like to remove this constructor and replace it with a default constructor.

Thanks for catching the default constructor for Expected<void, E>. I looked for something like boost::outcome_v2::success() in the paper and didn't find it, so I punted. Matching the currently proposed version of the paper seems like the best approach. I've included an additional [FOLD] to make this change.

seelabs

👍 Great job on this!

I'm approving this now, but please see my comment about adding the assert. I'm approving now because however you want to resolve that is fine with me (including keeping it as-is).

seelabs · 2021-08-30T20:46:02Z

src/ripple/basics/base_uint.h

@@ -212,20 +213,16 @@ class base_uint
            return ParseResult::okay;
        };

-        std::pair<ParseResult, decltype(data_)> ret{ParseResult::okay, {}};
+        decltype(data_) out{};
+        std::size_t i = 0u;


Nit: Consider moving the var i decl to just before the while loop; I'd probably also call this variable ret instead of out (you could have an auto out = ret.data() if you wanted to keep something closer to the original code too), but also fine as-is.

seelabs · 2021-08-30T21:22:29Z

src/ripple/basics/base_uint.h


-        if (result.first == ParseResult::badChar)
+            assert(result.error() == ParseResult::badChar);


Why does badChar assert but badLength doesn't assert? Consider losing the assert. It's changing the behavior of debug builds, and the change doesn't involve the major focus of this PR - which is expected. I think it's better if PRs focus on one thing only.

Having said that, I only weakly object. If you feel strongly, then it's your call.

The assert was intended to be a way to guarantee that all of the non-okay ParseResult values are covered. We can see that by inspection now. But in case another error code was added in the future the assert would let us know that the Throw had the wrong text. But I don't feel strongly about it. I'm fine with removing the assert.

seelabs

Still 👍 from me

scottschurr · 2021-09-01T20:40:42Z

I squashed and pushed and expected CI to run. Apparently Travis needs a payment before CI will run: https://app.travis-ci.com/github/ripple/rippled/requests

I'll wait until CI has passed before marking the pull request as passed.

Also integrates use of ripple::Expected into the code base.

scottschurr · 2021-09-03T15:13:55Z

Re-pushed to kick Travis into gear.

scottschurr requested review from thejohnfreeman, seelabs and HowardHinnant August 26, 2021 21:04

scottschurr assigned thejohnfreeman, seelabs and HowardHinnant Aug 26, 2021

seelabs reviewed Aug 27, 2021

View reviewed changes

seelabs suggested changes Aug 27, 2021

View reviewed changes

HowardHinnant approved these changes Aug 27, 2021

View reviewed changes

seelabs reviewed Aug 30, 2021

View reviewed changes

seelabs approved these changes Aug 30, 2021

View reviewed changes

seelabs approved these changes Aug 31, 2021

View reviewed changes

thejohnfreeman approved these changes Aug 31, 2021

View reviewed changes

scottschurr force-pushed the wrapper-expected branch from 9a3b50e to 10b0518 Compare September 1, 2021 20:34

Add ripple::Expected which simulates std::expected:

2b3e454

Also integrates use of ripple::Expected into the code base.

scottschurr force-pushed the wrapper-expected branch from 10b0518 to 2b3e454 Compare September 3, 2021 15:12

scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 3, 2021

manojsdoshi mentioned this pull request Sep 9, 2021

Proposed 1.8.0-b5 #3927

Closed

nbougalis mentioned this pull request Sep 9, 2021

Proposed 1.8.0-b5 #3928

Merged

nbougalis closed this in #3928 Sep 14, 2021

scottschurr deleted the wrapper-expected branch April 30, 2024 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make a stand-in for `std::expected` available in the code base #3916

Make a stand-in for `std::expected` available in the code base #3916

scottschurr commented Aug 26, 2021

seelabs left a comment

seelabs left a comment

seelabs Aug 27, 2021 •

edited

Loading

seelabs Aug 27, 2021 •

edited

Loading

scottschurr Aug 27, 2021

seelabs Aug 27, 2021

seelabs Aug 27, 2021

HowardHinnant left a comment

scottschurr commented Aug 30, 2021

seelabs left a comment

seelabs Aug 30, 2021 •

edited

Loading

scottschurr Aug 30, 2021

seelabs left a comment

seelabs Aug 30, 2021

seelabs Aug 30, 2021

scottschurr Aug 30, 2021

seelabs left a comment

scottschurr commented Sep 1, 2021

scottschurr commented Sep 3, 2021


		if (result.first == ParseResult::badChar)
		assert(result.error() == ParseResult::badChar);

Make a stand-in for std::expected available in the code base #3916

Make a stand-in for std::expected available in the code base #3916

Conversation

scottschurr commented Aug 26, 2021