Improve Caml_obj equal function to support errors #6937
Conversation
| if a_type == "function" || b_type == "function" { | ||
| raise(Invalid_argument("equal: functional value")) | ||
| } /* first, check using reference equality */ |
There was a problem hiding this comment.
Since we checked the functions using strict equal a === b, it doesn't make sense to raise an error at this point.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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`) { |
There was a problem hiding this comment.
The same, but shorter, faster, easier to maintain, and prevents from forgetting symbol and other types.
| /* a_type = "object" || "symbol" */ | ||
| b_type == "number" || (b_type == "bigint" || (b_type == "undefined" || b === %raw(`null`))) | ||
| ) { | ||
| if b_type !== "object" || b === %raw(`null`) { |
There was a problem hiding this comment.
A good example of forgetting checking various types
| 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")) |
| if tag_a !== tag_b { | ||
| false |
There was a problem hiding this comment.
I'm not sure of the usefulness of the optimisation 🤔
But let's keep it.
| } else if O.isArray(a) { | ||
| let len_a = Obj.size(a) | ||
| let len_b = Obj.size(b) |
There was a problem hiding this comment.
Length check makes sense only for arrays, so moved it inside of the if O.isArray(a) block
| } else if %raw(`a instanceof Date`) { | ||
| if %raw(`b instanceof Date`) { |
There was a problem hiding this comment.
Split the check so it earlier returns false when b is not an instance of Date without running some unexpected code.
| false | ||
| } | ||
| } else { | ||
| aux_obj_equal(a, b) |
There was a problem hiding this comment.
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.
jscomp/runtime/caml_obj.res
Outdated
| } 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"]) | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
for in doesn't work on Error, so have a special case for them.
DZakh
force-pushed
the
make-obj-equal-support-errors
branch
from
1c5b0bf to
c912856
Compare
|
Can't find the convo anymore. The other way is not perfect either Just saying: there are trade offs and one just needs to decide, with no clear superior choice |
After the change |
|
I don't see a real downside to the function equality change either. We need to document it as a (potentially) breaking change, but I think it is unlikely in practice that people are using this with functions and relying on an exception being thrown in that case. |
Agree. I didn't include it in changelog, since I can't imagine a case when somebody can rely on it |
There was a problem hiding this comment.
Looks good.
I remember there are some corner case tests in rescript-core that would make sense to check.
@zth do you remember?
|
@cristianoc @zth I ran the tests with my changes and it fails. But that's because of the exns changes I'm fixing right now. In other words, It's not working with rescript-12-alpha 
|
|
I missed here.. What are the use cases of the equality check for Error objects and its messages? Can you add some examples or tests here? I see... so this is just for completeness |
And improve performance