Violation of PartialEq, Eq, PartialOrd and Ord contracts in minijinja::Value

[bertptrs]

Description

The Ord and PartialEq implementations of minijinja::Value work as follows:

1. Coerce the types of the two Values to be equal
   • Equal types are left as-is
   • When both sides are integers, everything is converted to i128
   • When either side is a float, both sides are coerced to a float,
   • Otherwise try to coerce to i128 on both sides
2. If this coercion was successful, do the comparison on the resulting values
3. Otherwise, try to compare the values as if they were objects

This violates the transitivity property of the comparison traits, which, in addition to producing unexpected but niche behaviour, also can cause panics while sorting due to the new sorting algorithms of Rust 1.81 panicking on invalid Ord implementations.

Reproduction steps

I wrote a small program that demonstrates the issue

use minijinja::Value;

fn main() {
    let a = Value::from(i64::MAX as i128);
    let b = Value::from(i64::MAX as f64);
    let c = Value::from(i64::MAX as i128 + 1);

    assert_eq!(a, b, "Equal because they are the same value");
    assert_eq!(b, c, "Equal because the float approximations are equal");
    assert_ne!(a, c, "Not equal because they are different values");
}

Additional helpful information:

• Version of minijinja: 2.2.0
• Version of rustc: 1.81.0
• Operating system and version: Ubuntu 22.04

Though none of the above should matter, I think this bug applies to all versions.

What did you expect

Equality and comparison operators should maintain their transitive properties. This is a hard problem with many corner cases. It would involve checking:

• if the float is an integer, otherwise the comparison should be false
• if the float is within the relevant integer range, otherwise the comparison should be false
• and then the comparison should happen in integers, not floats

This is much more complicated than the existing solution, so it's understandable you went this route, but it does have the issue described above.

Also the above fix only works for (Partial)Eq, fixing Ord is much more complicated.

[mitsuhiko]

Unfortunately the strictness in the new sort behavior is very annoying to deal with for types like Jinja's Value type. I don't think the current implementation of Ord can cause a panic with the new sort algorithm but I'm not 100% sure.

The code internally undergoes a coercion step that is lossy if one side is a float. That one is obviously incorrect but I'm not sure yet what the right fix is.

[mitsuhiko]

Refs rust-lang/rust#129561

[mitsuhiko]

That BTW is incorrect: Value::from(i64::MAX as f64). The largest value that can be represented as a float is 2^53.

[bertptrs]

That BTW is incorrect: Value::from(i64::MAX as f64). The largest value that can be represented as a float is 2^53.

You are right, though my intent was not to showcase the first value where this goes wrong, just "a" value where this goes wrong. i64::MAX was a convenient constant for that.