fix!: make NaN == NaN inside Expression

[Shadow53]

This is the easiest way to resolve #316 in a way that doesn't break the API (the other way being to use OrderedFloat).

Fixes #316.

[BatmanAoD]

Looks correct to me; I don't think I'm the one to make the call on actually deciding we want NaN==NaN for our users, though.

[notmgsk]

Hm. Not sure how to feel about this one. Part of me wants something like Expression::Nan, or Expression::Invalid(DivisionByZero)).

[antalsz]

This showed up again in #432

[antalsz]

In discussion with @erichulburd today, it seems like the best approach is to take this method, but make it available under a different name rather than PartialEq, and have the round_trip test use that. This should resolve the concern with NaN behaving unexpectedly politely while still allowing us to get the polite behavior.

[antalsz]

On second thought, I'm warming to the approach here actually being correct. We claim that Expressions are Eq; that requires promising that equality is an equivalence relation, which is not currently true. It will be true if NaNs compare equal; maybe all that we need to do is document that well and then roll with this change. Thoughts? cc. @erichulburd, @Shadow53, @kalzoo

[github-actions]

PR Preview Action v1.6.0
Preview removed because the pull request was closed.
2025-01-14 21:33 UTC

[antalsz]

@Shadow53 I hope you don't mind, I rebased this branch, updated the proptest seed, and added the documentation I mentioned. If this looks good to folks, I think this is ready to merge (subject to CI).

[erichulburd]

On second thought, I'm warming to the approach here actually being correct. We claim that Expressions are Eq; that requires promising that equality is an equivalence relation, which is not currently true. It will be true if NaNs compare equal; maybe all that we need to do is document that well and then roll with this change. Thoughts? cc. @erichulburd, @Shadow53, @kalzoo

I don't feel too strongly either way and am happy to defer to you and @Shadow53 on this.