1325062 - Potential footgun with MOZ_TRY and implicit conversion

Status: NEW

(Core :: MFBT, defect)

Description

[Jan de Mooij [:jandem]]

Ehsan mentioned this on the mailing list. This compiles:

  Result<Ok, bool> MyTest1() { return Ok(); }
  Result<Ok, int> MyTest2() {
    MOZ_TRY(MyTest1());
    return Ok();
  }

Not sure what we should do about this. Maybe in addition to static_asserting IsConvertible, we should assert the types are the same if integral? Or add an explicit ErrorIsConvertible<From, To> mechanism?

Comment 1

[Jeff Walden [:Waldo]]

Rust defines its try! macro as follows:

  macro_rules! try {
      ($e:expr) => (match $e {
          Ok(val) => val,
          Err(err) => return Err(::std::convert::From::from(err)),
      });
  }

with the error being passed through a conversion trait.

It seems to me like we might do similarly with a template trait that, by default, is deleted -- with one specialization for exact-type-to-exact-type conversion.  Anyone who wants conversion behavior would be free to specialize for that conversion.

That's my tentative thinking, at least.

Comment 2

[Jason Orendorff [:jorendorff]]

The specific motivating case for using C++ implicit conversion is something like:

    MOZ_TRY_VAR(obj, NewObject(...));  // can only fail with OOM
    MOZ_TRY(ZapObject(cx, obj, ...));  // fails with the more general type JS::Error&

The type that represents OOM would promote implicitly to JS::Error&.
Comment 1 definitely covers this case adequately.

Other possible cases where C++ implicit conversion might be handy are:

*   Result<T, Variant<E1, E2>> to indicate that two different types of error are possible

*   Result<T, CommonErrorBaseClass*> as a return type when calling functions that return
    Result<U, ErrorSubclass1*> and Result<V, ErrorSubclass2*>.

Comment 3

[Jason Orendorff [:jorendorff]]

I bring those last two cases up because comment 1 wouldn't handle them as nicely.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> I bring those last two cases up because comment 1 wouldn't handle them as
> nicely.

Anything is possible as long as you throw the right template specializations at it.  :-)  #zomboc++

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Anything is possible as long as you throw the right template specializations
> at it.  :-)  #zomboc++

When you have a hammer...

Comment 6

[(no longer active)]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Other possible cases where C++ implicit conversion might be handy are:
> 
> *   Result<T, Variant<E1, E2>> to indicate that two different types of error
> are possible

I think this can be dealt with by partially specializing the trait class in comment 1 for Variant<T, U>.  We'd probably need two specializations, providing conversions from Variant<T, U> to T and U.

> *   Result<T, CommonErrorBaseClass*> as a return type when calling functions
> that return
>     Result<U, ErrorSubclass1*> and Result<V, ErrorSubclass2*>.

This can also be dealt with by providing an specialization From T* to U* provided that IsBaseOf<U, T>::value using EnableIf.

I doubt there will be any case where we cannot provide a suitable trait specialization for.  I really like the idea in comment 1.