Skip to content
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.

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

Source of truth for standard library types #7126

Open
cknitt opened this issue Oct 24, 2024 · 23 comments
Open

Source of truth for standard library types #7126

cknitt opened this issue Oct 24, 2024 · 23 comments
Assignees
Milestone

Comments

@cknitt
Copy link
Member

cknitt commented Oct 24, 2024

Currently we have e.g. in (Core) Map.res:

type t<'k, 'v> = Js.Map.t<'k, 'v>

or in (Core) Null.res:

@unboxed
type t<'a> = Js.Null.t<'a> =
  | Value('a)
  | @as(null) Null

i.e., the Js modules are the source of truth.

This should be changed, as Js should be deprecated.

Two options:

  1. Invert this to make the Core modules the source of truth, i.e., Js.Map.t would refer to Map.t.
  2. Create a new module Runtime_types.res, Types.res or similar, collect all the types there and make it the source of truth.

Any opinions @cristianoc @zth @cometkim?

@cknitt cknitt self-assigned this Oct 24, 2024
@cknitt cknitt added this to the v12 milestone Oct 24, 2024
@cometkim
Copy link
Member

cometkim commented Oct 24, 2024

For null and undefined, Primitive_js_extern is the source of truth right now

Core -> Js -> Primitive

@cometkim
Copy link
Member

My theory here is that Js is more like core, unless we assume that we can add more backend other than JS later.

Core is not really "core" because it has a lot of host dependencies. (Js, Html, Intl, etc)

@cknitt
Copy link
Member Author

cknitt commented Oct 24, 2024

The way I perceive it is that Js is the legacy stdlib and Core (although the name is actually not user-visible anymore) the new stdlib.

Primitive_js_extern has more than the types themselves though.
And the question is what will the user see in hover help etc. "Primitive_js_extern" does not sound very user-friendly.

@cknitt
Copy link
Member Author

cknitt commented Oct 24, 2024

The way I prototyped it in https://github.com/rescript-lang/experimental-rescript-stdlib-build/blob/main/runtime/src/runtime_types.res, I had

Core -> Runtime_types
Js -> Runtime_types
Belt -> Runtime_types

@cristianoc
Copy link
Collaborator

cristianoc commented Oct 24, 2024

Another question is the module name, if any.

Should it be Map.t or just map.

@cristianoc
Copy link
Collaborator

Same question for the existing built in types.
Which ones should have their own module name.

@cometkim
Copy link
Member

Should it be Map.t or just map.

Js maps are not transparent in their implementation. I would never recommend treating them as primitives.

@cknitt
Copy link
Member Author

cknitt commented Oct 24, 2024

It would certainly be nice to have date instead of Date.t, regExp instead of RegExp.t and symbol instead of Symbol.t etc.
Like we have dict instead of Dict.t now.

@cometkim
Copy link
Member

cometkim commented Oct 24, 2024

It would certainly be nice to have date instead of Date.t, regExp instead of RegExp.t and symbol instead of Symbol.t etc.

As I mentioned in Discord before, Date is poorly designed and soon to be replaced by Temporal

Array and list structures are simple and predictable, but map/set have much wider use cases. Whether it is a tree or a hashtable should be determined at the call site, and the only thing that the build depends on is the abstract type.

Dict is something that we already had in other forms.

Symbol is a primitive type in JS, so it is worth considering, but RegExp is just an Object with a different prototype.

IMHO we have to be very careful when choosing these. Treating something that isn't primitive as if it were a primitive is a decision we can never reverse. And it limits the quality of the implementation beyond just the interface.

@cometkim
Copy link
Member

Do you think Belt is no longer needed once Core bindings are provided? In fact, there are examples where Belt Map works more efficiently than Js Map.

https://github.com/cometkim/benchmark-rescript-cache-impl

We don't have to specify the properties of implementations in the language, even if Js does. I keep emphasizing the different responsibilities of primitives and stdlib.

@DZakh
Copy link
Contributor

DZakh commented Oct 24, 2024

As I mentioned in Discord before, Date is poorly designed and soon to be replaced by Temporal

I disagree that it should be the reason not to include the date type. It's a standard of the platform, and even though it's poorly designed, it's still a primitive which is widely used.

@cometkim
Copy link
Member

I am not against adding widely used utilities to the standard library.

However, "widely used" does not imply the existence of primitives. Primitives are the most fundamental part of a language and have a lasting impact on subsequent designs. Beyond simply declaring built-in types, they interact with other features of the language, adding complexity and side effects of its semantic. Even with poorly designed one, they are more cumbersome to work with and lower the average quality of software written in ReScript. Especially Date has never been progressively enhanced in ECMAScript specification. In fact, many codebases are trying to avoid relying on it and use third-party libraries like moment.js or dayjs.

There are reasons why stuffs like Date and RegExp are not chosen as primitives in other languages ​​(except Raku), despite their obvious popularity.

@cometkim
Copy link
Member

If pattern match is possible for arbitrary constructors like Date or RegExp, I would ask why it is not supported for all other constructors. In most codebases, there are custom constructors that are obviously used more frequently than those.

@cometkim
Copy link
Member

If we support Date more conveniently because it already exists, then that is literally why people use Date. Because it is better supported, users avoid better alternatives than Date, even if they are standards driven.

This is exactly what happened between CommonJS and ESM. ESM offers a better future, but CommonJS didn't switch simply because it was work and advanced use cases.

The interoperability story is not simple, because the two specifications have distinctly different semantics. The same thing probably happens between Date and Temporal.

@cknitt
Copy link
Member Author

cknitt commented Oct 25, 2024

I was not saying that we need to make date and regExp built-in types (as in https://github.com/rescript-lang/rescript-compiler/blob/master/compiler/ml/predef.ml) and implement pattern matching for them.

This is just about the fact that the "source of truth" cannot stay in the Js modules as these are legacy modules that will eventually be moved out to some compatibility package, and we need to decide where to put them instead.

The easiest solution without any additional modules or circular dependencies:

Date.res:

type t

Js_date.res:

type t = Js.Date.t

and, for convenience, Pervasives.res:

type date = Date.t

Also not saying that we necessarily need to have aliases for Map.t and Set.t in Pervasives.

When Temporal is added later, I see no harm in having both

type date = Date.t
type temporal = Temporal.t

in Pervasives.

@zth
Copy link
Collaborator

zth commented Oct 25, 2024

I was not saying that we need to make date and regExp built-in types (as in https://github.com/rescript-lang/rescript-compiler/blob/master/compiler/ml/predef.ml) and implement pattern matching for them.

This is just about the fact that the "source of truth" cannot stay in the Js modules as these are legacy modules that will eventually be moved out to some compatibility package, and we need to decide where to put them instead.

The easiest solution without any additional modules or circular dependencies:

Date.res:

type t

Js_date.res:

type t = Js.Date.t

and, for convenience, Pervasives.res:

type date = Date.t

Also not saying that we necessarily need to have aliases for Map.t and Set.t in Pervasives.

When Temporal is added later, I see no harm in having both

type date = Date.t
type temporal = Temporal.t

in Pervasives.

This sounds like the best decision to me. I know it's not what you meant @cknitt , but just for the record, I think @cometkim has a good point about what to make builtins and not (which I also think we all agree on already). But just aliasing is fine.

@cristianoc
Copy link
Collaborator

cristianoc commented Oct 25, 2024 via email

@cometkim
Copy link
Member

The important difference, I think, is whether it is treated as something the compiler can depend on.

@zth
Copy link
Collaborator

zth commented Oct 25, 2024

The important difference, I think, is whether it is treated as something the compiler can depend on.

There are exceptions to this though. Regexp and Date are already special cased in unboxed variants, when though they're not defined in the compiler per se.

@cometkim
Copy link
Member

There are exceptions to this though. Regexp and Date are already special cased in unboxed variants, when though they're not defined in the compiler per se.

That's the concern. Some things go beyond declaring types and going to be deeply integrated into the compiler.

@cometkim
Copy link
Member

I had the same thought when moving null and undefined types to Primitive_js_extern module. If possible, it should not be integrated too deeply, because in ReScript, option is our primitive, and null and undefined are just details of the host language.

What does that translate to if we support WASM targets in the future? I'm not trying to expand the topic we're discussing now, but it's about the direction it could take as we design the language.

@cknitt
Copy link
Member Author

cknitt commented Oct 25, 2024

I was not saying that we need to make date and regExp built-in types (as in https://github.com/rescript-lang/rescript-compiler/blob/master/compiler/ml/predef.ml) and implement pattern matching for them.
This is just about the fact that the "source of truth" cannot stay in the Js modules as these are legacy modules that will eventually be moved out to some compatibility package, and we need to decide where to put them instead.
The easiest solution without any additional modules or circular dependencies:
Date.res:

type t

Js_date.res:

type t = Js.Date.t

and, for convenience, Pervasives.res:

type date = Date.t

Also not saying that we necessarily need to have aliases for Map.t and Set.t in Pervasives.
When Temporal is added later, I see no harm in having both

type date = Date.t
type temporal = Temporal.t

in Pervasives.

This sounds like the best decision to me. I know it's not what you meant @cknitt , but just for the record, I think @cometkim has a good point about what to make builtins and not (which I also think we all agree on already). But just aliasing is fine.

There is a problem with this approach though. If my project also has a Date.res (like, for example in gentype_tests/typescript-react-example), weird things start to happen like

  It's possible that your build is stale.
  Try to clean the artifacts and build again?

  Here's the original error message
  The files /Users/christoph/projects/cca/rescript-compiler/darwinarm64/../lib/ocaml/pervasives.cmi
  and src/date.cmi make inconsistent assumptions over interface Date

(BTW notice that this error message refers to the non-existing path src/date.cmi) or

  The module or file Date can't be found.
  - If it's a third-party dependency:
    - Did you add it to the "bs-dependencies" or "bs-dev-dependencies" in bsconfig.json?
  - Did you include the file's directory to the "sources" in bsconfig.json?

So it might be better if those types were defined in a different file with a name less likely to occur in user projects.

@cknitt
Copy link
Member Author

cknitt commented Oct 25, 2024

This way works fine for me: 5828460

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready
Development

No branches or pull requests

5 participants