parent c10929d

author Luuk van der Duim <[email protected]> 1686656206 +0200
committer Luuk van der Duim <[email protected]> 1686917212 +0200

🩹 🚸 zv: `Signature` equality regardless of outer parentheses

This commit adjusts the `PartialEq` impls to evaluate equality irrespective of
their marshalled state.

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()` embedding the same `MyType` body, would return
a "ii" `Signature`

This may lead to unexpected inequality of `Signature`s.

zvariant/src/signature.rs

@@ -83,7 +83,7 @@ impl<'b> std::ops::Deref for Bytes<'b> {
 ///
 /// [identifies]: https://dbus.freedesktop.org/doc/dbus-specification.html#type-system
 /// [`slice`]: #method.slice
-#[derive(Eq, Hash, Clone)]
+#[derive(Hash, Clone)]
 pub struct Signature<'a> {
     bytes: Bytes<'a>,
     pos: usize,
@@ -354,24 +354,52 @@ impl<'a> std::ops::Deref for Signature<'a> {
     }
 }
 
-impl<'a, 'b> PartialEq<Signature<'a>> for Signature<'b> {
-    fn eq(&self, other: &Signature<'_>) -> bool {
-        self.as_bytes() == other.as_bytes()
+fn has_balanced_parentheses(bytes: &[u8]) -> bool {
+    bytes.iter().fold(0, |count, byte| match byte {
+        b'(' => count + 1,
+        b')' if count > 0 => count - 1,
+        _ => count,
+    }) == 0
+}
+
+fn has_outer_parentheses(bytes: &[u8]) -> bool {
+    bytes.starts_with(&[b'('])
+        && bytes.ends_with(&[b')'])
+        && has_balanced_parentheses(&bytes[1..bytes.len() - 1])
+}
+
+impl<'a, 'b> PartialEq<Signature<'b>> for Signature<'a> {
+    fn eq(&self, other: &Signature<'b>) -> bool {
+        match (
+            has_outer_parentheses(self.as_bytes()),
+            has_outer_parentheses(other.as_bytes()),
+        ) {
+            (true, false) => self.slice(1..self.len() - 1).as_bytes() == other.as_bytes(),
+            (false, true) => self.as_bytes() == other.slice(1..other.len() - 1).as_bytes(),
+            _ => self.as_bytes() == other.as_bytes(),
+        }
     }
 }
 
+// Assumes that `str` is a valid signature string.
 impl<'a> PartialEq<str> for Signature<'a> {
     fn eq(&self, other: &str) -> bool {
-        self.as_bytes() == other.as_bytes()
+        let other = Signature::from_str_unchecked(other);
+        self.eq(&other)
     }
 }
 
+// Assumes that `&str` is a valid signature string.
+// Assumes that `&str` is a valid signature string.
 impl<'a> PartialEq<&str> for Signature<'a> {
     fn eq(&self, other: &&str) -> bool {
-        self.as_bytes() == other.as_bytes()
+        let other = Signature::from_str_unchecked(other);
+        self.eq(&other)
     }
 }
 
+impl Eq for Signature<'_> {}
+
 impl<'a> Display for Signature<'a> {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         std::fmt::Display::fmt(&self.as_str(), f)
@@ -497,7 +525,9 @@ impl std::fmt::Display for OwnedSignature {
 
 #[cfg(test)]
 mod tests {
-    use super::Signature;
+    use crate::signature::has_balanced_parentheses;
+
+    use super::{has_outer_parentheses, Signature};
 
     #[test]
     fn signature_slicing() {
@@ -515,4 +545,54 @@ mod tests {
         assert_eq!(slice, "t");
         assert_eq!(slice.slice(1..), "");
     }
+
+    #[test]
+    fn test_has_outer_parentheses() {
+        let sig = Signature::from_str_unchecked("(asta{sv})");
+        assert!(has_outer_parentheses(sig.as_bytes()));
+
+        let sig = Signature::from_str_unchecked("asta{sv}");
+        assert!(!has_outer_parentheses(sig.as_bytes()));
+
+        let sig = Signature::from_str_unchecked("((so)ii(uu))");
+        assert!(has_outer_parentheses(sig.as_bytes()));
+
+        let sig = Signature::from_str_unchecked("(so)ii(uu)");
+        assert!(!has_outer_parentheses(sig.as_bytes()));
+    }
+
+    #[test]
+    fn test_balance_parentheses() {
+        let sig = Signature::from_str_unchecked("(asta{sv})");
+        assert!(has_balanced_parentheses(sig.as_bytes()));
+        assert!(has_balanced_parentheses(
+            sig.slice(1..sig.len() - 1).as_bytes()
+        ));
+
+        let sig = Signature::from_str_unchecked("((so)ii(uu))");
+        assert!(has_balanced_parentheses(sig.as_bytes()));
+        assert!(has_balanced_parentheses(
+            sig.slice(1..sig.len() - 1).as_bytes()
+        ));
+
+        let sig = Signature::from_str_unchecked(")ii(");
+        assert!(!has_balanced_parentheses(sig.as_bytes()));
+
+        let sig = Signature::from_str_unchecked("(so)ii(uu)");
+        assert!(has_balanced_parentheses(sig.as_bytes()));
+        assert!(!has_balanced_parentheses(
+            sig.slice(1..sig.len() - 1).as_bytes()
+        ));
+    }
+
+    #[test]
+    fn signature_equality() {
+        let paren = Signature::from_str_unchecked("(asta{sv})");
+        let parenless = Signature::from_str_unchecked("asta{sv}");
+        assert_eq!(paren, parenless);
+
+        let paren = Signature::from_str_unchecked("((so)ii(uu))");
+        let parenless = Signature::from_str_unchecked("(so)ii(uu)");
+        assert_eq!(paren, parenless);
+    }
 }

zvariant_derive/src/type.rs

@@ -25,7 +25,7 @@ pub fn expand_derive(ast: DeriveInput) -> Result<TokenStream, Error> {
                 fn signature() -> #zv::Signature<'static> {
                     // FIXME: Would be nice if we had a parsed `Signature` in the macro code already so
                     // it's checked at the build time but currently that's not easily possible w/o
-                    // zvariant_derive requiring zvariant and we don't want it as it creates a cyclic
+                    // zvariant_derive requiring zvaraint and we don't want it as it creates a cyclic
                     // dep. Maybe we can find a way to share the `Signature` type between the two
                     // crates?
                     #zv::Signature::from_static_str(#signature).unwrap()