Skip to content

Commit f67835d

Browse files
authored
Merge pull request #1435 from godot-rust/qol/enforce-func-bounds
Enforce `ToGodot`/`FromGodot` bounds in `#[func]`, remove raw pointer impls
2 parents 1c57aa4 + 711cbc2 commit f67835d

File tree

8 files changed

+139
-27
lines changed

8 files changed

+139
-27
lines changed

godot-codegen/src/generator/native_structures.rs

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -88,17 +88,15 @@ fn make_native_structure(
8888

8989
let imports = util::make_imports();
9090
let (fields, methods) = make_native_structure_fields_and_methods(structure, ctx);
91-
let doc = format!("[`ToGodot`] and [`FromGodot`] are implemented for `*mut {class_name}` and `*const {class_name}`.");
9291

9392
// mod re_export needed, because class should not appear inside the file module, and we can't re-export private struct as pub
9493
let tokens = quote! {
9594
#imports
9695
use std::ffi::c_void; // for opaque object pointer fields
97-
use crate::meta::{GodotConvert, FromGodot, ToGodot};
96+
use crate::meta::{GodotConvert, EngineFromGodot, EngineToGodot};
9897

9998
/// Native structure; can be passed via pointer in APIs that are not exposed to GDScript.
10099
///
101-
#[doc = #doc]
102100
#[derive(Clone, PartialEq, Debug)]
103101
#[repr(C)]
104102
pub struct #class_name {
@@ -113,36 +111,53 @@ fn make_native_structure(
113111
type Via = i64;
114112
}
115113

116-
impl ToGodot for *mut #class_name {
114+
// Native structure pointers implement internal-only conversion traits for use in engine APIs.
115+
impl EngineToGodot for *mut #class_name {
117116
type Pass = crate::meta::ByValue;
118117

119-
fn to_godot(&self) -> Self::Via {
118+
fn engine_to_godot(&self) -> crate::meta::ToArg<'_, Self::Via, Self::Pass> {
120119
*self as i64
121120
}
121+
122+
fn engine_to_variant(&self) -> crate::builtin::Variant {
123+
crate::builtin::Variant::from(*self as i64)
124+
}
122125
}
123126

124-
impl FromGodot for *mut #class_name {
125-
fn try_from_godot(via: Self::Via) -> Result<Self, crate::meta::error::ConvertError> {
127+
impl EngineFromGodot for *mut #class_name {
128+
fn engine_try_from_godot(via: Self::Via) -> Result<Self, crate::meta::error::ConvertError> {
126129
Ok(via as Self)
127130
}
131+
132+
fn engine_try_from_variant(variant: &crate::builtin::Variant) -> Result<Self, crate::meta::error::ConvertError> {
133+
variant.try_to::<i64>().map(|i| i as Self)
134+
}
128135
}
129136

130137
impl GodotConvert for *const #class_name {
131138
type Via = i64;
132139
}
133140

134-
impl ToGodot for *const #class_name {
141+
impl EngineToGodot for *const #class_name {
135142
type Pass = crate::meta::ByValue;
136143

137-
fn to_godot(&self) -> Self::Via {
144+
fn engine_to_godot(&self) -> crate::meta::ToArg<'_, Self::Via, Self::Pass> {
138145
*self as i64
139146
}
147+
148+
fn engine_to_variant(&self) -> crate::builtin::Variant {
149+
crate::builtin::Variant::from(*self as i64)
150+
}
140151
}
141152

142-
impl FromGodot for *const #class_name {
143-
fn try_from_godot(via: Self::Via) -> Result<Self, crate::meta::error::ConvertError> {
153+
impl EngineFromGodot for *const #class_name {
154+
fn engine_try_from_godot(via: Self::Via) -> Result<Self, crate::meta::error::ConvertError> {
144155
Ok(via as Self)
145156
}
157+
158+
fn engine_try_from_variant(variant: &crate::builtin::Variant) -> Result<Self, crate::meta::error::ConvertError> {
159+
variant.try_to::<i64>().map(|i| i as Self)
160+
}
146161
}
147162
};
148163
// note: TypePtr -> ObjectPtr conversion OK?

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

