xso-proc: fix order-dependent compilation bug

Jonas Schäfer created

The test case which is added fails to compile unless one puts the
`parent` field before the `id` field. The cause is explained somewhat by
the change, but I'll spell it out here nontheless.

Previously, the loop in `Compound::make_as_item_iter_statemachine`
assumed that the serialisation order of fields would match their
declaration order. That is not generally true: attributes must be
serialised before element content, because they must be emitted before
the element header is closed.

This change thus splits the generated states into "header" states (for
everything before the end of the element header (think `>`)) and
"body" states (for everything after and including the end of the
element header). After all fields have been processed, we can then
add the data fields of the body fields to the header states so that
they are carried through the generated state machine until they are
needed in the body.

Change summary

parsers/src/util/macro_tests.rs | 36 ++++++++++++++
xso-proc/src/compound.rs        | 89 +++++++++++++++++++++-------------
xso/ChangeLog                   |  3 +
3 files changed, 94 insertions(+), 34 deletions(-)

Detailed changes

parsers/src/util/macro_tests.rs 🔗

@@ -2436,3 +2436,39 @@ fn enum_deserialize_callback_can_fail() {
         other => panic!("unexpected result: {:?}", other),
     }
 }
+
+/// This struct failed to compile at some point, failing to find the
+/// (internally generated) identifier `fid`.
+#[derive(AsXml, FromXml, PartialEq, Debug, Clone)]
+#[xml(namespace = NS1, name = "thread")]
+struct TextVsAttributeOrderingCompileBug {
+    #[xml(text)]
+    id: String,
+
+    #[xml(attribute(default))]
+    parent: ::core::option::Option<String>,
+}
+
+#[test]
+fn text_vs_attribute_ordering_compile_bug_roundtrip() {
+    #[allow(unused_imports)]
+    use core::{
+        option::Option::{None, Some},
+        result::Result::{Err, Ok},
+    };
+    roundtrip_full::<TextVsAttributeOrderingCompileBug>(
+        "<thread xmlns='urn:example:ns1'>foo</thread>",
+    );
+}
+
+#[test]
+fn text_vs_attribute_ordering_compile_bug_roundtrip_with_parent() {
+    #[allow(unused_imports)]
+    use core::{
+        option::Option::{None, Some},
+        result::Result::{Err, Ok},
+    };
+    roundtrip_full::<TextVsAttributeOrderingCompileBug>(
+        "<thread xmlns='urn:example:ns1' parent='bar'>foo</thread>",
+    );
+}

xso-proc/src/compound.rs 🔗

