From e706fbd29286aaadb989911957c3a9fcdcf8e2a6 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Thu, 6 Mar 2025 16:21:28 +0100 Subject: [PATCH 1/6] ser: introduce some abstraction for serialization --- instant-xml-macros/src/ser.rs | 257 +++++++++++++++++----------------- 1 file changed, 128 insertions(+), 129 deletions(-) diff --git a/instant-xml-macros/src/ser.rs b/instant-xml-macros/src/ser.rs index e92be6d..1f672fd 100644 --- a/instant-xml-macros/src/ser.rs +++ b/instant-xml-macros/src/ser.rs @@ -1,7 +1,7 @@ use std::collections::BTreeSet; use proc_macro2::TokenStream; -use quote::quote; +use quote::{quote, ToTokens}; use syn::spanned::Spanned; use super::{discard_lifetimes, meta_items, ContainerMeta, FieldMeta, Mode, VariantMeta}; @@ -169,31 +169,29 @@ fn serialize_struct( meta: ContainerMeta, ) -> proc_macro2::TokenStream { let tag = meta.tag(); - let mut body = TokenStream::new(); - let mut attributes = TokenStream::new(); - let mut borrowed = BTreeSet::new(); + let mut out = StructOutput::default(); match &data.fields { syn::Fields::Named(fields) => { - body.extend(quote!(serializer.end_start()?;)); + out.body.extend(quote!(serializer.end_start()?;)); for field in &fields.named { - if let Err(err) = - named_field(field, &mut body, &mut attributes, &mut borrowed, &meta) - { + if let Err(err) = out.named_field(field, &meta) { return err.to_compile_error(); } } - body.extend(quote!(serializer.write_close(prefix, #tag)?;)); + out.body + .extend(quote!(serializer.write_close(prefix, #tag)?;)); } syn::Fields::Unnamed(fields) => { - body.extend(quote!(serializer.end_start()?;)); + out.body.extend(quote!(serializer.end_start()?;)); for (index, field) in fields.unnamed.iter().enumerate() { - if let Err(err) = unnamed_field(field, index, &mut body, &mut borrowed) { + if let Err(err) = out.unnamed_field(field, index) { return err.to_compile_error(); } } - body.extend(quote!(serializer.write_close(prefix, #tag)?;)); + out.body + .extend(quote!(serializer.write_close(prefix, #tag)?;)); } - syn::Fields::Unit => body.extend(quote!(serializer.end_empty()?;)), + syn::Fields::Unit => out.body.extend(quote!(serializer.end_empty()?;)), } let default_namespace = meta.default_namespace(); @@ -234,8 +232,7 @@ fn serialize_struct( let old = serializer.push(new)?; // Finalize start element - #attributes - #body + #out serializer.pop(old); Ok(()) @@ -266,19 +263,15 @@ fn serialize_inline_struct( .to_compile_error(); } - let mut body = TokenStream::new(); - let mut attributes = TokenStream::new(); - let mut borrowed = BTreeSet::new(); + let mut out = StructOutput::default(); match &data.fields { syn::Fields::Named(fields) => { for field in &fields.named { - if let Err(err) = - named_field(field, &mut body, &mut attributes, &mut borrowed, &meta) - { + if let Err(err) = out.named_field(field, &meta) { return err.to_compile_error(); } - if !attributes.is_empty() { + if !out.attributes.is_empty() { return syn::Error::new( input.span(), "no attributes allowed on inline structs", @@ -289,12 +282,12 @@ fn serialize_inline_struct( } syn::Fields::Unnamed(fields) => { for (index, field) in fields.unnamed.iter().enumerate() { - if let Err(err) = unnamed_field(field, index, &mut body, &mut borrowed) { + if let Err(err) = out.unnamed_field(field, index) { return err.to_compile_error(); } } } - syn::Fields::Unit => body.extend(quote!(serializer.end_empty()?;)), + syn::Fields::Unit => out.body.extend(quote!(serializer.end_empty()?;)), } let mut generics = input.generics.clone(); @@ -313,57 +306,68 @@ fn serialize_inline_struct( field: Option<::instant_xml::Id<'_>>, serializer: &mut instant_xml::Serializer, ) -> ::std::result::Result<(), instant_xml::Error> { - #body + #out Ok(()) } }; ) } -fn named_field( - field: &syn::Field, - body: &mut TokenStream, - attributes: &mut TokenStream, - borrowed: &mut BTreeSet, - meta: &ContainerMeta, -) -> Result<(), syn::Error> { - let field_name = field.ident.as_ref().unwrap(); - let field_meta = match FieldMeta::from_field(field, meta) { - Ok(meta) => meta, - Err(err) => { - body.extend(err.into_compile_error()); - return Ok(()); - } - }; +#[derive(Default)] +struct StructOutput { + body: TokenStream, + attributes: TokenStream, + borrowed: BTreeSet, +} - let tag = field_meta.tag; - let default_ns = match &meta.ns.uri { - Some(ns) => quote!(#ns), - None => quote!(""), - }; +impl StructOutput { + fn named_field(&mut self, field: &syn::Field, meta: &ContainerMeta) -> Result<(), syn::Error> { + let field_name = field.ident.as_ref().unwrap(); + let field_meta = match FieldMeta::from_field(field, meta) { + Ok(meta) => meta, + Err(err) => { + self.body.extend(err.into_compile_error()); + return Ok(()); + } + }; - if field_meta.attribute { - if field_meta.direct { - return Err(syn::Error::new( - field.span(), - "direct attribute is not supported on attributes", - )); - } + let tag = field_meta.tag; + let default_ns = match &meta.ns.uri { + Some(ns) => quote!(#ns), + None => quote!(""), + }; - let (ns, error) = match &field_meta.ns.uri { - Some(Namespace::Path(path)) => match path.get_ident() { - Some(prefix) => match &meta.ns.prefixes.get(&prefix.to_string()) { - Some(ns) => (quote!(#ns), quote!()), + if field_meta.attribute { + if field_meta.direct { + return Err(syn::Error::new( + field.span(), + "direct attribute is not supported on attributes", + )); + } + + let (ns, error) = match &field_meta.ns.uri { + Some(Namespace::Path(path)) => match path.get_ident() { + Some(prefix) => match &meta.ns.prefixes.get(&prefix.to_string()) { + Some(ns) => (quote!(#ns), quote!()), + None => ( + quote!(""), + syn::Error::new( + field_meta.ns.uri.span(), + format!("unknown prefix `{prefix}` (prefix must be defined on the field's type)"), + ) + .into_compile_error(), + ), + }, None => ( quote!(""), syn::Error::new( field_meta.ns.uri.span(), - format!("unknown prefix `{prefix}` (prefix must be defined on the field's type)"), + "attribute namespace must be a prefix identifier", ) .into_compile_error(), ), }, - None => ( + Some(Namespace::Literal(_)) => ( quote!(""), syn::Error::new( field_meta.ns.uri.span(), @@ -371,90 +375,85 @@ fn named_field( ) .into_compile_error(), ), - }, - Some(Namespace::Literal(_)) => ( - quote!(""), - syn::Error::new( - field_meta.ns.uri.span(), - "attribute namespace must be a prefix identifier", - ) - .into_compile_error(), - ), - None => (default_ns, quote!()), + None => (default_ns, quote!()), + }; + + self.attributes.extend(quote!( + #error + if self.#field_name.present() { + serializer.write_attr(#tag, #ns, &self.#field_name)?; + } + )); + return Ok(()); + } + + let ns = match field_meta.ns.uri { + Some(ref ns) => quote!(#ns), + None => default_ns, }; - attributes.extend(quote!( - #error - if self.#field_name.present() { - serializer.write_attr(#tag, #ns, &self.#field_name)?; + let mut no_lifetime_type = field.ty.clone(); + discard_lifetimes(&mut no_lifetime_type, &mut self.borrowed, false, true); + if let Some(with) = field_meta.serialize_with { + if field_meta.direct { + return Err(syn::Error::new( + field.span(), + "direct serialization is not supported with `serialize_with`", + )); } - )); - return Ok(()); - } - let ns = match field_meta.ns.uri { - Some(ref ns) => quote!(#ns), - None => default_ns, - }; + let path = with.to_string(); + let path = syn::parse_str::(path.trim_matches('"')).map_err(|err| { + syn::Error::new( + with.span(), + format!("failed to parse serialize_with as path: {err}"), + ) + })?; - let mut no_lifetime_type = field.ty.clone(); - discard_lifetimes(&mut no_lifetime_type, borrowed, false, true); - if let Some(with) = field_meta.serialize_with { - if field_meta.direct { + self.body + .extend(quote!(#path(&self.#field_name, serializer)?;)); + return Ok(()); + } else if field_meta.direct { + self.body.extend(quote!( + <#no_lifetime_type as ToXml>::serialize( + &self.#field_name, None, serializer + )?; + )); + } else { + self.body.extend(quote!( + <#no_lifetime_type as ToXml>::serialize( + &self.#field_name, + Some(::instant_xml::Id { ns: #ns, name: #tag }), + serializer, + )?; + )); + } + + Ok(()) + } + + fn unnamed_field(&mut self, field: &syn::Field, index: usize) -> Result<(), syn::Error> { + if !field.attrs.is_empty() { return Err(syn::Error::new( field.span(), - "direct serialization is not supported with `serialize_with`", + "unnamed fields cannot have attributes", )); } - let path = with.to_string(); - let path = syn::parse_str::(path.trim_matches('"')).map_err(|err| { - syn::Error::new( - with.span(), - format!("failed to parse serialize_with as path: {err}"), - ) - })?; - - body.extend(quote!(#path(&self.#field_name, serializer)?;)); - return Ok(()); - } else if field_meta.direct { - body.extend(quote!( - <#no_lifetime_type as ToXml>::serialize( - &self.#field_name, None, serializer - )?; - )); - } else { - body.extend(quote!( - <#no_lifetime_type as ToXml>::serialize( - &self.#field_name, - Some(::instant_xml::Id { ns: #ns, name: #tag }), - serializer, - )?; + let mut no_lifetime_type = field.ty.clone(); + discard_lifetimes(&mut no_lifetime_type, &mut self.borrowed, false, true); + let index = syn::Index::from(index); + self.body.extend(quote!( + self.#index.serialize(None, serializer)?; )); - } - Ok(()) + Ok(()) + } } -fn unnamed_field( - field: &syn::Field, - index: usize, - body: &mut TokenStream, - borrowed: &mut BTreeSet, -) -> Result<(), syn::Error> { - if !field.attrs.is_empty() { - return Err(syn::Error::new( - field.span(), - "unnamed fields cannot have attributes", - )); +impl ToTokens for StructOutput { + fn to_tokens(&self, tokens: &mut TokenStream) { + self.attributes.to_tokens(tokens); + self.body.to_tokens(tokens); } - - let mut no_lifetime_type = field.ty.clone(); - discard_lifetimes(&mut no_lifetime_type, borrowed, false, true); - let index = syn::Index::from(index); - body.extend(quote!( - self.#index.serialize(None, serializer)?; - )); - - Ok(()) } From 31f7ce9c0f9b952e47631328653fc287bc9da251 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Thu, 6 Mar 2025 16:36:12 +0100 Subject: [PATCH 2/6] ser: move more field handling into StructOutput --- instant-xml-macros/src/ser.rs | 97 ++++++++++++++++++++++++----------- 1 file changed, 66 insertions(+), 31 deletions(-) diff --git a/instant-xml-macros/src/ser.rs b/instant-xml-macros/src/ser.rs index 1f672fd..59e0821 100644 --- a/instant-xml-macros/src/ser.rs +++ b/instant-xml-macros/src/ser.rs @@ -168,28 +168,17 @@ fn serialize_struct( data: &syn::DataStruct, meta: ContainerMeta, ) -> proc_macro2::TokenStream { - let tag = meta.tag(); let mut out = StructOutput::default(); match &data.fields { syn::Fields::Named(fields) => { - out.body.extend(quote!(serializer.end_start()?;)); - for field in &fields.named { - if let Err(err) = out.named_field(field, &meta) { - return err.to_compile_error(); - } + if let Err(err) = out.named_fields(fields, false, &meta) { + return err; } - out.body - .extend(quote!(serializer.write_close(prefix, #tag)?;)); } syn::Fields::Unnamed(fields) => { - out.body.extend(quote!(serializer.end_start()?;)); - for (index, field) in fields.unnamed.iter().enumerate() { - if let Err(err) = out.unnamed_field(field, index) { - return err.to_compile_error(); - } + if let Err(err) = out.unnamed_fields(fields, false, &meta) { + return err; } - out.body - .extend(quote!(serializer.write_close(prefix, #tag)?;)); } syn::Fields::Unit => out.body.extend(quote!(serializer.end_empty()?;)), } @@ -266,25 +255,13 @@ fn serialize_inline_struct( let mut out = StructOutput::default(); match &data.fields { syn::Fields::Named(fields) => { - for field in &fields.named { - if let Err(err) = out.named_field(field, &meta) { - return err.to_compile_error(); - } - - if !out.attributes.is_empty() { - return syn::Error::new( - input.span(), - "no attributes allowed on inline structs", - ) - .to_compile_error(); - } + if let Err(err) = out.named_fields(fields, true, &meta) { + return err; } } syn::Fields::Unnamed(fields) => { - for (index, field) in fields.unnamed.iter().enumerate() { - if let Err(err) = out.unnamed_field(field, index) { - return err.to_compile_error(); - } + if let Err(err) = out.unnamed_fields(fields, true, &meta) { + return err; } } syn::Fields::Unit => out.body.extend(quote!(serializer.end_empty()?;)), @@ -321,6 +298,39 @@ struct StructOutput { } impl StructOutput { + fn named_fields( + &mut self, + fields: &syn::FieldsNamed, + inline: bool, + meta: &ContainerMeta, + ) -> Result<(), proc_macro2::TokenStream> { + if !inline { + self.body.extend(quote!(serializer.end_start()?;)); + } + + for field in &fields.named { + if let Err(err) = self.named_field(field, meta) { + return Err(err.to_compile_error()); + } + + if inline && !self.attributes.is_empty() { + return Err(syn::Error::new( + field.span(), + "no attributes allowed on inline structs", + ) + .to_compile_error()); + } + } + + if !inline { + let tag = meta.tag(); + self.body + .extend(quote!(serializer.write_close(prefix, #tag)?;)); + } + + Ok(()) + } + fn named_field(&mut self, field: &syn::Field, meta: &ContainerMeta) -> Result<(), syn::Error> { let field_name = field.ident.as_ref().unwrap(); let field_meta = match FieldMeta::from_field(field, meta) { @@ -432,6 +442,31 @@ impl StructOutput { Ok(()) } + fn unnamed_fields( + &mut self, + fields: &syn::FieldsUnnamed, + inline: bool, + meta: &ContainerMeta, + ) -> Result<(), proc_macro2::TokenStream> { + if !inline { + self.body.extend(quote!(serializer.end_start()?;)); + } + + for (index, field) in fields.unnamed.iter().enumerate() { + if let Err(err) = self.unnamed_field(field, index) { + return Err(err.to_compile_error()); + } + } + + if !inline { + let tag = meta.tag(); + self.body + .extend(quote!(serializer.write_close(prefix, #tag)?;)); + } + + Ok(()) + } + fn unnamed_field(&mut self, field: &syn::Field, index: usize) -> Result<(), syn::Error> { if !field.attrs.is_empty() { return Err(syn::Error::new( From 5bf8dd66747173f44fff76912dc97ee8aaf6231e Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Thu, 6 Mar 2025 16:42:58 +0100 Subject: [PATCH 3/6] ser: extract FieldMeta creation step --- instant-xml-macros/src/ser.rs | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/instant-xml-macros/src/ser.rs b/instant-xml-macros/src/ser.rs index 59e0821..445eebb 100644 --- a/instant-xml-macros/src/ser.rs +++ b/instant-xml-macros/src/ser.rs @@ -304,12 +304,19 @@ impl StructOutput { inline: bool, meta: &ContainerMeta, ) -> Result<(), proc_macro2::TokenStream> { + let fields = fields + .named + .iter() + .map(|field| FieldMeta::from_field(field, meta).map(|meta| (field, meta))) + .collect::, _>>() + .map_err(|err| err.to_compile_error())?; + if !inline { self.body.extend(quote!(serializer.end_start()?;)); } - for field in &fields.named { - if let Err(err) = self.named_field(field, meta) { + for (field, field_meta) in fields { + if let Err(err) = self.named_field(field, field_meta, meta) { return Err(err.to_compile_error()); } @@ -331,15 +338,13 @@ impl StructOutput { Ok(()) } - fn named_field(&mut self, field: &syn::Field, meta: &ContainerMeta) -> Result<(), syn::Error> { + fn named_field( + &mut self, + field: &syn::Field, + field_meta: FieldMeta, + meta: &ContainerMeta, + ) -> Result<(), syn::Error> { let field_name = field.ident.as_ref().unwrap(); - let field_meta = match FieldMeta::from_field(field, meta) { - Ok(meta) => meta, - Err(err) => { - self.body.extend(err.into_compile_error()); - return Ok(()); - } - }; let tag = field_meta.tag; let default_ns = match &meta.ns.uri { From 75a5e35a5e4e5114a02f63cb274f62ffdda8ef42 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Thu, 6 Mar 2025 16:55:54 +0100 Subject: [PATCH 4/6] ser: write empty element if direct child not present --- instant-xml-macros/src/ser.rs | 35 ++++++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/instant-xml-macros/src/ser.rs b/instant-xml-macros/src/ser.rs index 445eebb..e17e703 100644 --- a/instant-xml-macros/src/ser.rs +++ b/instant-xml-macros/src/ser.rs @@ -311,8 +311,30 @@ impl StructOutput { .collect::, _>>() .map_err(|err| err.to_compile_error())?; + let mut direct = None; + for (field, field_meta) in &fields { + if direct.is_some() { + return Err( + syn::Error::new(field.span(), "direct field must be the last") + .into_compile_error(), + ); + } + + if field_meta.direct { + direct = Some(field.ident.as_ref().unwrap()); + } + } + if !inline { - self.body.extend(quote!(serializer.end_start()?;)); + self.body.extend(match direct { + Some(field) => quote!( + match self.#field.present() { + true => serializer.end_start()?, + false => serializer.end_empty()?, + } + ), + None => quote!(serializer.end_start()?;), + }) } for (field, field_meta) in fields { @@ -331,8 +353,15 @@ impl StructOutput { if !inline { let tag = meta.tag(); - self.body - .extend(quote!(serializer.write_close(prefix, #tag)?;)); + self.body.extend(match direct { + Some(field) => quote!( + match self.#field.present() { + true => serializer.write_close(prefix, #tag)?, + false => (), + } + ), + None => quote!(serializer.write_close(prefix, #tag)?;), + }); } Ok(()) From 82f81e4b61b64d93082766dc08e139973cfaffcb Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Thu, 6 Mar 2025 18:04:34 +0100 Subject: [PATCH 5/6] de: only serialize direct value if necessary --- instant-xml-macros/src/de.rs | 37 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/instant-xml-macros/src/de.rs b/instant-xml-macros/src/de.rs index a273c2e..e8d3f2d 100644 --- a/instant-xml-macros/src/de.rs +++ b/instant-xml-macros/src/de.rs @@ -234,7 +234,6 @@ fn deserialize_struct( let mut declare_values = TokenStream::new(); let mut return_val = TokenStream::new(); let mut direct = TokenStream::new(); - let mut after_loop = TokenStream::new(); let mut borrowed = BTreeSet::new(); for (index, field) in fields.named.iter().enumerate() { @@ -264,7 +263,6 @@ fn deserialize_struct( field_meta, &input.ident, &container_meta, - &mut after_loop, ); if let Err(err) = result { @@ -363,7 +361,6 @@ fn deserialize_struct( node => return Err(Error::UnexpectedNode(format!("{:?} in {}", node, #ident_str))), } } - #after_loop *into = Some(Self { #return_val }); Ok(()) @@ -404,7 +401,6 @@ fn deserialize_inline_struct( let mut declare_values = TokenStream::new(); let mut return_val = TokenStream::new(); let mut direct = TokenStream::new(); - let mut after_loop = TokenStream::new(); let mut borrowed = BTreeSet::new(); let mut matches = TokenStream::new(); @@ -437,7 +433,6 @@ fn deserialize_inline_struct( field_meta, &input.ident, &meta, - &mut after_loop, ); let data = match result { @@ -553,7 +548,6 @@ fn named_field<'a>( mut field_meta: FieldMeta, type_name: &Ident, container_meta: &ContainerMeta, - after_loop: &mut TokenStream, ) -> Result, syn::Error> { let field_name = field.ident.as_ref().unwrap(); let field_tag = field_meta.tag; @@ -650,16 +644,6 @@ fn named_field<'a>( <#no_lifetime_type as FromXml>::deserialize(&mut #val_name, #field_str, &mut nested)?; } )); - // We can only enter this FromXml impl if the caller found the opening - // tag, so if we don't see the text node before the closing tag that is - // implied by terminating the loop, we need to populate the - // direct field with the implied empty text node. - after_loop.extend(quote!( - if !seen_direct { - let mut nested = deserializer.for_node(Node::Text("".into())); - <#no_lifetime_type as FromXml>::deserialize(&mut #val_name, #field_str, &mut nested)?; - } - )); } else { tokens.r#match.extend(quote!( __Elements::#enum_name => match <#no_lifetime_type as FromXml>::KIND { @@ -700,9 +684,24 @@ fn named_field<'a>( } }; - return_val.extend(quote!( - #field_name: #val_name.try_done(#field_str)?, - )); + if !field_meta.direct { + return_val.extend(quote!( + #field_name: #val_name.try_done(#field_str)?, + )); + } else { + return_val.extend(quote!( + #field_name: match #val_name.try_done(#field_str) { + Ok(value) => value, + Err(Error::MissingValue(_)) => { + let mut acc = <#no_lifetime_type as FromXml>::Accumulator::default(); + let mut nested = deserializer.for_node(Node::Text("".into())); + <#no_lifetime_type as FromXml>::deserialize(&mut acc, #field_str, &mut nested)?; + acc.try_done(#field_str)? + } + Err(e) => return Err(e), + } + )); + } Ok(FieldData { field_name, From 7affad680024922460a224f5da2bbebeda94fbeb Mon Sep 17 00:00:00 2001 From: Shashitharan Velosamy Date: Tue, 4 Mar 2025 14:24:15 +0100 Subject: [PATCH 6/6] Added unit test of direct element with no value --- instant-xml/tests/direct-no-value.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 instant-xml/tests/direct-no-value.rs diff --git a/instant-xml/tests/direct-no-value.rs b/instant-xml/tests/direct-no-value.rs new file mode 100644 index 0000000..3339d7f --- /dev/null +++ b/instant-xml/tests/direct-no-value.rs @@ -0,0 +1,21 @@ +use instant_xml::{from_str, to_string, FromXml, ToXml}; + +#[derive(ToXml, FromXml, Debug, PartialEq, Eq)] +struct Foo { + #[xml(attribute)] + attribute: Option, + #[xml(direct)] + direct: Option, +} + +#[test] +fn serde_direct_no_value_test() { + let v = Foo { + attribute: Some("Attribute text".to_string()), + direct: None, + }; + let xml = r#""#; + + assert_eq!(xml, to_string(&v).unwrap()); //this fails because the serializer still writes "" + assert_eq!(from_str::(xml).unwrap(), v); //this fails because the serializer still writes Some("") to direct +}