Lines changed: 89 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -460,18 +460,27 @@ macro_rules! impl_pointer_convert {
460460
type Via = i64;
461461
}
462462

463-
impl ToGodot for $Ptr {
463+
// Pointers implement internal-only conversion traits for use in engine APIs and virtual methods.
464+
impl meta::EngineToGodot for $Ptr {
464465
type Pass = meta::ByValue;
465466

466-
fn to_godot(&self) -> Self::Via {
467+
fn engine_to_godot(&self) -> meta::ToArg<'_, Self::Via, Self::Pass> {
467468
*self as i64
468469
}
470+
471+
fn engine_to_variant(&self) -> Variant {
472+
Variant::from(*self as i64)
473+
}
469474
}
470475

471-
impl FromGodot for $Ptr {
472-
fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
476+
impl meta::EngineFromGodot for $Ptr {
477+
fn engine_try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
473478
Ok(via as Self)
474479
}
480+
481+
fn engine_try_from_variant(variant: &Variant) -> Result<Self, ConvertError> {
482+
variant.try_to::<i64>().map(|i| i as Self)
483+
}
475484
}
476485
};
477486
}
@@ -480,10 +489,85 @@ impl_pointer_convert!(*const std::ffi::c_void);
480489
impl_pointer_convert!(*mut std::ffi::c_void);
481490

482491
// Some other pointer types are used by various other methods, see https://github.com/godot-rust/gdext/issues/677
483-
// TODO: Find better solution to this, this may easily break still if godot decides to add more pointer arguments.
492+
// Keep manually extending this; no point in automating with how rarely Godot adds new pointer types.
484493

485494
impl_pointer_convert!(*mut *const u8);
486495
impl_pointer_convert!(*mut i32);
487496
impl_pointer_convert!(*mut f64);
488497
impl_pointer_convert!(*mut u8);
489498
impl_pointer_convert!(*const u8);
499+
500+
// ----------------------------------------------------------------------------------------------------------------------------------------------
501+
// Tests for ToGodot/FromGodot missing impls
502+
//
503+
// Sanity check: comment-out ::godot::meta::ensure_func_bounds in func.rs, the 3 latter #[func] ones should fail.
504+
505+
/// Test that `*mut i32` cannot be converted to variant.
506+
///
507+
/// ```compile_fail
508+
/// # use godot::prelude::*;
509+
/// let ptr: *mut i32 = std::ptr::null_mut();
510+
/// let variant = ptr.to_variant(); // Error: *mut i32 does not implement ToGodot
511+
/// ```
512+
fn __doctest_i32_ptr_to_variant() {}
513+
514+
/// Test that void-pointers cannot be converted from variant.
515+
///
516+
/// ```compile_fail
517+
/// # use godot::prelude::*;
518+
/// let variant = Variant::nil();
519+
/// let ptr: *const std::ffi::c_void = variant.to();
520+
/// ```
521+
fn __doctest_void_ptr_from_variant() {}
522+
523+
/// Test that native struct pointers cannot be used as `#[func]` parameters.
524+
///
525+
/// ```compile_fail
526+
/// # use godot::prelude::*;
527+
/// # use godot::classes::native::AudioFrame;
528+
/// #[derive(GodotClass)]
529+
/// #[class(init)]
530+
/// struct MyClass {}
531+
///
532+
/// #[godot_api]
533+
/// impl MyClass {
534+
/// #[func]
535+
/// fn take_pointer(&self, ptr: *mut AudioFrame) {}
536+
/// }
537+
/// ```
538+
fn __doctest_native_struct_pointer_param() {}
539+
540+
/// Test that native struct pointers cannot be used as `#[func]` return types.
541+
///
542+
/// ```compile_fail
543+
/// # use godot::prelude::*;
544+
/// # use godot::classes::native::AudioFrame;
545+
/// #[derive(GodotClass)]
546+
/// #[class(init)]
547+
/// struct MyClass {}
548+
///
549+
/// #[godot_api]
550+
/// impl MyClass {
551+
/// #[func]
552+
/// fn return_pointer(&self) -> *const AudioFrame {
553+
/// std::ptr::null()
554+
/// }
555+
/// }
556+
/// ```
557+
fn __doctest_native_struct_pointer_return() {}
558+
559+
/// Test that `u64` cannot be returned from `#[func]`.
560+
///
561+
/// ```compile_fail
562+
/// # use godot::prelude::*;
563+
/// #[derive(GodotClass)]
564+
/// #[class(init)]
565+
/// struct MyClass {}
566+
///
567+
/// #[godot_api]
568+
/// impl MyClass {
569+
/// #[func]
570+
/// fn return_pointer(&self) -> u64 { 123 }
571+
/// }
572+
/// ```
573+
fn __doctest_u64_return() {}

