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.

Sign up for GitHub

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

Jump to bottom

Support bool untagged variants #6368

Merged
merged 5 commits into from
Oct 3, 2023
Merged

Support bool untagged variants #6368

merged 5 commits into from
Oct 3, 2023

Conversation

DZakh
Copy link
Contributor

@DZakh DZakh commented Aug 26, 2023

No description provided.

@cristianoc
Copy link
Collaborator

@zth can't remember what was the result of the last discussion on this?
W.r.t. the fact that @as(true) and @as(false) are already supported so this could be considered a bit redundant.

For the implementation, in case the consensus is to go ahead:
In terms of overlap. One needs to consider also that Bool(true) overlaps with a case @as(true) so I'd expect an error if both are present. Otherwise, some cases in pattern matching will never fire.

Also, gentype needs to be also adapted so it generates the expected type.

@zth
Copy link
Collaborator

zth commented Aug 26, 2023

Here's an old closed PR doing the same thing: #6231

My opinion is the same as before - I'm for adding this if there's consensus. Not having it makes working with booleans in untagged variants less ergonomic than it can be. Maybe asking on the forum would be a good next step, to find consensus?

@cristianoc
Copy link
Collaborator

I think we can just go ahead.
Not a big topic to need to ask the forum.

@fhammerschmidt
Copy link
Member

Do it!

cristianoc
cristianoc reviewed Aug 28, 2023
View reviewed changes
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.

Other than checking for overlap, some small change is required in the gentype code too so Bool emits type bool.

jscomp/test/UntaggedVariants.js Outdated
} else {
return "true";
}
} else {
Copy link
Collaborator

@cristianoc cristianoc Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

See comment in the thread that this type declaration should give an error (of overlap between bool and one bool constant) to avoid surprises like this one where the final else branch is dead code.

@DZakh
Copy link
Contributor Author

DZakh commented Aug 28, 2023

From @zth initial PR:

I think it's confusing to allow strings and numbers but not booleans, purely from a "this is what I'd expect as an end user" perspective. Even if it's expressible with the two cases, they both require using as-annotations and adding them both explicitly to cover the bool case. Feels like an odd technicality when the relation a JS dev has to string/number/bool are that they're all primitives, not whether they conceptually fit 100% into an ADT or not.

I actually had the feeling you mentioned. This is why I've decided to create the PR.

@DZakh
Copy link
Contributor Author

DZakh commented Aug 28, 2023

@cristianoc I'm going to have little time this week. So I'll fix it the next one

@cristianoc
Copy link
Collaborator

@cristianoc I'm going to have little time this week. So I'll fix it the next one

No rush whatsoever.

@zth
Copy link
Collaborator

zth commented Sep 19, 2023

This would be nice to get into v11. @DZakh any chance you'll be able to finish this up soonish?

@DZakh DZakh force-pushed the support-bool-untagged-variants branch from fc2cf04 to 9fd7821 Compare September 21, 2023 23:14
zth
zth reviewed Sep 23, 2023
View reviewed changes
jscomp/test/variantsMatching.res Outdated

module UntaggedWithBool = {
@unboxed @genType
type t<'a> = String(string) | Float(float) | Bool(bool) | Object({name: string})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Redundant 'a..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, thanks

@zth
Copy link
Collaborator

zth commented Sep 27, 2023

@DZakh is this ready to go?

@DZakh
Copy link
Contributor Author

DZakh commented Sep 27, 2023

Besides the comment it is. #6368 (comment)

I'm on vacation trip right now and I don't have much free time.

@DZakh DZakh force-pushed the support-bool-untagged-variants branch from d320bb0 to 2fb0cf7 Compare September 27, 2023 16:31
@DZakh
Copy link
Contributor Author

DZakh commented Sep 27, 2023

@zth done

@zth zth self-requested a review September 27, 2023 17:18
zth
zth approved these changes Sep 27, 2023
View reviewed changes
Copy link
Collaborator

@zth zth 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 good to me! @cristianoc what do you think?

cristianoc
cristianoc approved these changes Sep 27, 2023
View reviewed changes
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.

Looks great.
Left comments about an unrelated issue.

jscomp/test/variantsMatching.gen.tsx
@@ -19,3 +19,6 @@ export type MyNullable_t<a> = null | undefined | a;

// tslint:disable-next-line:interface-over-type-literal
export type MyNullableExtended_t<a> = null | undefined | "WhyNotAnotherOne" | a;

// tslint:disable-next-line:interface-over-type-literal
export type UntaggedWithBool_t = string | number | boolean | string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated: this does not look right.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this unrelated to this specific PR?

jscomp/test/variantsMatching.res

module UntaggedWithBool = {
@unboxed @genType
type t = String(string) | Float(float) | Bool(bool) | Object({name: string})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelate: this defines string twice.
It should be an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does it define string twice? The object prop name + the constructor..?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it doesn't look right with this output.

export type UntaggedWithBool_t = string | number | boolean | string;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, the two comments are connected! Now I see.

@zth
Copy link
Collaborator

zth commented Sep 27, 2023

Reminder for us: We should probably update Js.Json.t to use this bool type instead of false/true after this lands.

@zth zth merged commit 4b06ba9 into rescript-lang:master Oct 3, 2023
@zth zth mentioned this pull request Oct 3, 2023
Merged
@fhammerschmidt fhammerschmidt mentioned this pull request Oct 16, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zth zth zth approved these changes

@mununki mununki mununki left review comments

@cristianoc cristianoc cristianoc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DZakh @cristianoc @zth @fhammerschmidt @mununki