From d0f971479f1d253b9a2340d4d9083597883d92ba Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Thu, 3 May 2018 13:29:23 +0200 Subject: [PATCH 1/3] Fix polymorphic compare on nullables. Polymorphic compare could crash when the second argument is null. --- jscomp/runtime/caml_obj.ml | 3 ++- jscomp/test/caml_compare_test.js | 30 +++++++++++++++++++++++++++++- jscomp/test/caml_compare_test.ml | 8 ++++++++ lib/js/caml_obj.js | 6 +++++- 4 files changed, 44 insertions(+), 3 deletions(-) diff --git a/jscomp/runtime/caml_obj.ml b/jscomp/runtime/caml_obj.ml index 8ecc9fe76a..ef246a6a7b 100644 --- a/jscomp/runtime/caml_obj.ml +++ b/jscomp/runtime/caml_obj.ml @@ -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 let a_type = Js.typeof a in let b_type = Js.typeof b in if a_type = "string" then @@ -182,7 +184,6 @@ let rec caml_compare (a : Obj.t) (b : Obj.t) : int = | false, false -> if a_type = "boolean" || a_type = "undefined" - || a == (Obj.repr Js_null.empty) then (* TODO: refine semantics when comparing with [null] *) unsafe_js_compare a b else if a_type = "function" || b_type = "function" diff --git a/jscomp/test/caml_compare_test.js b/jscomp/test/caml_compare_test.js index c777c9d755..52d18e1f74 100644 --- a/jscomp/test/caml_compare_test.js +++ b/jscomp/test/caml_compare_test.js @@ -883,7 +883,35 @@ var suites_001 = /* :: */[ ]); }) ], - /* [] */0 + /* :: */[ + /* tuple */[ + "File \"caml_compare_test.ml\", line 81, characters 4-11", + (function () { + return /* Eq */Block.__(0, [ + Caml_obj.caml_compare(null, /* :: */[ + 3, + /* [] */0 + ]), + -1 + ]); + }) + ], + /* :: */[ + /* tuple */[ + "File \"caml_compare_test.ml\", line 84, characters 4-11", + (function () { + return /* Eq */Block.__(0, [ + Caml_obj.caml_compare(/* :: */[ + 3, + /* [] */0 + ], null), + 1 + ]); + }) + ], + /* [] */0 + ] + ] ] ] ] diff --git a/jscomp/test/caml_compare_test.ml b/jscomp/test/caml_compare_test.ml index 3c06fff916..eefa56c8fc 100644 --- a/jscomp/test/caml_compare_test.ml +++ b/jscomp/test/caml_compare_test.ml @@ -77,6 +77,14 @@ let suites = Mt.[ "eq_in_list2", (fun _ -> Eq ([[%bs.obj {x=2}]] = [[%bs.obj {x=2}]], true)); "eq_with_list", (fun _ -> Eq ([%bs.obj {x=[0]}] = [%bs.obj {x=[0]}], true)); "eq_with_list2", (fun _ -> Eq ([%bs.obj {x=[0]}] = [%bs.obj {x=[1]}], false)); + + __LOC__ , begin fun _ -> + Eq(compare Js.null (Js.Null.return [3]), -1) + end; + __LOC__ , begin fun _ -> + Eq(compare (Js.Null.return [3]) Js.null, 1) + end; + ] ;; diff --git a/lib/js/caml_obj.js b/lib/js/caml_obj.js index 3b6eb60d8d..16ff296f7c 100644 --- a/lib/js/caml_obj.js +++ b/lib/js/caml_obj.js @@ -66,6 +66,10 @@ function caml_compare(_a, _b) { var a = _a; if (a === b) { return 0; + } else if (a === null) { + return -1; + } else if (b === null) { + return 1; } else { var a_type = typeof a; var b_type = typeof b; @@ -82,7 +86,7 @@ function caml_compare(_a, _b) { } } else if (is_b_number) { return 1; - } else if (a_type === "boolean" || a_type === "undefined" || a === null) { + } else if (a_type === "boolean" || a_type === "undefined") { var x = a; var y = b; if (x === y) { From e18fd34fd870381eef6b49f17062f330aa3be3ce Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Mon, 7 May 2018 10:15:01 +0200 Subject: [PATCH 2/3] Add comparison for undefined, and tests. --- jscomp/runtime/caml_obj.ml | 6 +++-- jscomp/test/caml_compare_test.js | 46 +++++++++++++++++++++++++++++++- jscomp/test/caml_compare_test.ml | 13 ++++++++- lib/js/caml_obj.js | 4 +++ 4 files changed, 65 insertions(+), 4 deletions(-) diff --git a/jscomp/runtime/caml_obj.ml b/jscomp/runtime/caml_obj.ml index ef246a6a7b..c8a93d5a9d 100644 --- a/jscomp/runtime/caml_obj.ml +++ b/jscomp/runtime/caml_obj.ml @@ -167,8 +167,10 @@ 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 + if a == (Obj.repr Js.null) then -1 else + if b == (Obj.repr Js.null) then 1 else + if a == (Obj.repr Js.undefined) then -1 else + if b == (Obj.repr Js.undefined) then 1 else let a_type = Js.typeof a in let b_type = Js.typeof b in if a_type = "string" then diff --git a/jscomp/test/caml_compare_test.js b/jscomp/test/caml_compare_test.js index 52d18e1f74..98cabd5474 100644 --- a/jscomp/test/caml_compare_test.js +++ b/jscomp/test/caml_compare_test.js @@ -909,7 +909,51 @@ var suites_001 = /* :: */[ ]); }) ], - /* [] */0 + /* :: */[ + /* tuple */[ + "File \"caml_compare_test.ml\", line 87, characters 4-11", + (function () { + return /* Eq */Block.__(0, [ + Caml_obj.caml_compare(null, 0), + -1 + ]); + }) + ], + /* :: */[ + /* tuple */[ + "File \"caml_compare_test.ml\", line 90, characters 4-11", + (function () { + return /* Eq */Block.__(0, [ + Caml_obj.caml_compare(0, null), + 1 + ]); + }) + ], + /* :: */[ + /* tuple */[ + "File \"caml_compare_test.ml\", line 93, characters 4-11", + (function () { + return /* Eq */Block.__(0, [ + Caml_obj.caml_compare(undefined, 0), + -1 + ]); + }) + ], + /* :: */[ + /* tuple */[ + "File \"caml_compare_test.ml\", line 96, characters 4-11", + (function () { + return /* Eq */Block.__(0, [ + Caml_obj.caml_compare(0, undefined), + 1 + ]); + }) + ], + /* [] */0 + ] + ] + ] + ] ] ] ] diff --git a/jscomp/test/caml_compare_test.ml b/jscomp/test/caml_compare_test.ml index eefa56c8fc..d284900b59 100644 --- a/jscomp/test/caml_compare_test.ml +++ b/jscomp/test/caml_compare_test.ml @@ -84,7 +84,18 @@ let suites = Mt.[ __LOC__ , begin fun _ -> Eq(compare (Js.Null.return [3]) Js.null, 1) end; - + __LOC__ , begin fun _ -> + Eq(compare Js.null (Js.Null.return 0), -1) + end; + __LOC__ , begin fun _ -> + Eq(compare (Js.Null.return 0) Js.null, 1) + end; + __LOC__ , begin fun _ -> + Eq(compare Js.Nullable.undefined (Js.Nullable.return 0), -1) + end; + __LOC__ , begin fun _ -> + Eq(compare (Js.Nullable.return 0) Js.Nullable.undefined, 1) + end; ] ;; diff --git a/lib/js/caml_obj.js b/lib/js/caml_obj.js index 16ff296f7c..c2dbb5d15e 100644 --- a/lib/js/caml_obj.js +++ b/lib/js/caml_obj.js @@ -70,6 +70,10 @@ function caml_compare(_a, _b) { return -1; } else if (b === null) { return 1; + } else if (a === undefined) { + return -1; + } else if (b === undefined) { + return 1; } else { var a_type = typeof a; var b_type = typeof b; From 13e8fa57f467864a80fe2d0363a07f8e9f58f64f Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Mon, 7 May 2018 10:18:22 +0200 Subject: [PATCH 3/3] Remove check for type undefined, which is now redundant. --- jscomp/runtime/caml_obj.ml | 1 - lib/js/caml_obj.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/jscomp/runtime/caml_obj.ml b/jscomp/runtime/caml_obj.ml index c8a93d5a9d..acf0386362 100644 --- a/jscomp/runtime/caml_obj.ml +++ b/jscomp/runtime/caml_obj.ml @@ -185,7 +185,6 @@ let rec caml_compare (a : Obj.t) (b : Obj.t) : int = | false, true -> 1 | false, false -> if a_type = "boolean" - || a_type = "undefined" then (* TODO: refine semantics when comparing with [null] *) unsafe_js_compare a b else if a_type = "function" || b_type = "function" diff --git a/lib/js/caml_obj.js b/lib/js/caml_obj.js index c2dbb5d15e..bc547b12c4 100644 --- a/lib/js/caml_obj.js +++ b/lib/js/caml_obj.js @@ -90,7 +90,7 @@ function caml_compare(_a, _b) { } } else if (is_b_number) { return 1; - } else if (a_type === "boolean" || a_type === "undefined") { + } else if (a_type === "boolean") { var x = a; var y = b; if (x === y) {