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

Record definition spread issues #6154

Merged
merged 8 commits into from
Apr 15, 2023
Merged

Record definition spread issues #6154

merged 8 commits into from
Apr 15, 2023

Conversation

zth
Copy link
Collaborator

@zth zth commented Apr 14, 2023

This does 2 things:

  1. Fixes an issue where the fields being optional wasn't propagated through the new record type spread functionality. Before this, any optional field from a record that was then spread onto another record would no longer be optional.
  2. Adds a commented out test for a case where the parser breaks as we're spreading a single type only, with no additional fields, as in copying the record to a new identical record. I think that's supposed to work?

@zth zth requested a review from cristianoc April 14, 2023 16:39
@cristianoc
Copy link
Collaborator

cristianoc commented Apr 14, 2023

2. Adds a commented out test for a case where the parser breaks as we're spreading a single type only, with no additional fields, as in copying the record to a new identical record. I think that's supposed to work?

Indeed. Forgot about that. The DotDotDot.res file already had a few examples not using the new syntax (but using \"..." ) because of that.

jscomp/ml/typedecl.ml Outdated Show resolved Hide resolved
@cristianoc
Copy link
Collaborator

From reviewing the code some reflections:

  • The fields from the record being spread are not type checked again, but their type is simply used as-is. (That might explain why the seemingly needed code to add option to the type has no effect).
  • Another possibility is to extract the untyped parse tree for those fields and type check again. Though doing that would preclude the possibility of, in future, know what parts are coming from a spread and what parts are not. It might be useful, or it might not, to be seen.

Now that all the new features exist in isolation, there are a bunch of questions to answer:

  • Are there unboxed records (perhaps with exactly 1 field if I remember)
  • If they exist, can one spread it?
  • Does it become boxed if it's not declared unboxed?
  • If it's boxed, can one make one unboxed out of it?

So it looks like the design is not finished here. We can try to answer some of these questions and iterate some more see what comes out of it.

@cristianoc
Copy link
Collaborator

The question about unboxed apply to type coercion too.
Are we simply allow to coerce a boxed to an unboxed record and the other way round silently? That would not be safe.
What about @as(...)? Are we taking that into account correctly? Both in terms of spread and of type coercions.

Most of these questions can be answered by writing a few little tests.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

I think that's is, if strictly speaking about the issue observed.
Notice there are a bunch of related questions in the issue, but this can be merged as-is (after updating the change log).

jscomp/ml/typedecl.ml Outdated Show resolved Hide resolved
@zth
Copy link
Collaborator Author

zth commented Apr 15, 2023

@cristianoc check again, good to merge?

@cristianoc cristianoc merged commit eddfbb7 into master Apr 15, 2023
@zth zth deleted the record-def-spread-fixes branch April 15, 2023 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants