Merge pull request #1136 from godot-rust/qol/virtuals-no-longer-required

Bromeon · web-flow · commit 4d2251bbd669 · 2025-05-14T20:52:25.000Z
Godot virtuals are no longer required if a derived Godot class implements them
diff --git a/godot-codegen/src/generator/virtual_traits.rs b/godot-codegen/src/generator/virtual_traits.rs
@@ -8,11 +8,13 @@
 use crate::generator::functions_common::FnCode;
 use crate::generator::{docs, functions_common};
 use crate::models::domain::{
-    ApiView, Class, ClassLike, ClassMethod, FnQualifier, Function, TyName,
+    ApiView, Class, ClassLike, ClassMethod, FnQualifier, Function, TyName, VirtualMethodPresence,
 };
+use crate::special_cases;
 use crate::util::ident;
 use proc_macro2::{Ident, TokenStream};
 use quote::quote;
+use std::fmt::Write;
 
 pub fn make_virtual_methods_trait(
     class: &Class,
@@ -25,19 +27,20 @@ pub fn make_virtual_methods_trait(
     let trait_name = ident(trait_name_str);
     let class_name = &class.name().rust_ty;
 
-    let virtual_method_fns = make_all_virtual_methods(class, all_base_names, view);
+    let (virtual_method_fns, extra_docs) = make_all_virtual_methods(class, all_base_names, view);
     let special_virtual_methods = make_special_virtual_methods(notification_enum_name);
 
     let trait_doc = docs::make_virtual_trait_doc(trait_name_str, class.name());
 
     quote! {
         #[doc = #trait_doc]
+        #[doc = #extra_docs]
         #[allow(unused_variables)]
         #[allow(clippy::unimplemented)]
         #cfg_attributes
         pub trait #trait_name: crate::obj::GodotClass<Base = #class_name> + crate::private::You_forgot_the_attribute__godot_api {
             #special_virtual_methods
-            #( #virtual_method_fns )*
+            #virtual_method_fns
         }
     }
 }
@@ -150,11 +153,22 @@ fn make_special_virtual_methods(notification_enum_name: &Ident) -> TokenStream {
     }
 }
 
