Builder lifetimes, small fixes

Author: Bromeon

godot-codegen/src/generator/default_parameters.rs

@@ -44,7 +44,12 @@ pub fn make_function_definition_with_defaults(
         builder_fields,
         builder_args,
         builder_inits,
-    } = make_extender(sig, object_fn_param, default_fn_params);
+    } = make_extender(
+        sig.name(),
+        object_fn_param,
+        &required_fn_params,
+        &default_fn_params,
+    );
 
     // ExBuilder::new() constructor signature.
     let FnParamTokens {
@@ -61,19 +66,6 @@ pub fn make_function_definition_with_defaults(
     );
     let required_params_builder_constructor = &required_params_class_methods;
 
-    // Parameters for some_function() and some_function_ex().
-    // TODO collapse with next assignment. Maybe next 2
-    /*let FnParamTokens {
-        params: required_params_class_methods,
-        ..
-    } = functions_common::make_params_exprs(
-        required_fn_params.iter().cloned(),
-        true,
-        true,
-        false,
-        false,
-    );*/
-
     // Forwarded args by some_function() and some_function_ex().
     let FnParamTokens {
         arg_names: required_args_internal,
@@ -92,13 +84,15 @@ pub fn make_function_definition_with_defaults(
     // then we need to annotate the _ex() function with an explicit lifetime. Also adjust &self -> &'a self.
     let receiver_self = &code.receiver.self_prefix;
 
-    let (builder_ret_lifetime, receiver_param);
-    if let Some(func_lifetime) = func_general_lifetime.as_ref() {
-        builder_ret_lifetime = Some(func_lifetime);
-        receiver_param = &code.receiver.param_lifetime_a;
+    let (simple_receiver_param, extended_receiver_param, func_or_builder_lifetime);
+    if let Some(func_lt) = func_general_lifetime.as_ref().or(builder_lifetime.as_ref()) {
+        simple_receiver_param = &code.receiver.param;
+        extended_receiver_param = &code.receiver.param_lifetime_a;
+        func_or_builder_lifetime = Some(func_lt);
     } else {
-        builder_ret_lifetime = None;
-        receiver_param = &code.receiver.param;
+        simple_receiver_param = &code.receiver.param;
+        extended_receiver_param = &code.receiver.param;
+        func_or_builder_lifetime = None;
     };
 
     // Technically, the builder would not need a lifetime -- it could just maintain an `object_ptr` copy.
@@ -146,7 +140,7 @@ pub fn make_function_definition_with_defaults(
         #[doc = #default_parameter_usage]
         #[inline]
         #vis fn #simple_fn_name #func_general_lifetime  (
-            #receiver_param
+            #simple_receiver_param
             #( #required_params_class_methods, )*
         ) #return_decl {
             #receiver_self #extended_fn_name(
@@ -157,10 +151,10 @@ pub fn make_function_definition_with_defaults(
         // _ex() function:
         // Lifetime is set if any parameter is a reference OR if the method is not static/global (and thus can refer to self).
         #[inline]
-        #vis fn #extended_fn_name #func_general_lifetime (
-            #receiver_param
+        #vis fn #extended_fn_name #func_or_builder_lifetime (
+            #extended_receiver_param
             #( #required_params_class_methods, )*
-        ) -> #builder_ty #builder_ret_lifetime {
+        ) -> #builder_ty #builder_lifetime {
             #builder_ty::new(
                 #object_arg
                 #( #required_args_internal, )*
@@ -278,26 +272,34 @@ fn make_extender_receiver(sig: &dyn Function) -> ExtenderReceiver {
 }
 
 fn make_extender(
-    sig: &dyn Function,
-    object_fn_param: Option<FnParam>,
-    default_fn_params: Vec<&FnParam>,
+    fn_name: &str,
+    receiver_param: Option<FnParam>,
+    required_params: &[&FnParam],
+    default_params: &[&FnParam],
 ) -> Extender {
     // Note: could build a documentation string with default values here, but the Rust tokens are not very readable,
     // and often not helpful, such as Enum::from_ord(13). Maybe one day those could be resolved and curated.
 
-    let lifetime = if sig.qualifier().is_static_or_global() {
-        None
-    } else {
+    let all_fn_params = receiver_param
+        .iter()
+        .chain(required_params.iter().cloned())
+        .chain(default_params.iter().cloned());
+    let len = all_fn_params.size_hint().0;
+
+    // If builder is a method with a receiver OR any *required* parameter is by-ref, use lifetime.
+    // Default parameters cannot be by-ref, since they need to store a default value. Potential optimization later.
+    let lifetime = if receiver_param.is_some() // !sig.qualifier().is_static_or_global()
+        || required_params.iter().any(|p| p.type_.is_pass_by_ref())
+    {
         Some(quote! { <'a> })
+    } else {
+        None
     };
 
-    let all_fn_params = object_fn_param.iter().chain(sig.params().iter());
-    let len = all_fn_params.size_hint().0;
-
     let mut result = Extender {
-        builder_ty: format_ident!("Ex{}", conv::to_pascal_case(sig.name())),
+        builder_ty: format_ident!("Ex{}", conv::to_pascal_case(fn_name)),
         builder_lifetime: lifetime,
-        builder_methods: Vec::with_capacity(default_fn_params.len()),
+        builder_methods: Vec::with_capacity(default_params.len()),
         builder_fields: Vec::with_capacity(len),
         builder_args: Vec::with_capacity(len),
         builder_inits: Vec::with_capacity(len),
@@ -316,7 +318,6 @@ fn make_extender(
         let init = if let Some(value) = default_value {
             make_field_init(name, Some(value), conversion)
         } else {
-            //quote! { #name }
             make_field_init(name, None, conversion)
         };
 
@@ -336,7 +337,7 @@ fn make_extender(
         result.builder_inits.push(init);
     }
 
-    for param in default_fn_params {
+    for param in default_params {
         let FnParam { name, type_, .. } = param;
         let param_type = type_.param_decl();
         let (_, field_needs_conversion) = default_extender_field_decl(type_, true);
@@ -397,9 +398,9 @@ fn make_field_init(
     match (conversion, expr) {
         (Conv::ObjectArg, Some(expr)) => quote! { #name: #expr.consume_object() },
         (Conv::ObjectArg, None) /*.             */ => quote! { #name: #name.consume_object() },
-        (Conv::Reference, Some(expr)) => quote! { #name: & #expr },
-        (Conv::Reference, None) /*.             */ => quote! { #name: & #name },
-        (Conv::None | Conv::ReferenceWithDefault, Some(expr)) => quote! { #name: #expr },
-        (Conv::None | Conv::ReferenceWithDefault, None) /*.             */ => quote! { #name },
+
+        // Currently no differentiation between None|Reference|ReferenceWithDefault; this may change...
+        (Conv::None | Conv::Reference | Conv::ReferenceWithDefault, Some(expr)) => quote! { #name: #expr },
+        (Conv::None | Conv::Reference | Conv::ReferenceWithDefault, None) /*.             */ => quote! { #name },
     }
 }

godot-core/src/lib.rs

@@ -48,6 +48,7 @@ compile_error!("Generating editor docs for Rust symbols requires at least Godot
 #[allow(clippy::let_unit_value)] // let args = ();
 #[allow(clippy::wrong_self_convention)] // to_string() is const
 #[allow(clippy::upper_case_acronyms)] // TODO remove this line once we transform names
+#[allow(clippy::needless_lifetimes)]  // the following explicit lifetimes could be elided: 'a
 #[allow(unreachable_code, clippy::unimplemented)] // TODO remove once #153 is implemented
 mod gen {
     include!(concat!(env!("OUT_DIR"), "/mod.rs"));

godot-core/src/meta/godot_convert/mod.rs

@@ -95,18 +95,12 @@ pub trait FromGodot: Sized + GodotConvert {
     }
 }
 
-// pub(crate) fn into_ffi<'v, T: ToGodot >(value: T) -> <T::ToVia<'v> as GodotType>::Ffi {
-//     let by_ref = value.to_godot();
-//     let ffi = by_ref.to_ffi();
-//
-//     ffi
-// }
-
+// Note: removing the implicit lifetime (by taking value: T instead of &T) causes issues due to allegedly returning a lifetime
+// to a local variable, even though the result Ffi is 'static by definition.
+#[allow(clippy::needless_lifetimes)] // eliding causes error: missing generics for associated type `godot_convert::ToGodot::ToVia`
 pub(crate) fn into_ffi<'v, T: ToGodot>(value: &'v T) -> <T::ToVia<'v> as GodotType>::Ffi {
     let by_ref = value.to_godot();
-    let ffi = by_ref.to_ffi();
-
-    ffi
+    by_ref.to_ffi()
 }
 
 pub(crate) fn into_ffi_variant<T: ToGodot>(value: &T) -> Variant {

itest/rust/src/builtin_tests/containers/array_test.rs

@@ -53,7 +53,7 @@ fn array_from_packed_array() {
     // This tests that the resulting array doesn't secretly have a runtime type assigned to it,
     // which is not reflected in our static type. It would make sense if it did, but Godot decided
     // otherwise: we get an untyped array.
-    array.push(&GString::from("hi").to_variant());
+    array.push(GString::from("hi").to_variant());
     assert_eq!(array, varray![42, "hi"]);
 }