Fix polymorphic compare on nullables. #2786
Conversation
Polymorphic compare could crash when the second argument is null.
jscomp/runtime/caml_obj.ml
Outdated
| @@ -167,6 +167,8 @@ let unsafe_js_compare x y = | |||
| let rec caml_compare (a : Obj.t) (b : Obj.t) : int = | |||
| if a == b then 0 else | |||
| (*front and formoest, we do not compare function values*) | |||
| if a == (Obj.repr Js_null.empty) then -1 else | |||
| if b == (Obj.repr Js_null.empty) then 1 else | |||
There was a problem hiding this comment.
The a = null is caught, would not the case when b = null be moved to line 192 ?
There was a problem hiding this comment.
That's where I had it before. I've moved it here because caml_int_compare and caml_string_compare give inconsistent results when comparing with null.
There was a problem hiding this comment.
E.g. caml_int_compare(-1,null) == -1 but caml_int_compare(1,null) == 1.
There was a problem hiding this comment.
I am a little confused why it is weird? -1 < null < 1?
Edit: maybe this is what you mean caml_int_compare (0, null) and caml_int_compare(null,0) is not consistent? shall we add such test case
There was a problem hiding this comment.
For the record: line 175 seems fishy too
since string and null/undefined can have the same type: string Js.null, maybe we should add undefined check as well.
if a == null then -1 else
if b == null then 1 else
if a == undefined then -1 else
if b == undefined then 1 elseassuming null < undefined
|
@cristianoc assuming |
|
Good point now that we prioritize undefined. Yes it should not break anything. |
Polymorphic compare could crash when the second argument is null, as it would try to read the tag of null.
Added test where previously it would crash.