Skip to content

Cannot instantiate variant with inline record that has optional fields #5824

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

Closed
tsnobip opened this issue Nov 17, 2022 · 6 comments · Fixed by #5827
Closed

Cannot instantiate variant with inline record that has optional fields #5824

tsnobip opened this issue Nov 17, 2022 · 6 comments · Fixed by #5827
Labels

Comments

@tsnobip
Copy link
Member

tsnobip commented Nov 17, 2022

I've noticed you can define a variant with an inline record that optional fields, but you can't instantiate it.

Example:

// this compiles
type foo = Foo({name: string, age?: int})

// Error: Some record fields are undefined: age
let foo = Foo({name: "foo"}) 

We should either forbid the creation of such a type or allow to instantiate it.

@tsnobip tsnobip added the bug label Nov 17, 2022
@mununki
Copy link
Member

mununki commented Nov 18, 2022

Quick peek, I guess type declaration for optional field record as constructor argument is needed.

https://github.com/rescript-lang/rescript-compiler/blob/master/jscomp/ml/typedecl.ml#L252:L255

I think cristianoc can fix super fast, but I'll look at it too.

@mununki
Copy link
Member

mununki commented Nov 18, 2022

Another quick look, then I thought we need to add more information to Record_inlined. How about adding optional_labels instead adding another representation?:

and record_representation =
  ...
  | Record_inlined of                     (* Inlined record *)
      { tag : int ; name : string; num_nonconsts : int; optional_labels : string list} (* <- here *)
  ...
  | Record_optional_labels of string list (* List of optional labels *)

@cristianoc
Copy link
Collaborator

Another quick look, then I thought we need to add more information to Record_inlined. How about adding optional_labels instead adding another representation?:

and record_representation =
  ...
  | Record_inlined of                     (* Inlined record *)
      { tag : int ; name : string; num_nonconsts : int; optional_labels : string list} (* <- here *)
  ...
  | Record_optional_labels of string list (* List of optional labels *)

That looks like a good way to go.
I won't be able to look at this until I finish some other things. Feel free to go ahead if you'd like.

@mununki
Copy link
Member

mununki commented Nov 18, 2022

Another quick look, then I thought we need to add more information to Record_inlined. How about adding optional_labels instead adding another representation?:

and record_representation =
  ...
  | Record_inlined of                     (* Inlined record *)
      { tag : int ; name : string; num_nonconsts : int; optional_labels : string list} (* <- here *)
  ...
  | Record_optional_labels of string list (* List of optional labels *)

That looks like a good way to go. I won't be able to look at this until I finish some other things. Feel free to go ahead if you'd like.

Thanks for your feedback. OK I'll take and try to fix it.

@mununki
Copy link
Member

mununki commented Nov 20, 2022

@tsnobip Can you try it again with master or 10.1_release?

@tsnobip
Copy link
Member Author

tsnobip commented Nov 23, 2022

I tried on master, it worked and generated this as expected:

// Generated by ReScript, PLEASE EDIT WITH CARE
'use strict';


var foo = /* Foo */{
  name: "foo"
};

exports.foo = foo;
/* No side effect */

@tsnobip tsnobip closed this as completed Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants