Document non-trivial function Code.constant_equal, and fix related bugs

[OlivierNicole]

I think this will be useful to future us.

(@vouillon excavated the semantics of the function.)

[OlivierNicole]

See

js_of_ocaml/compiler/lib/code.ml

Lines 404 to 405 in 3204c33

	| Int _, (String _ | NativeString _ | Float_array _ | Int64 _ | Tuple (_, _, _)) ->
	Some false

Is it definitely the case that in Javascript a == b is false when a is an Int (whatever that means) and b is an Int64?

[vouillon]

An int64 is implemented as a JavaScript object, which is never equal to a JavaScript number.

[vouillon]

Also Float.equal 0. (-0.) is false. So (0., 0.) = (-0., 0.) is miscompiled into true.

I think the fix in #1048 is wrong and we should not use Code.constant_equal in Flow.the_const_of.

[hhugo]

Also Float.equal 0. (-0.) is false. So (0., 0.) = (-0., 0.) is miscompiled into true.

I'm guessing you mean

Also `Float.equal 0. (-0.)` is false. So `(0., 0.) = (-0., 0.)` is miscompiled into `false`

[hhugo]

@vouillon, can you explain what's wrong with using Code.constant_equal in Flow.the_const_of ?

I think the way forward is to come up with regression tests showing how the current implementation is wrong. @OlivierNicole, do you want to look at this ?

[OlivierNicole]

I can look at this, but so far i’m confused:

Also Float.equal 0. (-0.) is false.

Testing in ocaml 5.2.0 yields true.

I'm guessing you mean

Also `Float.equal 0. (-0.)` is false. So `(0., 0.) = (-0., 0.)` is miscompiled into `false`

This equality is true in both ocaml 5.2.0 and Node (with (==)).

[vouillon]

I can look at this, but so far i’m confused:

Also Float.equal 0. (-0.) is false.

Testing in ocaml 5.2.0 yields true.

We have this definition in stdlib.ml, which is really confusing:

  let equal (a : float) (b : float) =
    Int64.equal (Int64.bits_of_float a) (Int64.bits_of_float b)

[OlivierNicole]

Yes, very!

[vouillon]

@vouillon, can you explain what's wrong with using Code.constant_equal in Flow.the_const_of ?

In Flow.the_const_of, we want to consider two constants the same if they cannot be distinguished. In particular, 0. and -0. are different.

On the other hand, we want that if Code.constant_equal a b = Some v, then Poly.(=) a b = v.

It is possible to define a function that satisfies both constraints by special-casing 0. and -0., but it seems simpler to use difference functions, especially since in the_const_of we only care about simple constants.

[OlivierNicole]

So (0., 0.) = (-0., 0.) is miscompiled into false

Regarding this, I suggest we stop shadowing the IEEE754-complying stdlib Float.equal with bitwise equality and call it bitwise_equal instead. This way we will use IEEE754-complying Float.equal in the rewritings and the problem should disappear.

[vouillon]

So (0., 0.) = (-0., 0.) is miscompiled into false

Regarding this, I suggest we stop shadowing the IEEE754-complying stdlib Float.equal with bitwise equality and call it bitwise_equal instead. This way we will use IEEE754-complying Float.equal in the rewritings and the problem should disappear.

I agree that we should rename this function. But Float.equal is not quite IEEE754-complying since Float.equal nan nan is true (but Poly.equal nan nan is false). So we should also review where it is used.

[OlivierNicole]

After checking, the only other place it is used is in compiler/lib/javascript.ml, in Num.of_float:

js_of_ocaml/compiler/lib/javascript.ml

Lines 106 to 128 in 3204c33

	let of_float v =
	match Float.classify_float v with
	| FP_nan -> "NaN"
	| FP_zero ->
	(* [1/-0] < 0. seems to be the only way to detect -0 in JavaScript *)
	if Float.(1. /. v < 0.) then "-0." else "0."
	| FP_infinite -> if Float.(v < 0.) then "-Infinity" else "Infinity"
	| FP_normal | FP_subnormal -> (
	let vint = int_of_float v in
	if Float.equal (float_of_int vint) v
	then Printf.sprintf "%d." vint
	else
	match
	find_smaller
	~f:(fun prec ->
	let s = float_to_string prec v in
	if Float.equal v (float_of_string s) then Some s else None)
	~bad:0
	~good:18
	~good_s:"max"
	with
	| "max" -> float_to_string 18 v |> fix_exponent
	| s -> fix_exponent s)

I’m not entirely sure what it does, but I expect we can adapt it if needed.

[vouillon]

After checking, the only other place it is used is in compiler/lib/javascript.ml, in Num.of_float:

[...]

I’m not entirely sure what it does, but I expect we can adapt it if needed.

NaN and zeroes have been excluded, so this is fine.

[OlivierNicole]

I have updated the comment, and attempted to fix the two compilation bugs that you have mentioned here.

Edit: I renamed a file from constant.ml to global_constant.ml because otherwise it clashed with the new module Code.Constant that I wanted to introduce.

[OlivierNicole]

I will also add regression tests

[OlivierNicole]

I have added a regression test.

[hhugo]

We have this definition in stdlib.ml, which is really confusing:

  let equal (a : float) (b : float) =
    Int64.equal (Int64.bits_of_float a) (Int64.bits_of_float b)

Maybe we should rename it Float.bits_equal and have Float.equal the one from the stdlib. I'd prefer to limit the use of Poly.equal as much as possible.

[dbuenzli]

We have this definition in stdlib.ml, which is really confusing:

If memory serves this is to make Float.equal compatible with Float.compare which is implemented as Stdlib.compare which does not have the IEEE 754 semantics on NaNs so that you have a total order on float values. That in turn makes things sound when you pass Float to a functor that expects an OrderedType signature ({Set,Map}.Make, etc.).

[hhugo]

We have this definition in stdlib.ml, which is really confusing:

If memory serves this is to make Float.equal compatible with Float.compare which is implemented as Stdlib.compare which does not have the IEEE 754 semantics on NaNs so that you have a total order on float values. That in turn makes things sound when you pass Float to a functor that expects an OrderedType signature ({Set,Map}.Make, etc.).

Thanks, I was indeed expecting equal to be the ieee one.