Unboxed variants stdlib types #6218
Conversation
|
CI seems to be failing but I don't really understand why. |
|
There are 2 js.ml files, you need to change both. |
|
@cristianoc fixed! |
|
Running locally against Core here to ensure things work as intended: #6218 |
| 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] |
There was a problem hiding this comment.
Shouldn't it be kept as an opaque type?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm also curious about why. I only see upsides, but maybe I'm missing something @mununki .
There was a problem hiding this comment.
Here is another example: circular reference object
let circularReferencingObj: Js.Json.t = Js.Dict.(
// making circular referencing object
...
)->Js.Json.Object There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think we have exhausted the exploration here. Merging.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
zth
force-pushed
the
unboxed-variants-stdlib-types
branch
from
19fa352
to
7783def
Compare
| type +'a nullable | ||
| type +'a nullable = | ||
| | Value of 'a | ||
| | Null [@as null] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
My logic is:
- Expose them all in the Js namespace. This would become problematic inference wise only for users who do
open Jsand who start using the new representation. - In
Core(which is the future), only exposeNullable.tconstructors in the global scope, notNull.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?
| @@ -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); | |||
There was a problem hiding this comment.
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.
This changes 3 type definitions in the stdlib:
Js.Json.tis now an untagged variant representing valid JSONJs.nullableis now an untagged variant representingValue('a) | Null | UndefinedJs.nullis now an untagged variant representingValue('a) | NullI'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
nullablein the globalJsscope of all of the nullable types (null,undefinedetc). I'm very much open to feedback here, but my reasoning is that that's the most important one, and having multiple definitions withValue/Null/Undefinedconstructors 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