Open Bug 1418587 Opened 6 years ago Updated 2 years ago

`Result<Maybe<BookmarkData>, nsresult>` fails static analysis

Categories

(Core :: MFBT, enhancement)

enhancement

Tracking

()

People

(Reporter: lina, Unassigned)

References

Details

In bug 1417101, I changed some methods to return a `Result<Maybe<BookmarkData>, nsresult>`. (`BookmarkData` is from https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/components/places/nsNavBookmarks.h#30-45).

This caused static analysis bustage across the board: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e0eab399db5982c6a13aa7d3a838aa302d8f9ec1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

I asked Nika about the errors:

> <mystor> kitcambridge: so basically
> 17:52 kitcambridge: This is because BookmarkData contains an alignas(_) member and apparently that doesn't work well with x86's calling convention IIRC
> 17:53 kitcambridge: (froydnj would know more I think?)
> 17:54 kitcambridge: mozilla::Result is apparently defining a method which would take a BookmarkData by value (or perhaps it's the Maybe)
> 17:54 and that could on some platforms cause alignment issues
> 17:54 so we reject it
> 17:54 I think

Nathan, do you have more insights into this? If it's easier, or just not possible to make a type like that, I can drop the `Result` and use out params instead...but I'm wondering if you have other suggestions.
Flags: needinfo?(nfroyd)
I'd bet if you throw perfect forwarding at the relevant functions in mozilla::Result, that should fix the problem.  Any chance you can do that?  Happy to review, or to write the patch if necessary.
Flags: needinfo?(nfroyd)
I tried to write a patch, but I don't think I understand how perfect forwarding works. Could you help me out, Jeff?

First, I tried changing the `Result` constructor, and all the `ResultImplementation` constructors that take `V aValue`, to take `V&& aValue`. So I ended up with signatures like this:

  explicit ResultImplementation(V&& aValue) : mStorage(Forward<V>(aValue)) {}
  MOZ_IMPLICIT Result(V&& aValue) : mImpl(Forward<V>(aValue)) { MOZ_ASSERT(isOk()); }

But then I had to change `TestResult::Task2` from `return value` to `return mozilla::Move(value)`, or I got compiler errors like:

> error: no viable conversion from returned value of type 'int' to function return type 'Result<int, Failed &>'
>  return value;  // implicit conversion from T to Result<T, E>
> candidate constructor not viable: no known conversion from 'int' to 'int &&' for 1st argument
>   MOZ_IMPLICIT Result(V&& aValue) : mImpl(Forward<V>(aValue)) { MOZ_ASSERT(isOk()); }

I'm guessing we don't want to make every function that relies on the implicit conversion to return a moved value (or do we?), so I tried this:

    MOZ_IMPLICIT Result(V aValue) : Result(Move(aValue)) {}
    // I also tried duplicating the constructor body instead of delegating.
    MOZ_IMPLICIT Result(V&& aValue) : mImpl(Forward<V>(aValue)) { MOZ_ASSERT(isOk()); }

But then the compiler complains that the constructor is ambiguous.
Flags: needinfo?(jwalden+bmo)
(In reply to Kit Cambridge (he/him) [:kitcambridge] from comment #2)
> First, I tried changing the `Result` constructor, and all the
> `ResultImplementation` constructors that take `V aValue`, to take `V&&
> aValue`. So I ended up with signatures like this:
> 
>   explicit ResultImplementation(V&& aValue) : mStorage(Forward<V>(aValue)) {}
>   MOZ_IMPLICIT Result(V&& aValue) : mImpl(Forward<V>(aValue)) {
> MOZ_ASSERT(isOk()); }

You would also need to not use V/E directly, but rather have them be inferred template parameters.  mfbt/Move.h has a bunch of discussion of how this is supposed to be done to effectuate perfect forwarding.

Unfortunately, even if you change to the proper form of perfect forwarding, then you hit the issue of distinguishing the value-directed overloads from the error-directed overloads.

This is a little gnarly.  Lemme look at it some.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(jwalden+bmo)
Bah, this is messy.  Potentially even intractably so.

So one approach is to have 6 constructors, like so:

  Result(V&);
  Result(const V&);
  Result(V&&);
  Result(E&);
  Result(const E&);
  Result(E&&);

This works up to the point where E is a reference type, because then E& and E&& collapse to the same type.  (Or V, but that's less common.  Still, we really should be able to support arbitrary non-identical types for V/E.  And if we figure out a solution for E, we might as well apply it to V.)

Another approach is the perfect forwarding thing I mentioned:

  Result(Result&&) = default; // necessary because templated-overload is not a true move constructor
  template<typename Value> Result(Value&&);
  template<typename Error> Result(Error&&);

(Note that perfect forwarding requires the template<> goo -- not enough to just use V&&/E&& directly.)  The problem here is that these signatures are identical overloads (which you ran into without the template<> goo).

So the usual approach is to throw SFINAE at this to have only one overload for each possible type.  But...how?  There's not an obvious distinguishing feature between V/E to permit them to be distinguished.  You can't quite do IsSame<Value, V>::value or something, because sometimes you do want to accept more than just V literally -- think const char[N] where V is const char*, for an example from our tests, or accept UniquePtr&& when the stored type is UniquePtr, &c.

Anyway.  Maybe there's a way to make a squircle here, but I seriously wonder whether this is really desirable, and whether it might not be better just to force everyone to use Ok() and Err() functions we provide, that can invoke private/hidden functionality in Result to get a properly constructed Result.  Thoughts?
Flags: needinfo?(nfroyd)
Oh, the other problem with throwing SFINAE at this -- the constructors you have here differ in explicitness, MOZ_IMPLICIT versus explicit.  And when you have an implicit constructor, it can't have the extra defaulted SFINAE argument without seeming to effectively disable permitting the implicit conversion.  Or at least when I tried various permutations of this, it seemed to do so.  (Conceivably I was messing something up here, but I'm at least somewhat dubious that's happening.)
(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #4)
> Anyway.  Maybe there's a way to make a squircle here, but I seriously wonder
> whether this is really desirable, and whether it might not be better just to
> force everyone to use Ok() and Err() functions we provide, that can invoke
> private/hidden functionality in Result to get a properly constructed Result.
> Thoughts?

Using Ok() and Err() functions seem desirable here, for explicitness, for implementation simplicity, and for similarity to what Rust already does (right?).  I don't know much about the rationale for the implicit conversions here; I guess they were intended for simplicity, but it looks like they might not be fulfilling that purpose in all cases?  nbp, is that guess correct?
Flags: needinfo?(nfroyd) → needinfo?(nicolas.b.pierron)
So the problem is moving the Result success/error value out of the function

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #4)
> Another approach is the perfect forwarding thing I mentioned:
> 
>   Result(Result&&) = default; // necessary because templated-overload is not
> a true move constructor
>   template<typename Value> Result(Value&&);
>   template<typename Error> Result(Error&&);
> 
> (Note that perfect forwarding requires the template<> goo -- not enough to
> just use V&&/E&& directly.)  The problem here is that these signatures are
> identical overloads (which you ran into without the template<> goo).

What about:

  class Result {
    …
    Result(Result&&) = default;
    template<typename Value> Result(Value&&);
    …
  };

  template<typename Value>
  Result::Result(GenericErrorResult<Value>&&);

  template<typename Value>
  Result::Result(Value&&);
Flags: needinfo?(nicolas.b.pierron) → needinfo?(jwalden+bmo)
To be honest, by the time you require the error-returns to type out Err(), I think it's basically reasonable to symmetrically require Ok() too.  Keep it simple, keep it consistent.
Flags: needinfo?(jwalden+bmo)
Ok, so we should enforce(¹) the use of the Ok/Err functions as result wrappers, in order to keep the C++ code of Results simpler, which would indeed be closer to what Rust do.

So we would replace the Ok type by a Ok() function, which returns a GernericOkResult<…>() or something similar (²).
Thus the Result constructors would be something like:

  Result(GenericOkResult<V>&&)
  Result(GenericErrorResult<E>&&)

This way the constructors would no longer be ambiguous, and we can specialize the Result implementation if one of the Result class parameters are a moved-type, to provided a non-packed storage.

The Ok type would have to be replaced, maybe by an Empty type, and we can overload the Ok() function to return the GenericOkResult<Empty> type when given no arguments.

¹: There is already a lot of code which uses that, so we should either make it a one big patch, or do some transition period.
²: Maybe we should also drop the "Generic" part of the name.
Flags: needinfo?(nicolas.b.pierron)
I'm not sure we need to specialize for moved-type storage -- just using mozilla::Decay is possibly enough.  Or whatever it is that Tuple does internally, is almost certainly the right tactic to use.

Maybe we could rename the Ok struct to Success to deal with that naming issue?  Shortness of name is not really necessary here, IMO, and Success seems adequate to me.
Flags: needinfo?(nicolas.b.pierron)

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: jwalden → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.