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

Disallow non-variant spreads in variants #6980

Merged
merged 7 commits into from
Aug 27, 2024
Merged

Conversation

zth
Copy link
Collaborator

@zth zth commented Aug 26, 2024

Disallows anything but regular variants as spreads in other variants. Closes #6952.

@zth zth requested a review from cristianoc August 26, 2024 20:35
@cknitt
Copy link
Member

cknitt commented Aug 27, 2024

Great! Some remarks:

  1. I am not sure this really closes Type spread failed on empty variant #6952, as there is still the question of how to create an empty variant type (Type spread failed on empty variant #6952 (comment))
  2. Can you undo the reformatting of the CHANGELOG? If we want to reformat it, we should do it in a separate PR, and I think the reformatting is partially incorrect.
  3. Why is release.ninja changed? Does the change go away when you re-run make lib?

@zth
Copy link
Collaborator Author

zth commented Aug 27, 2024

Great! Some remarks:

  1. I am not sure this really closes Type spread failed on empty variant #6952, as there is still the question of how to create an empty variant type (Type spread failed on empty variant #6952 (comment))

This PR disallows using extensible variants.

  1. Can you undo the reformatting of the CHANGELOG? If we want to reformat it, we should do it in a separate PR, and I think the reformatting is partially incorrect.

Yes, sorry, didn't see I reformatted it.

  1. Why is release.ninja changed? Does the change go away when you re-run make lib?

No idea why it changed. I'll try re-running that and see what happens.

@cknitt
Copy link
Member

cknitt commented Aug 27, 2024

This PR disallows using extensible variants.

Yes, but @glennsl also wrote

In OCaml, an empty variant type would be type a = |. That doesn't seem to be supported in Rescript though.

Do we want a syntax for an empty variant type in ReScript? If so, should we open a separate issue for that?

@zth
Copy link
Collaborator Author

zth commented Aug 27, 2024

This PR disallows using extensible variants.

Yes, but @glennsl also wrote

In OCaml, an empty variant type would be type a = |. That doesn't seem to be supported in Rescript though.

Do we want a syntax for an empty variant type in ReScript? If so, should we open a separate issue for that?

Oh, right. Feels like a separate topic to this, so yes, a separate issue would be good.

@zth
Copy link
Collaborator Author

zth commented Aug 27, 2024

@cknitt seems to have done it. Would you look again?

@zth zth force-pushed the disallow-non-variant-spreads branch from a8eb622 to 395aa40 Compare August 27, 2024 07:52
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@cknitt
Copy link
Member

cknitt commented Aug 27, 2024

Now all the format changes are back.

@zth
Copy link
Collaborator Author

zth commented Aug 27, 2024

@cknitt check again.

@zth zth merged commit 094fb15 into master Aug 27, 2024
20 checks passed
@zth zth deleted the disallow-non-variant-spreads branch August 27, 2024 08:26
cknitt pushed a commit to cknitt/rescript-compiler that referenced this pull request Sep 8, 2024
* disallow non-variant spreads in variants

* changelog

* undo formatting in changelog

* run make lib

* changelog

* fix

* changelog
# Conflicts:
#	CHANGELOG.md
cknitt pushed a commit that referenced this pull request Sep 10, 2024
* disallow non-variant spreads in variants

* changelog

* undo formatting in changelog

* run make lib

* changelog

* fix

* changelog
# Conflicts:
#	CHANGELOG.md
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 spread failed on empty variant
3 participants