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

Support {} for empty inlined record literals and types #5900

Merged
merged 3 commits into from
Dec 12, 2022

Conversation

mununki
Copy link
Member

@mununki mununki commented Dec 12, 2022

This PR fixes #5899

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

@cknitt Could you let me know what branch this PR goes to? Both 10.1_release in the syntax repo and master like the previous PR?

@cknitt
Copy link
Member

cknitt commented Dec 12, 2022

I would say both 10.1_release and master, but the change is in jscomp/ml which is not part of the syntax, so no need to touch the syntax repo.

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

Got it! I'll make another PR that goes to 10.1_release.
How about changelog? Adding log to 10.1?

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.

This looks great!

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

@cristianoc Is the support for the empty inlined record type also needed? If it is needed, then I think all we need to remove https://github.com/rescript-lang/rescript-compiler/blob/master/res_syntax/src/res_core.ml#L4657:L4664

type inlinedOptional3 = V0({}) // empty inlined record
let ir6 = V0({})

@cristianoc
Copy link
Collaborator

@cristianoc Is the support for the empty inlined record type also needed? If it is needed, then I think all we need to remove https://github.com/rescript-lang/rescript-compiler/blob/master/res_syntax/src/res_core.ml#L4657:L4664

type inlinedOptional3 = V0({}) // empty inlined record
let ir6 = V0({})

Can't imagine why one would want that. Do you think there's a possible use case?

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

Can't imagine why one would want that. Do you think there's a possible use case?

Neither do I. I just want to ask your idea if it is needed.

@cknitt
Copy link
Member

cknitt commented Dec 12, 2022

How about changelog? Adding log to 10.1?

Changelog entry should go to version 10.1.1 in both branches.

@mununki
Copy link
Member Author

mununki commented Dec 12, 2022

How about changelog? Adding log to 10.1?

Changelog entry should go to version 10.1.1 in both branches.

Updated: eb3ff55

@mununki mununki merged commit 6a58feb into master Dec 12, 2022
@mununki mununki deleted the fix-inlined-empty-record branch December 12, 2022 15:56
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.

Type error of the inlined empty record
3 participants