Skip to content

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