[BUG] <Variant as FromIterator<Item = T>>::from_iter panics if T contains Variant

[kawadakk]

Bug description

<Variant as FromIterator<Item = T>>::from_iter seems to assume that <T as Into<Variant>>::into returns a variant of type <T as StaticVariantType>::static_variant_type(), but this assumption does not hold if T or one of its constituents is Variant.

#[test]
fn test_dict_array_collect() {
    let a = [
        // `DictEntry<i32, Variant>`
        DictEntry::new(1, "foo".to_variant()),
        DictEntry::new(2, 42.to_variant()),
    ]
    .into_iter()
    .collect::<Variant>();
    assert_eq!(a.type_().as_str(), "a{iv}");
    assert_eq!(a.n_children(), 2);
}

Backtrace

thread 'variant::tests::test_dict_array_collect' panicked at glib/src/variant.rs:454:21:
assertion failed: value.is_type(type_)
stack backtrace:
...
  19:     0x55c61135ce7c - core::panicking::panic::h75b3c9209f97d725
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:146:5
  20:     0x55c6113c1733 - glib::variant::Variant::array_from_iter_with_type::h3fc38c771955ff47
                               at .../glib/src/variant.rs:454:21
  21:     0x55c6113c0c42 - glib::variant::Variant::array_from_iter::hfa13497b86ba0abd
                               at .../glib/src/variant.rs:430:9
  22:     0x55c6113c4eaa - <glib::variant::Variant as core::iter::traits::collect::FromIterator<T>>::from_iter::h703e07392ae239a2
                               at .../glib/src/variant.rs:1844:9
  23:     0x55c6113b72e6 - core::iter::traits::iterator::Iterator::collect::hdf28181c29380c98
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/iter/traits/iterator.rs:2004:9
  24:     0x55c61136dea3 - glib::variant::tests::test_dict_array_collect::h72e222461f9f1d92
                               at .../glib/src/variant.rs:2362:17
...

[sdroege]

No the problem is that your dict entries are {is} and {ii} but {iv} would be required.

type_ passed to array_from_iter_with_type() is {iv}. The Into<Variant> impl is producing {is} and {ii}, but when using ToVariant it's {iv} as expected.

E.g. this change fixes it

diff --git a/glib/src/variant.rs b/glib/src/variant.rs
index 14e73f797c5..2852bbe1ef8 100644
--- a/glib/src/variant.rs
+++ b/glib/src/variant.rs
@@ -451,7 +451,11 @@ impl Variant {
                     == ffi::GFALSE
                 {
                     ffi::g_variant_builder_clear(&mut builder);
-                    assert!(value.is_type(type_));
+                    assert!(
+                        value.is_type(type_),
+                        "Expected type {type_} but got {}",
+                        value.type_()
+                    );
                 }
 
                 ffi::g_variant_builder_add_value(&mut builder, value.to_glib_none().0);
@@ -1839,9 +1843,9 @@ tuple_impls! {
     16 => (0 T0 1 T1 2 T2 3 T3 4 T4 5 T5 6 T6 7 T7 8 T8 9 T9 10 T10 11 T11 12 T12 13 T13 14 T14 15 T15)
 }
 
-impl<T: Into<Variant> + StaticVariantType> FromIterator<T> for Variant {
+impl<T: ToVariant + StaticVariantType> FromIterator<T> for Variant {
     fn from_iter<I: IntoIterator<Item = T>>(iter: I) -> Self {
-        Variant::array_from_iter::<T>(iter.into_iter().map(|v| v.into()))
+        Variant::array_from_iter::<T>(iter.into_iter().map(|v| v.to_variant()))
     }
 }
 
@@ -2517,4 +2521,17 @@ mod tests {
         let hashmap: Option<HashMap<u64, u64>> = FromVariant::from_variant(&variant);
         assert!(hashmap.is_some());
     }
+
+    #[test]
+    fn test_dict_array_collect() {
+        let a = [
+            // `DictEntry<i32, Variant>`
+            DictEntry::new(1, "foo".to_variant()),
+            DictEntry::new(2, 42.to_variant()),
+        ]
+        .into_iter()
+        .collect::<Variant>();
+        assert_eq!(a.type_().as_str(), "a{iv}");
+        assert_eq!(a.n_children(), 2);
+    }
 }

This needs some more thought, and it seems quite unexpected that Into<Variant> and ToVariant return variants of different types.

[sdroege]

Basically

    #[test]
    fn test_dict_entry_types() {
        let e = DictEntry::new(1, "foo".to_variant());
        let v1 = e.to_variant();
        let v2 = Variant::from(e);

        assert_eq!(v1.type_(), v2.type_());
    }

Fails with

assertion `left == right` failed
  left: VariantTy { inner: "{iv}" }
 right: VariantTy { inner: "{is}" }

[sdroege]

And the reason for that is that From<Variant> for Variant doesn't do anything, so the underlying type stays the same. And ToVariant for Variant boxes the value.

That seems like a problematic inconsistency 🙃