Merge pull request #1391 from godot-rust/bugfix/register-doc-ci

Fix non-deterministic `register-docs` XML output

Author: Bromeon

.gitignore

@@ -29,3 +29,6 @@ exp.rs
 
 # Windows specific
 desktop.ini
+
+# Test outputs
+/itest/rust/src/register_tests/res/actual_registered_docs.xml

godot-core/src/docs.rs

@@ -60,7 +60,7 @@ pub struct StructDocs {
     pub description: &'static str,
     pub experimental: &'static str,
     pub deprecated: &'static str,
-    pub members: &'static str,
+    pub properties: &'static str,
 }
 
 /// Keeps documentation for inherent `impl` blocks (primary and secondary), such as:
@@ -82,17 +82,20 @@ pub struct StructDocs {
 /// All fields are XML parts, escaped where necessary.
 #[derive(Default, Clone, Debug)]
 pub struct InherentImplDocs {
-    pub methods: &'static str,
-    pub signals: &'static str,
-    pub constants: &'static str,
+    pub methods_xml: &'static str,
+    pub signals_xml: &'static str,
+    pub constants_xml: &'static str,
 }
 
+/// Godot editor documentation for a class, combined from individual definitions (struct + impls).
+///
+/// All fields are collections of XML parts, escaped where necessary.
 #[derive(Default)]
-struct DocPieces {
+struct AggregatedDocs {
     definition: StructDocs,
-    methods: Vec<&'static str>,
-    signals: Vec<&'static str>,
-    constants: Vec<&'static str>,
+    methods_xmls: Vec<&'static str>,
+    signals_xmls: Vec<&'static str>,
+    constants_xmls: Vec<&'static str>,
 }
 
 /// This function scours the registered plugins to find their documentation pieces,
@@ -110,77 +113,79 @@ struct DocPieces {
 /// strings of not-yet-parented XML tags (or empty string if no method has been documented).
 #[doc(hidden)]
 pub fn gather_xml_docs() -> impl Iterator<Item = String> {
-    let mut map = HashMap::<ClassId, DocPieces>::new();
-    crate::private::iterate_docs_plugins(|x| {
-        let class_name = x.class_name;
-        match &x.item {
-            DocsItem::Struct(s) => {
-                map.entry(class_name).or_default().definition = *s;
+    let mut map = HashMap::<ClassId, AggregatedDocs>::new();
+
+    crate::private::iterate_docs_plugins(|shard| {
+        let class_name = shard.class_name;
+        match &shard.item {
+            DocsItem::Struct(struct_docs) => {
+                map.entry(class_name).or_default().definition = *struct_docs;
             }
+
             DocsItem::InherentImpl(trait_docs) => {
-                let InherentImplDocs {
-                    methods,
-                    constants,
-                    signals,
-                } = trait_docs;
-                map.entry(class_name).or_default().methods.push(methods);
                 map.entry(class_name)
-                    .and_modify(|pieces| pieces.constants.push(constants));
+                    .or_default()
+                    .methods_xmls
+                    .push(trait_docs.methods_xml);
+
                 map.entry(class_name)
-                    .and_modify(|pieces| pieces.signals.push(signals));
+                    .and_modify(|pieces| pieces.constants_xmls.push(trait_docs.constants_xml));
+
+                map.entry(class_name)
+                    .and_modify(|pieces| pieces.signals_xmls.push(trait_docs.signals_xml));
             }
-            DocsItem::ITraitImpl(methods) => {
-                map.entry(class_name).or_default().methods.push(methods);
+
+            DocsItem::ITraitImpl(methods_xml) => {
+                map.entry(class_name)
+                    .or_default()
+                    .methods_xmls
+                    .push(methods_xml);
             }
         }
     });
 
     map.into_iter().map(|(class, pieces)| {
-            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"?>
+        let StructDocs {
+            base,
+            description,
+            experimental,
+            deprecated,
+            properties,
+        } = pieces.definition;
+
+        let methods_block = wrap_in_xml_block("methods", pieces.methods_xmls);
+        let signals_block = wrap_in_xml_block("signals", pieces.signals_xmls);
+        let constants_block = wrap_in_xml_block("constants", pieces.constants_xmls);
+
+        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>
 {methods_block}
 {constants_block}
 {signals_block}
-<members>{members}</members>
+<members>{properties}</members>
 </class>"#)
-        },
-        )
+    })
+}
+
+fn wrap_in_xml_block(tag: &str, mut blocks: Vec<&'static str>) -> String {
+    // We sort the blocks for deterministic output. No need to sort individual methods/signals/constants, this is already done by Godot.
+    // See https://github.com/godot-rust/gdext/pull/1391 for more information.
+    blocks.sort();
+
+    let content = String::from_iter(blocks);
+
+    if content.is_empty() {
+        String::new()
+    } else {
+        format!("<{tag}>{content}</{tag}>")
+    }
 }
 
 /// # Safety

godot-macros/src/docs/extract_docs.rs

@@ -34,27 +34,28 @@ pub fn document_struct(
     description: &[venial::Attribute],
     fields: &[Field],
 ) -> TokenStream {
-    let base_escaped = xml_escape(base);
     let XmlParagraphs {
         description_content,
         deprecated_attr,
         experimental_attr,
     } = attribute_docs_to_xml_paragraphs(description).unwrap_or_default();
 
-    let members = fields
+    let properties = fields
         .iter()
         .filter(|field| field.var.is_some() || field.export.is_some())
         .filter_map(format_member_xml)
         .collect::<String>();
 
+    let base_escaped = xml_escape(base);
+
     quote! {
-            ::godot::docs::StructDocs {
-                base: #base_escaped,
-                description: #description_content,
-                experimental: #experimental_attr,
-                deprecated: #deprecated_attr,
-                members: #members,
-            }
+        ::godot::docs::StructDocs {
+            base: #base_escaped,
+            description: #description_content,
+            experimental: #experimental_attr,
+            deprecated: #deprecated_attr,
+            properties: #properties,
+        }
     }
 }
 

godot-macros/src/docs/mod.rs

@@ -51,11 +51,11 @@ mod docs_generators {
 
         quote! {
             ::godot::sys::plugin_add!(#prv::__GODOT_DOCS_REGISTRY; #prv::DocsPlugin::new::<#class_name>(
-                    #prv::DocsItem::InherentImpl(#prv::InherentImplDocs {
-                        methods: #method_xml_elems,
-                        signals: #signal_xml_elems,
-                        constants: #constant_xml_elems
-                    })
+                #prv::DocsItem::InherentImpl(#prv::InherentImplDocs {
+                    methods_xml: #method_xml_elems,
+                    signals_xml: #signal_xml_elems,
+                    constants_xml: #constant_xml_elems
+                })
             ));
         }
     }
@@ -69,7 +69,7 @@ mod docs_generators {
 
         quote! {
             ::godot::sys::plugin_add!(#prv::__GODOT_DOCS_REGISTRY; #prv::DocsPlugin::new::<#class_name>(
-                    #prv::DocsItem::ITraitImpl(#virtual_methods)
+                #prv::DocsItem::ITraitImpl(#virtual_methods)
             ));
         }
     }

