Merge pull request #1174 from godot-rust/qol/rename-unsafe-virtuals

Add `_rawptr` suffix to all unsafe virtual functions

Author: Bromeon

godot-codegen/src/conv/name_conversions.rs

@@ -7,9 +7,8 @@
 
 //! Identifier renamings (Godot -> Rust)
 
-use proc_macro2::Ident;
-
 use crate::util::ident;
+use proc_macro2::Ident;
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Case conversions
@@ -104,6 +103,13 @@ pub fn shout_to_pascal(shout_case: &str) -> String {
     result
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+// Virtual functions
+
+pub fn make_unsafe_virtual_fn_name(rust_fn_name: &str) -> String {
+    format!("{rust_fn_name}_rawptr")
+}
+
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Enum conversions
 

godot-codegen/src/generator/functions_common.rs

@@ -122,7 +122,7 @@ pub fn make_function_definition(
     // Thus, let's keep things simple and more conservative.
     let (maybe_unsafe, maybe_safety_doc) = if let Some(safety_doc) = safety_doc {
         (quote! { unsafe }, safety_doc)
-    } else if function_uses_pointers(sig) {
+    } else if sig.common().is_unsafe {
         (
             quote! { unsafe },
             quote! {
@@ -614,7 +614,7 @@ pub(crate) fn make_params_exprs_virtual<'a>(
             RustTy::EngineClass { .. }
                 if !special_cases::is_class_method_param_required(
                     function_sig.surrounding_class().unwrap(),
-                    function_sig.name(),
+                    function_sig.godot_name(),
                     param_name,
                 ) =>
             {
@@ -637,15 +637,3 @@ pub(crate) fn make_params_exprs_virtual<'a>(
 
     ret
 }
-
-fn function_uses_pointers(sig: &dyn Function) -> bool {
-    let has_pointer_params = sig
-        .params()
-        .iter()
-        .any(|param| matches!(param.type_, RustTy::RawPointer { .. }));
-
-    let has_pointer_return = matches!(sig.return_value().type_, Some(RustTy::RawPointer { .. }));
-
-    // No short-circuiting due to variable decls, but that's fine.
-    has_pointer_params || has_pointer_return
-}

godot-codegen/src/generator/virtual_traits.rs

@@ -212,8 +212,10 @@ fn make_all_virtual_methods(
         for method in base_class.methods.iter() {
             // Certain derived classes in Godot implement a virtual method declared in a base class, thus no longer
             // making it required. This isn't advertised in the extension_api, but instead manually tracked via special cases.
-            let derived_presence =
-                special_cases::get_derived_virtual_method_presence(class.name(), method.name());
+            let derived_presence = special_cases::get_derived_virtual_method_presence(
+                class.name(),
+                method.godot_name(),
+            );
 
             // Collect all changes in a Markdown table.
             let new = match derived_presence {

godot-codegen/src/models/domain.rs

@@ -284,6 +284,8 @@ pub struct FunctionCommon {
     pub is_vararg: bool,
     pub is_private: bool,
     pub is_virtual_required: bool,
+    /// Whether raw pointers appear in signature. Affects safety, and in case of virtual methods, the name.
+    pub is_unsafe: bool,
     pub direction: FnDirection,
 }
 

godot-codegen/src/models/domain_mapping.rs

@@ -10,7 +10,7 @@ use crate::models::domain::{
     BuildConfiguration, BuiltinClass, BuiltinMethod, BuiltinSize, BuiltinVariant, Class,
     ClassCommons, ClassConstant, ClassConstantValue, ClassMethod, ClassSignal, Constructor, Enum,
     Enumerator, EnumeratorValue, ExtensionApi, FnDirection, FnParam, FnQualifier, FnReturn,
-    FunctionCommon, GodotApiVersion, ModName, NativeStructure, Operator, Singleton, TyName,
+    FunctionCommon, GodotApiVersion, ModName, NativeStructure, Operator, RustTy, Singleton, TyName,
     UtilityFunction,
 };
 use crate::models::json::{
@@ -374,6 +374,7 @@ impl BuiltinMethod {
                 is_vararg: method.is_vararg,
                 is_private: false, // See 'exposed' below. Could be special_cases::is_method_private(builtin_name, &method.name),
                 is_virtual_required: false,
+                is_unsafe: false, // Builtin methods don't use raw pointers.
                 direction: FnDirection::Outbound {
                     hash: method.hash.expect("hash absent for builtin method"),
                 },
@@ -456,6 +457,7 @@ impl ClassMethod {
             },
         };
 
+        // May still be renamed further, for unsafe methods. Not done here because data to determine safety is not available yet.
         let rust_method_name = Self::make_virtual_method_name(class_name, &method.name);
 
         Self::from_json_inner(method, rust_method_name, class_name, direction, ctx)
@@ -488,7 +490,7 @@ impl ClassMethod {
         // Since Godot 4.4, GDExtension advertises whether virtual methods have a default implementation or are required to be overridden.
         #[cfg(before_api = "4.4")]
         let is_virtual_required =
-            special_cases::is_virtual_method_required(&class_name, rust_method_name);
+            special_cases::is_virtual_method_required(&class_name, &method.name);
 
         #[cfg(since_api = "4.4")]
         #[allow(clippy::let_and_return)]
@@ -507,15 +509,29 @@ impl ClassMethod {
             is_required_in_json
         };
 
+        let parameters = FnParam::new_range(&method.arguments, ctx);
+        let return_value = FnReturn::new(&method.return_value, ctx);
+        let is_unsafe = Self::function_uses_pointers(&parameters, &return_value);
+
+        // Future note: if further changes are made to the virtual method name, make sure to make it reversible so that #[godot_api]
+        // can match on the Godot name of the virtual method.
+        let rust_method_name = if is_unsafe && method.is_virtual {
+            // If the method is unsafe, we need to rename it to avoid conflicts with the safe version.
+            conv::make_unsafe_virtual_fn_name(rust_method_name)
+        } else {
+            rust_method_name.to_string()
+        };
+
         Some(Self {
             common: FunctionCommon {
-                name: rust_method_name.to_string(),
+                name: rust_method_name,
                 godot_name: godot_method_name,
-                parameters: FnParam::new_range(&method.arguments, ctx),
-                return_value: FnReturn::new(&method.return_value, ctx),
+                parameters,
+                return_value,
                 is_vararg: method.is_vararg,
                 is_private,
                 is_virtual_required,
+                is_unsafe,
                 direction,
             },
             qualifier,
@@ -524,12 +540,28 @@ impl ClassMethod {
     }
 
     fn make_virtual_method_name<'m>(class_name: &TyName, godot_method_name: &'m str) -> &'m str {
-        // Remove leading underscore from virtual method names.
-        let method_name = godot_method_name
+        // Hardcoded overrides.
+        if let Some(rust_name) =
+            special_cases::maybe_rename_virtual_method(class_name, godot_method_name)
+        {
+            return rust_name;
+        }
+
+        // In general, just rlemove leading underscore from virtual method names.
+        godot_method_name
             .strip_prefix('_')
-            .unwrap_or(godot_method_name);
+            .unwrap_or(godot_method_name)
+    }
+
+    fn function_uses_pointers(parameters: &[FnParam], return_value: &FnReturn) -> bool {
+        let has_pointer_params = parameters
+            .iter()
+            .any(|param| matches!(param.type_, RustTy::RawPointer { .. }));
+
+        let has_pointer_return = matches!(return_value.type_, Some(RustTy::RawPointer { .. }));
 
-        special_cases::maybe_rename_virtual_method(class_name, method_name)
+        // No short-circuiting due to variable decls, but that's fine.
+        has_pointer_params || has_pointer_return
     }
 }
 
@@ -584,6 +616,7 @@ impl UtilityFunction {
                 is_vararg: function.is_vararg,
                 is_private,
                 is_virtual_required: false,
+                is_unsafe: false, // Utility functions don't use raw pointers.
                 direction: FnDirection::Outbound {
                     hash: function.hash,
                 },