Fix function callback lifetimes

[Protryon]

See commit in my fork for my personal fix: https://github.com/Protryon/rusty_v8/commit/0a4b6f2a1a3585b57be5849809a9e03c550520a2

Issue is that the implied lifetimes for the callback type are distinct, which does not mirror the presumably intended behavior, requiring unsafe code to overcome in third party code.

[piscisaureus]

I was under the impression that eliding the lifetimes would mean they'd all be inferred to the same lifetime. But I guess I was wrong about that...

Can you share a snippet of code that compiles but shouldn't? Then I can add a trybuild test for that.

[Protryon]

@piscisaureus I added a test in theory that should be affected, basically anything that interacts between scope, arguments, and return_value without some implicit cast happening (i.e. like with ReturnValue::set).

The example included in my commit actually doesn't quite cover it now that I look, see below:

fn fn_callback_lifetimes_proxy<'sc>(scope: &mut impl v8::ToLocal<'sc>, arg: v8::Local<'sc, v8::Value>) -> v8::Local<'sc, v8::Value> {
  v8::String::new(scope, "Hello lifetimed callback!").unwrap().into()
}

fn fn_callback_lifetimes(
  scope: v8::FunctionCallbackScope,
  args: v8::FunctionCallbackArguments,
  mut rv: v8::ReturnValue,
) {
  rv.set(fn_callback_lifetimes_proxy(scope, args.get(0)));
}

That should fail to compile because args.get(0) will have a lifetime equal to the lifetime of this callback, but not equivalent to that of the callback scope, which is asserted by fn_callback_lifetimes_proxy.

By the way, I pushed up another commit to my fork that fixes this issue further by keeping scope lifetimes fully attached to FunctionCallbackArguments and ReturnValue. This is a greatly breaking change though, as it more or less breaks using closures as a callback.

A more elegant way forward might be to not track lifetimes explicitly with FunctionCallbackArguments and ReturnValue, but to require a scope to be passed in like other places to provide that lifetime. At this point I'm just hacking stuff together to get my own project working, so I didn't drop that in my fork.

[Protryon]

I have confirmed that my own fix has solved this issue for the project I'm working on. Previously I had been transmuting the lifetimes to avoid the issue...

[Protryon]

@piscisaureus See https://doc.rust-lang.org/nomicon/lifetime-elision.html

[piscisaureus]

@Protryon

The example included in my commit actually doesn't quite cover it now that I look, see below:

fn fn_callback_lifetimes_proxy<'sc>(scope: &mut impl v8::ToLocal<'sc>, arg: v8::Local<'sc, v8::Value>) -> v8::Local<'sc, v8::Value> {
  v8::String::new(scope, "Hello lifetimed callback!").unwrap().into()
}

fn fn_callback_lifetimes(
  scope: v8::FunctionCallbackScope,
  args: v8::FunctionCallbackArguments,
  mut rv: v8::ReturnValue,
) {
  rv.set(fn_callback_lifetimes_proxy(scope, args.get(0)));
}

That should fail to compile because args.get(0) will have a lifetime equal to the lifetime of this callback, but not equivalent to that of the callback scope, which is asserted by fn_callback_lifetimes_proxy.

So, with all due respect, I think the above example is perfectly safe and therefore it should compile. What am I missing?

[piscisaureus]

@piscisaureus See https://doc.rust-lang.org/nomicon/lifetime-elision.html

Thanks. "Each elided lifetime in input position becomes a distinct lifetime parameter." 🤦🏻‍♂️