Add `Core` standard library support for `genType` #6019

cristianoc · 2023-02-28T12:30:29Z

See https://forum.rescript-lang.org/t/difficulty-with-shims-and-undefined-t-and-null-t/4240

cristianoc · 2023-02-28T12:33:06Z

jscomp/gentype/TranslateTypeExprFromTypes.ml

@@ -201,9 +201,14 @@ let translateConstr ~config ~paramsTranslation ~(path : Path.t) ~typeEnv =
    {dependencies = []; type_ = EmitType.typeReactElement}
  | (["FB"; "option"] | ["option"]), [paramTranslation] ->
    {paramTranslation with type_ = Option paramTranslation.type_}
-  | (["Js"; "Null"; "t"] | ["Js"; "null"]), [paramTranslation] ->
+  | ( (["Js"; "Undefined"; "t"] | ["Undefined"; "t"] | ["Js"; "undefined"]),


@zth what else from paths hardcoded in this file should be supported for Core, in addition to Undefined.t and Null.t and Nullable.t and Dict.t and Promise.t?

Things like Option.t or Int.t or Float.t?
I think we're under the assumption that we're under "-open RescriptCore".

Undefined.t is getting axed, but the rest of them for sure. Here are a few other candidates (some are perhaps already supported):

BigInt (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-2.html#bigint)

Date

Map

Set

RegExp (?)

WeakMap

WeakSet

On my phone now can’t verify but I think undefined<…> and null<…> are at the top level of Core for easy usage in code. Don’t need to prefix with a module name if you open Core. Js.Undefined.t is used frequently in my genType bindings - bummed if the Undefined module is being axed.

On my phone now can’t verify but I think undefined<…> and null<…> are at the top level of Core for easy usage in code. Don’t need to prefix with a module name if you open Core. Js.Undefined.t is used frequently in my genType bindings - bummed if the Undefined module is being axed.

Why use Js.Undefined.t<typ> when you can use option<typ>? The definition of Js.Undefined predates the runtime representation of None as undefined.

I was writing bindings yesterday with gentype and the function I was binding to expected an undefined or T but not a null. I think option, which I used first, compiles to null or undefined or T. I always start with option but sometimes it doesn’t work with gentype.

Undefined.t is getting axed, but the rest of them for sure. Here are a few other candidates (some are perhaps already supported):

BigInt (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-2.html#bigint)

Date

Map

Set

RegExp (?)

WeakMap

WeakSet

All supported now.

I was writing bindings yesterday with gentype and the function I was binding to expected an undefined or T but not a null. I think option, which I used first, compiles to null or undefined or T. I always start with option but sometimes it doesn’t work with gentype.

Can you give an example of what function you were binding to? Did you get a TypeScript type error, or is it a worry that things don't match?

cristianoc · 2023-02-28T18:00:36Z

@zth this should be complete now and ready for review.
There's the question of Undefined.t, but that can also be handled in the stdlib, as genType won't do anything if that's not present in the library. The question to resolve is whether there are certain bindings that require using Undefined.t<_> and not option<_>, at the level of functionality. The typing question follows from that.

zth · 2023-02-28T18:22:52Z

Fantastic! Yes I'd be interested in hearing more about what use cases option does not handle from @jmagaram .

zth

Nice to read some gentype code.

jmagaram · 2023-02-28T18:47:34Z

I'm writing bindings for Firebase SDK, a very popular library from Google. These functions require types that are undefined | T. Using option with genType won't work. I've only started writing bindings; just for what I need. Probably a lot more use cases. If Undefined is going to be removed from Core library I'm going to have to keep using Js.Undefined. If makes sense to support all flavors of null | T, undefined | T, and null | undefined | T. The first two are probably identical code with all the same methods like toOption. What remove this from the new Core library?

export declare function getAnalytics(app?: FirebaseApp): Analytics;
export declare function getAuth(app?: FirebaseApp): Auth;
export declare function connectAuthEmulator(auth: Auth, url: string, options?: {
    disableWarnings: boolean;
}): void;

My binding for the first. I suppose I could create multiple bindings, one with the optional parameter and one without, but this is a hassle. I'd rather create all my friendly overrides in Rescript.

  @gentype.import("firebase/analytics")
  external _make: Js.undefined<FirebaseApp.t> => t = "getAnalytics"

zth · 2023-02-28T19:44:57Z

I still don't understand why option doesn't work where T | undefined is expected. Could you examplify that part @jmagaram ?

jmagaram · 2023-02-28T21:04:13Z

I still don't understand why option doesn't work where T | undefined is expected. Could you examplify that part @jmagaram ?

I'm confused I thought I just did. Those function declarations are straight from the Firebase SDK. I'm trying to write genType import statements for those. If I use option<FirebaseApp.t> it does not work. I get errors in Typescript saying the type is expecting undefined | T not undefined | null | T. See optionals in the genType documentation.

If you want to try it yourself, put this in a .ts file...

/* eslint-disable @typescript-eslint/no-unused-vars */
export const go1 = (a: string | undefined) => "x";
export const go2 = (args: { a: string | undefined }) => "x";

Then try to import it like this and look at the errors in the gen.tsx file.

@gentype.import("./MyFuncs")
external go1: option<string> => string = "go1"

type args2 = {a?: option<string>}

@gentype.import("./MyFuncs")
external go2: args2 => string = "go2"

There is tricky stuff I can't get my head around here. Like if the ReScript args is {a?:string} or a:option<string> there is different behavior and errors. In the TypeScript {a?:string} makes errors while {a:string|undefined} does not.

zth · 2023-03-01T08:00:48Z

Right, so this particular issue is that gentype treats option as null | undefined?

cristianoc · 2023-03-01T09:06:15Z

Right, so this particular issue is that gentype treats option as null | undefined?

genType takes the most general approach of accepting both null ad undefined, and converting the value to undefined on the way in from JS.
It would be possible to strictly accept only undefined but not null. Though one would need a proper assessment of what are the actual consequences for bindings: which choice covers the largest surface for typical bindings.
CC @cknitt

cristianoc · 2023-03-01T09:22:51Z

Generally speaking, treating option<T> as (undefined | T) would make importing JS functions taking optional arguments more general.
But, it would make exporting ReScript functions taking optional arguments less general. So that when those functions are consumes from JS, when called, only undefined can be passed but not null.

See https://forum.rescript-lang.org/t/difficulty-with-shims-and-undefined-t-and-null-t/4240

cristianoc · 2023-03-01T10:15:29Z

Here's a PR that treats option<t> as t | undefined #6022. It's quite attractive in that it produces fewer conversion functions. Unless the payload requires conversion, as in e.g. variants, the typed bindings are zero-cost.
It would be nice to test it on some codebase which used genType, and see if there are surprises.

cristianoc · 2023-03-01T10:23:37Z

There is tricky stuff I can't get my head around here. Like if the ReScript args is {a?:string} or a:option<string> there is different behavior and errors. In the TypeScript {a?:string} makes errors while {a:string|undefined} does not.

In TS these two types are different:

type t1 = {x?: string}
type t2 = {x: string | undefined}

in that t2 is more restrictive: it does not accept {} as a value.

The two ReScript types corresponding to them, as things currently stand, are:

type t1 = {x?: string}
type t2 = {x: Js.undefined<string>}

See #6022 as relevant.

This produces strictly fewer conversion functions, as it does not need to transform `null` to `undefined`.

This is a breaking changes, but aligns more closely with TS. The two TS types: ```ts type t1 = { x?: string }; type t2 = { x: (undefined | string) }; ``` now correspond to the ReScript types: ```res @genType type t1 = {x?: string} @genType type t2 = {x: Js.undefined<string>} ``` The special treatment of fields of option type comes from a time where records with optional fields did not exist.

cristianoc force-pushed the gentype_core branch from 996c83c to 3f37edd Compare February 28, 2023 12:31

cristianoc commented Feb 28, 2023

View reviewed changes

cristianoc requested a review from zth February 28, 2023 12:36

cristianoc added this to the v10.1 milestone Feb 28, 2023

zth approved these changes Feb 28, 2023

View reviewed changes

cristianoc added 8 commits March 1, 2023 10:32

Add Core standard library support for genType

c4f8c53

See https://forum.rescript-lang.org/t/difficulty-with-shims-and-undefined-t-and-null-t/4240

genType: treat Dict.t type as built-in.

3782ff5

Support Promise.t.

e42ac18

Support Date.t.

6c5b2fa

Support BigInt

c4b21b5

Support RegExp.

0e6ca40

Support Map and WeakMap

4a781b4

Support Set and WeakSet.

82534ac

cristianoc force-pushed the gentype_core branch from 071760e to b2361e3 Compare March 1, 2023 09:33

Update CHANGELOG.md

e4a26c8

cristianoc force-pushed the gentype_core branch from b2361e3 to e4a26c8 Compare March 1, 2023 09:58

cristianoc added 4 commits March 1, 2023 11:46

Clean up unused Record and RecordC.

65fe259

genType: treat option<t> as t | undefined

d2794d0

This produces strictly fewer conversion functions, as it does not need to transform `null` to `undefined`.

Add examples of what can and cannot be used in bindings to functions.

f2dfb4e

Update CHANGELOG.md

bd8ad8b

cristianoc merged commit e31365e into master Mar 2, 2023

reck753 mentioned this pull request Jan 12, 2024

GenType: Issue typing JSON.t and Dict.t from RescriptCore #6563

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Core` standard library support for `genType` #6019

Add `Core` standard library support for `genType` #6019

cristianoc commented Feb 28, 2023 •

edited

Loading

cristianoc Feb 28, 2023 •

edited

Loading

cristianoc Feb 28, 2023

zth Feb 28, 2023

jmagaram Feb 28, 2023

cristianoc Feb 28, 2023 •

edited

Loading

jmagaram Feb 28, 2023

cristianoc Feb 28, 2023

cristianoc Feb 28, 2023

cristianoc commented Feb 28, 2023

zth commented Feb 28, 2023

zth left a comment

jmagaram commented Feb 28, 2023 •

edited

Loading

zth commented Feb 28, 2023

jmagaram commented Feb 28, 2023

zth commented Mar 1, 2023

cristianoc commented Mar 1, 2023 •

edited

Loading

cristianoc commented Mar 1, 2023

cristianoc commented Mar 1, 2023 •

edited

Loading

cristianoc commented Mar 1, 2023 •

edited

Loading

Add Core standard library support for genType #6019

Add Core standard library support for genType #6019

Conversation

cristianoc commented Feb 28, 2023 • edited Loading

cristianoc Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

cristianoc Feb 28, 2023

Choose a reason for hiding this comment

zth Feb 28, 2023

Choose a reason for hiding this comment

jmagaram Feb 28, 2023

Choose a reason for hiding this comment

cristianoc Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

jmagaram Feb 28, 2023

Choose a reason for hiding this comment

cristianoc Feb 28, 2023

Choose a reason for hiding this comment

cristianoc Feb 28, 2023

Choose a reason for hiding this comment

cristianoc commented Feb 28, 2023

zth commented Feb 28, 2023

zth left a comment

Choose a reason for hiding this comment

jmagaram commented Feb 28, 2023 • edited Loading

zth commented Feb 28, 2023

jmagaram commented Feb 28, 2023

zth commented Mar 1, 2023

cristianoc commented Mar 1, 2023 • edited Loading

cristianoc commented Mar 1, 2023

cristianoc commented Mar 1, 2023 • edited Loading

cristianoc commented Mar 1, 2023 • edited Loading

Add `Core` standard library support for `genType` #6019

Add `Core` standard library support for `genType` #6019

cristianoc commented Feb 28, 2023 •

edited

Loading

cristianoc Feb 28, 2023 •

edited

Loading

cristianoc Feb 28, 2023 •

edited

Loading

jmagaram commented Feb 28, 2023 •

edited

Loading

cristianoc commented Mar 1, 2023 •

edited

Loading

cristianoc commented Mar 1, 2023 •

edited

Loading

cristianoc commented Mar 1, 2023 •

edited

Loading