DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
504 stars 63 forks source link

Remove `Class::from_js_ref` #168

Closed DelSkayn closed 1 year ago

DelSkayn commented 1 year ago

This PR is an attempt to fix #167.

It removes Class::from_js_ref as it's implementation was unsound. This has some significant consequences.

&C where C: ClassDef no longer implements FromJs. This trait implementation is what allowed methods to borrow class objects in their arguments. With this PR it is no longer possible to pass a function like fn foo(a: &C) -> Foo{ ... } directly into rust. Instead if one wants to borrow a class def object the function should instead be fn foo(a: class::Ref<C>) -> Foo{ ... }. Ref is a new type which functions as a borrowed class def objects. It keeps the object which holds the class def alive and implements deref into C. This makes referencing C safe and will allow us to implement a RefCell like borrowing algorithm in the future.

In order to facilitate methods with &self to still function I implemented the SelfMethod type. This type implements AsFunction for with a signature similar to fn foo(&self, arg1: (), arg2) -> Foo. This type is required due to implementing Method over both Fn(T) and Fn(&T) separately is not allowed. SelfMethod only allows taking self as a borrow.

I think the current way we handle passing functions to javascript is becoming a bit unwieldy. The lifetime requirements are rather complicated and when a function is not completely right rust returns errors which are almost completely unhelpful. We should probably move away from all the function wrappers and maybe rely somewhat more on macros to implement functions, only allowing the function wrappers for closures.

This current PR is to remove the unsound behavior with the current approach and we should probably look at rewriting the class and function modules in a later version.