HashMaps with tuple keys are not sorted, even with setting sort_maps set to true

[AlexCid]

What happened?

As the title says. This test should pass when triggered several times:

#[test]
pub fn insta_sort_order() {
        let mut m = HashMap::new();
        m.insert((1, 3), 4);
        m.insert((2, 3), 4);
        m.insert((1, 4), 4);
        m.insert((3, 3), 4);
        m.insert((9, 3), 4);
        insta::with_settings!({sort_maps =>true}, {
            insta::assert_ron_snapshot!(m);
        });
    }

However, this test fails spuriously. Indeed, when reviewing the test result, one can easily see that the map is not sorted. One such result is the following:

Expression: m
────────────────────────────────────────
+new results
────────────┬───────────────────────────
          0 │+{
          1 │+  (1, 3): 4,
          2 │+  (1, 4): 4,
          3 │+  (3, 3): 4,
          4 │+  (9, 3): 4,
          5 │+  (2, 3): 4,
          6 │+}

Reproduction steps

No response

Insta Version

1.21

rustc Version

1.64

What did you expect?

HashMaps using tuples as key should be sorted before serialization if the appropriate setting is set.

[AlexCid]

Looking at the code, the issue seems to stem from

insta/src/content/serialization.rs

Line 53 in 89f2c2f

items.sort_by(|a, b| a.0.as_key().cmp(&b.0.as_key()));

A tuple key has type Content::Tuple(Vec<Content>), and when transformed into the Key enum, we get a Key::Other variant. Thus, the ordering is basically random.

I wonder if the Key enum type is really necessary. Content already implements PartialOrd and PartialEq. I replaced the aforementioned line with

                items.sort_by(|a, b| a.0.partial_cmp(&b.0).unwrap_or(Ordering::Less));

and nothing bad has happened yet. My motivating example however now passes, and the serialization produced by insta is sorted correctly.

What do you think ?

[mitsuhiko]

The motivation is that 1 < 2 when Content::I32(1) and Content::U64(2) which otherwise would not be the case.

[AlexCid]

Thanks for your answer !

Ok, I see. Then the current code could simply be adapted by adding one more Key variant as an escape hatch of sorts:

#[derive(PartialEq, PartialOrd, Debug)]
pub enum Key<'a> {
    U64(u64),
    I64(i64),
    F64(f64),
    U128(u128),
    I128(i128),
    Other(&'a Content),
}

impl<'a> Eq for Key<'a> {}

impl<'a> Ord for Key<'a> {
    fn cmp(&self, other: &Self) -> Ordering {
        self.partial_cmp(other).unwrap_or(Ordering::Less)
    }
}

impl Content {
    pub(crate) fn as_key(&self) -> Key<'_> {
        match self.resolve_inner() {
            Content::Char(val) => Key::U64(*val as u64),
            Content::U16(val) => Key::U64((*val).into()),
            Content::U32(val) => Key::U64((*val).into()),
            Content::U64(val) => Key::U64(*val),
            Content::U128(val) => Key::U128(*val),
            Content::I16(val) => Key::I64((*val).into()),
            Content::I32(val) => Key::I64((*val).into()),
            Content::I64(val) => Key::I64(*val),
            Content::I128(val) => Key::I128(*val),
            Content::F32(val) => Key::F64((*val).into()),
            Content::F64(val) => Key::F64(*val),
            other => Key::Other(other),
        }
    }
}

(the Bool case now becomes redundant with the Other case)

[AlexCid]

Thanks for the fix :)