From f1cfa06ad41078b5f1c6bbdcd6d20e51a3b4fbda Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 15 Sep 2024 12:31:36 +0200 Subject: [PATCH] Builder lifetimes, small fixes --- .../src/generator/default_parameters.rs | 83 ++++++++++--------- godot-core/src/lib.rs | 1 + godot-core/src/meta/godot_convert/mod.rs | 14 +--- .../builtin_tests/containers/array_test.rs | 2 +- 4 files changed, 48 insertions(+), 52 deletions(-) diff --git a/godot-codegen/src/generator/default_parameters.rs b/godot-codegen/src/generator/default_parameters.rs index 9d2d3ac78..24c588e69 100644 --- a/godot-codegen/src/generator/default_parameters.rs +++ b/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, - default_fn_params: Vec<&FnParam>, + fn_name: &str, + receiver_param: Option, + 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 }, } } diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs index a2b621ed7..e32a4b7e4 100644 --- a/godot-core/src/lib.rs +++ b/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")); diff --git a/godot-core/src/meta/godot_convert/mod.rs b/godot-core/src/meta/godot_convert/mod.rs index 62fcef171..e9ee39108 100644 --- a/godot-core/src/meta/godot_convert/mod.rs +++ b/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) -> 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) -> 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(value: &T) -> Variant { diff --git a/itest/rust/src/builtin_tests/containers/array_test.rs b/itest/rust/src/builtin_tests/containers/array_test.rs index 37ad6064e..ba09a655a 100644 --- a/itest/rust/src/builtin_tests/containers/array_test.rs +++ b/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"]); }