-fn make_virtual_method(method: &ClassMethod) -> Option<TokenStream> {
+fn make_virtual_method(
+    method: &ClassMethod,
+    presence: VirtualMethodPresence,
+) -> Option<TokenStream> {
     if !method.is_virtual() {
         return None;
     }
 
+    // Possibly change behavior of required/optional-ness of the virtual method in derived classes.
+    // It's also possible that it's removed, which would not declare it at all in the `I*` trait.
+    let is_virtual_required = match presence {
+        VirtualMethodPresence::Inherit => method.is_virtual_required(),
+        VirtualMethodPresence::Override { is_required } => is_required,
+        VirtualMethodPresence::Remove => return None,
+    };
+
     // Virtual methods are never static.
     let qualifier = method.qualifier();
     assert!(matches!(qualifier, FnQualifier::Mut | FnQualifier::Const));
@@ -166,7 +180,7 @@ fn make_virtual_method(method: &ClassMethod) -> Option<TokenStream> {
             // make_return() requests following args, but they are not used for virtual methods. We can provide empty streams.
             varcall_invocation: TokenStream::new(),
             ptrcall_invocation: TokenStream::new(),
-            is_virtual_required: method.is_virtual_required(),
+            is_virtual_required,
             is_varcall_fallible: true,
         },
         None,
@@ -181,24 +195,76 @@ fn make_all_virtual_methods(
     class: &Class,
     all_base_names: &[TyName],
     view: &ApiView,
-) -> Vec<TokenStream> {
-    let mut all_tokens = vec![];
+) -> (TokenStream, String) {
+    let mut all_tokens = TokenStream::new();
 
     for method in class.methods.iter() {
         // Assumes that inner function filters on is_virtual.
-        if let Some(tokens) = make_virtual_method(method) {
-            all_tokens.push(tokens);
+        if let Some(tokens) = make_virtual_method(method, VirtualMethodPresence::Inherit) {
+            all_tokens.extend(tokens);
         }
     }
 
+    let mut changes_from_base = String::new();
+
     for base_name in all_base_names {
         let base_class = view.get_engine_class(base_name);
         for method in base_class.methods.iter() {
-            if let Some(tokens) = make_virtual_method(method) {
-                all_tokens.push(tokens);
+            // 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());
+
+            // Collect all changes in a Markdown table.
+            let new = match derived_presence {
+                VirtualMethodPresence::Inherit => None,
+                VirtualMethodPresence::Override { is_required } => {
+                    Some(format_required(is_required))
+                }
+                VirtualMethodPresence::Remove => Some("removed"),
+            };
+            if let Some(new) = new {
+                let orig = format_required(method.is_virtual_required());
+                let base_name = &base_name.rust_ty;
+                let method_name = format_method_name(method);
+
+                write!(
+                    changes_from_base,
+                    "\n| [`I{base_name}::{method_name}`](crate::classes::I{base_name}::{method_name}) | {orig} | {new} |"
+                )
+                .unwrap();
+            }
+
+            if let Some(tokens) = make_virtual_method(method, derived_presence) {
+                all_tokens.extend(tokens);
             }
         }
     }
 
-    all_tokens
+    let extra_docs = if changes_from_base.is_empty() {
+        String::new()
+    } else {
+        let class_name = &class.name().rust_ty;
+        format!(
+            "\n\n# Changes from base interface traits\n\
+            The following virtual methods originally declared in direct/indirect base classes have their presence (required/optional) changed \
+            in `I{class_name}`. This can happen if Godot already overrides virtual methods, discouraging the user from further overriding them.\n\
+            \n\n| Base method | Original | New |\n| --- | --- | --- |{changes_from_base}"
+        )
+    };
+
+    (all_tokens, extra_docs)
+}
+
+fn format_required(is_required: bool) -> &'static str {
+    if is_required {
+        "required"
+    } else {
+        "optional"
+    }
+}
+
+fn format_method_name(method: &ClassMethod) -> &str {
+    // TODO when we have `_unsafe` or similar postfix with raw pointers, update this here.
+    method.name()
 }
diff --git a/godot-codegen/src/models/domain.rs b/godot-codegen/src/models/domain.rs
@@ -723,6 +723,20 @@ pub enum ArgPassing {
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
+/// Behavior of virtual methods in derived classes.
+pub enum VirtualMethodPresence {
+    /// Preserve default behavior of base class (required or optional).
+    Inherit,
+
+    /// Virtual method is now required/optional according to `is_required`, independent of base method declaration.
+    Override { is_required: bool },
+
+    /// Virtual method is removed in derived classes (no longer appearing in their interface trait).
+    Remove,
+}
+
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+
 /// Contains multiple naming conventions for types (classes, builtin classes, enums).
 #[derive(Clone, Eq, PartialEq, Hash)]
 pub struct TyName {
diff --git a/godot-codegen/src/models/domain_mapping.rs b/godot-codegen/src/models/domain_mapping.rs
@@ -487,20 +487,26 @@ 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_ty.to_string(),
-            rust_method_name,
-        );
+        let is_virtual_required =
+            special_cases::is_virtual_method_required(&class_name, rust_method_name);
 
         #[cfg(since_api = "4.4")]
-        let is_virtual_required = method.is_virtual
-            && method.is_required.unwrap_or_else(|| {
+        #[allow(clippy::let_and_return)]
+        let is_virtual_required = method.is_virtual && {
+            // Evaluate this always first (before potential manual overrides), to detect mistakes in spec.
+            let is_required_in_json = method.is_required.unwrap_or_else(|| {
                 panic!(
                     "virtual method {}::{} lacks field `is_required`",
                     class_name.rust_ty, rust_method_name
                 );
             });
 
+            // Potential special cases come here. The situation "virtual function is required in base class, but not in derived"
+            // is not handled here, but in virtual_traits.rs. Here, virtual methods appear only once, in their base.
+
+            is_required_in_json
+        };
+
         Some(Self {
             common: FunctionCommon {
                 name: rust_method_name.to_string(),
diff --git a/godot-codegen/src/special_cases/special_cases.rs b/godot-codegen/src/special_cases/special_cases.rs
@@ -26,7 +26,7 @@
 #![allow(clippy::match_like_matches_macro)] // if there is only one rule
 
 use crate::conv::to_enum_type_uncached;
-use crate::models::domain::{Enum, RustTy, TyName};
+use crate::models::domain::{Enum, RustTy, TyName, VirtualMethodPresence};
 use crate::models::json::{JsonBuiltinMethod, JsonClassMethod, JsonSignal, JsonUtilityFunction};
 use crate::special_cases::codegen_special_cases;
 use crate::util::option_as_slice;
@@ -659,9 +659,11 @@ pub fn get_interface_extra_docs(trait_name: &str) -> Option<&'static str> {
 }
 
 #[cfg(before_api = "4.4")]
-pub fn is_virtual_method_required(class_name: &str, method: &str) -> bool {
-    match (class_name, method) {
-        ("ScriptLanguageExtension", _) => method != "get_doc_comment_delimiters",
+pub fn is_virtual_method_required(class_name: &TyName, rust_method_name: &str) -> bool {
+    // Do not call is_derived_virtual_method_required() here; that is handled in virtual_traits.rs.
+
+    match (class_name.godot_ty.as_str(), rust_method_name) {
+        ("ScriptLanguageExtension", method) => method != "get_doc_comment_delimiters",
 
         ("ScriptExtension", "editor_can_reload_from_file")
         | ("ScriptExtension", "can_instantiate")
@@ -699,7 +701,7 @@ pub fn is_virtual_method_required(class_name: &str, method: &str) -> bool {
         | ("EditorExportPlugin", "customize_scene")
         | ("EditorExportPlugin", "get_customization_configuration_hash")
         | ("EditorExportPlugin", "get_name")
-        | ("EditorVcsInterface", _)
+        | ("EditorVCSInterface", _)
         | ("MovieWriter", _)
         | ("TextServerExtension", "has_feature")
         | ("TextServerExtension", "get_name")
@@ -805,23 +807,23 @@ pub fn is_virtual_method_required(class_name: &str, method: &str) -> bool {
         | ("PacketPeerExtension", "get_available_packet_count")
         | ("PacketPeerExtension", "get_max_packet_size")
         | ("StreamPeerExtension", "get_available_bytes")
-        | ("WebRtcDataChannelExtension", "poll")
-        | ("WebRtcDataChannelExtension", "close")
-        | ("WebRtcDataChannelExtension", "set_write_mode")
-        | ("WebRtcDataChannelExtension", "get_write_mode")
-        | ("WebRtcDataChannelExtension", "was_string_packet")
-        | ("WebRtcDataChannelExtension", "get_ready_state")
-        | ("WebRtcDataChannelExtension", "get_label")
-        | ("WebRtcDataChannelExtension", "is_ordered")
-        | ("WebRtcDataChannelExtension", "get_id")
-        | ("WebRtcDataChannelExtension", "get_max_packet_life_time")
-        | ("WebRtcDataChannelExtension", "get_max_retransmits")
-        | ("WebRtcDataChannelExtension", "get_protocol")
-        | ("WebRtcDataChannelExtension", "is_negotiated")
-        | ("WebRtcDataChannelExtension", "get_buffered_amount")
-        | ("WebRtcDataChannelExtension", "get_available_packet_count")
-        | ("WebRtcDataChannelExtension", "get_max_packet_size")
-        | ("WebRtcPeerConnectionExtension", _)
+        | ("WebRTCDataChannelExtension", "poll")
+        | ("WebRTCDataChannelExtension", "close")
+        | ("WebRTCDataChannelExtension", "set_write_mode")
+        | ("WebRTCDataChannelExtension", "get_write_mode")
+        | ("WebRTCDataChannelExtension", "was_string_packet")
+        | ("WebRTCDataChannelExtension", "get_ready_state")
+        | ("WebRTCDataChannelExtension", "get_label")
+        | ("WebRTCDataChannelExtension", "is_ordered")
+        | ("WebRTCDataChannelExtension", "get_id")
+        | ("WebRTCDataChannelExtension", "get_max_packet_life_time")
+        | ("WebRTCDataChannelExtension", "get_max_retransmits")
+        | ("WebRTCDataChannelExtension", "get_protocol")
+        | ("WebRTCDataChannelExtension", "is_negotiated")
+        | ("WebRTCDataChannelExtension", "get_buffered_amount")
+        | ("WebRTCDataChannelExtension", "get_available_packet_count")
+        | ("WebRTCDataChannelExtension", "get_max_packet_size")
+        | ("WebRTCPeerConnectionExtension", _)
         | ("MultiplayerPeerExtension", "get_available_packet_count")
         | ("MultiplayerPeerExtension", "get_max_packet_size")
         | ("MultiplayerPeerExtension", "set_transfer_channel")
@@ -839,7 +841,20 @@ pub fn is_virtual_method_required(class_name: &str, method: &str) -> bool {
         | ("MultiplayerPeerExtension", "get_unique_id")
         | ("MultiplayerPeerExtension", "get_connection_status") => true,
 
-        (_, _) => false,
+        _ => false,
+    }
+}
+
+// Adjustments for Godot 4.4+, where a virtual method is no longer needed (e.g. in a derived class).
+#[rustfmt::skip]
+pub fn get_derived_virtual_method_presence(class_name: &TyName, rust_method_name: &str) -> VirtualMethodPresence {
+     match (class_name.godot_ty.as_str(), rust_method_name) {
+         // Required in base class, no longer in derived; https://github.com/godot-rust/gdext/issues/1133.
+         | ("AudioStreamPlaybackResampled", "mix")
+         => VirtualMethodPresence::Remove,
+
+         // Default: inherit presence from base class.
+         _ => VirtualMethodPresence::Inherit,
     }
 }
 
