"expected the value signature" error somehow ignored with DeserializeDict, causing confusing "Unexpected non-0 padding byte" error

[ids1024]

I was getting a weird "Unexpected non-0 padding byte" error. It seems another parse error somehow occurred earlier but was ignored, and it tried to continue parsing, naturally not finding the bytes it expected...

After some debugging, here's a minimal test case:

use std::future::pending;
use zbus::{connection, interface, zvariant};

#[derive(Debug, zvariant::DeserializeDict, zvariant::Type)]
#[zvariant(signature = "a{sv}")]
pub struct Props {
    foo: String,
}

struct Test;

#[interface(name = "com.ids1024.test")]
impl Test {
    fn test(&mut self, props: Props) {
        dbg!(props);
    }
}

#[tokio::main]
async fn main() -> zbus::Result<()> {
    let _conn = connection::Builder::session()?
        .name("com.ids1024.test")?
        .serve_at("/com/ids1024/test", Test)?
        .build()
        .await?;

    pending::<()>().await;

    Ok(())
}

If this is invoked in d-feet with {"foo": GLib.Variant("u", 42)}, it produces:

('g-io-error-quark: GDBus.Error:org.freedesktop.zbus.Error: Unexpected non-0 '
 'padding byte `42` (36)')

This is a rather confusing error. Is something not aligned properly? But in reality, an error occured earlier in:

zbus/zvariant/src/dbus/de.rs

Lines 313 to 315 in 87ed52c

	let value_de = ValueDeserializer::new(self);
	visitor.visit_seq(value_de)

If this is changed to Ok(visitor.visit_seq(value_de).unwrap()), it outputs:

called `Result::unwrap()` on an `Err` value: Message("invalid value: string \"u\", expected the value signature")

So given the code and the message it was trying to parse, that makes sense. But why was that error not propagated properly, and it instead tried to continue parsing? I guess the error isn't being handled properly somewhere up the stack...

[ids1024]

Ah, so ignoring the error is part of the derive macro, and is intentional. Which makes a lot of sense... although it doesn't work for this particular error.

zbus/zvariant_derive/src/dict.rs

Lines 122 to 123 in 87ed52c

	// FIXME: add an option about strict parsing (instead of silently skipping the field)
	#name = access.next_value::<#zv::DeserializeValue<_>>().map(|v| v.0).ok();

Ignoring a parse error won't work properly if it means the bytes have not been consumed, and will be interpreted as something else. Not sure if there's a simple way to address this with how things currently work.

[zeenix]

Ah, so ignoring the error is part of the derive macro, and is intentional. Which makes a lot of sense... although it doesn't work for this particular error.

I am not sure if it makes sense actually. Seems like an oversight to me (unless @elmarco happen to remember when he wrote this code?). IMO, it should just error out if deserialization fails. The D-Bus format is very much context-sensitive and deserialization depends completely on the signature provided.

[ids1024]

This is for DeserializeDict, so it's entirely possible for the dict to contain a different type. Since it's deserializing from variant types. And ignoring the field if it has an unexpected type may be reasonable for some uses, though silently doing that could definitely be unexpected.

In this case I fixed the issue by just removing the field, which I wasn't using anyway. pop-os/cosmic-applets@09b2616 (I think I did see that with type type s elsewhere; but with the app that I ran into this with it's an aas).

[zeenix]

This is for DeserializeDict, so it's entirely possible for the dict to contain a different type.

Not sure I follow. The outer encoding is a variant but in practice you'll have specific types for each key. The *Dict macros are there for exactly that reason: allowing you to map your dict entries to specific names and types. So I'm not seeing the value in ignoring conversion errors (unless user explicitly ask us to though some annotation).

[ids1024]

I mean it probably does make sense to have the option to ignore dict entries with the wrong type, as the current implementation attempts to do. Maybe not by default though; only with some explicit annotation.