xmlgen fails to produce working code for org.freedesktop.systemd1

[zeenix]

In GitLab by @richard.maw on Aug 24, 2021, 22:39

gdbus introspect --session --dest org.freedesktop.systemd1 \
    --object-path /org/freedesktop/systemd1 --xml \
    >src/org.freedesktop.systemd1.xml
zbus-xmlgen src/org.freedesktop.systemd1.xml >src/systemd1.rs
cargo build

The generated code fails to build with:

error[E0277]: the size for values of type `[(&str, zbus::zvariant::Value<'_>)]` cannot be known at compilation time
   --> src/systemd1.rs:416:16
    |
416 |         arg_4: &[(&str, [(&str, zbus::zvariant::Value<'_>)])],
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
    = help: within `(&str, [(&str, zbus::zvariant::Value<'_>)])`, the trait `Sized` is not implemented for `[(&str, zbus::zvariant::Value<'_>)]`
    = note: required because it appears within the type `(&str, [(&str, zbus::zvariant::Value<'_>)])`
    = note: slice and array elements must have `Sized` type

error: aborting due to previous error

The method causing error is:

      StartTransientUnit(in  s name,
                         in  s mode,
                         in  a(sv) properties,
                         in  a(sa(sv)) aux,
                         out o job);

which gets turned into:

    /// StartTransientUnit method
    fn start_transient_unit(
        &self,
        arg_1: &str,
        arg_2: &str,
        arg_3: &[(&str, zbus::zvariant::Value<'_>)],
        arg_4: &[(&str, [(&str, zbus::zvariant::Value<'_>)])],
    ) -> zbus::Result<zbus::zvariant::OwnedObjectPath>;

Changing the arg_4 definition to:

        arg_4: &[(&str, &[(&str, zbus::zvariant::Value<'_>)])],

makes it build and at least works when passing an empty slice.

This change to xmlgen makes it generate output the same as the fixed version, but I don't know the code well enough to know whether it'd break something else so I'm wary of submitting it as a PR.

diff --git a/zbus_xmlgen/src/gen.rs b/zbus_xmlgen/src/gen.rs
index f3e839a..6807260 100644
--- a/zbus_xmlgen/src/gen.rs
+++ b/zbus_xmlgen/src/gen.rs
@@ -209,7 +209,7 @@ fn to_rust_type(ty: &str, input: bool, as_ref: bool) -> String {
                     _ => {
                         let ty = iter_to_rust_type(it, input, false);
                         if input {
-                            format!("{}[{}]", if as_ref { "&" } else { "" }, ty)
+                            format!("&[{}]", ty)
                         } else {
                             format!("{}Vec<{}>", if as_ref { "&" } else { "" }, ty)
                         }

[zeenix]

This change to xmlgen makes it generate output the same as the fixed version, but I don't know the code well enough to know whether it'd break something else so I'm wary of submitting it as a PR.

Can you check if instead this change:

diff --git a/zbus_xmlgen/src/gen.rs b/zbus_xmlgen/src/gen.rs
index f3e839a..e2998ba 100644
--- a/zbus_xmlgen/src/gen.rs
+++ b/zbus_xmlgen/src/gen.rs
@@ -204,7 +204,7 @@ fn to_rust_type(ty: &str, input: bool, as_ref: bool) -> String {
                 match **c as char {
                     '{' => format!(
                         "std::collections::HashMap<{}>",
-                        iter_to_rust_type(it, input, false)
+                        iter_to_rust_type(it, input, as_ref)

helps? If so, that's the correct change AFAICT.

[zeenix]

In GitLab by @richard.maw on Aug 25, 2021, 20:44

This change generates code the same as the broken version.

Passing as_ref to the non-hashmap case with this patch:

diff --git a/zbus_xmlgen/src/gen.rs b/zbus_xmlgen/src/gen.rs
index f3e839a..aa314be 100644
--- a/zbus_xmlgen/src/gen.rs
+++ b/zbus_xmlgen/src/gen.rs
@@ -204,10 +204,10 @@ fn to_rust_type(ty: &str, input: bool, as_ref: bool) -> String {
                 match **c as char {
                     '{' => format!(
                         "std::collections::HashMap<{}>",
-                        iter_to_rust_type(it, input, false)
+                        iter_to_rust_type(it, input, as_ref)
                     ),
                     _ => {
-                        let ty = iter_to_rust_type(it, input, false);
+                        let ty = iter_to_rust_type(it, input, as_ref);
                         if input {
                             format!("{}[{}]", if as_ref { "&" } else { "" }, ty)
                         } else {

produces even stranger code:

    fn start_transient_unit(
        &self,
        arg_1: &str,
        arg_2: &str,
        arg_3: &[&(&str, zbus::zvariant::Value<'_>)],
        arg_4: &[&(&str, [(&str, zbus::zvariant::Value<'_>)])],
    ) -> zbus::Result<zbus::zvariant::OwnedObjectPath>;

[zeenix]

This change generates code the same as the broken version.

Passing as_ref to the non-hashmap case with this patch:

Hmm.. sorry I actually meant passing as_ref to only the non-hashmap case. Does that not work either?

[zeenix]

In GitLab by @richard.maw on Aug 25, 2021, 22:14

Like this?

diff --git a/zbus_xmlgen/src/gen.rs b/zbus_xmlgen/src/gen.rs
index f3e839a..045cea2 100644
--- a/zbus_xmlgen/src/gen.rs
+++ b/zbus_xmlgen/src/gen.rs
@@ -207,7 +207,7 @@ fn to_rust_type(ty: &str, input: bool, as_ref: bool) -> String {
                         iter_to_rust_type(it, input, false)
                     ),
                     _ => {
-                        let ty = iter_to_rust_type(it, input, false);
+                        let ty = iter_to_rust_type(it, input, as_ref);
                         if input {
                             format!("{}[{}]", if as_ref { "&" } else { "" }, ty)
                         } else {

Same output as passing it to both.

[zeenix]

Like this?

Yeah.

Same output as passing it to both.

Darn, needs deeper look then. :( Your hardcoding fix might actually be valid but can't say for sure.

[zeenix]

In GitLab by @ids1024 on Jan 12, 2022, 24:44

mentioned in commit ids1024/zbus@687b7cb26ad478ad01365b38d0edae18fd35a557

[zeenix]

In GitLab by @ids1024 on Jan 12, 2022, 24:46

mentioned in commit ids1024/zbus@0c82046aa98df9f4c6888cf2bea4f63dc8863851

[zeenix]

mentioned in commit 79cb5d111094aaf089754a1c76769dca54ded543