🩹 🚸 zv: Equal Signatures irrespective of D-Bus marshalled state

[luukvanderduim]

This adjusts the PartialEq impls to evaluate equality irrespective of
of marshalling of the type.

Two signatures representing the same type can differ as a result of
D-Bus marshalling. D-Bus marshalling will omit outer parentheses from body
singatures.

Consider a MyType { a: i32, b: i32}:

MyType::signature() returns a "(ii)" Signature whereas
Message::body_signature() with MyType as body, would return
a "ii" Signature, which may lead to unexpected inequality of Signatures.

Note: PartialEq on Signature accounts for marshalling only.
One can construct- or deserialize into deeper nested structs or tuples than
than the signature describes but at that point the divergence is expected.

[zeenix]

Looks good otherwise. Also, it would be great to modify zbus code to make use of this in message-handling code.

[zeenix]

I understand you're still grasping around git rebasing etc so I understand completely (I'd make a lot more mistakes in your shoes for sure) but kindly review each line of every commit before (or after) pushing) to catch such mistakes.

[luukvanderduim]

Unfortunately I have messed up. I will take your advise to heart, sorry to trouble you with it.

[luukvanderduim]

More comfortable with the commits' outline, but shape is just a prerequisite

I remember you mention a preference for string comparisons in favor of the byte slice comparisons.
This is what I ended up with:

    fn eq(&self, other: &Signature<'_>) -> bool {
        match (
            self.without_outer_parentheses(),
            other.without_outer_parentheses(),
        ) {
            (Some(lhs_without), None) => *lhs_without == **other,
            (None, Some(rhs_without)) => **self == *rhs_without,
            _ => **self == **other,
        }
    }

I am a bit torn, as the dereferences do different things and make it less clear what actually happens but on the other hand, this is prettier than having six .as_bytes() calls around, and the alternative of having six .as_str() about the same.
I am sure you know what you like better.

[zeenix]

More comfortable with the commits' outline, but shape is just a prerequisite

👍

I remember you mention a preference for string comparisons in favor of the byte slice comparisons. This is what I ended up with:

hmm..

• why can't without_outer_parentheses method always return a slice? If there are no parantheses, it just returns the input as is.
• I wasn't objecting to use of as_bytes but rather byte literals because of b' suffix. Looks a bit ugly so when there is no need, I'd very much like to avoid it.

[luukvanderduim]

* why can't `without_outer_parentheses` method always return a slice? If there are no parantheses, it just returns the input as is.

It can - and it does now.
This simplifies PartialEq nicely.

Now, I am pondering whether the method without_outer_parentheses should be called either marshall_signature or just 'marshalled_str' because it is not necessarily removing outer parentheses.

I misunderstood how Signature::slice is not intended to hand out a slice of its bytes.
I see now how ``sliceupdatespos` and `end` and Signature::as_str` does do the slicing.
I presume this is a more widely used pattern I had just not looked at this closely before.

as_marshalled_str?

[luukvanderduim]

Being curious what the differences would be between the approaches, I rolled a quick bench crate

[zeenix]

* why can't `without_outer_parentheses` method always return a slice? If there are no parantheses, it just returns the input as is.

It can - and it does now.
This simplifies PartialEq nicely.

Nice. 👍

Now, I am pondering whether the method without_outer_parentheses should be called either marshall_signature or just 'marshalled_str' because it is not necessarily removing outer parentheses.

Nah, without_outer_parentheses is fine. Just add doc string to the method, explaining that it doesn't remove parens if there are none.

[zeenix]

You mentioned zbus earlier but I think I read 'could' then.

I wrote would. :) But not worries.

I take it that this refers to MessageBuilder?

Yes (at least, see if we do something like that elsewhere too in code please).

[zeenix]

Actually apart from these minor things, I don't see why we can't merge this work. Am I forgetting something from our conversations on Matrix? 🤔

You'll have to rebase on main in any case.

[luukvanderduim]

Funny we end up in this situation where you suggest to merge and I have concerns.

On matrix I shared that I had come to realize that R (the Rust type), B (the Message body type), and D (the de-serialized Rust type) are not necessarily the same, despite being closely related. They share "a list of zero or more single complete types, each made up of one or more type codes" (in D-Bus spec speak) - but may differ by zero or more outer structs or tuples.

An "ii" body deserializes to "(((ii)))" just fine, which is understandable, because D-Bus and zvariant don't care about the outer shape, and happily accommodate how the user wants the data. But if some user does this, we are more than one pair of parentheses apart

The question is, should PartialEq reflect on equality of the underlying data and be as lenient as going 'R->B->D' can be?, Or should it support the common case of allowing divergence by one pair of parentheses, as this PR?
Or should it just stick to being a trait to a type and tell it like it is?