godot-core/src/meta/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ pub use element_type::{ElementScript, ElementType};
6969
pub use godot_convert::{EngineFromGodot, EngineToGodot, FromGodot, GodotConvert, ToGodot};
7070
pub use method_info::MethodInfo;
7171
pub use object_to_owned::ObjectToOwned;
72-
pub use param_tuple::{InParamTuple, OutParamTuple, ParamTuple};
72+
pub use param_tuple::{InParamTuple, OutParamTuple, ParamTuple, TupleFromGodot};
7373
pub use property_info::{PropertyHintInfo, PropertyInfo};
7474
#[cfg(feature = "trace")]
7575
pub use signature::trace;

godot-core/src/meta/param_tuple.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,10 @@ pub trait OutParamTuple: ParamTuple {
9999
/// Converts `array` to `Self` by calling [`to_variant`](crate::meta::ToGodot::to_variant) on each argument.
100100
fn to_variant_array(&self) -> Vec<Variant>;
101101
}
102+
103+
/// Helper trait to verify that all tuple elements implement `FromGodot`.
104+
///
105+
/// Used internally by [`crate::meta::ensure_func_bounds()`] to ensure each parameter in a `#[func]` method
106+
/// implements `FromGodot`, not just `EngineFromGodot`.
107+
#[doc(hidden)]
108+
pub trait TupleFromGodot: Sized {}

godot-core/src/meta/param_tuple/impls.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ use sys::GodotFfi;
1616
use crate::builtin::Variant;
1717
use crate::meta::error::{CallError, CallResult};
1818
use crate::meta::{
19-
ArgPassing, CallContext, EngineFromGodot, EngineToGodot, GodotConvert, GodotType, InParamTuple,
20-
OutParamTuple, ParamTuple,
19+
ArgPassing, CallContext, EngineFromGodot, EngineToGodot, FromGodot, GodotConvert, GodotType,
20+
InParamTuple, OutParamTuple, ParamTuple, TupleFromGodot,
2121
};
2222

