Merge #973

973: Make ptrcalls opt-in r=chitoyuu a=chitoyuu

This implements cuddlefishie's suggestion in #814, but with the semantics reversed to better conform with Cargo's additive combination of features.

- Added the new feature flag `ptrcall`, which enables performant API calls at the cost of forward binary compatibility with the engine.
- Added tests with and without the feature to the full CI suite.

I believe the current semantics to be better, since by making the safer option the default, it helps reduce the surprise factor when someone tries to use the crate with a supposedly compatible version of Godot. Different opinions are welcome.

Note that this only addresses binary compatibility -- #814 is also in a large part an API problem.

Co-authored-by: Chitose Yuuzaki <[email protected]>

.github/workflows/full-ci.yml

@@ -273,19 +273,34 @@ jobs:
           - rust: stable
             godot: "3.5.1-stable"
             postfix: ''
+          - rust: stable
+            godot: "3.5.1-stable"
+            postfix: ' (ptrcall)'
+            build_args: '--features ptrcall'
           - rust: nightly
             godot: "3.5.1-stable"
             postfix: ' (nightly)'
+          - rust: nightly
+            godot: "3.5.1-stable"
+            postfix: ' (nightly, ptrcall)'
+            build_args: '--features ptrcall'
           - rust: '1.63'
             godot: "3.5.1-stable"
             postfix: ' (msrv 1.63)'
+          - rust: '1.63'
+            godot: "3.5.1-stable"
+            postfix: ' (msrv 1.63, ptrcall)'
+            build_args: '--features ptrcall'
 
           # Test with oldest supported engine version
           - rust: stable
             godot: "3.2-stable"
             postfix: ''
             build_args: '--features custom-godot'
-
+          - rust: stable
+            godot: "3.2-stable"
+            postfix: ' (ptrcall)'
+            build_args: '--features custom-godot,ptrcall'
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v3

bindings-generator/Cargo.toml

@@ -13,6 +13,7 @@ rust-version = "1.63"
 
 [features]
 debug = []
+ptrcall = []
 custom-godot = ["which"]
 
 [dependencies]

bindings-generator/src/api.rs

@@ -612,6 +612,48 @@ impl Ty {
             }
         }
     }
+
+    pub fn to_return_post_variant(&self) -> TokenStream {
+        match self {
+            Ty::Void => Default::default(),
+            Ty::F64 | Ty::I64 | Ty::Bool => {
+                let rust_type = self.to_rust();
+                quote! {
+                    <#rust_type>::coerce_from_variant(&ret)
+                }
+            }
+
+            // The `sys` type aliases here can point to different types depending on the platform.
+            // Do not simplify to (Linux) primitive types.
+            Ty::Result => {
+                quote! { GodotError::result_from_sys(sys::godot_error::from_variant(&ret).expect("Unexpected variant type")) }
+            }
+            Ty::VariantType => {
+                quote! { VariantType::from_sys(sys::godot_variant_type::from_variant(&ret).expect("Unexpected variant type")) }
+            }
+            Ty::VariantOperator => {
+                quote! {
+                    VariantOperator::try_from_sys(sys::godot_variant_operator::from_variant(&ret).expect("Unexpected variant type"))
+                        .expect("enum variant should be valid")
+                }
+            }
+
+            Ty::Vector3Axis => {
+                quote! {
+                    unsafe {
+                        mem::transmute::<u32, Axis>(u32::from_variant(&ret).expect("Unexpected variant type") as _)
+                    }
+                }
+            }
+            _ => {
+                let rust_type = self.to_rust();
+                // always a variant returned, use FromVariant
+                quote! {
+                    <#rust_type>::from_variant(&ret).expect("Unexpected variant type")
+                }
+            }
+        }
+    }
 }
 
 pub fn module_name_from_class_name(class_name: &str) -> String {

bindings-generator/src/classes.rs

@@ -135,6 +135,13 @@ pub(crate) fn generate_enums(class: &GodotClass) -> TokenStream {
                     v.0
                 }
             }
+
+            impl FromVariant for #typ_name {
+                #[inline]
+                fn from_variant(v: &Variant) -> Result<Self, FromVariantError> {
+                    i64::from_variant(v).map(Self::from)
+                }
+            }
         }
     });
 

bindings-generator/src/methods.rs

@@ -10,11 +10,14 @@ use std::collections::HashMap;
 /// Types of icalls.
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
 pub(crate) enum IcallType {
+    #[cfg(feature = "ptrcall")]
     Ptr,
     Varargs,
     Var,
 }
 
