Unboxed variants stdlib types #6218

zth · 2023-04-27T08:18:28Z

This changes 3 type definitions in the stdlib:

Js.Json.t is now an untagged variant representing valid JSON
Js.nullable is now an untagged variant representing Value('a) | Null | Undefined
Js.null is now an untagged variant representing Value('a) | Null

I'm trying to do as little as possible here, and the rest will be done in Core. But, in order to be able to make using these types smooth, I'd like the source definitions to be in here so it can be reused everywhere.

I've opted to only do nullable in the global Js scope of all of the nullable types (null, undefined etc). I'm very much open to feedback here, but my reasoning is that that's the most important one, and having multiple definitions with Value/Null/Undefined constructors is going to create a headache inference wise. Choosing just one should let us expose it in the global scope (which will be very useful for interop) without causing headaches.

Changelog coming as soon as we agree this is a good plan.

cc @cristianoc

zth · 2023-04-27T08:31:20Z

CI seems to be failing but I don't really understand why.

cristianoc · 2023-04-27T09:30:15Z

There are 2 js.ml files, you need to change both.
Why are there 2 js files. That's a great question for another day.

zth · 2023-04-27T11:30:17Z

@cristianoc fixed!

zth · 2023-04-27T12:07:10Z

Running locally against Core here to ensure things work as intended: #6218

mununki · 2023-04-27T12:36:19Z

jscomp/others/js_json.mli

+type t =
+  | False [@as false]
+  | True [@as true]
+  | Null [@as null]
+  | String of string
+  | Number of float
+  | Object of t Js.Dict.t
+  | Array of t array
+[@@unboxed]


Shouldn't it be kept as an opaque type?

I think it's better to define a type as an opaque type, hiding its internal structure, so that values can only be manipulated using the functions provided by the module's interface, like the previous Js.Json.t.

What's the advantage of hiding the fact that json is json?
Do you want to not rely on the fact that json is a standard? Perhaps there's some abstraction that's useful in some way? Not sure I get it. But curious about what's the thought.

I'm also curious about why. I only see upsides, but maybe I'm missing something @mununki .

Here is another example: circular reference object

let circularReferencingObj: Js.Json.t = Js.Dict.( // making circular referencing object ... )->Js.Json.Object

While true, those type of problems are not new, not introduced by this change, and it's not something making the type abstract magically fixes unless we restrict it to only allowing JSON.parse to return a JSON.t, which seems unpractical.

For example, there are a bunch of functions like string: string => t and object_: Js.Dict.t<t> => t in the JSON module right now that operates on the abstract type, but that has the same problem that you're describing, since they're just identity functions.

I think we have exhausted the exploration here. Merging.

While I respect the maintainer's decision and the fact that it's been merged, I still don't think the current situation justifies using non-opaque type for Js.Json.t. Rather, it might be possible to justify the existing functions due to the decision to use non-opaque types.

there are a bunch of functions like string: string => t and object_: Js.Dict.t => t in the JSON module right now that operate on the abstract type, but they have the same problem that you're describing, since they're just identity functions.

Not sure what's the issue here.
What I see is a very talented contributor who has shipped awesome features executed extremely well: jsx4 and dynamic loading.
This issue does not seem very important.

jscomp/others/js.ml

cristianoc · 2023-04-27T23:23:24Z

jscomp/others/js.ml

-type +'a nullable
+type +'a nullable =
+  | Value of 'a
+  | Null [@as null]


In the Js scope there are 2 things called Value and Null. And Js.Null is the last one listed.
Just pointing this out, and see if that's OK, and if the order is the desired one.
I guess perhaps Core will take care of being the entry point of these and this stuff in Js will only be a "storage" of type definitions not used directly?

My logic is:

Expose them all in the Js namespace. This would become problematic inference wise only for users who do open Js and who start using the new representation.

In Core (which is the future), only expose Nullable.t constructors in the global scope, not Null.t. That way, no clashes with inference will happen there.

The reasoning for 1 is that I want to ensure types are "linked together" correctly, which I assume means I need to duplicate the constructor definitions everywhere people might be using the type for things to line up and be compatible in Core. But maybe this is incorrect?

cristianoc · 2023-04-27T23:35:43Z

jscomp/test/js_json_test.js

@@ -81,7 +81,7 @@ add_test("File \"js_json_test.res\", line 22, characters 11-18", (function (para
                  _0: false
                };
        }
-        var ty2 = Js_json.classify(Caml_option.valFromOption(v$1));
+        var ty2 = Js_json.classify(v$1);


This difference means the compiler now unboxes the optional.
Well OK it makes assumptions about values of json type being stored unboxed (in the sense of optionals).
While correct in this case, I think it's incidental. And the option unboxing mechanism needs to be taught about untagged variants.
I will investigate in a separate issue. No need to worry about that here.

Good catch!

zth requested a review from cristianoc April 27, 2023 08:18

zth mentioned this pull request Apr 27, 2023

[WIP] Rescript v11 rescript-lang/rescript-core#135

Closed

mununki reviewed Apr 27, 2023

View reviewed changes

cristianoc reviewed Apr 27, 2023

View reviewed changes

jscomp/others/js.ml Outdated Show resolved Hide resolved

zth added 5 commits April 27, 2023 21:28

untagged variant for Json.t

b5ff62b

untagged variant for nullable

f366ee8

copy change to runtime js file

0d3c199

duplicate definition of Nullable.t in more places as needed

1171f4c

Present -> Value, and add equivalent unboxed variant type for Null.t

7783def

zth force-pushed the unboxed-variants-stdlib-types branch from 19fa352 to 7783def Compare April 27, 2023 19:28

changelog

da80f23

cristianoc reviewed Apr 27, 2023

View reviewed changes

Add compile artifacts.

502b38d

cristianoc reviewed Apr 27, 2023

View reviewed changes

cristianoc mentioned this pull request Apr 27, 2023

Investigate option unboxing logic to make it compatible with untagged variants. #6222

Closed

cristianoc approved these changes Apr 27, 2023

View reviewed changes

cknitt approved these changes Apr 30, 2023

View reviewed changes

cristianoc merged commit a7b1ac4 into master Apr 30, 2023

zth deleted the unboxed-variants-stdlib-types branch April 30, 2023 18:58

fhammerschmidt mentioned this pull request Oct 2, 2023

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

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unboxed variants stdlib types #6218

Unboxed variants stdlib types #6218

zth commented Apr 27, 2023 •

edited

Loading

zth commented Apr 27, 2023

cristianoc commented Apr 27, 2023 •

edited

Loading

zth commented Apr 27, 2023

zth commented Apr 27, 2023

mununki Apr 27, 2023

zth Apr 27, 2023

mununki Apr 27, 2023

cristianoc Apr 27, 2023

zth Apr 27, 2023

mununki Apr 30, 2023 •

edited

Loading

zth Apr 30, 2023 •

edited

Loading

cristianoc Apr 30, 2023

mununki Apr 30, 2023

cristianoc Apr 30, 2023 •

edited

Loading

cristianoc Apr 27, 2023 •

edited

Loading

zth Apr 28, 2023

cristianoc Apr 27, 2023

zth Apr 28, 2023

Unboxed variants stdlib types #6218

Unboxed variants stdlib types #6218

Conversation

zth commented Apr 27, 2023 • edited Loading

zth commented Apr 27, 2023

cristianoc commented Apr 27, 2023 • edited Loading

zth commented Apr 27, 2023

zth commented Apr 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mununki Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

zth Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

cristianoc Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zth commented Apr 27, 2023 •

edited

Loading

cristianoc commented Apr 27, 2023 •

edited

Loading

mununki Apr 30, 2023 •

edited

Loading

zth Apr 30, 2023 •

edited

Loading

cristianoc Apr 30, 2023 •

edited

Loading

cristianoc Apr 27, 2023 •

edited

Loading