Add `get`/`set` to `property` attribute to specify custom getter/setter #841

Bogay · 2022-01-03T09:03:39Z

Bromeon

Thanks a lot for your contribution! 🙂

Custom getters/setters are a very useful feature. I think they should even supersede the clumsy {before|after}_{set|get} group of attributes; with a good design they will be powerful enough to do everything the former are capable of. Those could be removed/deprecated in a separate PR though, once this is finished.

Regarding the example from the test, leaving out the Cell logic:

#[derive(NativeClass)]
#[inherit(Node)]
struct CustomGetSet {
    #[property(get = "Self::get_foo", set = "Self::set_foo")]
    foo: i32,
}

#[methods]
impl CustomGetSet {
    fn get_foo<'a>(&'a self, _owner: TRef<Node>) -> &'a i32 {
        &self.foo
    }

    fn set_foo(&mut self, _owner: TRef<Node>, value: i32) {
        self.foo = value;
    }
}

While a nice test, this is likely not how this feature would be used in practice. The getters/setters do what #[property] already provides out of the box.

As mentioned on Discord, I think a more general scenario includes the case when there is no backing field for the property, and getters/setters access internal state in an indirect way.

For example, a container-like class that provides access to the size.

#[derive(NativeClass)]
#[inherit(Node)]
struct MyCollection {
    #[property(get = "Self::get_size")]
    size: Property<i32>,

    vec: Vec<u8>,
}

#[methods]
impl CustomGetSet {
    fn get_size(&self, _owner: TRef<Node>) -> i32 {
        self.vec.len() as i32
    }
}

Where Property is a simple marker struct:

// ZST (zero-sized type)
struct Property<T> {}

We also need to define what happens for each combination of get and set. We have the following cases:

#[property] (no attributes) keep working as-is, auto-generating getter and setter
#[property(get = "...")] has a custom getter -- I would say no setter is generated (consistent with GDScript setget)
#[property(set = "...")] has a custom setter -- also, no getter
#[property(get = "...", set = "...")] has custom getter and setter

Maybe we should require that, as soon as at least one of get or set attributes are present, the field type must be Property<T>.

In this light, it could also make sense that the default getter is not by-ref but by-value. By-ref is mostly an optimization that's more limiting and verbose to use; we could always add support for it later, but I'd start small.

Bromeon · 2022-01-03T09:23:43Z

gdnative-derive/src/lib.rs

