Skip to content

Unboxed variants stdlib types #6218

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

Merged
merged 7 commits into from
Apr 30, 2023
Merged

Unboxed variants stdlib types #6218

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b5ff62b
untagged variant for Json.t
zth Apr 27, 2023
f366ee8
untagged variant for nullable
zth Apr 27, 2023
0d3c199
copy change to runtime js file
zth Apr 27, 2023
1171f4c
duplicate definition of Nullable.t in more places as needed
zth Apr 27, 2023
7783def
Present -> Value, and add equivalent unboxed variant type for Null.t
zth Apr 27, 2023
da80f23
changelog
zth Apr 27, 2023
502b38d
Add compile artifacts.
cristianoc Apr 27, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

- Add location information to duplicate type definition error messages. https://github.com/rescript-lang/rescript-compiler/pull/6199
- Replace normal module errors with Super_error module, and clean up Super_error. https://github.com/rescript-lang/rescript-compiler/pull/6199
- `Js.Json.t`, `Js.null` and `Js.nullable` are now untagged variants representing their runtime values, instead of abstract types. https://github.com/rescript-lang/rescript-compiler/pull/6218

# 11.0.0-alpha.5

Expand Down
12 changes: 10 additions & 2 deletions jscomp/others/js.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,10 @@ end

(**/**)

type +'a null
type +'a null =
| Value of 'a
| Null [@as null]
[@@unboxed]
(**
Nullable value of this type can be either null or 'a. This type is equivalent to Js.Null.t.
*)
Expand All @@ -96,7 +99,12 @@ type +'a undefined
A value of this type can be either undefined or 'a. This type is equivalent to Js.Undefined.t.
*)

type +'a nullable
type +'a nullable =
| Value of 'a
| Null [@as null]
Copy link
Collaborator

@cristianoc cristianoc Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My logic is:

  1. 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.
  2. 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?

| Undefined [@as undefined]
[@@unboxed]

(**
A value of this type can be undefined, null or 'a. This type is equivalent to Js.Null_undefined.t.
*)
Expand Down
10 changes: 9 additions & 1 deletion jscomp/others/js_json.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,15 @@

(** Efficient JSON encoding using JavaScript API *)

type t
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]

type _ kind =
| String : Js_string.t kind
Expand Down
10 changes: 9 additions & 1 deletion jscomp/others/js_json.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,15 @@

(** ## Types *)

type t
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]
Comment on lines +33 to +41
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be kept as an opaque type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@mununki mununki Apr 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here is another example: circular reference object

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

Copy link
Collaborator Author

@zth zth Apr 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we have exhausted the exploration here. Merging.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@cristianoc cristianoc Apr 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

(** The JSON data structure *)

(** Underlying type of a JSON value *)
Expand Down
5 changes: 4 additions & 1 deletion jscomp/others/js_null.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@
(** Provides functionality for dealing with the `'a Js.null` type *)


type + 'a t = 'a Js.null
type + 'a t = 'a Js.null =
| Value of 'a
| Null [@as null]
[@@unboxed]

external to_opt : 'a t -> 'a option = "#null_to_opt"
external toOption : 'a t -> 'a option = "#null_to_opt"
Expand Down
5 changes: 4 additions & 1 deletion jscomp/others/js_null.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@

(** Provides functionality for dealing with the `Js.null('a)` type *)

type +'a t = 'a Js.null
type +'a t = 'a Js.null =
| Value of 'a
| Null [@as null]
[@@unboxed]
(** Local alias for `Js.null('a)` *)

external return : 'a -> 'a t = "%identity"
Expand Down
7 changes: 6 additions & 1 deletion jscomp/others/js_null_undefined.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@

(** Contains functionality for dealing with values that can be both `null` and `undefined` *)

type + 'a t = 'a Js.nullable
type + 'a t = 'a Js.nullable =
| Value of 'a
| Null [@as null]
| Undefined [@as undefined]
[@@unboxed]

external toOption : 'a t -> 'a option = "#nullable_to_opt"
external to_opt : 'a t -> 'a option = "#nullable_to_opt"
external return : 'a -> 'a t = "%identity"
Expand Down
6 changes: 5 additions & 1 deletion jscomp/others/js_null_undefined.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@

(** Contains functionality for dealing with values that can be both `null` and `undefined` *)

type +'a t = 'a Js.null_undefined
type +'a t = 'a Js.nullable =
| Value of 'a
| Null [@as null]
| Undefined [@as undefined]
[@@unboxed]
(** Local alias for `Js.null_undefined('a)`. *)

external return : 'a -> 'a t = "%identity"
Expand Down
11 changes: 9 additions & 2 deletions jscomp/runtime/js.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ end
(**/**)


type + 'a null
type + 'a null =
| Value of 'a
| Null [@as null]
[@@unboxed]
(** nullable, value of this type can be either [null] or ['a]
this type is the same as type [t] in {!Null}
*)
Expand All @@ -66,7 +69,11 @@ type + 'a undefined
(** value of this type can be either [undefined] or ['a]
this type is the same as type [t] in {!Undefined} *)

type + 'a nullable
type + 'a nullable =
| Value of 'a
| Null [@as null]
| Undefined [@as undefined]
[@@unboxed]
(** value of this type can be [undefined], [null] or ['a]
this type is the same as type [t] n {!Null_undefined} *)

Expand Down