bigInt comparision by jmagaram · Pull Request #6097 · rescript-lang/rescript

jmagaram · 2023-03-26T05:12:37Z

Support bigInt comparison. Fix for #6096
Some comments and questions:

The different syntax compared to ReScript really was tough. I didn't realize there was a <> or =.
A CodeSpace might have been easier for setting up the environment. I got the error "No config found for file jscomp/runtime/caml_obj.ml. Try calling dune build. Compiler version mismatch: this project seems to be compiled with version 4.06 of the OCaml compiler, but the running ocamllsp supports OCaml version 4.14. OCaml language support will not work properly until this problem is fixed. And I couldn't figure out how to fix this. and so had lots of red squiggly errors though things seemed to compile and tests pass.
I wasn't sure if comparing different types is a legal operation in OCaml. So all the tests compare bigInt with bigInt even though in JavaScript you can compare bigInt with float.
In compare there is a clause about if b == Obj.repr Js.null || Caml_option.isNested b. I didn't understand this. Just added bigInt as another condition.
In equal did not understand the part about b_type = "number" || b_type = "bigint" || b_type = "undefined". Confused about why this isn't parallel to the checks on a_type that include string and boolean.
For my tests I couldn't access parts of the Obj module directly like lessthan - just could get to compare (global namespace somehow) and the operators. But my tests look similar to the other compare tests, so probably ok.
I didn't want to mess up the existing compare tests so I put mine in a separate file.
There is a lot of "number" in the code. So maybe other places need work to support BigInt? It was too much for me to get my head around.

cristianoc · 2023-03-26T06:29:33Z

This is a good opportunity to convert to .res syntax. Sorry that wasted some time.
bsc -format file.ml >file.res

Then the 2 functions that show up in the output, we don't want them. If you make them private they won't.

cristianoc · 2023-03-26T06:30:10Z

https://rescript-lang.org/docs/manual/latest/let-binding#private-let-bindings

cristianoc · 2023-03-26T06:40:52Z

I wasn't sure if comparing different types is a legal operation in OCaml. So all the tests compare bigInt with bigInt even though in JavaScript you can compare bigInt with float.

Once they're converted to ReScript this is less confusing. This is just a ReScript file, and we really only care about the generated JS, which goes into a library shipped with the compiler. So their idea is: whatever gives the desired JS is good.
Equivalently, tests can do whatever necessary so the generated .js code performs the desired test on the compiler output.

cristianoc · 2023-03-26T06:42:09Z

In general, we want to limit the number of tests in the compiler. For test running speed and maintenance reasons.
These would be a bit much for such a small change, but since there were so few tests about compare, it seems just about right to me.

cristianoc · 2023-03-26T06:46:39Z

In compare there is a clause about if b == Obj.repr Js.null || Caml_option.isNested b. I didn't understand this. Just added bigInt as another condition.

If such a condition is not already there for number and string, then it's very unlikely that it will be needed for bigint.

jmagaram · 2023-03-26T06:50:47Z

For the main file I converted to ReScript and did the same with the mli. Is that what you want? The resi doesn't mention the helper functions and neither did the mli so I don't know what needs to be private.

jmagaram · 2023-03-26T06:51:42Z

The test file could be res too. But it looks weird. This doesn't look like a LIST or an ARRAY. Not sure what it is supposed to be.

let suites: Mt.pair_suites = \"@"(
  isLessThan("123 and 555555", bigint("123"), bigint("555555")),
  \"@"(
    isEqual("98765 and 98765", bigint("98765"), bigint("98765")),
    isEqual("same instance", five, five),
  ),
)

Left this as-is after automatic conversion. Tests seem to run.

cristianoc

Left some small comments.
Not sure why the code generated by compare has changed so much, let me take a look and get back.

jscomp/runtime/caml_obj.ml

cristianoc · 2023-03-26T06:59:27Z

lib/es6/caml_obj.js

  }
  var a_type = typeof a;
  var b_type = typeof b;
+  var exit = 0;


There are all these exit now. Let me take a look at the changes in the source and see what might have caused this.

cristianoc · 2023-03-26T07:00:25Z

lib/es6/caml_obj.js

