Merge pull request #883 from godot-rust/bugfix/virtual-methods-nullable

Virtual methods take `Option<Gd<T>>` (unless whitelisted)

Cargo.toml

@@ -15,3 +15,8 @@ members = [
     "examples/dodge-the-creeps/rust",
     "examples/hot-reload/rust",
 ]
+
+# Note about Jetbrains IDEs: "IDE Sync" (Refresh Cargo projects) may cause static analysis errors such as
+# "at most one `api-*` feature can be enabled". This is because by default, all Cargo features are enabled,
+# which isn't a setup we support. To address this, individual features can be enabled in the various
+# Cargo.toml files: https://www.jetbrains.com/help/rust/rust-cfg-support.html#enable-disable-feature-manually.

godot-codegen/src/generator/default_parameters.rs

@@ -6,7 +6,7 @@
  */
 
 use crate::generator::functions_common;
-use crate::generator::functions_common::FnCode;
+use crate::generator::functions_common::{FnCode, FnParamTokens};
 use crate::models::domain::{FnParam, FnQualifier, Function, RustTy, TyName};
 use crate::util::{ident, safe_ident};
 use crate::{conv, special_cases};
@@ -49,11 +49,15 @@ pub fn make_function_definition_with_defaults(
     let receiver_param = &code.receiver.param;
     let receiver_self = &code.receiver.self_prefix;
 
-    let [required_params_impl_asarg, _, _] =
-        functions_common::make_params_exprs(required_fn_params.iter().cloned(), false, true, true);
+    let FnParamTokens {
+        params: required_params_impl_asarg,
+        ..
+    } = functions_common::make_params_exprs(required_fn_params.iter().cloned(), true, true);
 
-    let [_, _, required_args_internal] =
-        functions_common::make_params_exprs(required_fn_params.into_iter(), false, false, false);
+    let FnParamTokens {
+        arg_names: required_args_internal,
+        ..
+    } = functions_common::make_params_exprs(required_fn_params.into_iter(), false, false);
 
     let return_decl = &sig.return_value().decl;
 

godot-codegen/src/generator/functions_common.rs

@@ -7,8 +7,9 @@
 
 use crate::generator::default_parameters;
 use crate::models::domain::{FnParam, FnQualifier, Function, RustTy};
+use crate::special_cases;
 use crate::util::safe_ident;
-use proc_macro2::TokenStream;
+use proc_macro2::{Ident, TokenStream};
 use quote::{format_ident, quote};
 
 pub struct FnReceiver {
@@ -83,6 +84,22 @@ impl FnDefinitions {
     }
 }
 
+// Gathers multiple token vectors related to function parameters.
+#[derive(Default)]
+pub struct FnParamTokens {
+    pub params: Vec<TokenStream>,
+    pub param_types: Vec<TokenStream>,
+    pub arg_names: Vec<TokenStream>,
+}
+
+impl FnParamTokens {
+    pub fn push_regular(&mut self, param_name: &Ident, param_ty: &RustTy) {
+        self.params.push(quote! { #param_name: #param_ty });
+        self.arg_names.push(quote! { #param_name });
+        self.param_types.push(quote! { #param_ty });
+    }
+}
+
 pub fn make_function_definition(
     sig: &dyn Function,
     code: &FnCode,
@@ -119,12 +136,19 @@ pub fn make_function_definition(
         (TokenStream::new(), TokenStream::new())
     };
 
-    let [params, param_types, arg_names] = make_params_exprs(
-        sig.params().iter(),
-        sig.is_virtual(),
-        !has_default_params, // For *_full function, we don't need impl AsObjectArg<T> parameters
-        !has_default_params, // or arg.as_object_arg() calls.
-    );
+    let FnParamTokens {
+        params,
+        param_types,
+        arg_names,
+    } = if sig.is_virtual() {
+        make_params_exprs_virtual(sig.params().iter(), sig)
+    } else {
+        make_params_exprs(
+            sig.params().iter(),
+            !has_default_params, // For *_full function, we don't need impl AsObjectArg<T> parameters
+            !has_default_params, // or arg.as_object_arg() calls.
+        )
+    };
 
     let rust_function_name_str = sig.name();
 
@@ -200,9 +224,11 @@ pub fn make_function_definition(
             // This can be made more complex if ever necessary.
 
             // A function() may call try_function(), its arguments should not have .as_object_arg().
-            let [_, _, arg_names_without_asarg] = make_params_exprs(
+            let FnParamTokens {
+                arg_names: arg_names_without_asarg,
+                ..
+            } = make_params_exprs(
                 sig.params().iter(),
-                false,
                 !has_default_params, // For *_full function, we don't need impl AsObjectArg<T> parameters
                 false,               // or arg.as_object_arg() calls.
             );
@@ -307,54 +333,81 @@ pub fn make_vis(is_private: bool) -> TokenStream {
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Implementation
 
-// Method could possibly be split -- only one invocation uses all 3 return values, the rest uses only index [0] or [2].
+/// 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>,
-    is_virtual: bool,
     param_is_impl_asarg: bool,
     arg_is_asarg: bool,
-) -> [Vec<TokenStream>; 3] {
-    let mut params = vec![];
-    let mut param_types = vec![]; // or non-generic params
-    let mut arg_names = vec![];
+) -> FnParamTokens {
+    let mut ret = FnParamTokens::default();
 
     for param in method_args {
         let param_name = &param.name;
         let param_ty = &param.type_;
 
-        // Objects (Gd<T>) use implicit conversions via AsObjectArg. Only use in non-virtual functions.
         match &param.type_ {
+            // Non-virtual functions: Objects (Gd<T>) use implicit conversions via AsObjectArg.
             RustTy::EngineClass {
                 object_arg,
                 impl_as_object_arg,
                 ..
-            } if !is_virtual => {
+            } => {
                 // Parameter declarations in signature: impl AsObjectArg<T>
                 if param_is_impl_asarg {
-                    params.push(quote! { #param_name: #impl_as_object_arg });
+                    ret.params.push(quote! { #param_name: #impl_as_object_arg });
                 } else {
-                    params.push(quote! { #param_name: #object_arg });
+                    ret.params.push(quote! { #param_name: #object_arg });
                 }
 
                 // Argument names in function body: arg.as_object_arg() vs. arg
                 if arg_is_asarg {
-                    arg_names.push(quote! { #param_name.as_object_arg() });
+                    ret.arg_names.push(quote! { #param_name.as_object_arg() });
                 } else {
-                    arg_names.push(quote! { #param_name });
+                    ret.arg_names.push(quote! { #param_name });
                 }
 
-                param_types.push(quote! { #object_arg });
+                ret.param_types.push(quote! { #object_arg });
             }
 
-            _ => {
-                params.push(quote! { #param_name: #param_ty });
-                arg_names.push(quote! { #param_name });
-                param_types.push(quote! { #param_ty });
+            // All other methods and parameter types: standard handling.
+            _ => ret.push_regular(param_name, param_ty),
+        }
+    }
+
+    ret
+}
+
+/// For virtual functions, returns the parameter declarations, type tokens, and names.
+pub(crate) fn make_params_exprs_virtual<'a>(
+    method_args: impl Iterator<Item = &'a FnParam>,
+    function_sig: &dyn Function,
+) -> FnParamTokens {
+    let mut ret = FnParamTokens::default();
+
+    for param in method_args {
+        let param_name = &param.name;
+        let param_ty = &param.type_;
+
+        match &param.type_ {
+            // Virtual methods accept Option<Gd<T>>, since we don't know whether objects are nullable or required.
+            RustTy::EngineClass { .. }
+                if !special_cases::is_class_method_param_required(
+                    function_sig.surrounding_class().unwrap(),
+                    function_sig.name(),
+                    param_name,
+                ) =>
+            {
+                ret.params.push(quote! { #param_name: Option<#param_ty> });
+                ret.arg_names.push(quote! { #param_name });
+                ret.param_types.push(quote! { #param_ty });
             }
+
+            // All other methods and parameter types: standard handling.
+            _ => ret.push_regular(param_name, param_ty),
         }
     }
 
-    [params, param_types, arg_names]
+    ret
 }
 
 fn function_uses_pointers(sig: &dyn Function) -> bool {

godot-codegen/src/special_cases/special_cases.rs

@@ -28,6 +28,7 @@ use crate::models::domain::{Enum, RustTy, TyName};
 use crate::models::json::{JsonBuiltinMethod, JsonClassMethod, JsonUtilityFunction};
 use crate::special_cases::codegen_special_cases;
 use crate::Context;
+use proc_macro2::Ident;
 // Deliberately private -- all checks must go through `special_cases`.
 
 #[rustfmt::skip]
@@ -269,6 +270,44 @@ pub fn is_class_method_const(class_name: &TyName, godot_method: &JsonClassMethod
     }
 }
 
+/// Currently only for virtual methods; checks if the specified parameter is required (non-null) and can be declared as `Gd<T>`
+/// instead of `Option<Gd<T>>`.
+pub fn is_class_method_param_required(
+    class_name: &TyName,
+    method_name: &str,
+    param: &Ident, // Don't use `&str` to avoid to_string() allocations for each check on call-site.
+) -> bool {
+    // Note: magically, it's enough if a base class method is declared here; it will be picked up by derived classes.
+
+    match (class_name.godot_ty.as_str(), method_name) {
+        // Nodes.
+        ("Node", "input") => true,
+        ("Node", "shortcut_input") => true,
+        ("Node", "unhandled_input") => true,
+        ("Node", "unhandled_key_input") => true,
+
+        // https://docs.godotengine.org/en/stable/classes/class_collisionobject2d.html#class-collisionobject2d-private-method-input-event
+        ("CollisionObject2D", "input_event") => true, // both parameters.
+
+        // UI.
+        ("Control", "gui_input") => true,
+
+        // Script instances.
+        ("ScriptExtension", "instance_create") => param == "for_object",
+        ("ScriptExtension", "placeholder_instance_create") => param == "for_object",
+        ("ScriptExtension", "inherits_script") => param == "script",
+        ("ScriptExtension", "instance_has") => param == "object",
+
+        // Editor.
+        ("EditorExportPlugin", "customize_resource") => param == "resource",
+        ("EditorExportPlugin", "customize_scene") => param == "scene",
+
+        ("EditorPlugin", "handles") => param == "object",
+
+        _ => false,
+    }
+}
+
 /// True if builtin method is excluded. Does NOT check for type exclusion; use [`is_builtin_type_deleted`] for that.
 pub fn is_builtin_method_deleted(_class_name: &TyName, method: &JsonBuiltinMethod) -> bool {
     // Currently only deleted if codegen.

itest/rust/src/object_tests/object_test.rs

@@ -1057,11 +1057,3 @@ fn double_use_reference() {
     double_use.free();
     emitter.free();
 }
-
-// ----------------------------------------------------------------------------------------------------------------------------------------------
-
-// There isn't a good way to test editor plugins, but we can at least declare one to ensure that the macro
-// compiles.
-#[derive(GodotClass)]
-#[class(no_init, base = EditorPlugin, editor_plugin, tool)]
-struct CustomEditorPlugin;

itest/rust/src/object_tests/virtual_methods_test.rs

@@ -19,9 +19,9 @@ use godot::classes::resource_loader::CacheMode;
 #[cfg(feature = "codegen-full")]
 use godot::classes::Material;
 use godot::classes::{
-    BoxMesh, INode, INode2D, IPrimitiveMesh, IRefCounted, IResourceFormatLoader, IRigidBody2D,
-    InputEvent, InputEventAction, Node, Node2D, PrimitiveMesh, RefCounted, ResourceFormatLoader,
-    ResourceLoader, Viewport, Window,
+    BoxMesh, IEditorPlugin, INode, INode2D, IPrimitiveMesh, IRefCounted, IResourceFormatLoader,
+    IRigidBody2D, InputEvent, InputEventAction, Node, Node2D, Object, PrimitiveMesh, RefCounted,
+    ResourceFormatLoader, ResourceLoader, Viewport, Window,
 };
 use godot::meta::ToGodot;
 use godot::obj::{Base, Gd, NewAlloc, NewGd};
@@ -153,12 +153,12 @@ impl IPrimitiveMesh for VirtualReturnTest {
     fn surface_get_format(&self, _index: i32) -> u32 { unreachable!() }
     fn surface_get_primitive_type(&self, _index: i32) -> u32 { unreachable!() }
     #[cfg(feature = "codegen-full")]
-    fn surface_set_material(&mut self, _index: i32, _material: Gd<Material>) { unreachable!() }
+    fn surface_set_material(&mut self, _index: i32, _material: Option<Gd<Material>>) { unreachable!() }
     #[cfg(feature = "codegen-full")]
     fn surface_get_material(&self, _index: i32) -> Option<Gd<Material>> { unreachable!() }
     fn get_blend_shape_count(&self) -> i32 { unreachable!() }
     fn get_blend_shape_name(&self, _index: i32) -> StringName { unreachable!() }
-    fn set_blend_shape_name(&mut self, _index: i32, _namee: StringName) { unreachable!() }
+    fn set_blend_shape_name(&mut self, _index: i32, _name: StringName) { unreachable!() }
     fn get_aabb(&self) -> godot::prelude::Aabb { unreachable!() }
 }
 
@@ -773,3 +773,25 @@ impl GetSetTest {
         self.always_get_100
     }
 }
+
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
+// There isn't a good way to test editor plugins, but we can at least declare one to ensure that the macro
+// compiles.
+#[derive(GodotClass)]
+#[class(no_init, base = EditorPlugin, editor_plugin, tool)]
+struct CustomEditorPlugin;
+
+// Just override EditorPlugin::edit() to verify method is declared with Option<T>.
+// See https://github.com/godot-rust/gdext/issues/494.
+#[godot_api]
+impl IEditorPlugin for CustomEditorPlugin {
+    fn edit(&mut self, _object: Option<Gd<Object>>) {
+        // Do nothing.
+    }
+
+    // This parameter is non-null.
+    fn handles(&self, _object: Gd<Object>) -> bool {
+        true
+    }
+}