The first way offers least chance of surprise and most expensive PartialEq , second, this PR, is pragmatic but does not cover all cases, the last seems most principled and is least expensive.

In tests, people probably will not care about more expensive equality.
Users might care if they check signatures in a hot loop?
Could SignalStream::filter_map by signature ,ale sense for some people, for example?

Either way, I do think the associated functions on Signature make sense.

[zeenix]

Funny we end up in this situation where you suggest to merge and I have concerns.

😆

On matrix I shared that I had come to realize that R (the Rust type), B (the Message body type), and D (the de-serialized Rust type) are not necessarily the same

Right, I recall now. :) As I tried to explain, I do not care much for theoretical issues. We already need to treat (x) the same as x and we can't help it, and in theory that's just wrong. So I'm more concerned about consistency here, above all.

Or should it support the common case of allowing divergence by one pair of parentheses, as this PR?

Yeah, I think this is a good compromise. Hence why I'm thinking this PR is the way to go.

second, this PR, is pragmatic but does not cover all cases

I don't think we need to cover all cases. You'll notice that message::Builder removes only one layer of (). So this approach is also consistent with what we've been doing already.

[zeenix]

@luukvanderduim You've more than a 100 commits on the PR now. Something went horribly wrong in your rebase. :(

[luukvanderduim]

Unfortunate.
Ill go through the motions again.

[luukvanderduim]

Sanity seems to have been restored. We are back at three commits, which - looking at the changes - are what we want.
One commit dropped because the function it modified no longer exists.

[zeenix]

Sanity seems to have been restored. We are back at three commits, which - looking at the changes - are what we want.

Great, thanks again for still not giving up. :)

One commit dropped because the function it modified no longer exists.

It exists in another form (see commit a0b92da). Please make sure the new code doesn't need these changes that were dropped.

Btw, I think the CI can be fixed by just rebasing on the latest main branch.

[luukvanderduim]

One commit dropped because the function it modified no longer exists.

It exists in another form (see commit a0b92da). Please make sure the new code doesn't need these changes that were dropped.

Looks like it does:

SignatureParser::validate() relies on SignatureParser::from_bytes_unchecked()
Checks max. length is <= 255.

SignatureParser::validate() then relies on SignatureParser's Iterator implementation.

SignatureParser as Iterator::next() ->
None if self.done(), otherrwise:

next_signature() parses by (start-) character and if it finds an opening parenthesis,
it calls next_structure_signature():

Which errors when it would find an empty struct.
Initializes a 'field parser' and keeps track of fields' lengths.
The while loop advances over the field_parser slice, calling parse_next_signature
and sums up the lengths of the 'single complete types' / struct field signatures it finds, until
either at premature end or when it finds ')'

• Would Err it next_char were not ')' (because it would mean the loop exited prematurely.

  returns Ok(Signature) signature of slice of the struct.
  caller next_signature() returns Ok(Signature)
  caller next() returns Some(Ok(Signature))

validate() evaluates these 'single complete type signature' results: s?.

Long story short, yes I believe 'balanced parantheses' are checked this way.

Note, there are things the parser does not do, but also is not that important:

• max. braces depth (32), max. total nesting depth (64)

Btw, I think the CI can be fixed by just rebasing on the latest main branch.

Seems #440 is related.

[zeenix]

Long story short, yes I believe 'balanced parantheses' are checked this way.

👍 so these changes are good then, good.

Btw, I think the CI can be fixed by just rebasing on the latest main branch.

Seems #440 is related.

Right, it should work now. I restarted the failing job. 🤞

max. braces depth (32), max. total nesting depth (64)

Hmm.. currently these are checked by the serializers and deserializers so it is ultimately checked. However, now I wonder if the checks should be on Signature creation itself. The signature is parsed for checking anyway.

[luukvanderduim]

Right, it should work now. I restarted the failing job. crossed_fingers

Rebased to main.

max. braces depth (32), max. total nesting depth (64)

Hmm.. currently these are checked by the serializers and deserializers so it is ultimately checked. However, now I wonder if the checks should be on Signature creation itself. The signature is parsed for checking anyway.

Does moving those checks solve a problem?

[zeenix]

Hmm.. currently these are checked by the serializers and deserializers so it is ultimately checked. However, now I wonder if the checks should be on Signature creation itself. The signature is parsed for checking anyway.

Does moving those checks solve a problem?

Well ser/de process isn't currently as fast as it should be so maybe it would help there. Signature creation is typically amortized. So it'll definitely optimize the overall code but can't be sure if it'd be significant enough to care.

[zeenix]

Sorry, i still found something not right. :)

[luukvanderduim]

Moved the associated functions out. I hope you are happy with where they landed (just above where they are needed, above impl PartialEq<Signature<>> for Signature<> and take note of the lifetime bounds on without_outer_parentheses which are new and are necessary to convey the relation between the borrow of the Signature and its internal reference.