1283562 - Introduce mozilla::Result<V, E> for fallible return values

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jason Orendorff [:jorendorff]]

(carved out from bug 1277368)

We've had problems with error handling in the JS engine for years. JSAPI functions have a fairly specific (but poorly documented) error-handling contract you're supposed to follow.

We get it right 99.9% of the time, but we are human; and there's still too much time spent chasing around OOM errors for the 0.1% mistakes. And it gets worse, because there are places in the codebase where the error-handling convention is something different (nsresult is one but there are also many minority conventions), and the boundaries between that and normal JSAPI error-handling are definitely fragile and error-prone.

The goal here is to
*   make the usual contract clearer, using C++ types
*   make it harder to screw it up, using C++ types
*   detect mistakes sooner by adding MOZ_MUST_USE_TYPE
*   support one-off different error-handling strategies sensibly

Instead of explaining how Result<> does that here, I'll ask you to wait for the patch, which contains a big comment.

Comment 1

[Jason Orendorff [:jorendorff]]

...But to summarize, the idea is to steal Rust's `Result` type.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Jason wrote all of this so I set him as author.

Comment 3

[Jan de Mooij [:jandem]]

Waldo, any chance you can review this patch this week? If not I should probably look for another MFBT reviewer, as bug 1277368 is a pain to rebase.

Comment 4

[Jeff Walden [:Waldo]]

I got somewhat into it, but there's at least one fundamental issue I haven't quite decided how we should handle, so I hadn't commented yet.  Will get to ideally later today.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 8803340 [details] [diff] [review]
Patch