2323
macro_rules! count_idents {
@@ -27,6 +27,8 @@ macro_rules! count_idents {
2727

2828
macro_rules! unsafe_impl_param_tuple {
2929
($(($p:ident, $n:tt): $P:ident),*) => {
30+
impl<$($P: FromGodot + fmt::Debug),*> TupleFromGodot for ($($P,)*) {}
31+
3032
impl<$($P),*> ParamTuple for ($($P,)*) where $($P: GodotConvert + fmt::Debug),* {
3133
const LEN: usize = count_idents!($($P)*);
3234

godot-core/src/meta/signature.rs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,17 @@ use crate::builtin::Variant;
1616
use crate::meta::error::{CallError, CallResult, ConvertError};
1717
use crate::meta::{
1818
EngineFromGodot, EngineToGodot, FromGodot, GodotConvert, GodotType, InParamTuple,
19-
MethodParamOrReturnInfo, OutParamTuple, ParamTuple, ToGodot,
19+
MethodParamOrReturnInfo, OutParamTuple, ParamTuple, ToGodot, TupleFromGodot,
2020
};
2121
use crate::obj::{GodotClass, ValidatedObject};
2222

23+
/// Checks for `#[func]` expansions that all parameters implement `FromGodot` and the return type implements `ToGodot`.
24+
///
25+
/// [`Signature`] itself only requires `EngineFromGodot` and `EngineToGodot`.
26+
#[inline(always)]
27+
#[doc(hidden)]
28+
pub fn ensure_func_bounds<Params: TupleFromGodot, Ret: ToGodot>() {}
29+
2330
/// A full signature for a function.
2431
///
2532
/// For in-calls (that is, calls from the Godot engine to Rust code) `Params` will implement [`InParamTuple`] and `Ret`
@@ -177,7 +184,6 @@ impl<Params: OutParamTuple, Ret: EngineFromGodot> Signature<Params, Ret> {
177184
/// Make a varcall to the Godot engine for a virtual function call.
178185
///
179186
/// # Safety
180-
///
181187
/// - `object_ptr` must be a live instance of a class with a method named `method_sname_ptr`
182188
/// - The method must expect args `args`, and return a value of type `Ret`
183189
#[cfg(since_api = "4.3")]
@@ -224,7 +230,6 @@ impl<Params: OutParamTuple, Ret: EngineFromGodot> Signature<Params, Ret> {
224230
/// Make a ptrcall to the Godot engine for a utility function that has varargs.
225231
///
226232
/// # Safety
227-
///
228233
/// - `utility_fn` must expect args `args`, varargs `varargs`, and return a value of type `Ret`
229234
// Note: this is doing a ptrcall, but uses variant conversions for it.
230235
#[inline]
@@ -253,7 +258,6 @@ impl<Params: OutParamTuple, Ret: EngineFromGodot> Signature<Params, Ret> {
253258
/// Make a ptrcall to the Godot engine for a builtin method that has varargs.
254259
///
255260
/// # Safety
256-
///
257261
/// - `builtin_fn` must expect args `args`, varargs `varargs`, and return a value of type `Ret`
258262
#[inline]
259263
pub unsafe fn out_builtin_ptrcall_varargs(
@@ -317,7 +321,6 @@ impl<Params: OutParamTuple, Ret: EngineFromGodot> Signature<Params, Ret> {
317321
/// Make a ptrcall to the Godot engine for a builtin method.
318322
///
319323
/// # Safety
320-
///
321324
/// - `builtin_fn` must expect explicit args `args`, and return a value of type `Ret`
322325
#[inline]
323326
pub unsafe fn out_builtin_ptrcall(
@@ -346,7 +349,6 @@ impl<Params: OutParamTuple, Ret: EngineFromGodot> Signature<Params, Ret> {
346349
/// Make a ptrcall to the Godot engine for a utility function.
347350
///
348351
/// # Safety
349-
///
350352
/// - `utility_fn` must expect explicit args `args`, and return a value of type `Ret`
351353
#[inline]
352354
pub unsafe fn out_utility_ptrcall(
@@ -371,7 +373,6 @@ impl<Params: OutParamTuple, Ret: EngineFromGodot> Signature<Params, Ret> {
371373
/// Performs a ptrcall and processes the return value to give nice error output.
372374
///
373375
/// # Safety
374-
///
375376
/// This calls [`GodotFfi::new_with_init`] and passes the ptr as the second argument to `f`, see that function for safety docs.
376377
unsafe fn raw_ptrcall(
377378
args: Params,

godot-macros/src/class/data_models/func.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,9 @@ pub fn make_method_registration(
163163
type CallParams = #sig_params;
164164
type CallRet = #sig_ret;
165165

166+
// Statically check that all parameters implement FromGodot + return type implements ToGodot.
167+
::godot::meta::ensure_func_bounds::<CallParams, CallRet>();
168+
166169
let method_name = StringName::from(#method_name_str);
167170

168171
#varcall_fn_decl;

itest/rust/src/engine_tests/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ mod engine_enum_test;
1313
mod gfile_test;
1414
mod match_class_test;
1515
mod native_st_niche_audio_test;
16-
mod native_st_niche_pointer_test;
16+
//mod native_st_niche_pointer_test; // FIXME(v0.5): re-enable once implemented.
1717
mod native_structures_test;
1818
mod node_test;
1919
mod save_load_test;

0 commit comments

Comments
 (0)