Object::call_with()

[dherman]

Add Object::call_with() convenience method to call a method on an object.

Example:

let s: Handle<JsString> = date.call_with(&mut cx, "toLocaleString")?
    .arg(cx.string("en-GB"))
    .apply(&mut cx)?;

Closes #873.

Design questions

• Are we OK with the inconsistencies with JsFunction::call_with()?
  • This is fallible.
  • This takes a mutable reference to the context.
• Alternative names: date.call_with() vs date.call_method() vs date.call_method_with()

[kjvalencik]

I like call_with because it's short. call_method_with is way too long.

call_method also seems reasonable.

[kjvalencik]

@dherman This looks great! Other than the small suggestion to simplify, my only open question is on naming.

I'm torn between call_with and call_method.

[mathieulj]

Alternative names: date.call_with() vs date.call_method() vs date.call_method_with()

I personally prefer call_method() as it makes more of a distinction from JsFunction::call_with() and communicates the extra layer while scanning/reviewing code. I feel as though I would get caught by confusion a few times when reviewing if the two .call_with methods return different types.

call_method_with is way too long.

I do agree it's a little unwieldy, but maybe not a deal breaker.

[kjvalencik]

@mathieulj Thanks for the feedback!

I feel as though I would get caught by confusion a few times when reviewing if the two .call_with methods return different types.

Apologies if this seems like too much of a nit, but the two methods return the same type, CallOptions. However, I think your argument stands in respect to method arguments.

[mathieulj]

Apologies if this seems like too much of a nit, but the two methods return the same type, CallOptions. However, I think your argument stands in respect to method arguments.

No worries, my thoughts are in flux, I value differences in opinions and I don't feel particularly strongly about it.

The two both return CallOptions but it is wrapped in a Result for this new one (Object::call_with) and returned bare for JsFunction::call_with. In most code bases, this will translate to just an extra ? and is probably not something to fret about on it's own. My point was about consistency and does also apply to arguments as you pointed out.

[dherman]

The inconsistencies both in arguments types and in fallibility of the return type bothers me too, @mathieulj. And I feel like without the word "method" it sounds too much like we're calling the object as if it were a function. So I think I'm pretty anti-call_with() at this point.

@kjvalencik I'm sympathetic to call_method_with() being too inconveniently long. I could get on board with call_method(), it just makes me feel a little guilty for having penalized JsFunction::call_with() with the extra suffix. But maybe in practice the difference in naming conventions will make it easier to remember the differences in the type signatures.

[kjvalencik]

I'm sufficiently convinced that call_with is the least optimal choice. I slightly favor call_method, but can appreciate the symmetry in call_method_with. Maybe the extra 5 characters really don't make that much of a difference?

 // I don't mind it at all when chaining.
 let n: Handle<JsNumber> = obj
-    .call_method(&mut cx, "getN")?
+    .call_method_with(&mut cx, "getN")?
     .arg(cx.number(42))
     .apply(&mut cx)?;
    
// It's a little awkward on one line, but not terrible
- let n: Handle<JsNumber> = obj.call_method(&mut cx, "getN")?.applyt(&mut cx)?;
+ let n: Handle<JsNumber> = obj.call_method_with(&mut cx, "getN")?.applyt(&mut cx)?;

[dherman]

Thanks for the example, @kjvalencik! I'm leaning towards call_method_with() for the consistency, but I feel like only real-world experience will tell. Should we go with call_method_with() for now, with an option to rename it via alias-and-deprecate later if usage experience shows it's too unwieldy?

[kjvalencik]

@dherman I like that plan!

[kjvalencik]

Nit, these additions are throwing unused warnings on the legacy backend. Probably not a big deal because that's getting removed very soon.

[dherman]

death to legacy…

[kjvalencik]

Flagging this for follow-up. JsValue::new_internal should be marked unsafe since it allows arbitrarily extending a lifetime.

This usage of it is safe because it's bound by a new Context which will always be the shortest possible.

[kjvalencik]

@dherman This looks fantastic and is ready to merge! I filed a nit and a follow-up, but no action is needed.