Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve Caml_obj equal function to support errors #6937

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Improve Caml_obj equal function to support errors #6937

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a95869
Improve Caml_obj equal function to support errors
DZakh Aug 4, 2024
de469bf
Fix tests
DZakh Aug 4, 2024
75a616e
Update changelog
DZakh Aug 4, 2024
e06248a
Revert changelog formatting
DZakh Aug 7, 2024
c912856
Fix caml_obj after review
DZakh Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions jscomp/core/js_exp_make.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,6 @@ let eight_int_literal : t =
let nine_int_literal : t =
{ expression_desc = Number (Int { i = 9l; c = None }); comment = None }

let obj_int_tag_literal : t =
{ expression_desc = Number (Int { i = 248l; c = None }); comment = None }

let int ?comment ?c i : t = { expression_desc = Number (Int { i; c }); comment }

let bigint ?comment sign i : t = { expression_desc = Number (BigInt {positive=sign; value=i}); comment}
Expand All @@ -330,7 +327,6 @@ let small_int i : t =
| 7 -> seven_int_literal
| 8 -> eight_int_literal
| 9 -> nine_int_literal
| 248 -> obj_int_tag_literal
| i -> int (Int32.of_int i)

let true_ : t = { comment = None; expression_desc = Bool true }
Expand Down
1 change: 0 additions & 1 deletion jscomp/core/js_exp_make.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ val zero_int_literal : t

(* val one_int_literal : t *)
val zero_float_lit : t
(* val obj_int_tag_literal : t *)

val zero_bigint_literal : t

Expand Down
24 changes: 10 additions & 14 deletions jscomp/runtime/caml_hash.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,22 +117,18 @@ let hash = (count: int, _limit, seed: int, obj: Obj.t): int => {
if size != 0 {
let obj_tag = Obj.tag(obj)
let tag = lor(lsl(size, 10), obj_tag)
if obj_tag == 248 /* Obj.object_tag */ {
s.contents = hash_mix_int(s.contents, (Obj.obj(Obj.field(obj, 1)): int))
} else {
s.contents = hash_mix_int(s.contents, tag)
let block = {
let v = size - 1
if v < num.contents {
v
} else {
num.contents
}
}
for i in 0 to block {
push_back(queue, Obj.field(obj, i))
s.contents = hash_mix_int(s.contents, tag)
let block = {
let v = size - 1
if v < num.contents {
v
} else {
num.contents
}
}
for i in 0 to block {
push_back(queue, Obj.field(obj, i))
}
} else {
let size: int = %raw(`function(obj,cb){
var size = 0
Expand Down
57 changes: 23 additions & 34 deletions jscomp/runtime/caml_obj.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,7 @@ let rec compare = (a: Obj.t, b: Obj.t): int =>
} else {
let tag_a = Obj.tag(a)
let tag_b = Obj.tag(b)
if tag_a == 248 /* object/exception */ {
Pervasives.compare((Obj.magic(Obj.field(a, 1)): int), Obj.magic(Obj.field(b, 1)))
} else if tag_a == 251 /* abstract_tag */ {
raise(Invalid_argument("equal: abstract value"))
} else if tag_a != tag_b {
if tag_a != tag_b {
if tag_a < tag_b {
-1
} else {
Expand Down Expand Up @@ -303,53 +299,46 @@ type eq = (Obj.t, Obj.t) => bool
basic type is not the same, it will not equal
*/
let rec equal = (a: Obj.t, b: Obj.t): bool =>
/* front and formoest, we do not compare function values */
if a === b {
true
} else {
let a_type = Js.typeof(a)
if (
a_type == "string" ||
(a_type == "number" ||
(a_type == "bigint" ||
(a_type == "boolean" ||
(a_type == "undefined" || a === %raw(`null`)))))
) {
if a_type !== "object" || a === %raw(`null`) {
Comment on lines -311 to +306
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same, but shorter, faster, easier to maintain, and prevents from forgetting symbol and other types.

false
} else {
let b_type = Js.typeof(b)
if a_type == "function" || b_type == "function" {
raise(Invalid_argument("equal: functional value"))
} /* first, check using reference equality */
Comment on lines -321 to -323
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we checked the functions using strict equal a === b, it doesn't make sense to raise an error at this point.

Copy link
Collaborator

@cristianoc cristianoc Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?
The error says you cannot compare functional values. (Unless they are identical, in which case you can).
We might change the stance on this, but both choices are valid design choices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think according to the funciton name equal it makes sense that when you compare two non-identical function values, it returns false. Also, it improves performance for non function values, since there's no need to specifically check for it.

else if (
/* a_type = "object" || "symbol" */
b_type == "number" || (b_type == "bigint" || (b_type == "undefined" || b === %raw(`null`)))
) {
if b_type !== "object" || b === %raw(`null`) {
Comment on lines -325 to +310
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good example of forgetting checking various types

false
} else {
/* [a] [b] could not be null, so it can not raise */
let tag_a = Obj.tag(a)
let tag_b = Obj.tag(b)
if tag_a == 248 /* object/exception */ {
Obj.magic(Obj.field(a, 1)) === Obj.magic(Obj.field(b, 1))
} else if tag_a == 251 /* abstract_tag */ {
raise(Invalid_argument("equal: abstract value"))
Comment on lines -333 to -336
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed legacy check

} else if tag_a != tag_b {
if tag_a !== tag_b {
false
Comment on lines +316 to 317
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure of the usefulness of the optimisation 🤔

But let's keep it.

} else {
} else if O.isArray(a) {
let len_a = Obj.size(a)
let len_b = Obj.size(b)
Comment on lines +318 to 320
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Length check makes sense only for arrays, so moved it inside of the if O.isArray(a) block

if len_a == len_b {
if O.isArray(a) {
aux_equal_length((Obj.magic(a): array<Obj.t>), (Obj.magic(b): array<Obj.t>), 0, len_a)
} else if %raw(`a instanceof Date && b instanceof Date`) {
!(Js.unsafe_gt(a, b) || Js.unsafe_lt(a, b))
} else {
aux_obj_equal(a, b)
}
if len_a !== len_b {
false
} else {
aux_equal_length((Obj.magic(a): array<Obj.t>), (Obj.magic(b): array<Obj.t>), 0, len_a)
}
} else if %raw(`a instanceof Error`) {
let a: {..} = Obj.magic(a)
let b: {..} = Obj.magic(b)
if %raw(`b instanceof Error`) && a["message"] === b["message"] {
equal(a["clause"], b["clause"])
cknitt marked this conversation as resolved.
Show resolved Hide resolved
} else {
false
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for in doesn't work on Error, so have a special case for them.

} else if %raw(`a instanceof Date`) {
if %raw(`b instanceof Date`) {
Comment on lines +334 to +335
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split the check so it earlier returns false when b is not an instance of Date without running some unexpected code.

!(Js.unsafe_gt(a, b) || Js.unsafe_lt(a, b))
} else {
false
}
} else {
aux_obj_equal(a, b)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a look at the generated code for the function, and it's not very optimised. Not the goal of the PR, so kept it as it is.

}
}
}
Expand Down
14 changes: 5 additions & 9 deletions lib/es6/caml_hash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,11 @@ function hash(count, _limit, seed, obj) {
if (size !== 0) {
let obj_tag = obj$1.TAG;
let tag = (size << 10) | obj_tag;
if (obj_tag === 248) {
s = Caml_hash_primitive.hash_mix_int(s, obj$1[1]);
} else {
s = Caml_hash_primitive.hash_mix_int(s, tag);
let v = size - 1 | 0;
let block = v < num ? v : num;
for (let i = 0; i <= block; ++i) {
push_back(queue, obj$1[i]);
}
s = Caml_hash_primitive.hash_mix_int(s, tag);
let v = size - 1 | 0;
let block = v < num ? v : num;
for (let i = 0; i <= block; ++i) {
push_back(queue, obj$1[i]);
}
} else {
let size$1 = (function(obj,cb){
Expand Down
159 changes: 73 additions & 86 deletions lib/es6/caml_obj.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,17 +132,6 @@ function compare(a, b) {
}
let tag_a = a.TAG;
let tag_b = b.TAG;
if (tag_a === 248) {
return Caml.int_compare(a[1], b[1]);
}
if (tag_a === 251) {
throw new Error("Invalid_argument", {
cause: {
RE_EXN_ID: "Invalid_argument",
_1: "equal: abstract value"
}
});
}
if (tag_a !== tag_b) {
if (tag_a < tag_b) {
return -1;
Expand Down Expand Up @@ -256,87 +245,85 @@ function aux_obj_compare(a, b) {
}
}

function equal(a, b) {
if (a === b) {
return true;
}
let a_type = typeof a;
if (a_type === "string" || a_type === "number" || a_type === "bigint" || a_type === "boolean" || a_type === "undefined" || a === null) {
return false;
}
let b_type = typeof b;
if (a_type === "function" || b_type === "function") {
throw new Error("Invalid_argument", {
cause: {
RE_EXN_ID: "Invalid_argument",
_1: "equal: functional value"
}
});
}
if (b_type === "number" || b_type === "bigint" || b_type === "undefined" || b === null) {
return false;
}
let tag_a = a.TAG;
let tag_b = b.TAG;
if (tag_a === 248) {
return a[1] === b[1];
}
if (tag_a === 251) {
throw new Error("Invalid_argument", {
cause: {
RE_EXN_ID: "Invalid_argument",
_1: "equal: abstract value"
}
});
}
if (tag_a !== tag_b) {
return false;
}
let len_a = a.length | 0;
let len_b = b.length | 0;
if (len_a === len_b) {
function equal(_a, _b) {
while (true) {
let b = _b;
let a = _a;
if (a === b) {
return true;
}
let a_type = typeof a;
if (a_type !== "object" || a === null) {
return false;
}
let b_type = typeof b;
if (b_type !== "object" || b === null) {
return false;
}
let tag_a = a.TAG;
let tag_b = b.TAG;
if (tag_a !== tag_b) {
return false;
}
if (Array.isArray(a)) {
let _i = 0;
while (true) {
let i = _i;
if (i === len_a) {
return true;
}
if (!equal(a[i], b[i])) {
let len_a = a.length | 0;
let len_b = b.length | 0;
if (len_a !== len_b) {
return false;
} else {
let _i = 0;
while (true) {
let i = _i;
if (i === len_a) {
return true;
}
if (!equal(a[i], b[i])) {
return false;
}
_i = i + 1 | 0;
continue;
};
}
}
if (!(a instanceof Error)) {
if ((a instanceof Date)) {
if ((b instanceof Date)) {
return !(a > b || a < b);
} else {
return false;
}
_i = i + 1 | 0;
continue;
};
} else if ((a instanceof Date && b instanceof Date)) {
return !(a > b || a < b);
} else {
let result = {
contents: true
};
let do_key_a = function (key) {
if (!Object.prototype.hasOwnProperty.call(b, key)) {
result.contents = false;
return;
}

};
let do_key_b = function (key) {
if (!Object.prototype.hasOwnProperty.call(a, key) || !equal(b[key], a[key])) {
result.contents = false;
return;
} else {
let result = {
contents: true
};
let do_key_a = function (key) {
if (!Object.prototype.hasOwnProperty.call(b, key)) {
result.contents = false;
return;
}

};
let do_key_b = function (key) {
if (!Object.prototype.hasOwnProperty.call(a, key) || !equal(b[key], a[key])) {
result.contents = false;
return;
}

};
for_in(a, do_key_a);
if (result.contents) {
for_in(b, do_key_b);
}

};
for_in(a, do_key_a);
if (result.contents) {
for_in(b, do_key_b);
return result.contents;
}
return result.contents;
}
} else {
return false;
}
if (!((b instanceof Error) && a.message === b.message)) {
return false;
}
_b = b.clause;
_a = a.clause;
continue;
};
}

function equal_null(x, y) {
Expand Down
14 changes: 5 additions & 9 deletions lib/js/caml_hash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,11 @@ function hash(count, _limit, seed, obj) {
if (size !== 0) {
let obj_tag = obj$1.TAG;
let tag = (size << 10) | obj_tag;
if (obj_tag === 248) {
s = Caml_hash_primitive.hash_mix_int(s, obj$1[1]);
} else {
s = Caml_hash_primitive.hash_mix_int(s, tag);
let v = size - 1 | 0;
let block = v < num ? v : num;
for (let i = 0; i <= block; ++i) {
push_back(queue, obj$1[i]);
}
s = Caml_hash_primitive.hash_mix_int(s, tag);
let v = size - 1 | 0;
let block = v < num ? v : num;
for (let i = 0; i <= block; ++i) {
push_back(queue, obj$1[i]);
}
} else {
let size$1 = (function(obj,cb){
Expand Down
Loading
Loading