@@ -218,6 +218,10 @@ pub fn profiled(meta: TokenStream, input: TokenStream) -> TokenStream {
 /// Call hook methods with `self` and `owner` before and/or after the generated property
 /// accessors.
 ///
+/// - `get` / `set` `= "Self::accessor_method"`


Maybe list the two separately?

/// - `get` = "Self::my_getter` /// - `set` = "Self::my_setter`

OK. Should I add an example to doc or somewhere else?

No, I think it's good for now. Maybe later, we can update the book accordingly. But before that, we need to converge on a final design.

Bromeon · 2022-01-03T10:16:35Z

gdnative-derive/src/native_script/property_args.rs

+                if let Some(old) = self.get.replace(path) {
+                    return Err(syn::Error::new(
+                        pair.span(),
+                        format!("there is already a get value set: {:?}", old),


"get value set" is a bit confusing, maybe "there is already a 'get' attribute with value:"

same below

Agree. Actually I copy-paste this from other arms inside the same method (add_pair) and others has almost the same format of error message. How about use a pair of back quote ("`") to surround the variable name?
For example, "there is already a `get` value set:\n{:?}". I think this will be more convenient to write a macro to generate each arm? (But I am not familiar with macro, not sure whether it is suitable to use in this case)

You can also use backticks, they will likely not be formatted though.

Why a macro? Not sure I understand it...

Because every arm (e.g. "set", "get") has a very similar implementation, only differs in field name. I think it will be better if these arms can be generated by macro. (Like how this crate implement Export trait for core types (source))

Bromeon · 2022-01-03T10:21:36Z

test/src/test_derive.rs

+                foo: 0,
+            }
+        }
+        fn get_foo<'a>(&'a self, _owner: TRef<Node>) -> &'a i32 {


This lifetime can be inferred, no? There's no other reference parameter than self.

fn get_foo(&self, _owner: TRef<Node>) -> &i32 {

It's a very subtle difference but TRef does have an elided lifetime parameter here: TRef<Node> = TRef<'_, Node>.

Bogay · 2022-01-03T11:56:13Z

@Bromeon I think the Property idea really make sense. Sorry I didn't notice that discussion on Discord before.
So I should implement it in this PR, right?

Bromeon · 2022-01-03T12:58:01Z

Sorry I didn't notice that discussion on Discord before.

No worries, I should have copied it to the GitHub issue. Discord is temporary aether ✨

So I should implement it in this PR, right?

If it's possible with reasonable effort, gladly. But don't hesitate to reach out when you encounter issues or want someone else to jump in 👍

chitoyuu · 2022-01-04T05:36:56Z

#[property] (no attributes) keep working as-is, auto-generating getter and setter

#[property(get = "...")] has a custom getter -- I would say no setter is generated (consistent with GDScript setget)

#[property(set = "...")] has a custom setter -- also, no getter

#[property(get = "...", set = "...")] has custom getter and setter

Maybe we should require that, as soon as at least one of get or set attributes are present, the field type must be Property<T>.

Generally agree. I believe it might be better if we allowed users to declare auto-generated accessors explicitly, a la C#, and only require the field to be Property<T> if none of the default logic is used:

#[property(get, set = "...")] - custom setter, auto-generated getter
#[property(get = "...", set)] - custom getter, auto-generated setter
#[property(get, set)] - same as #[property] without arguments, but explicit

Having this option is good for many simple use cases where the custom setters boil down to "assign with side effects". For example:

struct SomeType {
    #[property(get, set = "Self::set_foo")]
    foo: Foo,
    bar: Bar,
    update_required: bool,
}

#[methods]
impl SomeType {
    #[export]
    fn _process(&mut self, _owner: TRef<'_, Node>, _delta: f32) {
        if self.update_required {
            self.update_required = false;
            unimplemented!("do expensive stuff");
        }
    }

    #[export]
    fn set_foo(&mut self, _owner: TRef<'_, Node>, value: Foo) {
        self.bar = value.compute_bar();
        self.foo = value;
        self.update_required = true;
    }
}

This becomes visibly more verbose if we require that the value be stored separately from the property declaration:

struct SomeType {
    #[property(get = "Self::get_foo", set = "Self::set_foo")]
    foo: Property<Foo>,
    foo_storage: Foo, // New field

    bar: Bar,
    update_required: bool,
}

#[methods]
impl SomeType {
    #[export]
    fn _process(&mut self, _owner: TRef<'_, Node>, _delta: f32) {
        if self.update_required {
            self.update_required = false;
            unimplemented!("do expensive stuff");
        }
    }

    // New method
    #[export]
    fn get_foo(&self, _owner: TRef<'_, Node>) -> &Foo {
        &self.foo_storage // Error-prone: users are tempted to write `&self.foo`, which won't compile
    }

    #[export]
    fn set_foo(&mut self, _owner: TRef<'_, Node>, value: Foo) {
        self.bar = value.compute_bar();
        self.foo_storage = value; // Error-prone, same as above.
        self.update_required = true;
    }
}

Bromeon · 2022-01-04T07:31:08Z

@chitoyuu That's a good addendum, thanks for mentioning C#.

I fully agree with your suggestions!

Bogay · 2022-02-02T06:26:21Z

@Bromeon I think it's ready to be reviewed now.

Bromeon

Thanks a lot for the update, it's really appreciated! 🙂

Added a few comments.

Clippy is currently failing, you can run it locally with this command:

cargo clippy --workspace --features gdnative/async,gdnative/serde -- -D clippy::style -D clippy::complexity -D clippy::perf -D clippy::dbg_macro -D clippy::todo -D clippy::unimplemented

(I should really write a local .sh which emulates CI -- there's an old commit hook but it's quite outdated).

Would be nice if you squashed the commits. If you wish, you can also reduce it to 2 (one with the initial implementation and one with the Property one), but please do what's easiest/least work for you. A very easy way is

git reset --soft origin/master && git commit

gdnative-core/src/export/property.rs

gdnative-derive/src/lib.rs

gdnative-derive/src/native_script.rs

gdnative-derive/src/native_script/property_args.rs

Bromeon · 2022-02-03T21:20:35Z

test/src/test_derive.rs

+struct CustomGetSet {
+    pub get_called: Cell<i32>,
+    pub set_called: Cell<i32>,
+    #[allow(dead_code)]


It seems like we always need #[allow(dead_code)] whenever Property is involved (and obviously not accessed)?

I wonder if adding a

quote! { #[allow(dead_code)] };

might do the job...

But this PR is already quite big, I'm also fine if we do this at some point in the future 🙂

test/src/test_derive.rs

these attributes are used to set custom getter and setter.

…et are provided

Bogay · 2022-02-05T06:51:51Z

@Bromeon I have finished the most of changes. But not sure how to generate #[allow(dead_code)] in macro. It seems that proc_macro_derive can't modify original struct?

Bromeon · 2022-02-05T11:20:12Z

bors try

bors · 2022-02-05T11:27:25Z

try

Build succeeded:

full-ci

Bromeon · 2022-02-05T15:06:07Z

One thing I noticed during testing:

#[derive(NativeClass)]
#[inherit(Node)]
struct MyStruct {
    #[property(get = "Self::get_i32")]
    #[allow(dead_code)]
    extra: Property<i32>,
}

#[methods]
impl MyStruct {
    fn new(_owner: &Node) -> Self {
        MyStruct {
            extra: Property::default()
        }
    }

    fn get_i32(&self, _owner: &Node) -> i32 {
        53
    }
}

fails to compile with:

error[E0308]: mismatched types
 --> examples\array_export\src\lib.rs:5:10
  |
5 | #[derive(NativeClass)]
  |          ^^^^^^^^^^^ expected `&gdnative::prelude::Node`, found struct `gdnative::prelude::TRef`
  |
  = note: expected reference `&gdnative::prelude::Node`
                found struct `gdnative::prelude::TRef<'_, gdnative::prelude::Node>`
  = note: this error originates in the derive macro `NativeClass` (in Nightly builds, run with -Z macro-backtrace for more info)

Changing the getter's parameter type &Node to TRef<Node> works.

But this seems to be a limitation of the builder in general and has existed before this PR, so I don't think we have to fix it here. It could be revisited when addressing #850.

Bromeon

The logic is more complex than I expected, I think we overlooked a few cases.

As far as I can see, we have now the following combinations (+their mirrored parts).
It looks like T and Property<T> are mutually exclusive? Also possible that I'm missing something.

Field type ->	`i32`	`Property<i32>`
`#[property]`	✔️ default get/set	❌
`#[property(get, set)]` (same as above)	✔️ default get/set	❌
`#[property(get)]`	✔️ default get (no set)	❌
`#[property(get="path")]`	❌	✔ custom get (no set)
`#[property(get="path", set)]`	✔️ custom get, default set	❌
`#[property(get="path", set="path")]`	❌	✔ custom get + set

We might also want to think about some smarter testing...

This just to write down my ideas, we can do the full thing outside the scope of this PR, also to unlock the CI for first-contributors. What do you think?

Bromeon · 2022-02-05T15:28:26Z

gdnative-derive/src/native_script.rs

+                && config.get.as_ref().map(|get| !matches!(get, PropertyGet::Default)).unwrap_or(false)
+                // custom setter used
+                && config.set.as_ref().map(|set| !matches!(set, PropertySet::Default)).unwrap_or(false)


These two conditions are quite hard to understand, with ref-conversion, map, negation, and unwrap_or.
See suggestion comment further below for possible simplification.

And small nitpick, could you leave an empty line between logically related blocks?

For example, empty line before

// #[property] is not attached on `Property<T>`

would make clear that this is a new condition being checked. Same for others.

Bromeon · 2022-02-05T15:37:14Z

gdnative-derive/src/native_script.rs

+                        "The `#[property]` attribute can only be used on a field of type `Property`, \
+                        if a path is provided for both get/set method(s)."


I realized this error message is ambiguous: "can only be used on field of type Property if..." can be understood as "you used it on field type Property but are not allowed so".

See suggestion below for possible error message.
Sorry for the back and forth...

Bromeon · 2022-02-05T15:52:57Z

gdnative-derive/src/native_script.rs

+                        if a path is provided for both get/set method(s)."
+                    ));
+                }
+                // if both of them are not set, i.e. `#[property]`. implicitly use both getter/setter


There is an extra case we should handle here, basically the opposite of the above.
All these (and their symmetric equivalents) should be an error:

#[property] field: Property<i32>, #[property(get)] field: Property<i32>, #[property(get, set ="path")] field: Property<i32>,

See suggestion below for possible implementation.

Would you have the time to add this to the tests?

Bromeon · 2022-02-05T17:23:47Z

gdnative-derive/src/native_script.rs

+                // #[property] is not attached on `Property<T>`
+                if property_ty.is_none()
+                // custom getter used
+                && config.get.as_ref().map(|get| !matches!(get, PropertyGet::Default)).unwrap_or(false)
+                // custom setter used
+                && config.set.as_ref().map(|set| !matches!(set, PropertySet::Default)).unwrap_or(false)
+                {
+                    return Err(syn::Error::new(
+                        ident.span(),
+                        "The `#[property]` attribute can only be used on a field of type `Property`, \
+                        if a path is provided for both get/set method(s)."
+                    ));
+                }
+                // if both of them are not set, i.e. `#[property]`. implicitly use both getter/setter
+                let (get, set) = if config.get.is_none() && config.set.is_none() {


This is what I tried. Test string needs to be adapted too.
I got some error procedural macro API is used outside of a procedural macro however; not sure why exactly.

Suggested change

// #[property] is not attached on `Property<T>`

if property_ty.is_none()

// custom getter used

&& config.get.as_ref().map(|get| !matches!(get, PropertyGet::Default)).unwrap_or(false)

// custom setter used

&& config.set.as_ref().map(|set| !matches!(set, PropertySet::Default)).unwrap_or(false)

{

return Err(syn::Error::new(

ident.span(),

"The `#[property]` attribute can only be used on a field of type `Property`, \

if a path is provided for both get/set method(s)."

));

}

// if both of them are not set, i.e. `#[property]`. implicitly use both getter/setter

let (get, set) = if config.get.is_none() && config.set.is_none() {

// Attribute is #[property] (or has other arguments which are not relevant here)

let is_standalone_attribute = config.get.is_none() && config.set.is_none();

// Attribute is #[property(get)] or #[property(get, set="path")]

let has_default_getter = matches!(config.get, Some(PropertyGet::Default));

// Attribute is #[property(set)] or #[property(get="path", set)]

let has_default_setter = matches!(config.set, Some(PropertySet::Default));

// Field type is not `Property<T>`

if property_ty.is_none()

&& !is_standalone_attribute

&& (has_default_getter || has_default_setter)

{

return Err(syn::Error::new(

ident.span(),

"A `#[property]` attribute which does not use any of the default `get` and `set` \

arguments -- e.g. #[property], #[property(get)] or #[property](get, set=\"path\") -- \

requires the field type to be `Property<T>`."

));

}

// Field type is `Property<T>`

if property_ty.is_some()

&& (is_standalone_attribute || has_default_getter || has_default_setter)

{

return Err(syn::Error::new(

ident.span(),

"The `#[property]` attribute requires explicit paths for `get` and `set` argument; \

the defaults #[property], #[property(get)] and #[property(set)] are not allowed."

));

}

// if both of them are not set, i.e. `#[property]`. implicitly use both getter/setter

let (get, set) = if is_standalone_attribute {

I have got the same error message when running test. It occurs when I got proc_macro::TokenStream in a test function.

It seems like I missed a ! in my logic. For some reason, this doesn't just fail the test, but creates this entirely unrelated error message.

Bromeon · 2022-02-05T20:05:51Z

Alternative semantics according to Discord discussion:

Field type ->	bare `T`	`Property<T>`
`#[property]`	✔️ default get/set	❌
`#[property(get, set)]` (same as above)	✔️ default get/set	❌
`#[property(get)]`	✔️ default get (no set)	❌
`#[property(get="path")]`	⚠️ custom get (no set) (consider using `Property<T>`)	✔ custom get (no set)
`#[property(get="path", set)]`	✔️ custom get, default set	❌
`#[property(get="path", set="path")]`	⚠️ custom get + set (consider using `Property<T>`)	✔ custom get + set

⚠️ means that it's possible to use #[property] on a field of bare type T, but godot-rust will never access the field directly.

As such, the field type might as well be Property<T>. If you do use the field on the Rust side, a bare type T can still be more ergonomic, as it doesn't force you to maintain Property<T> in addition to a backing field T.

Bromeon

Thanks a lot for the great work so far! 🙂

As discussed on Discord, I tried to tweak the logic a bit according to the table. I managed to get the unit tests running by working with proc_macro2::TokenStream instead of proc_macro1::TokenStream (using a thin wrapper function). Also added Property to prelude and documented it.

Let me know what you think, I think it's soon ready to merge!

bors try

Bromeon · 2022-02-05T23:59:49Z

GitHub Actions is down 📉
https://twitter.com/githubstatus/status/1490089225819148289

Will retry tomorrow.

Bogay

@Bromeon LGTM!
Just one small suggestion about derive_native_class (but I think it's also OK to leave it as is), other parts are awesome.
Thanks a lot about the documentation, I really can't explain it so detailed! 👍

Bogay · 2022-02-06T07:05:01Z

gdnative-derive/src/native_script.rs

@@ -49,6 +49,10 @@ pub(crate) fn impl_empty_nativeclass(derive_input: &DeriveInput) -> TokenStream2
 }

 pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result<TokenStream, syn::Error> {


This wrapper seems to unnecessary?
I found that other function in gdnative_derive (e.g. methods::derive_methods, variant::derive_to_variant, etc.) returns a Result<proc_macro2::TokenStream, syn::Error>, and convert the result into proc_macro::TokenStream in lib.rs.
I think it might be more consistent if native_script::derive_native_class also returns proc_macro2::TokenStream?

Bromeon · 2022-02-06T09:19:11Z

Very good point, I removed the wrapper.
Thanks for the feedback and the whole effort! Glad to move ahead with this feature 🚀

bors r+

841: Add `get`/`set` to `property` attribute to specify custom getter/setter r=Bromeon a=Bogay Fix #547 Co-authored-by: bogay <[email protected]> Co-authored-by: Jan Haller <[email protected]>

bors · 2022-02-06T09:28:44Z

try

Timed out.

bors · 2022-02-06T09:29:03Z

Build failed:

full-ci

Changes: * Logic that allows/disallows property <-> field type combinations * Unit tests * Create internal proc_macro2 derive that can be used in tests * Add Property to prelude * Extensive documentation for Property

Bromeon · 2022-02-06T09:44:02Z

MSRV 1.51 with its array iteration is a sneaky one. Glad they fixed this in more recent Rust...

bors r+

bors · 2022-02-06T09:55:51Z

Build succeeded:

full-ci

Bromeon reviewed Jan 3, 2022

View reviewed changes

Bromeon added c: export feature Adds functionality to the library labels Jan 3, 2022

Bromeon added c: export Component: export (mod export, derive) and removed c: export labels Jan 4, 2022

Bogay force-pushed the property-getter-setter branch from d9d4744 to 3416667 Compare February 2, 2022 05:42

Bogay requested a review from Bromeon February 2, 2022 06:22

Bromeon requested changes Feb 3, 2022

View reviewed changes

Bogay added 2 commits February 4, 2022 18:37

feat(property): add get, get_ref and set attributes

5dc6382

these attributes are used to set custom getter and setter.

feat(property): enforce it should be used on Property if both get/s…

de96485

…et are provided

Bogay force-pushed the property-getter-setter branch from fa0d2c4 to de96485 Compare February 4, 2022 18:49

Bogay requested a review from Bromeon February 5, 2022 06:49

bors bot added a commit that referenced this pull request Feb 5, 2022

Try #841:

12e544f

Bromeon requested changes Feb 5, 2022

View reviewed changes

Bromeon reviewed Feb 5, 2022

View reviewed changes

Bromeon approved these changes Feb 5, 2022

View reviewed changes

bors bot added a commit that referenced this pull request Feb 6, 2022

Try #841:

d11c45a

Bogay commented Feb 6, 2022

View reviewed changes

Bromeon force-pushed the property-getter-setter branch from 4a03dc1 to 44bb3f9 Compare February 6, 2022 09:12

bors bot added a commit that referenced this pull request Feb 6, 2022

Merge #841

5efb347

841: Add `get`/`set` to `property` attribute to specify custom getter/setter r=Bromeon a=Bogay Fix #547 Co-authored-by: bogay <[email protected]> Co-authored-by: Jan Haller <[email protected]>

Bromeon added this to the v0.10.0 milestone Feb 6, 2022

Handle all #[property(get, set)] combinations + test

768020f

Changes: * Logic that allows/disallows property <-> field type combinations * Unit tests * Create internal proc_macro2 derive that can be used in tests * Add Property to prelude * Extensive documentation for Property

Bromeon force-pushed the property-getter-setter branch from 44bb3f9 to 768020f Compare February 6, 2022 09:43

bors bot merged commit c112d06 into godot-rust:master Feb 6, 2022

Bogay deleted the property-getter-setter branch July 5, 2022 09:44

		"The `#[property]` attribute can only be used on a field of type `Property`, \
		if a path is provided for both get/set method(s)."

		@@ -49,6 +49,10 @@ pub(crate) fn impl_empty_nativeclass(derive_input: &DeriveInput) -> TokenStream2
		}

		pub(crate) fn derive_native_class(derive_input: &DeriveInput) -> Result<TokenStream, syn::Error> {

Add get/set to property attribute to specify custom getter/setter #841

Add get/set to property attribute to specify custom getter/setter #841

Conversation

Bogay commented Jan 3, 2022

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bogay commented Jan 3, 2022 • edited Loading

Bromeon commented Jan 3, 2022

chitoyuu commented Jan 4, 2022

Bromeon commented Jan 4, 2022

Bogay commented Feb 2, 2022

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bogay commented Feb 5, 2022 • edited Loading

Bromeon commented Feb 5, 2022

bors bot commented Feb 5, 2022

try

Bromeon commented Feb 5, 2022

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Bromeon Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Bromeon Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Feb 5, 2022 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon commented Feb 5, 2022 • edited Loading

Bogay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Feb 6, 2022

bors bot commented Feb 6, 2022

try

bors bot commented Feb 6, 2022

Bromeon commented Feb 6, 2022

bors bot commented Feb 6, 2022

Add `get`/`set` to `property` attribute to specify custom getter/setter #841

Add `get`/`set` to `property` attribute to specify custom getter/setter #841

Bromeon left a comment •

edited

Loading

Bogay commented Jan 3, 2022 •

edited

Loading

Bogay commented Feb 5, 2022 •

edited

Loading

Bromeon left a comment •

edited

Loading

Bromeon Feb 5, 2022 •

edited

Loading

Bromeon Feb 5, 2022 •

edited

Loading

Bromeon Feb 5, 2022 •

edited

Loading

Bromeon commented Feb 5, 2022 •

edited

Loading

Bromeon commented Feb 5, 2022 •

edited

Loading