+
 function notequal(a, b) {
-  if (typeof a === "number" && typeof b === "number") {
+  if (canNumericCompare(a, b)) {


It's better inlined.
I suspect this will be inlined again once canNumericCompare is made a private function.

I'm not sure it is re-building. I changed it to == number or == string or == bigint but the javascript is the same. What command to build with rescript? make lib not doing it. I have the reScript extension on my computer and installed Rescript into package.json.

Need to go to sleep. Do you think I need tests related to comparing bigInt and float? In JavaScript you can type bigInt < 3.0 but I think ReScript prevents this unless you do an unsafe cast. Will look at this in the morning.

I can take it from here. Thanks for this!

cristianoc · 2023-03-26T07:10:38Z

The test file could be res too. But it looks weird. This doesn't look like a LIST or an ARRAY. Not sure what it is supposed to be.
let suites: Mt.pair_suites = \"@"(
  isLessThan("123 and 555555", bigint("123"), bigint("555555")),
  \"@"(
    isEqual("98765 and 98765", bigint("98765"), bigint("98765")),
    isEqual("same instance", five, five),
  ),
)
Left this as-is after automatic conversion. Tests seem to run.

I think it's OK. That's a custom infix operator. It's going to be defined somewhere.
One can just define it with a human function name instead.

cristianoc

You removed .ml but forgot to remove .mli

jscomp/runtime/caml_obj.res

cristianoc

OK found what was causing the code to change that much.
After this, it should be pretty much ready to merge.

Thanks for doing this!!!

cristianoc · 2023-03-26T07:20:58Z

jscomp/runtime/caml_obj.res

+    | ("bigint", "bigint")
+    | ("number", "number") =>
+      Pervasives.compare((Obj.magic(a): float), (Obj.magic(b): float))
+    | ("bigint", _) | ("number", _) =>


Can you remove this change?
The type system does not let us compare bigint and int.
The only reason to care about this is this change complicates the generated code.

Sorry not totally understanding. Do you want me to take out the bigint on this line? The check above I think is essential.

| ("bigint", "bigint") | ("number", "number") => Pervasives.compare((Obj.magic(a): float), (Obj.magic(b): float)) >>> | ("bigint", _) | ("number", _) => if b === Obj.repr(Js.null) || Caml_option.isNested(b) { 1 } else { /* Some (Some ..) < x */

cristianoc · 2023-03-26T07:21:06Z

jscomp/runtime/caml_obj.res

+        /* Some (Some ..) < x */
+        -1
+      } /* Integer < Block in OCaml runtime GPR #1195, except Some.. */
+    | (_, "bigint") | (_, "number") =>


And this line switch back to just "number"?

-1 } /* Integer < Block in OCaml runtime GPR #1195, except Some.. */ >>>> | (_, "bigint") | (_, "number") => if a === Obj.repr(Js.null) || Caml_option.isNested(a) { -1

cristianoc · 2023-03-26T07:23:22Z

A CodeSpace might have been easier for setting up the environment. I got the error "No config found for file jscomp/runtime/caml_obj.ml. Try calling dune build. Compiler version mismatch: this project seems to be compiled with version 4.06 of the OCaml compiler, but the running ocamllsp supports OCaml version 4.14. OCaml language support will not work properly until this problem is fixed. And I couldn't figure out how to fix this. and so had lots of red squiggly errors though things seemed to compile and tests pass.

I think if one follows the instructions in the contributing doc these issue should not arise.
If they do, feel free to clarify the contributing docs if it's missing something or it's unclear.

cristianoc · 2023-03-26T07:25:33Z

make lib is the command to update the library files

It's done automatically every time you do make test

The lib is built in a custom way because of... historical reasons and complications left in there.

…into pr/6097

cristianoc

Should be OK now.
I'll merge a little later.

jmagaram · 2023-03-26T07:41:43Z

make lib is the command to update the library files

It's done automatically every time you do make test

Sorry not working for me. After I mangle the isNumberOrBigInt function it still just checks for "number" or "bigint". Have tried make lib and make test.

I'm going to bed. You can take it from here if you want. Or I'll try in the morning.

jmagaram added 9 commits March 25, 2023 17:27

attempt at bigint compare

5927f25

bigint compare tests

ae78e7b

better tests

51671a8

do not compare float and bigint

914ca6b

more bigint compare

77033b3

add compare bigint to null ??

3b54efc

ugh didn't realize about <> and = operators

3fc523b

only compare similar types

64f42bf

change log

0cc4020

convert test and caml_obj to RES format

7edeea3

cristianoc reviewed Mar 26, 2023

View reviewed changes

remove duplication in switch

9bc88f1

cristianoc reviewed Mar 26, 2023

View reviewed changes

jscomp/runtime/caml_obj.res Outdated Show resolved Hide resolved

cristianoc requested changes Mar 26, 2023

View reviewed changes

jmagaram and others added 4 commits March 26, 2023 00:29

rename to @res.doc

7af0ef4

Teach the custom command for ninja to build .res files

7d2dacc

The lib is built in a custom way because of... historical reasons and complications left in there.

Merge branch 'master' of https://github.com/jmagaram/rescript-compiler …

b3919dc

…into pr/6097

remove big change

e1c6308

cristianoc approved these changes Mar 26, 2023

View reviewed changes

inline

2676f21

Convert the old test too.

653e7aa

cristianoc merged commit 8fbe1c1 into rescript-lang:master Mar 26, 2023

anmonteiro mentioned this pull request May 7, 2023

Apply upstream patches melange-re/melange#563

Merged

fhammerschmidt mentioned this pull request Oct 2, 2023

Document v11 features. rescript-lang/rescript-lang.org#697

Closed

4 tasks

Conversation

jmagaram commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristianoc commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristianoc commented Mar 26, 2023

Uh oh!

cristianoc commented Mar 26, 2023

Uh oh!

cristianoc commented Mar 26, 2023

Uh oh!

cristianoc commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagaram commented Mar 26, 2023

Uh oh!

jmagaram commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristianoc commented Mar 26, 2023

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristianoc commented Mar 26, 2023

Uh oh!

cristianoc commented Mar 26, 2023

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

jmagaram commented Mar 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jmagaram commented Mar 26, 2023 •

edited

Loading

cristianoc commented Mar 26, 2023 •

edited

Loading

cristianoc commented Mar 26, 2023 •

edited

Loading

jmagaram commented Mar 26, 2023 •

edited

Loading

jmagaram commented Mar 26, 2023 •

edited

Loading