@@ -556,14 +556,15 @@ impl Compound {
         let name_ident = quote::format_ident!("name");
         let ns_ident = quote::format_ident!("ns");
         let dummy_ident = quote::format_ident!("dummy");
-        let mut states = Vec::new();
+        let mut header_states = Vec::new();
+        let mut body_states = Vec::new();
 
         let is_tuple = !input_name.is_path();
         let mut destructure = TokenStream::default();
         let mut start_init = TokenStream::default();
         let mut extra_defs = TokenStream::default();
 
-        states.push(
+        header_states.push(
             State::new(element_head_start_state_ident.clone())
                 .with_field(&dummy_ident, &phantom_lifetime_ty(lifetime.clone()))
                 .with_field(&ns_ident, &namespace_ty(Span::call_site()))
@@ -573,12 +574,12 @@ impl Compound {
                 ),
         );
 
-        let mut element_head_end_idx = states.len();
-        states.push(
+        body_states.push((
+            None,
             State::new(element_head_end_state_ident.clone()).with_impl(quote! {
                 ::core::option::Option::Some(::xso::Item::ElementHeadEnd)
             }),
-        );
+        ));
 
         for (i, field) in self.fields.iter().enumerate() {
             let member = field.member();
@@ -589,20 +590,23 @@ impl Compound {
 
             match part {
                 FieldIteratorPart::Header { generator } => {
-                    // we have to make sure that we carry our data around in
+                    // We have to make sure that we carry our data around in
                     // all the previous states.
-                    for state in &mut states[..element_head_end_idx] {
+                    // For header states, it is sufficient to do it here.
+                    // For body states, we have to do it in a separate loop
+                    // below to correctly handle the case when a field with a
+                    // body state is placed before a field with a header
+                    // state.
+                    for state in header_states.iter_mut() {
                         state.add_field(&bound_name, &ty);
                     }
-                    states.insert(
-                        element_head_end_idx,
+                    header_states.push(
                         State::new(state_name)
                             .with_field(&bound_name, &ty)
                             .with_impl(quote! {
                                 #generator
                             }),
                     );
-                    element_head_end_idx += 1;
 
                     if is_tuple {
                         destructure.extend(quote! {
@@ -619,20 +623,22 @@ impl Compound {
                 }
 
                 FieldIteratorPart::Text { generator } => {
-                    // we have to make sure that we carry our data around in
-                    // all the previous states.
-                    for state in states.iter_mut() {
+                    // We have to make sure that we carry our data around in
+                    // all the previous body states.
+                    // We also have to make sure that our data is carried
+                    // by all *header* states, but we can only do that once
+                    // we have visited them all, so that happens at the bottom
+                    // of the loop.
+                    for (_, state) in body_states.iter_mut() {
                         state.add_field(&bound_name, &ty);
                     }
-                    states.push(
-                        State::new(state_name)
-                            .with_field(&bound_name, &ty)
-                            .with_impl(quote! {
-                                #generator.map(|value| ::xso::Item::Text(
-                                    value,
-                                ))
-                            }),
-                    );
+                    let state = State::new(state_name)
+                        .with_field(&bound_name, &ty)
+                        .with_impl(quote! {
+                            #generator.map(|value| ::xso::Item::Text(
+                                value,
+                            ))
+                        });
                     if is_tuple {
                         destructure.extend(quote! {
                             #bound_name,
@@ -645,6 +651,7 @@ impl Compound {
                     start_init.extend(quote! {
                         #bound_name,
                     });
+                    body_states.push((Some((bound_name, ty)), state));
                 }
 
                 FieldIteratorPart::Content {
@@ -652,20 +659,22 @@ impl Compound {
                     value: FieldTempInit { ty, init },
                     generator,
                 } => {
-                    // we have to make sure that we carry our data around in
-                    // all the previous states.
-                    for state in states.iter_mut() {
+                    // We have to make sure that we carry our data around in
+                    // all the previous body states.
+                    // We also have to make sure that our data is carried
+                    // by all *header* states, but we can only do that once
+                    // we have visited them all, so that happens at the bottom
+                    // of the loop.
+                    for (_, state) in body_states.iter_mut() {
                         state.add_field(&bound_name, &ty);
                     }
 
-                    states.push(
-                        State::new(state_name.clone())
-                            .with_field(&bound_name, &ty)
-                            .with_mut(&bound_name)
-                            .with_impl(quote! {
-                                #generator?
-                            }),
-                    );
+                    let state = State::new(state_name.clone())
+                        .with_field(&bound_name, &ty)
+                        .with_mut(&bound_name)
+                        .with_impl(quote! {
+                            #generator?
+                        });
                     if is_tuple {
                         destructure.extend(quote! {
                             #bound_name,
@@ -680,11 +689,12 @@ impl Compound {
                     });
 
                     extra_defs.extend(field_extra_defs);
+                    body_states.push((Some((bound_name, ty)), state));
                 }
             }
         }
 
-        states[0].set_impl(quote! {
+        header_states[0].set_impl(quote! {
             {
                 ::core::option::Option::Some(::xso::Item::ElementHeadStart(
                     #ns_ident,
@@ -693,6 +703,17 @@ impl Compound {
             }
         });
 
+        for (data, _) in body_states.iter() {
+            if let Some((bound_name, ty)) = data.as_ref() {
+                for state in header_states.iter_mut() {
+                    state.add_field(bound_name, ty);
+                }
+            }
+        }
+
+        header_states.extend(body_states.into_iter().map(|(_, state)| state));
+        let mut states = header_states;
+
         states.push(
             State::new(element_foot_state_ident.clone()).with_impl(quote! {
                 ::core::option::Option::Some(::xso::Item::ElementFoot)

xso/ChangeLog 🔗

@@ -46,6 +46,9 @@ Version NEXT:
         doc(hidden), to not clutter hand-written documentation with auto
         generated garbage (something certain big tech companies could take
         an example of, honestly).
+      - Fixed bug where putting an attribute field below any non-attribute
+        field in a struct definition would cause a compile-time error when
+        deriving `AsXml`.
 
 Version 0.1.2:
 2024-07-26 Jonas Schäfer <jonas@zombofant.net>