+// The `return_type` field is currently only used when ptrcall is enabled
+#[cfg_attr(not(feature = "ptrcall"), allow(dead_code))]
 pub(crate) struct MethodSig {
     pub(crate) return_type: Ty,
     pub(crate) arguments: Vec<Ty>,
@@ -107,6 +110,7 @@ impl MethodSig {
         // Only ptrcalls have "static" typing on their return types.
         // The other calling types always return `Variant`.
         let mut name = match icall_ty {
+            #[cfg(feature = "ptrcall")]
             IcallType::Ptr => format!("icallptr_{}", ty_arg_name(&self.return_type)),
             IcallType::Varargs => String::from("icallvarargs_"),
             IcallType::Var => String::from("icallvar_"),
@@ -121,6 +125,7 @@ impl MethodSig {
     }
 
     #[allow(clippy::single_match)]
+    #[cfg(feature = "ptrcall")]
     pub(crate) fn icall_type(&self) -> IcallType {
         if self.has_varargs {
             return IcallType::Varargs;
@@ -133,6 +138,15 @@ impl MethodSig {
 
         IcallType::Ptr
     }
+
+    #[cfg(not(feature = "ptrcall"))]
+    pub(crate) fn icall_type(&self) -> IcallType {
+        if self.has_varargs {
+            return IcallType::Varargs;
+        }
+
+        IcallType::Var
+    }
 }
 
 fn skip_method(method: &GodotMethod, name: &str) -> bool {
@@ -544,29 +558,26 @@ fn unsafe_reason(
 
 fn ret_recover(ty: &Ty, icall_ty: IcallType) -> TokenStream {
     match icall_ty {
+        #[cfg(feature = "ptrcall")]
         IcallType::Ptr => ty.to_return_post(),
         IcallType::Varargs => {
             // only variant possible
             quote! { ret }
         }
-        IcallType::Var => {
-            let rust_type = ty.to_rust();
-            // always a variant returned, use FromVariant
-            quote! {
-                <#rust_type>::from_variant(&ret).expect("Unexpected variant type")
-            }
-        }
+        IcallType::Var => ty.to_return_post_variant(),
     }
 }
 
 pub(crate) fn generate_icall(name: String, sig: MethodSig) -> TokenStream {
     match sig.icall_type() {
+        #[cfg(feature = "ptrcall")]
         IcallType::Ptr => ptrcall::generate_icall(name, sig),
         IcallType::Varargs => varargs_call::generate_icall(name, sig),
         IcallType::Var => varcall::generate_icall(name, sig),
     }
 }
 
+#[cfg(feature = "ptrcall")]
 mod ptrcall {
     use super::*;
     use quote::{format_ident, quote};

bindings-generator/src/special_methods.rs

@@ -80,12 +80,18 @@ pub fn generate_queue_free_impl(api: &Api, class: &GodotClass) -> TokenStream {
     let class_name = format_ident!("{}", class.name);
 
     let queue_free_output = if class.name == "Node" || api.class_inherits(class, "Node") {
+        #[cfg(feature = "ptrcall")]
+        let icall_ident = proc_macro2::Ident::new("icallptr_void", proc_macro2::Span::call_site());
+
+        #[cfg(not(feature = "ptrcall"))]
+        let icall_ident = proc_macro2::Ident::new("icallvar_", proc_macro2::Span::call_site());
+
         quote! {
             impl QueueFree for #class_name {
                 #[inline]
                 unsafe fn godot_queue_free(obj: *mut sys::godot_object) {
                     let method_bind: *mut sys::godot_method_bind = crate::generated::node::NodeMethodTable::get(get_api()).queue_free;
-                    crate::icalls::icallptr_void(method_bind, obj)
+                    crate::icalls::#icall_ident(method_bind, obj);
                 }
             }
         }

gdnative-bindings/Cargo.toml

@@ -15,6 +15,7 @@ rust-version = "1.63"
 formatted = []
 one-class-one-file = []
 custom-godot = ["gdnative_bindings_generator/custom-godot"]
+ptrcall = ["gdnative_bindings_generator/ptrcall"]
 
 [dependencies]
 gdnative-core = { path = "../gdnative-core", version = "=0.11.0" }

gdnative-bindings/src/generated.rs

@@ -1,6 +1,8 @@
 #![allow(unused_variables)]
 
+#[cfg(feature = "ptrcall")]
 use libc;
+
 use libc::c_char;
 use std::mem;
 use std::ptr;

gdnative-bindings/src/icalls.rs

@@ -1,11 +1,14 @@
 #![allow(unused_variables)]
 
+#[cfg(feature = "ptrcall")]
+use gdnative_core::*;
+#[cfg(feature = "ptrcall")]
 use libc;
+
 use std::ptr;
 
 use gdnative_core::core_types::*;
 use gdnative_core::private::get_api;
 use gdnative_core::sys;
-use gdnative_core::*;
 
 include!(concat!(env!("OUT_DIR"), "/icalls.rs"));

gdnative/Cargo.toml

@@ -19,6 +19,7 @@ default = []
 async = ["gdnative-async"]
 custom-godot = ["gdnative-bindings/custom-godot"]
 formatted = ["gdnative-bindings/formatted", "gdnative-bindings/one-class-one-file"]
+ptrcall = ["gdnative-bindings/ptrcall"]
 serde = ["gdnative-core/serde"]
 
 # Internal

gdnative/src/lib.rs

@@ -69,6 +69,16 @@
 //!   Enable if the generated binding source code should be human-readable and split
 //!   into multiple files. This can also help IDEs that struggle with a single huge file.
 //!
+//! * **`ptrcall`**<br>
+//!   Enables the `ptrcall` convention for calling Godot API methods. This increases performance, at the
+//!   cost of forward binary compatibility with the engine. Binaries built with `ptrcall` enabled
+//!   **may exhibit undefined behavior** when loaded by a different version of Godot, even when there are
+//!   no breaking API changes as far as GDScript is concerned. Notably, the addition of new default
+//!   parameters breaks any code using `ptrcall`.
+//!
+//!   Cargo features are additive, and as such, it's only necessary to enable this feature for the final
+//!   `cdylib` crates, whenever desired.
+//!
 //! [thread-safety]: https://docs.godotengine.org/en/stable/tutorials/threads/thread_safe_apis.html
 //! [gdnative-overview]: https://godot-rust.github.io/book/gdnative-overview.html
 //! [custom-godot]: https://godot-rust.github.io/book/advanced-guides/custom-godot.html

test/Cargo.toml

@@ -14,6 +14,7 @@ crate-type = ["cdylib"]
 default = []
 type-tag-fallback = ["gdnative/type-tag-fallback"]
 custom-godot = ["gdnative/custom-godot"]
+ptrcall = ["gdnative/ptrcall"]
 
 [dependencies]
 gdnative = { path = "../gdnative", features = ["gd-test", "serde", "async"] }