Merge pull request #1173 from godot-rust/bugfix/virtual-dispatch-renamed

Virtual dispatch: fix incorrect matching of renamed methods

Author: Bromeon

godot-codegen/src/generator/mod.rs

@@ -27,11 +27,9 @@ pub mod native_structures;
 pub mod notifications;
 pub mod signals;
 pub mod utility_functions;
+pub mod virtual_definition_consts;
 pub mod virtual_traits;
 
-#[cfg(since_api = "4.4")]
-pub mod virtual_hashes;
-
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
 // Some file generation functions are in specific modules:
@@ -43,11 +41,6 @@ pub mod virtual_hashes;
 pub fn generate_sys_module_file(sys_gen_path: &Path, submit_fn: &mut SubmitFn) {
     // Don't delegate #[cfg] to generated code; causes issues in release CI, reproducible with:
     // cargo clippy --features godot/experimental-godot-api,godot/codegen-rustfmt,godot/serde
-    let virtual_hashes_mod = if cfg!(since_api = "4.4") {
-        quote! { pub mod virtual_hashes; }
-    } else {
-        quote! {}
-    };
 
     let code = quote! {
         pub mod table_builtins;
@@ -56,11 +49,11 @@ pub fn generate_sys_module_file(sys_gen_path: &Path, submit_fn: &mut SubmitFn) {
         pub mod table_scene_classes;
         pub mod table_editor_classes;
         pub mod table_utilities;
-        #virtual_hashes_mod
 
         pub mod central;
         pub mod gdextension_interface;
         pub mod interface;
+        pub mod virtual_consts;
     };
 
     submit_fn(sys_gen_path.join("mod.rs"), code);
@@ -93,12 +86,9 @@ pub fn generate_sys_classes_file(
 
     // From 4.4 onward, generate table that maps all virtual methods to their known hashes.
     // This allows Godot to fall back to an older compatibility function if one is not supported.
-    #[cfg(since_api = "4.4")]
-    {
-        let code = virtual_hashes::make_virtual_hashes_file(api, ctx);
-        submit_fn(sys_gen_path.join("virtual_hashes.rs"), code);
-        watch.record("generate_virtual_hashes_file");
-    }
+    let code = virtual_definition_consts::make_virtual_consts_file(api, ctx);
+    submit_fn(sys_gen_path.join("virtual_consts.rs"), code);
+    watch.record("generate_virtual_consts_file");
 }
 
 pub fn generate_sys_utilities_file(

godot-codegen/src/generator/virtual_hashes.rs renamed to godot-codegen/src/generator/virtual_definition_consts.rs

@@ -10,7 +10,7 @@ use crate::models::domain::{Class, ClassLike, ExtensionApi, FnDirection, Functio
 use proc_macro2::TokenStream;
 use quote::quote;
 
-pub fn make_virtual_hashes_file(api: &ExtensionApi, ctx: &mut Context) -> TokenStream {
+pub fn make_virtual_consts_file(api: &ExtensionApi, ctx: &mut Context) -> TokenStream {
     make_virtual_hashes_for_all_classes(&api.classes, ctx)
 }
 
@@ -39,13 +39,24 @@ fn make_virtual_hashes_for_class(class: &Class, ctx: &mut Context) -> TokenStrea
     };
 
     let constants = class.methods.iter().filter_map(|method| {
-        let FnDirection::Virtual { hash } = method.direction() else {
+        let FnDirection::Virtual {
+            #[cfg(since_api = "4.4")]
+            hash,
+        } = method.direction()
+        else {
             return None;
         };
 
-        let method_name = method.name_ident();
+        let rust_name = method.name_ident();
+        let godot_name_str = method.godot_name();
+
+        #[cfg(since_api = "4.4")]
+        let constant = quote! {
+            pub const #rust_name: (&'static str, u32) = (#godot_name_str, #hash);
+        };
+        #[cfg(before_api = "4.4")]
         let constant = quote! {
-            pub const #method_name: u32 = #hash;
+            pub const #rust_name: &'static str = #godot_name_str;
         };
 
         Some(constant)

godot-ffi/src/lib.rs

@@ -82,8 +82,7 @@ pub use gen::table_editor_classes::*;
 pub use gen::table_scene_classes::*;
 pub use gen::table_servers_classes::*;
 pub use gen::table_utilities::*;
-#[cfg(since_api = "4.4")]
-pub use gen::virtual_hashes as known_virtual_hashes;
+pub use gen::virtual_consts as godot_virtual_consts;
 
 // Other
 pub use extras::*;

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

@@ -102,14 +102,13 @@ pub fn transform_trait_impl(mut original_impl: venial::Impl) -> ParseResult<Toke
         && !decls
             .overridden_virtuals
             .iter()
-            .any(|v| v.method_name == "_ready")
+            .any(|v| v.rust_method_name == "_ready")
     {
         let match_arm = OverriddenVirtualFn {
             cfg_attrs: vec![],
-            method_name: "_ready".to_string(),
-            // Can't use `hashes::ready` here, as the base class might not be `Node` (see above why such a branch is still added).
-            #[cfg(since_api = "4.4")]
-            hash_constant: quote! { ::godot::sys::known_virtual_hashes::Node::ready },
+            rust_method_name: "_ready".to_string(),
+            // Can't use `virtuals::ready` here, as the base class might not be `Node` (see above why such a branch is still added).
+            godot_name_hash_constant: quote! { ::godot::sys::godot_virtual_consts::Node::ready },
             signature_info: SignatureInfo::fn_ready(),
             before_kind: BeforeKind::OnlyBefore,
             interface_trait: None,
@@ -136,16 +135,12 @@ pub fn transform_trait_impl(mut original_impl: venial::Impl) -> ParseResult<Toke
     };
 
     // See also __default_virtual_call() codegen.
-    let (hash_param, hashes_use, match_expr);
+    let (hash_param, match_expr);
     if cfg!(since_api = "4.4") {
         hash_param = quote! { hash: u32, };
-        hashes_use = quote! {
-            use ::godot::sys::known_virtual_hashes::#trait_base_class as hashes;
-        };
         match_expr = quote! { (name, hash) };
     } else {
         hash_param = TokenStream::new();
-        hashes_use = TokenStream::new();
         match_expr = quote! { name };
     };
 
@@ -164,9 +159,9 @@ pub fn transform_trait_impl(mut original_impl: venial::Impl) -> ParseResult<Toke
             fn __virtual_call(name: &str, #hash_param) -> ::godot::sys::GDExtensionClassCallVirtual {
                 //println!("virtual_call: {}.{}", std::any::type_name::<Self>(), name);
                 use ::godot::obj::UserClass as _;
+                use ::godot::sys::godot_virtual_consts::#trait_base_class as virtuals;
                 #tool_check
 
-                #hashes_use
                 match #match_expr {
                     #( #virtual_match_arms )*
                     _ => None,
@@ -459,7 +454,6 @@ fn handle_regular_virtual_fn<'a>(
     cfg_attrs: Vec<&'a venial::Attribute>,
     decls: &mut IDecls<'a>,
 ) -> Option<(venial::Punctuated<venial::FnParam>, Group)> {
-    #[cfg(since_api = "4.4")]
     let method_name_ident = original_method.name.clone();
     let method = util::reduce_to_signature(original_method);
 
@@ -518,12 +512,11 @@ fn handle_regular_virtual_fn<'a>(
     // each distinct method.
     decls.overridden_virtuals.push(OverriddenVirtualFn {
         cfg_attrs,
-        method_name: virtual_method_name,
+        rust_method_name: virtual_method_name,
         // If ever the `I*` verbatim validation is relaxed (it won't work with use-renames or other weird edge cases), the approach
-        // with known_virtual_hashes module could be changed to something like the following (GodotBase = nearest Godot base class):
+        // with godot_virtual_consts module could be changed to something like the following (GodotBase = nearest Godot base class):
         // __get_virtual_hash::<Self::GodotBase>("method")
-        #[cfg(since_api = "4.4")]
-        hash_constant: quote! { hashes::#method_name_ident },
+        godot_name_hash_constant: quote! { virtuals::#method_name_ident },
         signature_info,
         before_kind,
         interface_trait: Some(trait_path.clone()),
@@ -573,9 +566,11 @@ fn make_inactive_class_check(_return_value: TokenStream) -> TokenStream {
 
 struct OverriddenVirtualFn<'a> {
     cfg_attrs: Vec<&'a venial::Attribute>,
-    method_name: String,
-    #[cfg(since_api = "4.4")]
-    hash_constant: TokenStream,
+    rust_method_name: String,
+    /// Path to a pre-defined constant storing a `("_virtual_func", 123456789)` tuple with name and hash of the virtual function.
+    ///
+    /// Before Godot 4.4, this just stores the name `"_virtual_func"`.
+    godot_name_hash_constant: TokenStream,
     signature_info: SignatureInfo,
     before_kind: BeforeKind,
     interface_trait: Option<venial::TypeExpr>,
@@ -584,16 +579,7 @@ struct OverriddenVirtualFn<'a> {
 impl OverriddenVirtualFn<'_> {
     fn make_match_arm(&self, class_name: &Ident) -> TokenStream {
         let cfg_attrs = self.cfg_attrs.iter();
-        let method_name_str = self.method_name.as_str();
-
-        #[cfg(since_api = "4.4")]
-        let pattern = {
-            let hash_constant = &self.hash_constant;
-            quote! { (#method_name_str, #hash_constant) }
-        };
-
-        #[cfg(before_api = "4.4")]
-        let pattern = method_name_str;
+        let godot_name_hash_constant = &self.godot_name_hash_constant;
 
         // Lazily generate code for the actual work (calling user function).
         let method_callback = make_virtual_callback(
@@ -605,7 +591,7 @@ impl OverriddenVirtualFn<'_> {
 
         quote! {
             #(#cfg_attrs)*
-            #pattern => #method_callback,
+            #godot_name_hash_constant => #method_callback,
         }
     }
 }

godot-macros/src/class/derive_godot_class.rs

@@ -400,13 +400,15 @@ fn make_user_class_impl(
         // See also __virtual_call() codegen.
         // This doesn't explicitly check if the base class inherits from Node (and thus has `_ready`), but the derive-macro already does
         // this for the `OnReady` field declaration.
-        let (hash_param, hash_check);
+        let (hash_param, matches_ready_hash);
         if cfg!(since_api = "4.4") {
             hash_param = quote! { hash: u32, };
-            hash_check = quote! { && hash == ::godot::sys::known_virtual_hashes::Node::ready };
+            matches_ready_hash = quote! {
+                (name, hash) == ::godot::sys::godot_virtual_consts::Node::ready
+            };
         } else {
             hash_param = TokenStream::new();
-            hash_check = TokenStream::new();
+            matches_ready_hash = quote! { name == "_ready" }
         }
 
         let default_virtual_fn = quote! {
@@ -417,7 +419,7 @@ fn make_user_class_impl(
                 use ::godot::obj::UserClass as _;
                 #tool_check
 
-                if name == "_ready" #hash_check {
+                if #matches_ready_hash {
                     #callback
                 } else {
                     None