Consider using `Float.equal` instead of `=` in `[@@deriving eq]`

[Niols]

For simple types, [@@deriving eq] falls back on =:

ppx_deriving/src_plugins/eq/ppx_deriving_eq.ml

Lines 70 to 74 in e58e37d

	| true, ([%type: int] | [%type: int32] | [%type: Int32.t] |
	[%type: int64] | [%type: Int64.t] | [%type: nativeint] |
	[%type: Nativeint.t] | [%type: float] | [%type: bool] |
	[%type: char] | [%type: string] | [%type: bytes]) ->
	[%expr (fun (a:[%t typ]) b -> a = b)]

It sounds reasonable, and in general is fine. In the very specific case of nan, though, it will reach the typical thing that nan <> nan. In the standard library, however, there is Float.equal that does equate nan with itself. I think we should use that one, because:

• [@@deriving eq] behaves differently on type t = float and on type t = Float.t.
• As long as a float appears in a structure, it does not hold that, with [@@deriving eq, ord], equal x y = (compare x y = 0).
• eq argues that monomorphic equality is better (which, hell yeah), so one might expect Float.equal rather than a fallback on =. (This one is pretty subjective, I suppose.)

The big downside, today, would be that this introduces a breaking change; bummer.

[gasche]

I agree, but I don't know how to manage the possibly compatibility-breaking aspect.

It might be useful to have an attribute tweak to control this point, either to make the change opt-in or opt-out.

[Niols]

I'm not sure either, and I would expect that many pieces of software depend on this, directly or indirectly. An opt-in flag sounds the safest, although maybe not the most satisfactory.

[gasche]

I agree, but then it is probably not the cast that users actually test the derived equalities on nan values, otherwise I suspect that someone would have complained about the current behavior earlier.

(If someone was very brave they could instrument the generated equality to log any call that receives a nan in /tmp, and then run the whole opam testsuite... and then they would find nothing, and still we would be worried about compatibility breakage for final applications that are themselves not stored on opam. Oh well.)

[Niols]

I stumbled upon this by combining Qcheck and eq to test round trips of some PPX converter I am writing, so one might say it happened “naturally”. However, indeed, the lack of issues so far probably indicates that not many people actually stumbled upon it before.

[Niols]

I forgot to write, but in case someone stumbles upon this same issue: in my case, the workaround has been to use [@@deriving ord] instead and to rely on compare x y = 0 as an equality.