Bug 165813 - Replace ExceptionOr with std::expected
Summary: Replace ExceptionOr with std::expected
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 163919 164526
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-13 12:10 PST by JF Bastien
Modified: 2020-01-24 09:18 PST (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-12-13 12:10:14 PST
As mentioned in bug #164526

I'm not 100% sure it's a great idea, but we won't know without trying it out! I'm currently trying out std::expected in bug #163919, if that works out well I'll try out some of ErrorOr. I may need to implement more of the std::expected API (I was lazy and didn't do all of it yet).

If the std::expected API isn't perfect we can also get the C++ proposal changed. I've already sent feedback to the author. The biggest thing I'd change right now is make std::expected a "delayed exception": stuff an error in it and if the user ignores it then it blows up (throws / aborts depending on exception support).

Current proposal is here:
https://github.com/viboes/std-make/blob/master/doc/proposal/expected/p0323r1.md

Anyhow, this is a fun side-project.
Comment 1 Darin Adler 2016-12-14 09:16:17 PST
I’ve spotted some places besides ExceptionOr that we could use std::expected. Examples:

WebCore::STPProcessor::ParsingResult

WebCore::SDPProcessor::generate
WebCore::SDPProcessor::parse
WebCore::SDPProcessor::generateCandidateLine
WebCore::SDPProcessor::callScript

Lots and lots of bool-returning functions named parse or parseXXX with out arguments, such as:

WebCore::BaseDateAndTimeInputType::parseToDateComponents
WebCore::parseHexColor
WebCore::parseRepetitionType
WebCore::ContentSecurityPolicyDirectiveList::parseDirective
WebCore::parseBlendMode
WebCore::parseCompositeAndBlendOperator
WebCore::parseLineCap
WebCore::parseLineJoin
WebCore::parseTextAlign
WebCore::parseTextBaseline
WebCore::parseMetaHTTPEquivRefresh
WebCore::parseMetaHTTPRefresh
WebCore::parseDescriptors
WebCore::parseQuad
WebCore::parserSVGRect
(the list goes on and on)

Or maybe std::optional is what we want for most of these.
Comment 2 JF Bastien 2016-12-14 09:24:55 PST
(In reply to comment #1)
> I’ve spotted some places besides ExceptionOr that we could use
> std::expected. Examples:
> 
> WebCore::STPProcessor::ParsingResult
> 
> WebCore::SDPProcessor::generate
> WebCore::SDPProcessor::parse
> WebCore::SDPProcessor::generateCandidateLine
> WebCore::SDPProcessor::callScript
> 
> Lots and lots of bool-returning functions named parse or parseXXX with out
> arguments, such as:
> 
> WebCore::BaseDateAndTimeInputType::parseToDateComponents
> WebCore::parseHexColor
> WebCore::parseRepetitionType
> WebCore::ContentSecurityPolicyDirectiveList::parseDirective
> WebCore::parseBlendMode
> WebCore::parseCompositeAndBlendOperator
> WebCore::parseLineCap
> WebCore::parseLineJoin
> WebCore::parseTextAlign
> WebCore::parseTextBaseline
> WebCore::parseMetaHTTPEquivRefresh
> WebCore::parseMetaHTTPRefresh
> WebCore::parseDescriptors
> WebCore::parseQuad
> WebCore::parserSVGRect
> (the list goes on and on)
> 
> Or maybe std::optional is what we want for most of these.

Thanks for the list! I'll go through these in sub-issues.

On std::optional: I think we want to use it to denote "I'll give you a thing or not, but either way nothing went wrong". e.g. in WebAssembly.Memory's constructor the user can provide a "maximum" value or not, either way we're cool. I'd use std::expected when one of the outcomes (the unexpected one) is an error (that we're delaying into the std::expected object).

I make this distinction because std::expected<void, E> makes a lot of sense to me: either we're all good but the code has nothing to give back, or the code has an error. For WebAssembly's validator I use this idiom: if validation succeeds then void is what you get, but if it fails you get an E.

Do you think this high-level approach makes sense?
Comment 3 Darin Adler 2016-12-14 09:35:04 PST
(In reply to comment #2)
> Do you think this high-level approach makes sense?

Yes, *if* the client code ends up readable.

My main concern is that the new idiom might be much harder to understand at call sites than the old one. The boolean thing was easy to understand:

    T result;
    if (!parse(result))
        return BAD_FAILURE;

There are lots of problems with that (initializing result just to ignore it when there is an error, requires if statement and so can't be used in expressions, etc.) but it's easy to understand. We have to make sure the most common ways of using the new idiom read really nicely.

I feel like ExceptionOr was already treading in dangerous territory. The idiom I came up with to call a function and then propagate the exception to the caller was pretty wordy and not so elegant. I’m sure there are other styles possible including some really fancy stuff, but I want something easy to read, tidy and clean. As close as possible to the elegant invisibility of exceptions, without being invisible and thus a bit too mysterious.