From 7aed5da1177a7b01a2bb8327a9a2ddfc362fb5c1 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 5 Feb 2023 15:07:23 +0100 Subject: [PATCH 1/2] Support static class + builtin methods --- godot-codegen/src/api_parser.rs | 2 +- godot-codegen/src/class_generator.rs | 50 +++++++++++++++++++--------- godot-core/src/builtin/color.rs | 1 - itest/rust/src/codegen_test.rs | 31 +++++++++++++++-- 4 files changed, 64 insertions(+), 20 deletions(-) diff --git a/godot-codegen/src/api_parser.rs b/godot-codegen/src/api_parser.rs index e4852fde0..da5d8643b 100644 --- a/godot-codegen/src/api_parser.rs +++ b/godot-codegen/src/api_parser.rs @@ -172,7 +172,7 @@ pub struct ClassMethod { pub name: String, pub is_const: bool, pub is_vararg: bool, - //pub is_static: bool, + pub is_static: bool, pub is_virtual: bool, pub hash: Option, pub return_value: Option, diff --git a/godot-codegen/src/class_generator.rs b/godot-codegen/src/class_generator.rs index 3b414f7a8..91186a687 100644 --- a/godot-codegen/src/class_generator.rs +++ b/godot-codegen/src/class_generator.rs @@ -510,8 +510,7 @@ fn make_method_definition( /*if method.map_args(|args| args.is_empty()) { // Getters (i.e. 0 arguments) will be stripped of their `get_` prefix, to conform to Rust convention if let Some(remainder) = method_name.strip_prefix("get_") { - // Do not apply for get_16 etc - // TODO also not for get_u16 etc, in StreamPeer + // TODO Do not apply for FileAccess::get_16, StreamPeer::get_u16, etc if !remainder.chars().nth(0).unwrap().is_ascii_digit() { method_name = remainder; } @@ -519,11 +518,13 @@ fn make_method_definition( }*/ let method_name_str = special_cases::maybe_renamed(class_name, &method.name); - let receiver = if method.is_const { - quote! { &self, } - } else { - quote! { &mut self, } - }; + + let (receiver, receiver_arg) = make_receiver( + method.is_static, + method.is_const, + quote! { self.object_ptr }, + ); + let hash = method.hash; let is_varcall = method.is_vararg; @@ -546,10 +547,10 @@ fn make_method_definition( let __call_fn = sys::interface_fn!(#function_provider); }; let varcall_invocation = quote! { - __call_fn(__method_bind, self.object_ptr, __args_ptr, __args.len() as i64, return_ptr, std::ptr::addr_of_mut!(__err)); + __call_fn(__method_bind, #receiver_arg, __args_ptr, __args.len() as i64, return_ptr, std::ptr::addr_of_mut!(__err)); }; let ptrcall_invocation = quote! { - __call_fn(__method_bind, self.object_ptr, __args_ptr, return_ptr); + __call_fn(__method_bind, #receiver_arg, __args_ptr, return_ptr); }; make_function_definition( @@ -579,11 +580,8 @@ fn make_builtin_method_definition( let method_name_str = &method.name; - let receiver = if method.is_const { - quote! { &self, } - } else { - quote! { &mut self, } - }; + let (receiver, receiver_arg) = + make_receiver(method.is_static, method.is_const, quote! { self.sys_ptr }); let return_value = method.return_type.as_deref().map(MethodReturn::from_type); let hash = method.hash; @@ -602,7 +600,7 @@ fn make_builtin_method_definition( let __call_fn = __call_fn.unwrap_unchecked(); }; let ptrcall_invocation = quote! { - __call_fn(self.sys_ptr, __args_ptr, return_ptr, __args.len() as i32); + __call_fn(#receiver_arg, __args_ptr, return_ptr, __args.len() as i32); }; make_function_definition( @@ -746,6 +744,28 @@ fn make_function_definition( } } +fn make_receiver( + is_static: bool, + is_const: bool, + receiver_arg: TokenStream, +) -> (TokenStream, TokenStream) { + let receiver = if is_static { + quote! {} + } else if is_const { + quote! { &self, } + } else { + quote! { &mut self, } + }; + + let receiver_arg = if is_static { + quote! { std::ptr::null_mut() } + } else { + receiver_arg + }; + + (receiver, receiver_arg) +} + fn make_params( method_args: &Option>, is_varcall: bool, diff --git a/godot-core/src/builtin/color.rs b/godot-core/src/builtin/color.rs index 395046dbc..b2785867b 100644 --- a/godot-core/src/builtin/color.rs +++ b/godot-core/src/builtin/color.rs @@ -17,7 +17,6 @@ pub struct Color { } impl Color { - #[allow(dead_code)] pub fn new(r: f32, g: f32, b: f32, a: f32) -> Self { Self { r, g, b, a } } diff --git a/itest/rust/src/codegen_test.rs b/itest/rust/src/codegen_test.rs index 82f0761c5..f220c9b68 100644 --- a/itest/rust/src/codegen_test.rs +++ b/itest/rust/src/codegen_test.rs @@ -4,17 +4,20 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -// This file tests the presence and naming of generated symbols, not their functionality. +// This file tests the presence, naming and accessibility of generated symbols. +// Functionality is only tested on a superficial level (to make sure general FFI mechanisms work). use crate::itest; - -use godot::engine::HttpRequest; +use godot::builtin::inner::{InnerColor, InnerString}; +use godot::engine::{FileAccess, HttpRequest}; use godot::prelude::*; pub fn run() -> bool { let mut ok = true; ok &= codegen_class_renamed(); ok &= codegen_base_renamed(); + ok &= codegen_static_builtin_method(); + ok &= codegen_static_class_method(); ok } @@ -36,6 +39,28 @@ fn codegen_base_renamed() { obj.free(); } +#[itest] +fn codegen_static_builtin_method() { + let pi = InnerString::num(std::f64::consts::PI, 3); + assert_eq!(pi, GodotString::from("3.142")); + + let col = InnerColor::html("#663399cc".into()); + assert_eq!(col, Color::new(0.4, 0.2, 0.6, 0.8)); +} + +#[itest] +fn codegen_static_class_method() { + let exists = FileAccess::file_exists("inexistent".into()); + assert!(!exists); + + let exists = FileAccess::file_exists("res://itest.gdextension".into()); + assert!(exists); + + // see also object_test for reference count verification +} + +// ---------------------------------------------------------------------------------------------------------------------------------------------- + #[derive(GodotClass)] #[class(base=HttpRequest)] pub struct TestBaseRenamed { From 31588250b1db48db99ee1e40707a924d5d667519 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 5 Feb 2023 15:51:09 +0100 Subject: [PATCH 2/2] Fix memory leak for RefCounted objects returned by engine methods The refcount was accidentally incremented on the Rust call-site, even though Godot already does that. Added regression test for this case. --- godot-core/src/obj/gd.rs | 7 ++++++- itest/rust/src/object_test.rs | 14 +++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 70312cbe5..235f1ee4d 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -488,6 +488,9 @@ impl Gd { /// Runs `init_fn` on the address of a pointer (initialized to null). If that pointer is still null after the `init_fn` call, /// then `None` will be returned; otherwise `Gd::from_obj_sys(ptr)`. /// + /// This method will **NOT** increment the reference-count of the object, as it assumes the input to come from a Godot API + /// return value. + /// /// # Safety /// `init_fn` must be a function that correctly handles a _type pointer_ pointing to an _object pointer_. #[doc(hidden)] @@ -496,7 +499,9 @@ impl Gd { // Initialize pointer with given function, return Some(ptr) on success and None otherwise let object_ptr = raw_object_init(init_fn); - sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys(ptr)) + + // Do not increment ref-count; assumed to be return value from FFI. + sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys_weak(ptr)) } } diff --git a/itest/rust/src/object_test.rs b/itest/rust/src/object_test.rs index 6dadd0c9e..d5f148d3a 100644 --- a/itest/rust/src/object_test.rs +++ b/itest/rust/src/object_test.rs @@ -7,7 +7,7 @@ use crate::{expect_panic, itest}; use godot::bind::{godot_api, GodotClass, GodotExt}; use godot::builtin::{FromVariant, GodotString, StringName, ToVariant, Variant, Vector3}; -use godot::engine::{Node, Node3D, Object, RefCounted}; +use godot::engine::{file_access, FileAccess, Node, Node3D, Object, RefCounted}; use godot::obj::Share; use godot::obj::{Base, Gd, InstanceId}; use godot::sys::GodotFfi; @@ -38,6 +38,7 @@ pub fn run() -> bool { ok &= object_engine_convert_variant(); ok &= object_user_convert_variant_refcount(); ok &= object_engine_convert_variant_refcount(); + ok &= object_engine_returned_refcount(); ok &= object_engine_up_deref(); ok &= object_engine_up_deref_mut(); ok &= object_engine_upcast(); @@ -265,6 +266,17 @@ fn check_convert_variant_refcount(obj: Gd) { assert_eq!(obj.get_reference_count(), 1); } +#[itest] +fn object_engine_returned_refcount() { + let Some(file) = FileAccess::open("res://itest.gdextension".into(), file_access::ModeFlags::READ) else { + panic!("failed to open file used to test FileAccess") + }; + assert!(file.is_open()); + + // There was a bug which incremented ref-counts of just-returned objects, keep this as regression test. + assert_eq!(file.get_reference_count(), 1); +} + #[itest] fn object_engine_up_deref() { let node3d: Gd = Node3D::new_alloc();