Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create a macro to create interfaces for externally defined types #200

Open
Tracked by #767
karroffel opened this issue Sep 6, 2019 · 3 comments
Open
Tracked by #767

create a macro to create interfaces for externally defined types #200

karroffel opened this issue Sep 6, 2019 · 3 comments
Labels
c: export Component: export (mod export, derive) feature Adds functionality to the library
Milestone

Comments

@karroffel
Copy link
Member

Objects that have scripts written in other languages attached can be passed to methods in Rust, the functionality provided by those scripts can't be accessed easily.

This is a proposal to create a procedural macro to use a trait as the description for an interface of an object.

An implementation of such a trait would use godot_variant_call to perform the calls.
All functions in the trait have to return a result type to represent the varcall failing.

When an object is used as an implementation of this interface no immediate checks are performed. Doing type-checking is not possible in the general case, so checks should be performed lazily.

This could work like this:

# adder.gd
extends Reference

var _value = 0

func add(amount: int) -> void:
    _value += amount

func tell() -> int:
    return _value
// src/lib.rs

// ... [ snip ] ...

#[gdnative::external_interface]
trait Adder: ExtInterface<Base = Reference> {

    fn add(&mut self, amount: i64) -> ExtResult<()>;
    fn tell(&self) -> ExtResult<i64>;
}

#[derive(gdnative::ExtInterfaceImpl)]
#[gdnative::ext_interface(Adder)]
struct AdderImpl<'a>;

/*
// possible definition of impl-trait
trait ExtInterfaceImpl<'a> {
    type Interface: ExtInterface;
    fn from(base: &'a Self::Interface::Base) -> Self;
}
*/

// ... [ snip ] ...

#[export]
fn set_to_one_hundred(&self, _owner: Reference, adder: Reference) {
    use gdnative::ExtInterfaceImpl;

    let adder = AdderImpl::from(&adder);
    let current = if let Ok(val) = adder.tell() {
        val
    } else {
        return;
    }
    let difference = 100 - current;
    if difference <= 0 {
        return;
    }

    // TODO check result, maybe the type does not actually implement the interface
    let _ = adder.add(different);
    
    let new_value = adder.tell().unwrap_or(0):
    assert_eq!(new_value, 100);
}
@karroffel karroffel added feature Adds functionality to the library c: export Component: export (mod export, derive) labels Sep 6, 2019
@ghost
Copy link

ghost commented Sep 6, 2019

This seems very nice to have, but with two types to define and a bunch of attributes to sprinkle around, I find the proposed API a little bit ugly. I would prefer a (procedural) function-like macro that defines a struct directly in this case.

Another problem that I think should be considered before implementation is the interaction with non-refcounted types. Calling methods on these objects are unsafe, as godot_variant_call only checks ObjectDB validity when compiled with DEBUG_ENABLED. With the current proposal, however, one can simply define an interface and call these methods from a safe context.

Also, since all calls are performed through godot_variant_call, there seems to be no point for the trait to have a specific base class.


All considered, I would propose an alternate syntax as follows:

external_interface! {
    // TODO: Maybe allow users to add custom trait bounds on T?
    // TODO: Maybe the syntax is too close to normal structs and might be confusing?
    //       The macro invocation and interface "keyword" should signify the difference though.
    pub interface struct Adder<T: GodotObject> {
        // Only implemented for reference types
        pub fn add(&mut self, amount: i64) -> ExtResult<()>;

        // Unsafe aliases must be explicitly named, as automatic addition of `unsafe_` versions
        // may result in name collisions
        pub unsafe fn unsafe_add = add(&mut self, amount: i64) -> ExtResult<()>;

        // Implemented for all GodotObject, but only unsafe version
        pub unsafe fn tell(&self) -> ExtResult<i64>;
    }
}

...which would expand to something like:

pub struct Adder<T> {
    variant: Variant,
    _marker: PhantomData<T>,
}

// TODO: Maybe a separate trait NewRef for semantic clarity?
impl<T: GodotObject + Clone> Adder<T> {
    pub fn add(&mut self, amount: i64) -> ExtResult<()> {
        // - snip -
    }
}

impl<T: GodotObject> Adder<T> {
    pub unsafe fn unsafe_add(&mut self, amount: i64) -> ExtResult<()> {
        // - snip -
    }

    pub unsafe fn tell(&self) -> ExtResult<i64> {
        // - snip -
    }
}

impl<T: GodotObject> From<T> for Adder<T> {
    fn from(base: T) -> Adder {
        Adder(base.to_variant(), PhantomData)
    }
}

@karroffel
Copy link
Member Author

karroffel commented Sep 7, 2019

but with two types to define and a bunch of attributes to sprinkle around, I find the proposed API a little bit ugly.

Yes, I understand and also think that it's not the nicest API, however what I like about it is that there is no "custom syntax" involved (if you don't count attributes as custom syntax) and also an IDE should be able to pick up all of this without effort.

One of the biggest problems I personally had with the bindings in the past was that it used a DSL inside a macro to define classes (which is still possible). IDEs had a hard time with this and also users had to remember the custom syntax. I personally think the procedural macro solution using attributes is a lot clearer, this is something that I also wanted to aim for with this.

We could probably easily get rid of the AddrImpl<'a> type by having the from<T: ToVariant>(value: T) -> Self in the super-trait, that should leave us with only a single attribute to place (as the base class is not really needed anyway).

That said, I am not married to this design, I would still prefer it to be based around attributes rather than direct macro invokations, just for clarity's sake.


Calling methods on these objects are unsafe, as godot_variant_call only checks ObjectDB validity when compiled with DEBUG_ENABLED.

That is interesting. We could also just force all of those function to always be marked as unsafe. I would say that is a fair thing to do as the implementor of such an interface could do whatever it wants, so in order to not have huge workarounds for this highly dynamic interaction I think unsafe-only is fair.


Also, since all calls are performed through godot_variant_call, there seems to be no point for the trait to have a specific base class.

Indeed, I was including it as it would disallow using a Node2D when you expect the interface to only be implementable by Reference types, for example. Probably this is not needed, that is a good point!


There is some prior art of this in the form of WebAssembly interface types, which are also implemented in wasmtime. It uses traits and procedural macros to define interfaces that might be implemented in other languages.

Here an example

@ghost
Copy link

ghost commented Sep 8, 2019

There is some prior art of this in the form of WebAssembly interface types, which are also implemented in wasmtime. It uses traits and procedural macros to define interfaces that might be implemented in other languages.

I read its implementation a bit. It looks like they're replacing the trait declaration with a struct that has the same name. An extra Impl type can surely be avoided by the same approach, then.

That's said, this is certainly magic, and I'm not sure if that will count as a form of "custom syntax", as it'll still require specific knowledge of the bindings for users to use properly, otherwise they'll get seemingly unreasonable error messages when they try to type impl Interface for or dyn Interface. IDEs might get a bit better at code completion, though.

Personally, I think this is a worse kind of custom syntax compared to function-like macros, as attributes are implicit in their custom-ness. When you use a function-like macro, the invocation reminds you that you're not using the base language. Attributes hide this away, and that is certainly useful in many cases, but "type trait to mean struct" is a bit too much, I'm afraid.

force all of those function to always be marked as unsafe

That's fair.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: export Component: export (mod export, derive) feature Adds functionality to the library
Projects
None yet
Development

No branches or pull requests

2 participants