Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion check.sh
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ function cmd_test() {

function cmd_itest() {
findGodot && \
run cargo build -p itest "${extraCargoArgs[@]}" || return 1
run cargo build -p itest --features itest/experimental-threads --features itest/codegen-full "${extraCargoArgs[@]}" || return 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't have codegen-full in default config, it takes too long to compile. But I just added the --full flag for this 🙂

Also experimental-threads is rather discouraged -- discrepancies in feature flags between individual compiles (itest, clippy, test etc) will require re-compilations. (I think this might already be a problem today...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I fully agree. This is all just so it's easier to test this PR locally. I will remove all changes here before moving things out of draft status.


# Keep in sync with: .github/composite/godot-itest/action.yml (steps "Run Godot integration tests" and "Check for memory leaks").

Expand Down
27 changes: 24 additions & 3 deletions godot-codegen/src/generator/default_parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@ use quote::{format_ident, quote};

use crate::context::Context;
use crate::generator::functions_common::{
FnArgExpr, FnCode, FnKind, FnMeta, FnParamDecl, make_arg_expr, make_param_or_field_type,
self, FnArgExpr, FnCode, FnKind, FnMeta, FnParamDecl, make_arg_expr,
make_arg_thread_validator_expr, make_param_or_field_type,
};
use crate::generator::{functions_common, import_docs};
use crate::generator::import_docs;
use crate::models::domain::{ApiView, FnParam, FnQualifier, Function, RustTy, TyName};
use crate::special_cases::is_method_threadsafe_return;
use crate::util::{ident, safe_ident};
use crate::{conv, special_cases};

Expand Down Expand Up @@ -65,7 +67,14 @@ pub fn make_function_definition_with_defaults(
&default_fn_params,
);

let return_decl = &sig.return_value().decl;
let return_decl = if let Some(class_name) = sig.surrounding_class()
&& is_method_threadsafe_return(class_name, sig.godot_name())
{
sig.return_value().thread_safe_decl()
} else {
sig.return_value().decl.clone()
};

let (maybe_deprecated, maybe_expect_deprecated) = fns::make_deprecation_attribute(sig);

// If either the builder has a lifetime (non-static/global method), or one of its parameters is a reference,
Expand All @@ -75,6 +84,14 @@ pub fn make_function_definition_with_defaults(
let extended_receiver_param = &code.receiver.param_lifetime_ex;
let cfg_attributes = &meta.cfg_attributes;
let maybe_specific_docs = &meta.specific_docs;
let required_arg_thread_verifiers = if !sig.is_private() {
required_fn_params
.iter()
.filter_map(|param| make_arg_thread_validator_expr(&param.name, &param.type_))
.collect()
} else {
Vec::with_capacity(0)
};

let mut maybe_godot_doc = TokenStream::new();
if let Some(doc) = import_docs::import_function_docs(sig, ctx, view) {
Expand Down Expand Up @@ -148,6 +165,7 @@ pub fn make_function_definition_with_defaults(
#extended_receiver_param
#( #class_method_required_params_lifetimed, )*
) -> #builder_ty<'ex> {
#(#required_arg_thread_verifiers)*
#builder_ty::new(
#object_arg
#( #class_method_required_args, )*
Expand Down Expand Up @@ -333,10 +351,13 @@ fn make_extender(
make_param_or_field_type(name, type_, param_decl, &mut dummy_lifetime_gen);

let arg_expr = make_arg_expr(name, type_, FnArgExpr::StoreInField);
let arg_thread_verifier = make_arg_thread_validator_expr(name, type_);

let method = quote! {
#[inline]
pub fn #name(self, #param_decl) -> Self {
#arg_thread_verifier

// Currently not testing whether the parameter was already set.
Self {
#name: #arg_expr,
Expand Down
1 change: 1 addition & 0 deletions godot-codegen/src/generator/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ pub fn make_enum_definition_with(

impl crate::meta::ToGodot for #name {
type Pass = crate::meta::ByValue;
type Threads = crate::meta::ThreadSafeArg;

fn to_godot(&self) -> Self::Via {
<Self as #engine_trait>::ord(*self)
Expand Down
98 changes: 89 additions & 9 deletions godot-codegen/src/generator/functions_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

use std::ops::Not;

use proc_macro2::{Ident, TokenStream};
use quote::{format_ident, quote};

use crate::context::Context;
use crate::generator::{default_parameters, import_docs};
use crate::models::domain::{ApiView, ArgPassing, FnParam, FnQualifier, Function, RustTy};
use crate::special_cases;
use crate::special_cases::{self, is_method_threadsafe_return};
use crate::util::lifetime;

pub struct FnReceiver {
Expand Down Expand Up @@ -108,6 +110,7 @@ pub struct FnParamTokens {
/// Generic argument list `<'a0, 'a1, ...>` after `type CallSig`, if available.
pub callsig_lifetime_args: Option<TokenStream>,
pub arg_exprs: Vec<TokenStream>,
pub arg_thread_validators: Vec<TokenStream>,
}

pub fn make_function_definition(
Expand Down Expand Up @@ -151,6 +154,7 @@ pub fn make_function_definition(
callsig_param_types: param_types,
callsig_lifetime_args,
arg_exprs: arg_names,
arg_thread_validators,
} = if sig.is_virtual() {
make_params_exprs_virtual(sig.params().iter(), sig)
} else {
Expand Down Expand Up @@ -210,13 +214,46 @@ pub fn make_function_definition(
}
};

let return_decl = &sig.return_value().decl;
let arg_thread_validators = if sig.is_exposed_outer_builtin()
|| (!sig.is_builtin() && !sig.is_private() && !has_default_params)
{
arg_thread_validators
} else {
Vec::with_capacity(0)
};

let return_decl = if let Some(class_name) = sig.surrounding_class()
&& is_method_threadsafe_return(class_name, sig.godot_name())
{
sig.return_value().thread_safe_decl()
} else {
sig.return_value().decl.clone()
};

let fn_body = if code.is_virtual_required {
quote! { ; }
} else {
quote! { { unimplemented!() } }
};

let return_wrapper = if let Some(class_name) = sig.surrounding_class()
&& is_method_threadsafe_return(class_name, sig.godot_name())
{
if matches!(
sig.return_value().type_,
Some(RustTy::EngineClass {
is_nullable: true,
..
})
) {
quote! { unsafe { crate::obj::Unique::new_optional_unchecked(return_value) } }
} else {
quote! { unsafe { crate::obj::Unique::new_unchecked(return_value) } }
}
} else {
quote! { return_value }
};

let receiver_param = &code.receiver.param;
let primary_function = if sig.is_virtual() {
// Virtual functions.
Expand All @@ -237,6 +274,13 @@ pub fn make_function_definition(
// If the return type is not Variant, then convert to concrete target type.
let varcall_invocation = &code.varcall_invocation;

let vararg_thread_validator = sig.is_private().not().then(|| {
quote! {
#[cfg(all(feature = "experimental-threads", safeguards_balanced))]
ThreadSafeArgContext::guarantee_thread_safe(&varargs);
}
});

// TODO Utility functions: update as well.
if !code.is_varcall_fallible {
quote! {
Expand All @@ -250,12 +294,16 @@ pub fn make_function_definition(
varargs: &[Variant]
) #return_decl {
#call_sig_decl
#(#arg_thread_validators)*
#vararg_thread_validator

let args = (#( #arg_names, )*);

unsafe {
let return_value = unsafe {
#varcall_invocation
}
};

#return_wrapper
}
}
} else {
Expand Down Expand Up @@ -284,6 +332,9 @@ pub fn make_function_definition(
#( #params, )*
varargs: &[Variant]
) #return_decl {
#(#arg_thread_validators)*
#vararg_thread_validator

Self::#try_fn_name(self, #( #arg_names_without_asarg, )* varargs)
.unwrap_or_else(|e| panic!("{e}"))
}
Expand All @@ -302,9 +353,11 @@ pub fn make_function_definition(

let args = (#( #arg_names, )*);

unsafe {
let return_value = unsafe {
#varcall_invocation
}
};

#return_wrapper
}
}
}
Expand All @@ -324,12 +377,15 @@ pub fn make_function_definition(
) #return_decl
{
#call_sig_decl
#(#arg_thread_validators)*

let args = (#( #arg_names, )*);

unsafe {
let return_value = unsafe {
#ptrcall_invocation
}
};

#return_wrapper
}
}
};
Expand Down Expand Up @@ -587,7 +643,8 @@ pub(crate) fn make_arg_expr(name: &Ident, ty: &RustTy, expr: FnArgExpr) -> Token
..
} => match expr {
FnArgExpr::PassToFfi => quote! { #name.into_arg() },
FnArgExpr::PassToFfiFromEx => quote! { #name }, // both field and parameter types are Cow -> forward.
// Both field and parameter types are Cow -> forward.
FnArgExpr::PassToFfiFromEx => quote! { #name },
FnArgExpr::Forward => quote! { #name },
FnArgExpr::StoreInField => quote! { #name.into_arg() },
FnArgExpr::StoreInDefaultField => quote! { CowArg::Owned(#name) },
Expand All @@ -614,6 +671,25 @@ pub(crate) fn make_arg_expr(name: &Ident, ty: &RustTy, expr: FnArgExpr) -> Token
}
}

pub(crate) fn make_arg_thread_validator_expr(name: &Ident, ty: &RustTy) -> Option<TokenStream> {
match ty {
// Objects.
RustTy::EngineClass { .. }
| RustTy::BuiltinIdent {
arg_passing: ArgPassing::ByRef | ArgPassing::ImplAsArg,
..
}
| RustTy::TypedArray { .. }
| RustTy::TypedDictionary { .. } => Some(quote! {
#[cfg(all(feature = "experimental-threads", safeguards_balanced))]
ThreadSafeArgContext::guarantee_thread_safe(&#name);
}),

// By value.
_ => None,
}
}

/// For non-virtual functions, returns the parameter declarations, type tokens, and names.
pub(crate) fn make_params_exprs<'a>(
method_args: impl Iterator<Item = &'a FnParam>,
Expand Down Expand Up @@ -653,6 +729,10 @@ pub(crate) fn make_params_exprs<'a>(
ret.param_decls.push(param_decl);
ret.arg_exprs.push(arg_expr);
ret.callsig_param_types.push(param_ty);

if let Some(arg_validator) = make_arg_thread_validator_expr(param_name, param_rust_ty) {
ret.arg_thread_validators.push(arg_validator);
}
}

ret.callsig_lifetime_args = lifetime_gen.all_generic_args();
Expand Down
23 changes: 23 additions & 0 deletions godot-codegen/src/models/domain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,29 @@ impl FnReturn {
}
}

pub fn type_tokens_non_null(&self) -> TokenStream {
match &self.type_ {
Some(ty) => ty.tokens_non_null(),
_ => quote! { () },
}
}

pub fn thread_safe_decl(&self) -> TokenStream {
let ret = self.type_tokens_non_null();

if matches!(
self.type_,
Some(RustTy::EngineClass {
is_nullable: true,
..
})
) {
quote! { -> Option<crate::obj::Unique<#ret>> }
} else {
quote! { -> crate::obj::Unique<#ret> }
}
}

pub fn call_result_decl(&self) -> TokenStream {
let ret = self.type_tokens();
quote! { -> Result<#ret, crate::meta::error::CallError> }
Expand Down
16 changes: 16 additions & 0 deletions godot-codegen/src/special_cases/special_cases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,9 @@ pub fn is_class_method_replaced_with_type_safe(class_ty: &TyName, godot_method_n
| ("Object", "get_script")
| ("Object", "set_script")

// thread_safe_unchecked
| ("Object", "emit_signal")

// u32 -> ConnectFlags
| ("Object", "connect")

Expand Down Expand Up @@ -871,6 +874,19 @@ pub fn is_utility_function_private(function: &JsonUtilityFunction) -> bool {
}
}

/// Whether a class or builtin methods return value is thread-safe.
///
/// This is a hand-picked list of methods which have been manually verified to return unique values.
#[rustfmt::skip]
pub fn is_method_threadsafe_return(class_or_builtin_ty: &TyName, godot_method_name: &str) -> bool {
match (class_or_builtin_ty.godot_ty.as_str(), godot_method_name) {
| ("SurfaceTool", "commit")
| ("SurfaceTool", "commit_to_arrays")

=> true, _ => false
}
}

pub fn maybe_rename_class_method<'m>(
class_name: &TyName,
godot_method_name: &'m str,
Expand Down
2 changes: 1 addition & 1 deletion godot-codegen/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub fn make_imports() -> TokenStream {
quote! {
use godot_ffi as sys;
use crate::builtin::*;
use crate::meta::{AsArg, ClassId, CowArg, InParamTuple, OutParamTuple, ParamTuple, RawPtr, RefArg};
use crate::meta::{AsArg, ClassId, CowArg, InParamTuple, OutParamTuple, ParamTuple, RawPtr, RefArg, ThreadSafeArgContext};
use crate::private::Signature;
use crate::classes::native::*;
use crate::classes::Object;
Expand Down
2 changes: 1 addition & 1 deletion godot-core/src/builtin/callable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ unsafe impl GodotFfi for Callable {
}
}

meta::impl_godot_as_self!(Callable: ByRef);
meta::impl_godot_as_self!(Callable: ByRef, NonThreadSafeArg);

impl fmt::Debug for Callable {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Expand Down
1 change: 1 addition & 0 deletions godot-core/src/builtin/collections/any_array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ impl GodotConvert for AnyArray {

impl ToGodot for AnyArray {
type Pass = meta::ByValue;
type Threads = meta::NonThreadSafeArg;

fn to_godot(&self) -> meta::ToArg<'_, Self::Via, Self::Pass> {
self.clone()
Expand Down
1 change: 1 addition & 0 deletions godot-core/src/builtin/collections/any_dictionary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,7 @@ impl GodotConvert for AnyDictionary {

impl ToGodot for AnyDictionary {
type Pass = meta::ByValue;
type Threads = meta::NonThreadSafeArg;

fn to_godot(&self) -> meta::ToArg<'_, Self::Via, Self::Pass> {
self.clone()
Expand Down
Loading