Skip to content

docs: add docs for equality and comparison #801

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.

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

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

jderochervlk
Copy link
Collaborator

Copy link

vercel bot commented Jan 23, 2024

@jderochervlk is attempting to deploy a commit to the ReScript Association Team on Vercel.

A member of the Team first needs to authorize it.

@fhammerschmidt
Copy link
Member

@ryyppy Will merge it if you think it's good to go.

@cristianoc
Copy link
Contributor

Small comments: when mentioning other deep comparison alternatives, it's not immediately clear that they behave the same. One would have to check as the semantics might be different in corner cases.

@jderochervlk
Copy link
Collaborator Author

Small comments: when mentioning other deep comparison alternatives, it's not immediately clear that they behave the same. One would have to check as the semantics might be different in corner cases.

Should those details be included on the docs page, or should I keep the specifics on the perf repo? I can setup some unit tests on the perf repo to make sure they act the same.

@cristianoc
Copy link
Contributor

It would be good to know if they behave the same. This is just one-off knowledge so I don't think we need unit tests.

@jderochervlk
Copy link
Collaborator Author

I re-worded some of the text around performance after running some more performance benchmarks to cover different cases.

  1. when we know the objects don't match
  2. when the objects are the same object equals(a, a)
  3. When the objects are both copy of the same object equals(Object.assign({}, a), Object.assign({})

With the additional runs it leaves fast-deep-compare as the leader in all cases. ReScript 2x over lodash for small objects with a single key, and lodash is slightly faster than rescript for large objects.

Performance of runtime equality checks

The runtime equality check ReScript uses is quite fast and should be adequate for almost all use cases.
For small objects it can be 2x times faster than alternative deep compare functions such as Lodash's _.isEqual.

For larger objects instead of using == you could manually use a faster alternative such as fast-deep-compare, or write a custom comparator function.

This repo has benchmarks comparing results of different libraries compared to ReScript's built in equality function.

@jderochervlk
Copy link
Collaborator Author

jderochervlk commented Jan 27, 2024

It would be good to know if they behave the same. This is just one-off knowledge so I don't think we need unit tests.

I've been checking using different randomly generated JSON data and I keep getting equivalent results for each library. To really hunt for edge cases I could use fast-check to make some property based tests to throw at it and see if they all behave the same.

@cristianoc
Copy link
Contributor

It would be good to know if they behave the same. This is just one-off knowledge so I don't think we need unit tests.

I've been checking using different randomly generated JSON data and I keep getting equivalent results for each library. To really hunt for edge cases I could use fast-check to make some property based tests to throw at it and see if they all behave the same.

@glennsl can you think of corner cases from the tests on math or other inequalities?

Copy link
Contributor

@glennsl glennsl left a comment

Choose a reason for hiding this comment

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

Nice docs!

A couple of things not addressed here, though, is the behaviour when comparing functions, generic, abstract and private types. More generally, the idea that the output is determined by which type is inferred, and what's known about that type, rather than the underlying data type or value, might not be entirely obvious coming from a dynamic language.

@jderochervlk
Copy link
Collaborator Author

Comparing functions will throw a runtime error, but will not show a compiler error.

let fn = () => ()
let f2 = () => ()

try {
  Console.log(fn == f2)
} catch {
| err => Console.error(err)
}
{
  RE_EXN_ID: "Invalid_argument",
  _1: "equal: functional value",
  Error: 269 |   var b_type = typeof b;
270 |   if (a_type === "function" || b_type === "function") {
271 |     throw {
272 |           RE_EXN_ID: "Invalid_argument",
273 |           _1: "equal: functional value",
274 |           Error: new Error()
                     ^

Would it be possible to have the compiler show an error when trying to use == with functions?

@jderochervlk
Copy link
Collaborator Author

jderochervlk commented Jan 29, 2024

Another part that throws is this section, which I am not sure what it means.

var tag_a = a.TAG;
var tag_b = b.TAG;
if (tag_a === 248) {
  return a[1] === b[1];
}
if (tag_a === 251) {
  throw {
    RE_EXN_ID: "Invalid_argument",
    _1: "equal: abstract value",
    Error: new Error()
  };
}

a and b are the 2 arguments passed to the equal function.

Searching for 248 and 251 doesn't find anything in the rescript folder in node_modules.

@glennsl
Copy link
Contributor

glennsl commented Jan 29, 2024

Would it be possible to have the compiler show an error when trying to use == with functions?

Not in general, unless you add something like type classes to the type system. Otherwise there's no type that can accept anything except functions.

Searching for 248 and 251 doesn't find anything in the rescript folder in node_modules.

Those are the tags for OCaml objects and abstract data types. I don't think either are officially supported by ReScript, so you can probably just ignore them.

@ryyppy ryyppy marked this pull request as ready for review January 30, 2024 21:14
@ryyppy ryyppy marked this pull request as draft January 30, 2024 21:15
Copy link
Member

@ryyppy ryyppy left a comment

Choose a reason for hiding this comment

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

This PR captures all the relevant details outlined in the original issue, so from my POV we could merge this and commit further refinements as we go.

Good job on the writing and capturing performance characteristics!

@jderochervlk jderochervlk marked this pull request as ready for review January 30, 2024 21:55
@jderochervlk
Copy link
Collaborator Author

This PR captures all the relevant details outlined in the original issue, so from my POV we could merge this and commit further refinements as we go.

Good job on the writing and capturing performance characteristics!

Thanks! It's ready to merge.

@zth zth merged commit 023dfd4 into rescript-lang:master Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants