1453125 - consider extending Result::unwrap to enable moves out of the Result when the value type is const

Status: RESOLVED DUPLICATE of bug 1661476

(Core :: MFBT, enhancement)

Description

[Nathan Froyd [:froydnj]]

I'm trying to add a variant of NS_NewCStringInputStream that takes a nsCString&&, so in cases where the caller knows it doesn't need to hold on to the string, we don't have to copy the string.  This is not that hard, and updating existing callers is easy.  But I ran into a problem with Result<> usage that is worth addressing.

URLPreloader::ReadURI has this signature:

static Result<const nsCString, nsresult>
URLPreloader::ReadURI(nsIURI* key, ReadType readType)

And is used in the stylesheet loader and the string bundle code thusly:

auto result = URLPreloader::ReadURI(...);
if (result.isOK()) {
  MOZ_TRY(NS_NewCStringInputStream(..., result.unwrap());
} else {
  ...error handling
}

The return type of `result.unwrap()` is the first template parameter of Result<const nsCString, nsresult>.  Which, um, in this case is `const nsCString`.  So we actually *copy* out the string to unwrap it, which seems suboptimal, and worthy of its own bug.  Oops.

But even if we didn't copy, and we returned `const nsCString&`, that'd still be suboptimal, because we know, in the above case, that we're done with `result`.  We cannot Move() from `result.unwrap()` because Move()'ing a `const&` doesn't work.  (`const T&&` is nonsensical, and will select `const T&` overloads anyway.)

It'd be nice if we could Move() from `result.unwrap()`, or have a separate function that did the Move()'ing under the hood for us (unwrapAndMove?).  Or maybe it'd be easier to have a separate unwrap() overload restricted to rvalue references so we could write:

  Move(result).unwrap();

and get back an object that's been Moved()'d out of `result`.

I'm open to ideas here, but as Result<> gets used more and more, it'd be nice to have something to enable us to do the efficient thing here.

Comment 1

[Kris Maglione [:kmag]]

(In reply to Nathan Froyd [:froydnj] from comment #0)
> It'd be nice if we could Move() from `result.unwrap()`, or have a separate
> function that did the Move()'ing under the hood for us (unwrapAndMove?).  Or
> maybe it'd be easier to have a separate unwrap() overload restricted to
> rvalue references so we could write:
> 
>   Move(result).unwrap();
> 
> and get back an object that's been Moved()'d out of `result`.

I've actually been thinking about doing this for ages, and having MOZ_TRY_VAR do it automatically. I even wound up writing a patch for it when I needed it for something else, but then wound up taking a different approach.

Comment 2

[Nathan Froyd [:froydnj]]

Variant has extract(), which is shorter than Move(result).unwrap(), but maybe less evocative of what's going on?

Comment 3

[Kris Maglione [:kmag]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Variant has extract(), which is shorter than Move(result).unwrap(), but
> maybe less evocative of what's going on?

I was actually on the fence about which of the two made more sense, which is part of the reason I threw away the patch. There were two main reasons I was considering ref qualifiers over another method:

1) It would work the same for unwrap() as unwrapErr(), which would mean fewer additional methods to add.
2) It would automatically do the write thing for temporaries. Though I'm not sure how much this matters, since I couldn't think of a legitimate case where you could unwrap a temporary Result without checking its status first.

1 probably isn't that big of a deal (we can always add extractErr, anyway), and I still can't think of a use case for 2, so it probably doesn't matter much. But a ref-qualified unwrap() method still feels more idiomatic to me at this point.

Comment 4

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

I'd vote for unwrap()&& too. (And maybe Maybe and Variant should have methods()&& too!)

But more generally, does it even make sense to have const types inside Result?
Result is a wrapper for return values; returning const objects is usually unhelpful, so wrapping a const object seems similarly bad(?)
So unless there are actual use cases for const objects, how about we just disallow them (e.g., with a static_assert(!IsConst<T>)).

Comment 5

[Simon Giesecke [:sg] [he/him]]

(In reply to Gerald Squelart [:gerald] (he/him) from comment #4)

But more generally, does it even make sense to have const types inside
Result?
Result is a wrapper for return values; returning const objects is usually
unhelpful, so wrapping a const object seems similarly bad(?)
So unless there are actual use cases for const objects, how about we just
disallow them (e.g., with a static_assert(!IsConst<T>)).

This was actually done by Bug 1661476.

So I think there's nothing left to do in this Bug.