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

Returns the dereferenced type in the exported method #870

Merged
merged 3 commits into from
May 10, 2022

Conversation

B-head
Copy link
Contributor

@B-head B-head commented Mar 5, 2022

Feature

Add #[export(deref_return)] to allow exported methods to return dereferenced types. This can be used with types that implement Deref traits.

For example.

#[derive(NativeClass)]
#[no_constructor]
struct Foo {
    bar: i32,
    baz: Rc<RefCell<f64>>,
}

#[methods]
impl Foo {
    #[export(deref_return)]
    fn get_bar(&self, _owner: &Reference) -> &i32 {
        &self.bar
    }

    #[export(deref_return)]
    fn get_baz(&self, _owner: &Reference) -> std::cell::Ref<f64> {
        self.baz.borrow()
    }
}

This feature has the following advantages.

  • Types such as std::cell::Ref<f64> can be written in the return type signature.
  • Clarify that the expoted method is not pass by reference to Godot engine, but converting and passing by value to Godot engine.
  • There is no conflict in the implementation of ToVariant Trait.

Incidental changes

Returning a reference without applying deref_return will display the following warning.

warning: use of deprecated macro `::gdnative::export::deprecated_reference_return`: This function does not actually pass by reference to the Godot engine. You can clarify by writing #[export(deref_return)].

Compatibility

The godot_wrap_method! macro is not compatible because its definition has changed. (Can be fixed for compatibility if needed)

@Bromeon
Copy link
Member

Bromeon commented Mar 6, 2022

Thanks for the PR!

Types such as std::cell::Ref<f64> can be written in the return type signature.

Just to understand -- why do you want to do that?

Since GDScript doesn't support such types, it's not immediately clear how the type would be marshalled. I mean this from a perspective that #[export] methods also act as an API to be invoked from GDScript. Wouldn't it be easier to just return f64 as an explicitly copied value?

@Bromeon Bromeon added c: export Component: export (mod export, derive) feature Adds functionality to the library labels Mar 6, 2022
@B-head
Copy link
Contributor Author

B-head commented Mar 6, 2022

Just to understand -- why do you want to do that?

Since GDScript doesn't support such types, it's not immediately clear how the type would be marshalled. I mean this from a perspective that #[export] methods also act as an API to be invoked from GDScript. Wouldn't it be easier to just return f64 as an explicitly copied value?

In this example f64, but usually what I want to pass by reference is type like Vec<_>.
To clone large values is costly. Therefore, I do not want to clone it many times.

If the return type is Variant, it avoids cloning twice, but there is no type checking.
Later, when #850 is implemented and the method is used from the Rust side, Variant type is difficult to use with return type.

#[derive(NativeClass)]
#[no_constructor]
struct Foo {
    bar: Rc<RefCell<Vec<i32>>>,
}

#[methods]
impl Foo {
    #[export]
    fn get_bar(&self) -> Variant {
        self.bar.borrow().to_variant()
    }
}

@parasyte
Copy link
Contributor

parasyte commented Mar 7, 2022

Is there a reason that #[export] does not support return values that implement ToVariant? Having that capability would solve your need, AFAICT. Even just this is enough to make it work:

diff --git a/gdnative-core/src/export/macros.rs b/gdnative-core/src/export/macros.rs
index 92b3bea..aab3c12 100644
--- a/gdnative-core/src/export/macros.rs
+++ b/gdnative-core/src/export/macros.rs
@@ -56,7 +56,7 @@ macro_rules! godot_wrap_method_inner {
                                     $($pname,)*
                                     $($opt_pname,)*
                                 );
-                                gdnative::core_types::OwnedToVariant::owned_to_variant(ret)
+                                gdnative::core_types::ToVariant::to_variant(&ret)
                             }
                         })
                         .unwrap_or_else(|err| {

But I can only assume there is a reason this wasn't done in the first place. The best reason I can think of is that feels unusual to a Rust programmer because it will implicitly clone from borrows, as in the example below.

#[derive(NativeClass)]
#[no_constructor]
struct Foo {
    bar: Vec<f32>,
}

#[methods]
impl Foo {
    #[export]
    fn get_bar(&self, _base: &Node) -> &[f32] {
        &self.bar
    }
}

B-head added 3 commits May 1, 2022 14:51
- Enable to use Path syntax.
- Remove abstractions that inhibit expansion.
- Remove code that only accepts [export = "wrong"].
- Add error message.
The ways of emit warnings is a terrible hack.
This is because there is no way to emit warnings
from macros in stable Rust.

Follow these steps to emit warnings.
- Detect whether reference types are used in derive_methods().
- Expand the call to deprecated_reference_return!() macro to user code.
@bors bors bot merged commit c3f5fe8 into godot-rust:master May 10, 2022
@B-head B-head deleted the deref_return branch May 11, 2022 00:29
KarimHamidou added a commit to KarimHamidou/godot-rust that referenced this pull request Feb 6, 2023
GuilhermeOrceziae added a commit to GuilhermeOrceziae/godot-rust that referenced this pull request Feb 9, 2023
hesuteia added a commit to hesuteia/godot-rust that referenced this pull request Feb 11, 2023
ecobiubiu added a commit to ecobiubiu/open-rust that referenced this pull request Mar 30, 2023
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

Successfully merging this pull request may close these issues.

3 participants