More complete #[export] implementation #177

ttencate · 2023-03-13T08:21:53Z

Also:

Remove unused property argument.
Rename arguments getter and setter to get and set to stay closer to GDScript and save keystrokes.
Check at compilation time that the referenced getter and setter actually exist (otherwise Godot gives a cryptic "invalid get/set index" error).

See #3.

TBD:

strip_quotes should go away, not sure if it even works correctly if using e.g. raw string literals. Use an actual Rust parser? Omit the quotes from the argument instead, i.e. get = get_my_field instead of get = "get_my_field"? See discussion below.
~~Make KvParser::parse take a closure so we can check that all fields have been consumed~~ See discussion below.
Omitting one of getter/setter should make field write/read only
Use get/set without arguments to generate a default one
Make generated getters and setters pub since they're public to Godot anyway

Bromeon

Thank you!

In general, I would keep the same behavior as gdnative, documented here.
This means:

#[export] generates both setter and getter
#[export(get, set)] is equivalent to the above, generates both setter and getter
#[export(get)] generates only getter -- setter is unavailable
#[export(get = "my_getter")] provides user-defined getter -- setter is unavailable
#[export(get = "my_getter", set = "my_setter")] provides user-defined getter and setter

Ultimately, this may need the Property<T> class, but maybe those which are possible without can be provided in a first step, to keep the PR small in scope.

Also, generated setters/getters should maybe be "private" to avoid naming collisions? I.e. if I have a property field, the setter shouldn't be called set_property as a user might also define that (for whatever reason). Instead, it could be something like __gdext_set_property. In GDScript, you should anyway use the obj.property = value syntax.

Bromeon · 2023-03-13T08:57:05Z

godot-macros/src/derive_godot_class.rs

+        let getter_lit = proc_macro2::Literal::from_str(&getter_name).unwrap();
+        let setter_lit = proc_macro2::Literal::from_str(&setter_name).unwrap();
+        let getter_ident =
+            proc_macro2::Ident::new(&strip_quotes(&getter_name), proc_macro2::Span::call_site());
+        let setter_ident =
+            proc_macro2::Ident::new(&strip_quotes(&setter_name), proc_macro2::Span::call_site());


Note that you can directly pass strings into quote! and they will be formatted as literals like "string" (the previous code was more complex than necessary, too).

For the other part, do we actually need strip_quotes here, or can we just use the ident() utility function? Possibly exported_field.getter should already be populated correctly in the first place, i.e. getting the unquoted string from KvParser directly.

Note that you can directly pass strings into quote! and they will be formatted as literals like "string" (the previous code was more complex than necessary, too).

OK, let me remove strlit in utils too then, since it does this same thing and is unused anyway.

For the other part, do we actually need strip_quotes here, or can we just use the ident() utility function? Possibly exported_field.getter should already be populated correctly in the first place, i.e. getting the unquoted string from KvParser directly.

