Doc bugfixes and improvements

- Bugfix: Handle docs in `#[godot_api(secondary)]`
- Code tweaks: Separate docs modules to not pollute rest of the code with `#[cfg(all(feature = "register-docs", since_api = "4.3"))]`. Simplify registration logic.
- Create separate PluginRegistry for Docs and Docs only.

Author: Yarwin

godot-core/src/docs.rs

@@ -8,7 +8,36 @@
 use std::collections::HashMap;
 
 use crate::meta::ClassId;
-use crate::registry::plugin::{ITraitImpl, InherentImpl, PluginItem, Struct};
+use crate::obj::GodotClass;
+
+/// Piece of information that is gathered by the self-registration ("plugin") system.
+///
+/// You should not manually construct this struct, but rather use [`DocsPlugin::new()`].
+#[derive(Debug)]
+pub struct DocsPlugin {
+    /// The name of the class to register docs for.
+    pub(crate) class_name: ClassId,
+
+    /// The actual item being registered.
+    pub item: DocsItem,
+}
+
+impl DocsPlugin {
+    /// Creates a new `DocsPlugin`, automatically setting the `class_name` to the values defined in [`GodotClass`].
+    pub fn new<T: GodotClass>(item: DocsItem) -> Self {
+        Self {
+            class_name: T::class_id(),
+            item,
+        }
+    }
+}
+
+#[derive(Debug)]
+pub enum DocsItem {
+    Struct(StructDocs),
+    InherentImpl(InherentImplDocs),
+    VirtualMethods(&'static str),
+}
 
 /// Created for documentation on
 /// ```ignore
@@ -29,7 +58,7 @@ pub struct StructDocs {
     pub members: &'static str,
 }
 
-/// Keeps documentation for inherent `impl` blocks, such as:
+/// Keeps documentation for inherent `impl` blocks (primary and secondary), such as:
 /// ```ignore
 /// #[godot_api]
 /// impl Struct {
@@ -46,18 +75,19 @@ pub struct StructDocs {
 /// }
 /// ```
 /// All fields are XML parts, escaped where necessary.
-#[derive(Default, Copy, Clone, Debug)]
+#[derive(Default, Clone, Debug)]
 pub struct InherentImplDocs {
-    pub methods: Vec<&'static str>,
-    pub signals: Vec<&'static str>,
-    pub constants: Vec<&'static str>,
+    pub methods: &'static str,
+    pub signals: &'static str,
+    pub constants: &'static str,
 }
 
 #[derive(Default)]
 struct DocPieces {
     definition: StructDocs,
-    inherent: InherentImplDocs,
-    virtual_methods: Vec<&'static str>,
+    methods: Vec<&'static str>,
+    signals: Vec<&'static str>,
+    constants: Vec<&'static str>,
 }
 
 /// This function scours the registered plugins to find their documentation pieces,
@@ -76,68 +106,66 @@ struct DocPieces {
 #[doc(hidden)]
 pub fn gather_xml_docs() -> impl Iterator<Item = String> {
     let mut map = HashMap::<ClassId, DocPieces>::new();
-    crate::private::iterate_plugins(|x| {
+    crate::private::iterate_docs_plugins(|x| {
         let class_name = x.class_name;
-
         match &x.item {
-            PluginItem::InherentImpl(InherentImpl { docs, .. }) => {
-                map.entry(class_name).or_default().inherent = docs.clone();
+            DocsItem::Struct(s) => {
+                map.entry(class_name).or_default().definition = *s;
             }
-            PluginItem::ITraitImpl(ITraitImpl {
-                virtual_method_docs,
-                ..
-            }) => map
-                .entry(class_name)
-                .or_default()
-                .virtual_methods
-                .push(virtual_method_docs),
-
-            PluginItem::Struct(Struct { docs, .. }) => {
-                map.entry(class_name).or_default().definition = *docs;
+            DocsItem::InherentImpl(i) => {
+                let InherentImplDocs {
+                    methods,
+                    constants,
+                    signals,
+                } = i;
+                map.entry(class_name).or_default().methods.push(methods);
+                map.entry(class_name)
+                    .and_modify(|pieces| pieces.constants.push(constants));
+                map.entry(class_name)
+                    .and_modify(|pieces| pieces.signals.push(signals));
+            }
+            DocsItem::VirtualMethods(methods) => {
+                map.entry(class_name).or_default().methods.push(methods);
             }
-
-            _ => (),
         }
     });
 
     map.into_iter().map(|(class, pieces)| {
-        let StructDocs {
-            base,
-            description,
-            experimental,
-            deprecated,
-            members,
-        } = pieces.definition;
-
-        let virtual_methods = pieces.virtual_methods;
-
-        let mut method_docs = String::from_iter(pieces.inherent.methods);
-        let signal_docs = String::from_iter(pieces.inherent.signals);
-        let constant_docs = String::from_iter(pieces.inherent.constants);
-
-        method_docs.extend(virtual_methods);
-        let methods_block = if method_docs.is_empty() {
-            String::new()
-        } else {
-            format!("<methods>{method_docs}</methods>")
-        };
-        let signals_block = if signal_docs.is_empty() {
-            String::new()
-        } else {
-            format!("<signals>{signal_docs}</signals>")
-        };
-        let constants_block = if constant_docs.is_empty() {
-            String::new()
-        } else {
-            format!("<constants>{constant_docs}</constants>")
-        };
-        let (brief, description) = match description
-            .split_once("[br]") {
-            Some((brief, description)) => (brief, description.trim_start_matches("[br]")),
-            None => (description, ""),
-        };
-
-        format!(r#"<?xml version="1.0" encoding="UTF-8"?>
+            let StructDocs {
+                base,
+                description,
+                experimental,
+                deprecated,
+                members,
+            } = pieces.definition;
+
+
+            let method_docs = String::from_iter(pieces.methods);
+            let signal_docs = String::from_iter(pieces.signals);
+            let constant_docs = String::from_iter(pieces.constants);
+
+            let methods_block = if method_docs.is_empty() {
+                String::new()
+            } else {
+                format!("<methods>{method_docs}</methods>")
+            };
+            let signals_block = if signal_docs.is_empty() {
+                String::new()
+            } else {
+                format!("<signals>{signal_docs}</signals>")
+            };
+            let constants_block = if constant_docs.is_empty() {
+                String::new()
+            } else {
+                format!("<constants>{constant_docs}</constants>")
+            };
+            let (brief, description) = match description
+                .split_once("[br]") {
+                Some((brief, description)) => (brief, description.trim_start_matches("[br]")),
+                None => (description, ""),
+            };
+
+            format!(r#"<?xml version="1.0" encoding="UTF-8"?>
 <class name="{class}" inherits="{base}"{deprecated}{experimental} xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd">
 <brief_description>{brief}</brief_description>
 <description>{description}</description>
@@ -146,8 +174,8 @@ pub fn gather_xml_docs() -> impl Iterator<Item = String> {
 {signals_block}
 <members>{members}</members>
 </class>"#)
-    },
-    )
+        },
+        )
 }
 
 /// # Safety

godot-core/src/private.rs

@@ -22,6 +22,8 @@ use crate::{classes, sys};
 // Public re-exports
 
 mod reexport_pub {
+    #[cfg(all(since_api = "4.3", feature = "register-docs"))]
+    pub use crate::docs::{DocsItem, DocsPlugin, InherentImplDocs, StructDocs};
     pub use crate::gen::classes::class_macros;
     #[cfg(feature = "trace")]
     pub use crate::meta::trace;
@@ -52,6 +54,8 @@ static CALL_ERRORS: Global<CallErrors> = Global::default();
 static ERROR_PRINT_LEVEL: atomic::AtomicU8 = atomic::AtomicU8::new(2);
 
 sys::plugin_registry!(pub __GODOT_PLUGIN_REGISTRY: ClassPlugin);
+#[cfg(all(since_api = "4.3", feature = "register-docs"))]
+sys::plugin_registry!(pub __GODOT_DOCS_REGISTRY: DocsPlugin);
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Call error handling
@@ -142,26 +146,15 @@ pub fn next_class_id() -> u16 {
     NEXT_CLASS_ID.fetch_add(1, atomic::Ordering::Relaxed)
 }
 
-// Don't touch unless you know what you're doing.
-#[doc(hidden)]
-pub fn edit_inherent_impl(class_name: crate::meta::ClassName, f: impl FnOnce(&mut InherentImpl)) {
-    let mut plugins = __GODOT_PLUGIN_REGISTRY.lock().unwrap();
-
-    for elem in plugins.iter_mut().filter(|p| p.class_name == class_name) {
-        match &mut elem.item {
-            PluginItem::InherentImpl(inherent_impl) => {
-                f(inherent_impl);
-                return;
-            }
-            PluginItem::Struct(_) | PluginItem::ITraitImpl(_) | PluginItem::DynTraitImpl(_) => {}
-        }
-    }
-}
-
 pub(crate) fn iterate_plugins(mut visitor: impl FnMut(&ClassPlugin)) {
     sys::plugin_foreach!(__GODOT_PLUGIN_REGISTRY; visitor);
 }
 
+#[cfg(all(since_api = "4.3", feature = "register-docs"))]
+pub(crate) fn iterate_docs_plugins(mut visitor: impl FnMut(&DocsPlugin)) {
+    sys::plugin_foreach!(__GODOT_DOCS_REGISTRY; visitor);
+}
+
 #[cfg(feature = "codegen-full")] // Remove if used in other scenarios.
 pub(crate) fn find_inherent_impl(class_name: crate::meta::ClassId) -> Option<InherentImpl> {
     // We do this manually instead of using `iterate_plugins()` because we want to break as soon as we find a match.

godot-core/src/registry/class.rs

@@ -115,10 +115,7 @@ impl ClassRegistrationInfo {
         // Note: when changing this match, make sure the array has sufficient size.
         let index = match item {
             PluginItem::Struct { .. } => 0,
-            PluginItem::InherentImpl(_) => {
-                // Inherent impls don't need to be unique.
-                return;
-            }
+            PluginItem::InherentImpl(_) => 1,
             PluginItem::ITraitImpl { .. } => 2,
 
             // Multiple dyn traits can be registered, thus don't validate for uniqueness.
@@ -427,8 +424,6 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) {
             is_editor_plugin,
             is_internal,
             is_instantiable,
-            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-                docs: _,
             reference_fn,
             unreference_fn,
         }) => {
@@ -476,8 +471,6 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) {
         PluginItem::InherentImpl(InherentImpl {
             register_methods_constants_fn,
             register_rpcs_fn: _,
-            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-                docs: _,
         }) => {
             c.register_methods_constants_fn = Some(register_methods_constants_fn);
         }
@@ -495,8 +488,6 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) {
             user_free_property_list_fn,
             user_property_can_revert_fn,
             user_property_get_revert_fn,
-            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-                virtual_method_docs: _,
             validate_property_fn,
         }) => {
             c.user_register_fn = user_register_fn;

godot-core/src/registry/plugin.rs

@@ -8,8 +8,6 @@
 use std::any::Any;
 use std::{any, fmt};
 
-#[cfg(all(since_api = "4.3", feature = "register-docs"))]
-use crate::docs::*;
 use crate::init::InitLevel;
 use crate::meta::ClassId;
 use crate::obj::{bounds, cap, Bounds, DynGd, Gd, GodotClass, Inherits, UserClass};
@@ -39,10 +37,10 @@ pub struct ClassPlugin {
     /// Incorrectly setting this value should not cause any UB but will likely cause errors during registration time.
     // Init-level is per ClassPlugin and not per PluginItem, because all components of all classes are mixed together in one
     // huge linker list. There is no per-class aggregation going on, so this allows to easily filter relevant classes.
-    pub init_level: InitLevel,
+    pub(crate) init_level: InitLevel,
 
     /// The actual item being registered.
-    pub item: PluginItem,
+    pub(crate) item: PluginItem,
 }
 
 impl ClassPlugin {
@@ -197,16 +195,10 @@ pub struct Struct {
 
     /// Whether the class has a default constructor.
     pub(crate) is_instantiable: bool,
-
-    /// Documentation extracted from the struct's RustDoc.
-    #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-    pub(crate) docs: StructDocs,
 }
 
 impl Struct {
-    pub fn new<T: GodotClass + cap::ImplementsGodotExports>(
-        #[cfg(all(since_api = "4.3", feature = "register-docs"))] docs: StructDocs,
-    ) -> Self {
+    pub fn new<T: GodotClass + cap::ImplementsGodotExports>() -> Self {
         let refcounted = <T::Memory as bounds::Memory>::IS_REF_COUNTED;
 
         Self {
@@ -222,8 +214,6 @@ impl Struct {
             is_editor_plugin: false,
             is_internal: false,
             is_instantiable: false,
-            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-            docs,
             // While Godot doesn't do anything with these callbacks for non-RefCounted classes, we can avoid instantiating them in Rust.
             reference_fn: refcounted.then_some(callbacks::reference::<T>),
             unreference_fn: refcounted.then_some(callbacks::unreference::<T>),
@@ -294,9 +284,6 @@ pub struct InherentImpl {
     // This field is only used during codegen-full.
     #[cfg_attr(not(feature = "codegen-full"), expect(dead_code))]
     pub(crate) register_rpcs_fn: Option<ErasedRegisterRpcsFn>,
-
-    #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-    pub docs: InherentImplDocs,
 }
 
 impl InherentImpl {
@@ -308,18 +295,12 @@ impl InherentImpl {
             register_rpcs_fn: Some(ErasedRegisterRpcsFn {
                 raw: callbacks::register_user_rpcs::<T>,
             }),
-            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-            docs: Default::default(),
         }
     }
 }
 
 #[derive(Default, Clone, Debug)]
 pub struct ITraitImpl {
-    #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-    /// Virtual method documentation.
-    pub(crate) virtual_method_docs: &'static str,
-
     /// Callback to user-defined `register_class` function.
     pub(crate) user_register_fn: Option<ErasedRegisterFn>,
 
@@ -434,12 +415,8 @@ pub struct ITraitImpl {
 }
 
 impl ITraitImpl {
-    pub fn new<T: GodotClass + cap::ImplementsGodotVirtual>(
-        #[cfg(all(since_api = "4.3", feature = "register-docs"))] virtual_method_docs: &'static str,
-    ) -> Self {
+    pub fn new<T: GodotClass + cap::ImplementsGodotVirtual>() -> Self {
         Self {
-            #[cfg(all(since_api = "4.3", feature = "register-docs"))]
-            virtual_method_docs,
             get_virtual_fn: Some(callbacks::get_virtual::<T>),
             ..Default::default()
         }