Treat wasmtime::component::Val::Float{32,64} NaNs as equal

[lann]

In #5510 we changed the value types of these variants from u{32,64} to f{32,64}. One side effect of this change was that two NaN values would no longer compare equal. While this is behavior complies with IEEE-754 floating point operations, it broke equality assumptions in fuzzing.

This commit changes equality for Val to make NaNs compare equal. Since the component model requires NaN canonicalization, all NaN bit representations compare equal, which is different from the original behavior.

This also gives Vals the semantics of Eq again, so that trait impl has been reintroduced to related types as well.

[lann]

As requested by @alexcrichton in #5510 (comment)

[github-actions]

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[jameysharp]

When we had a similar concern in Cranelift we ended up introducing a separate function that can just special-case floats and use a derived implementation of PartialEq otherwise: bitwise_eq.

Although you don't want bitwise equality here, the same pattern should work. It avoids the need to list every enum variant, and the possibility of bugs when new variants are added.

Another way to avoid bugs is to have two cases for each variant, like the implementation of PartialEq for DataValue. But I think here it's fine to use assert! instead of assert_eq! in the fuzz target and call a specialized function to do the right check.

[lann]

a separate function that can just special-case floats

That certainly could work, but it is significantly more code because of the component model's composite and parameterized types.

[jameysharp]

That certainly could work, but it is significantly more code because of the component model's composite and parameterized types.

Huh, I don't see why. I'm suggesting putting back the derive(PartialEq) for Val, and then writing something like this:

impl Val {
    pub fn funny_eq(&self, other: &Val) -> bool {
        match (self, other) {
            // This breaks conformance with IEEE-754 equality to simplify testing logic.
            (Self::Float32(l), Self::Float32(r)) => l == r || (l.is_nan() && r.is_nan()),
            (Self::Float64(l), Self::Float64(r)) => l == r || (l.is_nan() && r.is_nan()),
            _ => self == other,
        }
    }
}

Or, if it's only intended to be used in one place, just put this match there.

[jameysharp]

I saw why immediately after I posted that comment. I forgot the derived implementations wouldn't be calling this. I take it all back!

So this PR seems about as good as it can be, except I'd still propose the second pattern I mentioned to ensure that the compiler will catch the error if new variants are added to Val without updating the implementation of PartialEq.

[lann]

Another way to avoid bugs is to have two cases for each variant, like the implementation of PartialEq for DataValue.

Done.

[sunfishcode]

I don't have full context here, but I wanted to make sure it's considered that floating-point equality considers -0 and 0 to be equal, while most testing and fuzzing use cases should treat them as inequal.

[lann]

I don't have full context here, but I wanted to make sure it's considered that floating-point equality considers -0 and 0 to be equal, while most testing and fuzzing use cases should treat them as inequal.

I think the issue is just NaN inequality, but I don't entirely understand the fuzzing code in question.

[alexcrichton]

I'm going to go ahead and merge this since it fixes the fuzz issues that have cropped up, but I think it'd be reasonable to have a follow-up for +/- 0 handling as well.