Review of attachment 8803340 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Result.h
@@ +97,5 @@
> +
> +  bool isOk() const { return mErrorValue == nullptr; }
> +
> +  Ok unwrap() const {
> +    MOZ_ASSERT(isOk());

The same isOk() asserts are duplicated in each ResultImplementation detail classes' unwrap/unwrapErr() member functions. You might consider consolidating the asserts in the Result wrapper class.

@@ +192,5 @@
> +   * (The rule is for efficiency reasons.)
> +   */
> +  explicit Result(E aErrorValue) : mImpl(aErrorValue) {
> +    MOZ_ASSERT(!detail::IsNull(aErrorValue));
> +    MOZ_ASSERT(isErr());

You might consider upgrading Result's ctor and unwrap/unwrapErr() asserts to MOZ_DIAGNOSTIC_ASSERT so they are checked in release builds of Nightly and Aurora.

::: mfbt/tests/TestResult.cpp
@@ +33,5 @@
> +    return x + y;
> +}
> +
> +void BasicTests() {
> +    MOZ_ASSERT(Task1(true).isOk());

Tests should use MOZ_RELEASE_ASSERT so release builds actually test something. :)

Comment 6

[Jeff Walden [:Waldo]]

Still working on that comment, ran out of time for the Value encoding dumbness bug.  Tomorrow.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8803340 [details] [diff] [review]
Patch

Review of attachment 8803340 [details] [diff] [review]:
-----------------------------------------------------------------

mozilla::Result as currently written doesn't seem to have the ability to support move-only types.  This is probably something we should fix eventually, but I'm not entirely sure how we'll want to fix it, so probably best left for later.

::: mfbt/Assertions.h
@@ +578,5 @@
> +  do { \
> +    if ( ( expr ).isOk() ) { \
> +      /* Silence MOZ_MUST_USE. */ \
> +    } \
> +  } while (0)

This indentation is all wrong compared to the others above/below, please fix.

::: mfbt/Result.h
@@ +3,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* Variants that carry either a success value or an error value. */

This is a little bit implementation-focused, when we should describe meaning in terms of how/where people should use it.  Maybe something like

"""
A type suitable for returning either a value or an error from a function.
"""

@@ +16,5 @@
> +#include "mozilla/TypeTraits.h"
> +#include "mozilla/Variant.h"
> +
> +/**
> + * MOZ_TRY(expr) is the C++ equivalent of Rust's `try!(expr);`. First, it

Bit nitpicky, but I'd prefer these macros move *below* all the type definitions, because understanding them requires understanding mozilla::Result's API, which presently only shows up *after* these #defines.

@@ +25,5 @@
> +#define MOZ_TRY(expr)                                                          \
> +  do {                                                                         \
> +    auto mozTryTempResult_ = (expr);                                           \
> +    if (mozTryTempResult_.isErr())                                             \
> +      return ::mozilla::MakeGenericErrorResult(mozTryTempResult_.unwrapErr()); \

Don't align the \ here, and in all the other places, just put them all a space after the end of each line.  Aligning these is attractive nuisance.

@@ +48,5 @@
> +
> +/**
> + * Empty struct, indicating success for operations that have no return value.
> + * For example, if you declare another empty struct `struct OutOfMemory {};`,
> + * then `Result<Ok, OutOfMemory>` is effectively a boolean.

We should probably implement this optimization to *actually* have the struct only store a bool in this case, if we're going to talk about something like this being "effectively".  Shouldn't be too hard, just add more IsEmpty testing and template parameters.  We can defer to a followup patch.

@@ +56,5 @@
> +template <typename E> struct GenericErrorResult;
> +
> +namespace detail {
> +
> +/** Most general implementation of Result. */

Doc-comments on these various detail namespace members is more distracting than helpful, IMO.  Someone skimming this header has a fair chance of seeing this code first and starting to try to read/understand the API it describes, when *really* they should only look at mozilla::Result's documentation.

@@ +57,5 @@
> +
> +namespace detail {
> +
> +/** Most general implementation of Result. */
> +template <typename T, typename E, bool TIsEmpty, bool Aligned>

Define enums to record emptiness and alignedness, then use well-named initializers in the template specializations below.  true/false aren't readable.

@@ +58,5 @@
> +namespace detail {
> +
> +/** Most general implementation of Result. */
> +template <typename T, typename E, bool TIsEmpty, bool Aligned>
> +class ResultImplementation {

This is nitpicky, and it negligibly deviates from Rust.  But for whatever reason (maybe because there are two separate types potentially stored in Result, so I can't just think of it as "an instance of the [one] contained type"?), I'm having a lot of difficulty reading "T" and associating that name with "value".

I can't see any reason to use T for this type, particularly not when everything in this header refers to variables of type T as "values".  Even the Rust docs themselves speak of Result's Ok variant as "representing success and containing a *value*" (emphasis added), not some other noun starting with T.  https://doc.rust-lang.org/std/result/index.html  Please do s/T/V/g on all the relevant Ts to accordingly enhance readability.

@@ +76,5 @@
> + * the error type is a pointer.
> + *
> + * In this case, Result<T, E*> is guaranteed to be pointer-sized, and all zero
> + * bits on success. Do not change this representation! There is JIT code that
> + * depends on it.

These comments are nice enough, but they're somewhat misplaced.  These comments should be on mozilla::Result, not on implementation details that nobody should be reading *unless* they want to know how the sausage is made.  It's mozilla::Result that guarantees this stuff -- not the thing that implements the guarantee.

@@ +80,5 @@
> + * depends on it.
> + *
> + * (This representation is the source of the weird rule that error values can't
> + * be null pointers. Since Results are very heavily used in the JS engine, it's
> + * important that the most common ones fit in a register.)

I don't think I'm okay with this weird edge case as part of the default Result type.  Admittedly, empty-value, nullable-pointer-result is likely an uncommon pairing.  But it represents a special case, an oddity, that everyone will have to avoid (or trip over), regardless.

That said, I think we can still have a thing like this -- we just have to use a reference type, rather than a pointer type.  If instead of E* here we had E&, and we had |explicit ResultImplementation(E& aErrorValue) : mErrorValue(&aErrorValue) {}| (and unwrapErr would change to return E&, of course), then we *can* have null as sentinel, without excluding any possible values from the set.  Please make that change.

@@ +83,5 @@
> + * be null pointers. Since Results are very heavily used in the JS engine, it's
> + * important that the most common ones fit in a register.)
> + */
> +template <typename T, typename E, bool AlignedOrNot>
> +class ResultImplementation<T, E*, true, AlignedOrNot> {

The style for template class definitions should be

template<typename T, typename E, bool AlignedOrNot>
class ResultImplementation<T, E&, true, AlignedOrNot>
{
  ...
};

with brace on its own line.  Apply this to all the other template classes, too.

@@ +98,5 @@
> +  bool isOk() const { return mErrorValue == nullptr; }
> +
> +  Ok unwrap() const {
> +    MOZ_ASSERT(isOk());
> +    return Ok();

Shouldn't this be returning T and T()?  Don't see why this should compile, if someone were to do, say, this:

  struct Fine {};
  mozilla::Result<Fine, int*>(Fine()).unwrap();

Of course, this should be added as a test.

@@ +119,5 @@
> +public:
> +  explicit ResultImplementation(T* aValue)
> +      : mBits(reinterpret_cast<uintptr_t>(aValue)) {}
> +  explicit ResultImplementation(E* aErrorValue)
> +      : mBits(reinterpret_cast<uintptr_t>(aErrorValue) | 1) {}

MOZ_ASSERT((uintptr_t(aValue) % MOZ_ALIGNOF(V)) == 0,
           "Result value pointers must not be misaligned");
MOZ_ASSERT((uintptr_t(aErrorValue) % MOZ_ALIGNOF(E)) == 0,
           "Result error pointers must not be misaligned");

@@ +150,5 @@
> +// as the pointed-to type is always aligned to some multiple of two.
> +template <typename T> struct HasFreeLSB<T*> {
> +  static const bool value = (MOZ_ALIGNOF(T) & 1) == 0;
> +};
> +}

Need a blank line before this, and a // namespace detail after it.  I had trouble skimming the patch to find exactly where the namespace block ended, just now, and I expect that'll happen again.

@@ +169,5 @@
> + * using `false` or `nullptr` to indicate errors. What screwups? See
> + * <https://bugzilla.mozilla.org/show_bug.cgi?id=912928> for a partial
> + * list.
> + */
> +template <typename T, typename E> class MOZ_MUST_USE_TYPE Result {

template<typename V, typename E>
class MOZ_MUST_USE_TYPE Result
{

Also, please make the class final -- no reason for people to be able to inherit from this

@@ +172,5 @@
> + */
> +template <typename T, typename E> class MOZ_MUST_USE_TYPE Result {
> +  typedef detail::ResultImplementation<T, E, IsEmpty<T>::value,
> +                                       detail::HasFreeLSB<T>::value &&
> +                                           detail::HasFreeLSB<E>::value> Impl;

using Impl = detail::ResultImplementation<...>;

@@ +188,5 @@
> +
> +  /**
> +   * Create an error result. There is a weird rule here:
> +   * if E is a pointer type, then the argument must not be a null pointer.
> +   * (The rule is for efficiency reasons.)

Happily, this one weird rule, IsNull's definitions, and IsNull's use below can all go away when the not-null thing is enforced by applying the compression trick only when E is a reference and E is empty.

@@ +201,5 @@
> +   * Create an error result from another error result.
> +   */
> +  template <typename E2>
> +  MOZ_IMPLICIT Result(const GenericErrorResult<E2>& aErrorResult)
> +      : mImpl(aErrorResult.mErrorValue) {

template<typename E2>
MOZ_IMPLICIT Result(const GenericErrorResult<E2>& aErrorResult)
  : mImpl(aErrorResult.mErrorValue)
{

@@ +203,5 @@
> +  template <typename E2>
> +  MOZ_IMPLICIT Result(const GenericErrorResult<E2>& aErrorResult)
> +      : mImpl(aErrorResult.mErrorValue) {
> +    MOZ_ASSERT(isErr());
> +  }

Can this constructor be private and have MakeGenericError be a friend of this class?

@@ +212,5 @@
> +  /** True if this Result is a success result. */
> +  bool isOk() const { return mImpl.isOk(); }
> +
> +  /** Get the success value from this Result, which must be a success result. */
> +  T unwrap() const { return mImpl.unwrap(); }

Yeah, putting the isOk() asserts in these functions makes more sense than in the implementation classes.

@@ +228,5 @@
> + * an error--functions designed to build and populate error objects. It's also
> + * useful in error-handling macros; see MOZ_TRY for an example.
> + */
> +template <typename E> struct MOZ_MUST_USE_TYPE GenericErrorResult {
> +  E mErrorValue;

Make this a class with only the constructor public, as usual.

@@ +233,5 @@
> +  explicit GenericErrorResult(E aErrorValue) : mErrorValue(aErrorValue) {}
> +};
> +
> +template <typename E>
> +inline GenericErrorResult<E> MakeGenericErrorResult(const E& aErrorValue) {

Function syntax should be

template<typename E>
inline GenericErrorResult<E>
MakeGenericErrorResult(const E& aErrorValue)
{

@@ +236,5 @@
> +template <typename E>
> +inline GenericErrorResult<E> MakeGenericErrorResult(const E& aErrorValue) {
> +  return GenericErrorResult<E>(aErrorValue);
> +}
> +}

Missing blank line, // namespace mozilla

::: mfbt/tests/TestResult.cpp
@@ +1,1 @@
> +#include "mozilla/Result.h"

Needs the usual license header.

@@ +1,3 @@
> +#include "mozilla/Result.h"
> +
> +using namespace mozilla;

mfbt tests don't open the entire mozilla namespace, rather they |using mozilla::Foo;| each individual thing they use.  Do that.

@@ +1,5 @@
> +#include "mozilla/Result.h"
> +
> +using namespace mozilla;
> +
> +struct Failed {

{ on new line.  And for all the various functions in this file.

@@ +8,5 @@
> +
> +GenericErrorResult<Failed*>
> +Fail() {
> +    static Failed failed;
> +    return MakeGenericErrorResult(&failed);

All the function bodies should have two-space indentation.

@@ +28,5 @@
> +Result<int, Failed*>
> +Task3(bool pass1, bool pass2, int value) {
> +    int x, y;
> +    MOZ_TRY_VAR(x, Task2(pass1, value));
> +    MOZ_TRY_VAR(y, Task2(pass2, value));

Blah.  Strongly not a fan of this syntax for assigning result values to variables.  :-(  But I guess we don't have any choice about it, do we.

@@ +41,5 @@
> +
> +    // MOZ_TRY works.
> +    MOZ_ASSERT(Task2(true, 3).isOk());
> +    MOZ_ASSERT(Task2(true, 3).unwrap() == 3);
> +    MOZ_ASSERT(Task2(false, 3).isErr());

Doing these operations on rvalues is all well and good, but could you do some of these on lvalue local variables as well?  Particularly, I want us testing that you can make multiple function calls on a single Result without disturbing its behavior.

@@ +82,5 @@
> +              "Result with two aligned pointer types should be optimized to fit in a register");
> +static_assert(sizeof(Result<char*, Failed*>) == sizeof(Variant<char*, Failed*>),
> +              "Result with unaligned success type `char*` must not be type-tagged");
> +static_assert(sizeof(Result<int*, char*>) == sizeof(Variant<int*, char*>),
> +              "Result with unaligned error type `char*` must not be type-tagged");

Move these assertions up top where Failed is declared, so that someone reading these doesn't have to look away very far to understand why they hold.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch v2

I think this addresses all your comments.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8808163 [details] [diff] [review]
Patch v2

Clearing review, adding some static asserts.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8808549 [details] [diff] [review]
Patch

Review of attachment 8808549 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Result.h
@@ +47,5 @@
> +};
> +
> +/**
> + * mozilla::Variant doesn't like storing a reference. This is a specialization
> + * to store E as pointer if it's a reference.

Waldo, do you know why I need this? When I remove this specialization, TestResult fails to compile (with Clang 3.9). The problem occurs when using Variant with a reference instead of a pointer, as far as I can see, but I'm not sure if it's a Variant<> bug.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #11)
> Waldo, do you know why I need this? When I remove this specialization,
> TestResult fails to compile (with Clang 3.9). The problem occurs when using
> Variant with a reference instead of a pointer, as far as I can see, but I'm
> not sure if it's a Variant<> bug.

I think it's similar to this:

  int32_t i = 10;

  // Works.
  using VP = Variant<int32_t*, int8_t*>;
  VP vp(&i);

  // Fails to compile.
  using VR = Variant<int32_t&, int8_t&>;
  VR vr(i);

Comment 13

[Jeff Walden [:Waldo]]

Hmm.  I think this is more or less a matter of safety, because storing references is super-prone to dangling.  C++ has std::reference_wrapper for people who want to nonetheless store references in containers and the like -- just, by doing so explicitly.

I don't think we quite *deliberately* decided not to have references in Variant.  It more falls out of how the storage of the various types, and the constructor parameters, are defined.  But in hindsight it's probably the safe thing to do.

Not using Variant internally seems a perfectly fine choice to me, and perhaps especially fine if we require particular binary layout for some instantiations of Variant.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8808549 [details] [diff] [review]
Patch

Review of attachment 8808549 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +576,5 @@
>       } while (0)
> +#  define MOZ_ALWAYS_OK(expr) \
> +     do { \
> +       if ((expr).isOk()) { \
> +         /* Silence MOZ_MUST_USE. */            \

Reel that '\' back a bit there.  :-)

::: mfbt/Result.h
@@ +29,5 @@
> +
> +namespace detail {
> +
> +enum class VEmptiness { IsEmpty = true, IsNotEmpty = false };
> +enum class Alignedness { IsAligned = true, IsNotAligned = false };

I'd prefer not specifying initializers for these -- we want them used only symbolically.

@@ +42,5 @@
> +  explicit ResultImplementation(E aErrorValue) : mStorage(aErrorValue) {}
> +
> +  bool isOk() const { return mStorage.template is<V>(); }
> +  V unwrap() const { return mStorage.template as<V>(); }
> +  E unwrapErr() const { return mStorage.template as<E>(); }

Maybe add a comment by the two unwrap functions noting that their callers will assert isOk() has the proper value, so these functions (in all ResultImplementation specializations) needn't do so.

@@ +56,5 @@
> +  mozilla::Variant<V, E*> mStorage;
> +
> +public:
> +  explicit ResultImplementation(V aValue) : mStorage(aValue) {}
> +  explicit ResultImplementation(E aErrorValue) : mStorage(&aErrorValue) {}

I'm pretty sure you need this argument to be |E& aErrorValue| -- otherwise this takes and stores a reference to a temporary.  This needs a test -- I think this (which fails now) is a proper test:

  struct MyError { int x = 0; };
  MyError merror;
  Result<int, MyError&> res(merror);
  MOZ_RELEASE_ASSERT(&res.unwrapErr() == &merror);

@@ +92,5 @@
> + * a tag bit.
> + */
> +template <typename V, typename E>
> +class ResultImplementation<V*, E&,
> +                           VEmptiness::IsNotEmpty, Alignedness::IsAligned>

Why do you need to specialize this to VEmptiness::IsNotEmpty?  Setting only the lowest bit shouldn't make the difference, because |struct S{}| and |char| should both align to (at least) 1 (not sure if empty class alignment is precisely specified).  Admittedly I don't think you could have empty V be aligned, but overspecifying this way makes the code harder to read.  I believe you can just add |VEmptiness EmptinessOfV| to the template parameters and pass |EmptinessOfV| here and not overspecify.

@@ +107,5 @@
> +  explicit ResultImplementation(E& aErrorValue)
> +    : mBits(reinterpret_cast<uintptr_t>(&aErrorValue) | 1)
> +  {
> +    MOZ_ASSERT((uintptr_t(&aErrorValue) % MOZ_ALIGNOF(E)) == 0,
> +               "Result error pointers must not be misaligned");

s/error pointers/errors/, IMO.

@@ +118,5 @@
> +};
> +
> +// A bit of help figuring out which of the above specializations to use.
> +//
> +// Most types do not have a spare bit that can be used for something else.

Maybe "We begin by safely assuming types don't have a spare bit."

@@ +122,5 @@
> +// Most types do not have a spare bit that can be used for something else.
> +template <typename T> struct HasFreeLSB { static const bool value = false; };
> +
> +// The least significant bit of a pointer type is free (always zero), as long
> +// as the pointed-to type is always aligned to some multiple of two.

Not sure why, but this reads slightly jargony to me.  My counterproposal:

"""
The lowest bit of a properly-aligned pointer is always zero if the pointee type is greater than byte-aligned.  That bit is free to use if it's masked out of such pointers before they're dereferenced.
"""

@@ +127,5 @@
> +template <typename T> struct HasFreeLSB<T*> {
> +  static const bool value = (MOZ_ALIGNOF(T) & 1) == 0;
> +};
> +
> +template <typename T> struct HasFreeLSB<T&> {

And maybe

"""
We store references as pointers, so they have a free bit if a pointer would have one.
"""

@@ +150,5 @@
> + *
> + * - If the success type is a pointer type and the error type is a reference
> + *   type, and the least significant bit is unused for both types when stored
> + *   as a pointer (due to alignment rules), Result<V*, E&> is guaranteed to be
> + *   pointer-sized. In this case, we use the LSB as tag bit.

s/the LSB as tag bit/the lowest bit as tag bit: 0 to indicate the Result's bits are a V, 1 to indicate the Result's bits (with the 1 masked out) encode an E*./

Properly speaking, we can't really have people relying on the lowest bit's meaning unless we're precise about which value means which.  :-)

@@ +152,5 @@
> + *   type, and the least significant bit is unused for both types when stored
> + *   as a pointer (due to alignment rules), Result<V*, E&> is guaranteed to be
> + *   pointer-sized. In this case, we use the LSB as tag bit.
> + *
> + * Rationale: The purpose of Result is to reduce the screwups caused by

No need for "Rationale: " in this.

@@ +175,5 @@
> +  /** Deprecated. */
> +  template <typename T>
> +  MOZ_IMPLICIT Result(T aOk) : mImpl(aOk) {
> +      static_assert(mozilla::IsSame<T, bool>::value,
> +                    "Types convertible to bool must not implicitly convert to Result");

I think you can remove the templating on this -- unless this constructor is called, the static_assert will never be tested because the function will never be instantiated.

@@ +231,5 @@
> + * an error--functions designed to build and populate error objects. It's also
> + * useful in error-handling macros; see MOZ_TRY for an example.
> + */
> +template <typename E>
> +class MOZ_MUST_USE_TYPE GenericErrorResult

Why not ErrorResult for the name, if this is going to be a public thing people should use?  "Generic" seems a bit redundant.  And MakeErrorResult for the other, of course.

@@ +268,5 @@
> +/**
> + * MOZ_TRY_VAR(target, expr) is the C++ equivalent of Rust's `target = try!(expr);`.
> + * First, it evaluates expr, which must produce a Result value.
> + * On success, the result's success value is assigned to target.
> + * On error, immediately returns the error result.

I think it would be best to state that |target| must evaluate to a reference without any side effects.

It might well be that any expression that evaluates to a reference would *work* here, but I don't think we should support it.  It's just too cute for people to be doing.  (And if I knew of a way to constrain |target| to be a name or a reference to a member field or similar, I would say use it in a heartbeat -- but I don't.)

::: mfbt/tests/TestResult.cpp
@@ +18,5 @@
> +  int x;
> +};
> +
> +static_assert(sizeof(Result<Ok, Failed&>) == sizeof(uintptr_t),
> +              "Result with empty value type should be optimized to fit in a register");

"should...register" should instead be "should be pointer-sized" (and same for the others).  However pointers are represented for most code, should be good enough for this -- and we can't really mention registers because they're ABI, when we're just writing C++.

@@ +22,5 @@
> +              "Result with empty value type should be optimized to fit in a register");
> +static_assert(sizeof(Result<int*, Failed&>) == sizeof(uintptr_t),
> +              "Result with two aligned pointer types should be optimized to fit in a register");
> +static_assert(sizeof(Result<char*, Failed*>) == sizeof(Variant<char*, Failed*>),
> +              "Result with unaligned success type `char*` must not be type-tagged");

Probably just assert sizeof > sizeof(char*).  That Variant is used internally is an implementation detail.  That it has to be bigger, is purely a function of the class's documented behavior and the pigeonhole principle.

@@ +24,5 @@
> +              "Result with two aligned pointer types should be optimized to fit in a register");
> +static_assert(sizeof(Result<char*, Failed*>) == sizeof(Variant<char*, Failed*>),
> +              "Result with unaligned success type `char*` must not be type-tagged");
> +static_assert(sizeof(Result<int*, char*>) == sizeof(Variant<int*, char*>),
> +              "Result with unaligned error type `char*` must not be type-tagged");

Same sizeof > sizeof thing.

@@ +26,5 @@
> +              "Result with unaligned success type `char*` must not be type-tagged");
> +static_assert(sizeof(Result<int*, char*>) == sizeof(Variant<int*, char*>),
> +              "Result with unaligned error type `char*` must not be type-tagged");
> +
> +GenericErrorResult<Failed&>

Make most/all of these functions static, please.

@@ +103,5 @@
> +    double d = 3.14;
> +
> +    Result<int*, double&> res = &i;
> +    static_assert(sizeof(res) == sizeof(uintptr_t),
> +                  "Should use pointer tagging to fit in a word");

s/Should/should/

@@ +110,5 @@
> +    MOZ_RELEASE_ASSERT(*res.unwrap() == 123);
> +
> +    res = MakeGenericErrorResult(d);
> +    MOZ_RELEASE_ASSERT(res.isErr());
> +    MOZ_RELEASE_ASSERT(res.unwrapErr() == 3.14);

MOZ_RELEASE_ASSERT(&res.unwrapErr() == &d);

maybe before the value-check.  The address in the reference is just as important as the value of the referent, if we're quasi-promoting people putting references into Result.

@@ +143,5 @@
> +void
> +EmptyValueTest()
> +{
> +  struct Fine {};
> +  mozilla::Result<Fine, int&> res((Fine()));

Extra parens seem unnecessary?  Also the mozilla::.

@@ +147,5 @@
> +  mozilla::Result<Fine, int&> res((Fine()));
> +  res.unwrap();
> +  MOZ_RELEASE_ASSERT(res.isOk());
> +  static_assert(sizeof(res) == sizeof(uintptr_t),
> +		"Result with empty value type should be optimized to fit in a register");

Same pointer-sized comment nitpicking.

Comment 15

[Jan de Mooij [:jandem]]

Thanks for the thorough review, Waldo!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> > +  /** Deprecated. */
> > +  template <typename T>
> > +  MOZ_IMPLICIT Result(T aOk) : mImpl(aOk) {
> > +      static_assert(mozilla::IsSame<T, bool>::value,
> > +                    "Types convertible to bool must not implicitly convert to Result");
> 
> I think you can remove the templating on this -- unless this constructor is
> called, the static_assert will never be tested because the function will
> never be instantiated.

The templating is to avoid taking a |bool aOk| argument, as in that case integers/pointers/EVERYTHING will silently coerce to bool and to Result. We want to allow this (temporarily!):

  Result<Ok, Failed&> res = false;

But not these:

  Result<Ok, Failed&> res = 0;
  Result<Ok, Failed&> res = nullptr;

Now after typing this, I realize this temporary coerce-from-bool behavior is worse now that our JS Result<> uses E& instead of E*, because res.unwrapErr() will return an invalid reference (to 0x1). Hmm, maybe we should just remove it.

> Why not ErrorResult for the name, if this is going to be a public thing
> people should use?  "Generic" seems a bit redundant.  And MakeErrorResult
> for the other, of course.

Unfortunately there already is a mozilla::ErrorResult and DXR reports more than 6200 hits (that's without the generated bindings code).

https://dxr.mozilla.org/mozilla-central/search?q=ErrorResult+-path%3Aobj-x86&redirect=false

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #15)
> Hmm, maybe we should just remove it.

Yeah, thinking about this more, I'll remove it. Let's start with a clean slate.

Waldo, that only leaves the ErrorResult/GenericErrorResult thing.

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Updated patch

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Updated patch

Comment 19

[Jeff Walden [:Waldo]]

Okay, generic is fine for now.  But I might poke bz on Monday about whether the DOM thing could change name, or be referenced as mozilla::dom::ErrorResult, or something.

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> But I might poke bz on Monday about whether
> the DOM thing could change name, or be referenced as
> mozilla::dom::ErrorResult, or something.

Not sure if the latter is a good idea, it will probably cause a ton of conflicts because a lot of files use both namespaces...

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97a18aee3431
Add mozilla::Result<V, E> for fallible return values. r=jwalden

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97a18aee3431

Comment 23

[Nicholas Nethercote [inactive]]

Nice work, Jan. It would be nice if you could email dev-platform to explain this type and your future plans for it. Thanks!

Comment 24

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #23)
> Nice work, Jan. It would be nice if you could email dev-platform to explain
> this type and your future plans for it. Thanks!

Sure, https://groups.google.com/forum/#!topic/mozilla.dev.platform/iXQeW6-iQmQ


Sorry for the delay!

Comment 25

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Sorry I'm late to the party! Cool stuff, thank you for introducing this type.

Some very-late remarks, with fresh non-Rusty(-yet) eyes. Feel free to open separate bugs (or tell me to do it myself!) if they're worth it. Or please let me know if we should discuss this elsewhere (or nowhere).

- 'isOk', etc., don't follow the Mozilla coding style, they should be capitalized. I.e.: 'IsOk'.

- Why the super-short 'isErr', can't we just add 2 characters to make it more English? I.e.: 'IsError'.

- For a quick IsOk() test, we could have a traditional `explicit operator bool()`. And to mimic Maybe and others, we could also have the corresponding `operator*()` and `operator->()` returning the success value like unwrap().

- Should constructors use perfect forwarding, to handle big types more efficiently? (Or do we assume optimizers will do good work?)

- No move constructor/assignment?

I'm guessing some of the above decisions are due to following Rust's example? While borrowing ideas from Rust, in general should we prefer consistency with Rust, or with our existing C++ code?

Comment 26

[Jeff Walden [:Waldo]]

(In reply to Gerald Squelart [:gerald] from comment #25)
> - 'isOk', etc., don't follow the Mozilla coding style, they should be
> capitalized. I.e.: 'IsOk'.

This came from it being originally a SpiderMonkey thing, I think.  There's a good bit of precedent for camelCaps naming in mfbt, in any event.

> - Why the super-short 'isErr', can't we just add 2 characters to make it
> more English? I.e.: 'IsError'.

Naming adopted from Rust, and nobody exercised enough to complain.  Additionally, generally people should be interacting with Result using the try-macros, not the specific member functions.

> - For a quick IsOk() test, we could have a traditional `explicit operator
> bool()`.

No.  (wait for it) Explicit function call is better than more-implicit explicit operator.

> And to mimic Maybe and others, we could also have the corresponding
> `operator*()` and `operator->()` returning the success value like unwrap().

Again, the macros.  And again, explicit named functions beat operators you can't search for as well.

> - Should constructors use perfect forwarding, to handle big types more
> efficiently? (Or do we assume optimizers will do good work?)

This was partly a perfect-is-the-enemy-of-good thing than anything.  (Or at least that's how I remember it.)  We can fuss perfect forwarding into place whenever someone needs it.

Perhaps more importantly, the first uses didn't need it enough to have to get it right.  There's some finesse to doing this, in terms of exactly where values and errors live in all places.  So it would have been a bad idea to give a buggy half-implementation that would maybe seem to work but then crash and burn later.

> - No move constructor/assignment?

Wasn't necessary for the first pass, we can add them as users appear.

> I'm guessing some of the above decisions are due to following Rust's
> example? While borrowing ideas from Rust, in general should we prefer
> consistency with Rust, or with our existing C++ code?

At this point I'm not sure I have a one-size-fits-all answer to this.  I think we should probably just take things case by case, at least for now.

Comment 27

[Alfredo Yang (:alfredo)]

It'd be more convenient if it has unwrap_or(), or() and other functions, just like rust Result.

https://bugzilla.mozilla.org/show_bug.cgi?id=1412183#c7