Nope, the value is quoted in the original attribute as well, i.e. get = "get_my_value" results in getter_name == "\"get_my_value\"". strip_quotes was just my first attempt, definitely not final -- we should throw an actual Rust parser at this task. But it seems venial can only parse declarations, not expressions :(

Alternatively, we omit the quotes from the macro syntax and parse it as an identifier. Somehow this is not commonly done (see e.g. the default attr in serde), but I don't know why.

OK, let me remove strlit in utils too then, since it does this same thing and is unused anyway.

Sounds good! I think I wrote that before I realized I could use strings directly 😀

we should throw an actual Rust parser at this task. But it seems venial can only parse declarations, not expressions :(

Why do we need a Rust parser if all we want is strip quotes? We don't have to support arbitrary function expressions, limiting it to methods for now is completely fine. venial is deliberately kept simple as a proc-macro processor, not a full language parser 🙂

I think we shouldn't overengineer it. strip_prefix + strip_postfix + error handling, and ident() will also panic if we pass it a weird string. We can always extend it in the future.

Alternatively, we omit the quotes from the macro syntax and parse it as an identifier. Somehow this is not commonly done (see e.g. the default attr in serde), but I don't know why.

There was some previous discussion in #31 (comment). I also start to believe quotes might be better, because:

ecosystem convention

possibly allows more complex expressions (not sure if relevant here)

visual separation between key and value, even more with syntax highlighting

I'm happy to hear more input on this topic though.

Why do we need a Rust parser if all we want is strip quotes?

I'm pretty sure that naive quote stripping will break on raw strings and escape sequences. I'll just document for now that getter/setter identifiers should be ASCII and not be Rust keywords, and thus be contained in a regular string literal without funny characters.

Good point. I think limiting to ASCII is a fair assumption to start with. If people really need Unicode support, I'm happy to accept PRs with a concrete use case 🙂

Bromeon · 2023-03-13T09:05:15Z

itest/godot/ManualFfiTests.gd

+	obj.int_val_get = 5
+	assert_eq(obj.int_val_get, 10)
+
+	obj.int_val_set = 5
+	assert_eq(obj.int_val_set, 10)
+
+	obj.int_val_get_set = 5
+	assert_eq(obj.int_val_get_set, 5)


Maybe to separate the operation from the read/writability, we could name them:

int_val_read int_val_write int_val_rw

This would avoid awkward names like get_int_val_get_set in favor of get_int_val_rw.

int_val_get is not currently read-only though. It has a default setter, and vice versa.

This argument becomes invalid if we model it after gdnative, which I think is a good idea.

ttencate · 2023-03-13T09:33:14Z

Also, generated setters/getters should maybe be "private" to avoid naming collisions? I.e. if I have a property field, the setter shouldn't be called set_property as a user might also define that (for whatever reason). Instead, it could be something like __gdext_set_property. In GDScript, you should anyway use the obj.property = value syntax.

The naming collision is on purpose. If someone manually wrote a set_property function, odds are good that they meant to write #[export(set = "set_property")] as well, but just forgot to add it.

It's true that you normally use obj.property = value syntax in GDScript, but especially now that functions are first-class citizens, it can be nice to explicitly use the setter. For example, $input_field.changed.connect(self.set_property).

You might also want to invoke the generated getter/setter from Rust, for encapsulation, in case you don't want to make the field pub and intend to add a hand-written getter/setter later.

Bromeon · 2023-03-13T09:49:42Z

It's true that you normally use obj.property = value syntax in GDScript, but especially now that functions are first-class citizens, it can be nice to explicitly use the setter. For example, $input_field.changed.connect(self.set_property).

Oh, this is a very good point 💡 in that case I agree.

You might also want to invoke the generated getter/setter from Rust, for encapsulation, in case you don't want to make the field pub and intend to add a hand-written getter/setter later.

Interesting, would you then declare the generated setters/getters automatically as pub?
It probably makes sense, as they're anyway publicly accessible through GDScript or reflection-based APIs like call().

ttencate · 2023-03-13T09:50:57Z

Good point, they should be pub.

ttencate · 2023-03-14T11:40:36Z

Based on #168 because I can't compile it on my machine otherwise. Apart from that, I think it's ready for final review.

Bromeon

Thanks for the update, few remarks left 🙂

bors try

Bromeon · 2023-03-20T17:17:31Z

godot-macros/src/derive_godot_class.rs

+                    venial::Error::new(format!(
+                        "argument to {key} must be a string literal, got: {name_lit}"
+                    ))


Could you reuse bail here? In that case, you also pass a span, so that the resulting compiler error can point directly to some tokens in the source code.

Bromeon · 2023-03-20T17:19:05Z

godot-core/src/obj/traits.rs

+/// You wouldn't usually implement this trait yourself; see the [documentation for the
+/// `derive(GodotClass)` macro](godot_macros::GodotClass).


Doc links should always point to symbols in the godot crate, as those are the public-facing API.

This should work because we have a reverse dev-dependency from godot-macros to godot:

gdext/godot-macros/Cargo.toml

Lines 13 to 15 in cccf247

# Reverse dev dependencies so doctests can use `godot::` prefix

[dev-dependencies]

godot = { path = "../godot" }

It works for doctests, but I can't get it to work for links. The syntax I wrote magically gets transformed to .../godot/prelude/derive.GodotClass.html though! No idea how that works.

Also added a link back from the macro to the trait, and that somehow resolves to .../godot/obj/trait.GodotClass.html, i.e. still the godot crate, but not prelude. Seems arbitrary.

Bromeon · 2023-03-20T17:22:49Z

godot-macros/src/derive_godot_class.rs

+                ::godot::builtin::meta::ClassName::of::<#class_name>(),
+                ::godot::builtin::StringName::from(#field_name),
+                ::godot::engine::global::PropertyHint::#hint_type,
+                GodotString::from(#description),


GodotString should be qualified too.

In general, if the resulting code is locally scoped, it's also fine to use these identifiers if it makes resulting code more readable. I'll leave that up to you 🙂

Bromeon · 2023-03-20T17:25:40Z

godot-macros/src/derive_godot_class.rs

+/// Checks at compile time that a function with the given name exists on `Self`.
+#[must_use]
+fn existence_check(ident: &Ident) -> TokenStream {
+    quote! {
+        #[allow(path_statements)]
+        Self::#ident;
+    }
+}


Maybe name it make_existence_check -- this currently sounds like it checks for the existence, not like it generates code that does so.

Usually functions named like a noun are functions in the mathematical sense, which compute something based on their arguments and return a value. E.g. cos(x) computes and returns the cosine of x; we don't call this compute_cos. A function that checks for the existence should be named check_existence instead.

But I'll rename this one anyway for consistency with its surroundings :)

One problem with English is that so many words are simultaneously nouns and verbs. There are many functions/macros which are named as prefix_action, e.g. godot_print, static_assert, variant_call, etc.

So I think make_, apart from being consistent, helps resolve this ambiguity.

Bromeon · 2023-03-20T17:28:06Z

godot-macros/src/derive_godot_class.rs

+/// Tests that the proc-macro doesn't panic. This is helpful because `RUST_BACKTRACE=1` does not
+/// affect panics in macro invocations (e.g. in our integration tests), but it works fine in unit
+/// tests.
+#[test]
+fn does_not_panic() {
+    let decl = venial::parse_declaration(quote! {
+        #[class(base=Node)]
+        struct HasProperty {
+            #[base]
+            base: Base<Node>,
+            #[export]
+            int_val_default: i32,
+            #[export(get = "get_int_val", set = "set_int_val")]
+            int_val: i32,
+        }
+    })
+    .unwrap();
+    transform(decl).unwrap();
+}


Not sure I understand the background of this test, despite the description. Isn't this already fully covered by integration tests, or what are we testing here? transform() is also not part of any public API.

We're not testing anything additional to integration tests, just allows getting the backtrace in case the macro does panic: https://ferrous-systems.com/blog/testing-proc-macros/#panics It was useful during development but I guess it can be removed now.

Ah, I see. That's a cool site by the way! 👍

bors · 2023-03-20T17:49:47Z

try

Build succeeded:

full-ci

- Rename arguments `getter` and `setter` to `get` and `set` to stay closer to GDScript and save keystrokes. - Allow `get` and `set` without arguments to generate the corresponding function. - Assume generated `get` and `set` if neither has been given. - Check at compile time that the referenced getter and setter actually exist (otherwise Godot gives a cryptic "invalid get/set index" error). - Remove unused `property` argument. - Add documentation for `GodotClass` derive macro.

Bromeon · 2023-03-21T10:29:32Z

Thanks for the update! Another great addition 💪

bors r+

177: More complete #[export] implementation r=Bromeon a=ttencate Also: - Remove unused `property` argument. - Rename arguments `getter` and `setter` to `get` and `set` to stay closer to GDScript and save keystrokes. - Check at compilation time that the referenced getter and setter actually exist (otherwise Godot gives a cryptic "invalid get/set index" error). See #3. TBD: - [ ] ~~`strip_quotes` should go away, not sure if it even works correctly if using e.g. raw string literals. Use an actual Rust parser? Omit the quotes from the argument instead, i.e. `get = get_my_field` instead of `get = "get_my_field"`?~~ See discussion below. - [ ] ~~Make `KvParser::parse` take a closure so we can check that all fields have been consumed~~ See discussion below. - [x] Omitting one of getter/setter should make field write/read only - [x] Use `get`/`set` without arguments to generate a default one - [x] Make generated getters and setters `pub` since they're public to Godot anyway Co-authored-by: Thomas ten Cate <ttencate@gmail.com>

bors · 2023-03-21T10:37:50Z

Build failed:

full-ci

ttencate · 2023-03-21T11:53:59Z

Could you retry? Seems like a transient failure in CI connectivity.

Bromeon · 2023-03-21T12:12:18Z

Absolutely! It's a bit annoying, we seem to be hit by lots of network errors lately. Not sure if the nightly.link servers have availability problems -- maybe our downloading should be more lenient regarding retries.

bors r+

177: More complete #[export] implementation r=Bromeon a=ttencate Also: - Remove unused `property` argument. - Rename arguments `getter` and `setter` to `get` and `set` to stay closer to GDScript and save keystrokes. - Check at compilation time that the referenced getter and setter actually exist (otherwise Godot gives a cryptic "invalid get/set index" error). See #3. TBD: - [ ] ~~`strip_quotes` should go away, not sure if it even works correctly if using e.g. raw string literals. Use an actual Rust parser? Omit the quotes from the argument instead, i.e. `get = get_my_field` instead of `get = "get_my_field"`?~~ See discussion below. - [ ] ~~Make `KvParser::parse` take a closure so we can check that all fields have been consumed~~ See discussion below. - [x] Omitting one of getter/setter should make field write/read only - [x] Use `get`/`set` without arguments to generate a default one - [x] Make generated getters and setters `pub` since they're public to Godot anyway Co-authored-by: Thomas ten Cate <ttencate@gmail.com>

Bromeon · 2023-03-21T12:18:37Z

On that topic, I now installed the https://nightly.link GitHub app for the organization (limited to this repo), which should help relieve the global rate-limit according to their website. Maybe that enhances connectivity.

bors · 2023-03-21T12:31:06Z

Build failed:

full-ci

ttencate · 2023-03-21T12:52:14Z

I don't know the first thing about GitHub CI, but maybe some more caching could help? https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows

Bromeon · 2023-03-21T13:18:22Z

We already cache Rust dependencies:

gdext/.github/composite/rust/action.yml

Lines 47 to 61 in 47027d3

    
           - name: "Reuse cached dependencies" 
        
             uses: Swatinem/rust-cache@v2 
        
             with: 
        
               # A cache key that is used instead of the automatic `job`-based key, and is stable over multiple jobs. 
        
               # default: empty 
        
               shared-key: "${{ runner.os }}-${{ inputs.rust }}${{ inputs.cache-key }}" 
        
               # An additional cache key that is added alongside the automatic `job`-based 
        
               # cache key and can be used to further differentiate jobs. 
        
               # default: empty 
        
               key: ${{ inputs.cache-key }} 
        
               # Determines if the cache should be saved even when the workflow has failed. 
        
               # default: "false" 
        
               cache-on-failure: true

We don't do it for Godot though. There is not that much benefit, because nightly builds are periodically renewed (every night), and restoring the cache is roughly equally slow as downloading the artifact.

We can maybe re-evaluate this alongside #107.

bors retry

177: More complete #[export] implementation r=Bromeon a=ttencate Also: - Remove unused `property` argument. - Rename arguments `getter` and `setter` to `get` and `set` to stay closer to GDScript and save keystrokes. - Check at compilation time that the referenced getter and setter actually exist (otherwise Godot gives a cryptic "invalid get/set index" error). See #3. TBD: - [ ] ~~`strip_quotes` should go away, not sure if it even works correctly if using e.g. raw string literals. Use an actual Rust parser? Omit the quotes from the argument instead, i.e. `get = get_my_field` instead of `get = "get_my_field"`?~~ See discussion below. - [ ] ~~Make `KvParser::parse` take a closure so we can check that all fields have been consumed~~ See discussion below. - [x] Omitting one of getter/setter should make field write/read only - [x] Use `get`/`set` without arguments to generate a default one - [x] Make generated getters and setters `pub` since they're public to Godot anyway Co-authored-by: Thomas ten Cate <ttencate@gmail.com>

bors · 2023-03-21T13:28:44Z

Build failed:

full-ci

Bromeon · 2023-03-21T13:33:02Z

It's interesting that some jobs fail, but some succeed. Really looks like a rate-limiting issue to me, but could also be general high load on either GitHub Action Artifacts or the nightly.link servers.

Will try one last time, otherwise wait a few hours.
bors retry

177: More complete #[export] implementation r=Bromeon a=ttencate Also: - Remove unused `property` argument. - Rename arguments `getter` and `setter` to `get` and `set` to stay closer to GDScript and save keystrokes. - Check at compilation time that the referenced getter and setter actually exist (otherwise Godot gives a cryptic "invalid get/set index" error). See #3. TBD: - [ ] ~~`strip_quotes` should go away, not sure if it even works correctly if using e.g. raw string literals. Use an actual Rust parser? Omit the quotes from the argument instead, i.e. `get = get_my_field` instead of `get = "get_my_field"`?~~ See discussion below. - [ ] ~~Make `KvParser::parse` take a closure so we can check that all fields have been consumed~~ See discussion below. - [x] Omitting one of getter/setter should make field write/read only - [x] Use `get`/`set` without arguments to generate a default one - [x] Make generated getters and setters `pub` since they're public to Godot anyway Co-authored-by: Thomas ten Cate <ttencate@gmail.com>

bors · 2023-03-21T13:37:53Z

Build failed:

full-ci

Bromeon · 2023-03-21T16:06:30Z

bors r+

bors · 2023-03-21T16:21:50Z

Build succeeded:

full-ci

Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Mar 13, 2023

Bromeon reviewed Mar 13, 2023

View reviewed changes

ttencate force-pushed the feature/properties branch from a71bc2b to 1731ff6 Compare March 14, 2023 11:39

ttencate changed the title ~~Allow omitting getter/setter from #[export]~~ More complete #[export] implementation Mar 14, 2023

ttencate marked this pull request as ready for review March 14, 2023 11:40

ttencate force-pushed the feature/properties branch 2 times, most recently from bec3d06 to 12a828f Compare March 15, 2023 11:05

Bromeon reviewed Mar 20, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Mar 20, 2023

Try #177:

f098597

ttencate force-pushed the feature/properties branch from 12a828f to 1460960 Compare March 21, 2023 09:26

Bromeon mentioned this pull request Mar 21, 2023

Project status overview #24

Open

bors bot merged commit 33c4e55 into godot-rust:master Mar 21, 2023

		/// You wouldn't usually implement this trait yourself; see the [documentation for the
		/// `derive(GodotClass)` macro](godot_macros::GodotClass).

	# Reverse dev dependencies so doctests can use `godot::` prefix
	[dev-dependencies]
	godot = { path = "../godot" }

More complete #[export] implementation #177

More complete #[export] implementation #177

Conversation

ttencate commented Mar 13, 2023 • edited Loading

Bromeon left a 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

ttencate Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Bromeon Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttencate commented Mar 13, 2023

Bromeon commented Mar 13, 2023

ttencate commented Mar 13, 2023

ttencate commented Mar 14, 2023

Bromeon left a 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

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

bors bot commented Mar 20, 2023

try

Bromeon commented Mar 21, 2023

bors bot commented Mar 21, 2023

ttencate commented Mar 21, 2023

Bromeon commented Mar 21, 2023 • edited Loading

Bromeon commented Mar 21, 2023

bors bot commented Mar 21, 2023

ttencate commented Mar 21, 2023

Bromeon commented Mar 21, 2023

bors bot commented Mar 21, 2023

Bromeon commented Mar 21, 2023

bors bot commented Mar 21, 2023

Bromeon commented Mar 21, 2023

bors bot commented Mar 21, 2023

ttencate commented Mar 13, 2023 •

edited

Loading

ttencate Mar 14, 2023 •

edited

Loading

Bromeon Mar 14, 2023 •

edited

Loading

Bromeon commented Mar 21, 2023 •

edited

Loading