itest/rust/src/framework/mod.rs

@@ -321,6 +321,14 @@ pub fn runs_release() -> bool {
     !Os::singleton().is_debug_build()
 }
 
+/// Whether we are running in GitHub Actions CI.
+///
+/// Must not be used to influence test logic. Only for logging and diagnostics.
+#[allow(dead_code)]
+pub fn runs_github_ci() -> bool {
+    std::env::var("GITHUB_ACTIONS").as_deref() == Ok("true")
+}
+
 /// Create a `GDScript` script from code, compiles and returns it.
 pub fn create_gdscript(code: &str) -> Gd<GDScript> {
     let mut script = GDScript::new_gd();

itest/rust/src/register_tests/register_docs_test.rs

@@ -316,18 +316,49 @@ impl FairlyDocumented {
 impl FairlyDocumented {
     /// Documented method in other godot_api secondary block
     #[func]
-    fn trinary_but_documented(&self, _smth: i64) {}
+    fn tertiary_but_documented(&self, _smth: i64) {}
 }
 
 #[itest]
 fn test_register_docs() {
-    let xml = find_class_docs("FairlyDocumented");
+    let actual_xml = find_class_docs("FairlyDocumented");
 
     // Uncomment if implementation changes and expected output file should be rewritten.
     // std::fs::write("../rust/src/register_tests/res/registered_docs.xml", &xml)
     //     .expect("failed to write docs XML file");
 
-    assert_eq!(include_str!("res/registered_docs.xml"), xml);
+    let expected_xml = include_str!("res/registered_docs.xml");
+
+    if actual_xml == expected_xml {
+        return; // All good.
+    }
+
+    // In GitHub Actions, print output of expected vs. actual.
+    if crate::framework::runs_github_ci() {
+        panic!(
+            "Registered docs XML does not match expected output.\
+            \n============================================================\
+            \nExpected:\n\n{expected_xml}\n\
+            \n------------------------------------------------------------\
+            \nActual:\n\n{actual_xml}\n\
+            \n============================================================\n"
+        );
+    }
+
+    // Locally, write to a file for manual inspection/diffing.
+    let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
+        .join("src")
+        .join("register_tests")
+        .join("res")
+        .join("actual_registered_docs.xml");
+
+    std::fs::write(&path, &actual_xml).expect("write `actual_registered_docs.xml` failed");
+
+    panic!(
+        "Registered docs XML does not match expected output.\n\
+        Actual output has been written to following file:\n  {path}\n",
+        path = path.display()
+    );
 }
 
 fn find_class_docs(class_name: &str) -> String {

itest/rust/src/register_tests/res/registered_docs.xml

@@ -25,6 +25,22 @@ public class Player : Node2D
   </description>
 </method>
 
+<method name="secondary_but_documented">
+  <return type="()" />
+  <param index="0" name="smth" type="i64" />
+  <description>
+  Documented method in godot_api secondary block
+  </description>
+</method>
+
+<method name="tertiary_but_documented">
+  <return type="()" />
+  <param index="0" name="smth" type="i64" />
+  <description>
+  Documented method in other godot_api secondary block
+  </description>
+</method>
+
 <method name="ye">
   <return type="f32" />
 
@@ -72,22 +88,6 @@ public class Player : Node2D
   ????? probably
   </description>
 </method>
-
-<method name="secondary_but_documented">
-  <return type="()" />
-  <param index="0" name="smth" type="i64" />
-  <description>
-  Documented method in godot_api secondary block
-  </description>
-</method>
-
-<method name="trinary_but_documented">
-  <return type="()" />
-  <param index="0" name="smth" type="i64" />
-  <description>
-  Documented method in other godot_api secondary block
-  </description>
-</method>
 </methods>
 <constants><constant name="RANDOM" value="4">Documentation.</constant><constant name="A" value="128" deprecated="Did you know that constants can be deprecated?">Hmmmm</constant><constant name="B" value="128" experimental="Did you know that constants can be experimental?">Who would know that!</constant><constant name="XML" value="1">this docstring has &lt; a special character</constant></constants>
 <signals>