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

Fix type error for variant constructor args as optional field record #5827

Merged
merged 17 commits into from
Nov 20, 2022
Merged

Fix type error for variant constructor args as optional field record #5827

merged 17 commits into from
Nov 20, 2022

Conversation

mununki
Copy link
Member

@mununki mununki commented Nov 18, 2022 •
edited
Loading

This PR fixes #5824

@mununki mununki force-pushed the fix-optional-pcstr-record branch from 7451c6e to 7185e0a Compare November 19, 2022 08:45
@mununki
Copy link
Member Author

mununki commented Nov 19, 2022 •
edited
Loading

I think this PR is ready to get a review.
And not sure of what base branch this PR goes to? master? 10.1_release?

cristianoc
cristianoc reviewed Nov 19, 2022
View reviewed changes
jscomp/core/js_dump.ml Outdated Show resolved Hide resolved
cristianoc
cristianoc reviewed Nov 20, 2022
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.

This looks great thanks.
Would you add a couple of tests for pattern matching analogous to those for records with optional fields.

@mununki
Copy link
Member Author

mununki commented Nov 20, 2022

This looks great thanks. Would you add a couple of tests for pattern matching analogous to those for records with optional fields.

Great!
Sure thing, I'll add more tests.

@mununki
Copy link
Member Author

mununki commented Nov 20, 2022

What branch does this PR go to? master or 10.1_release?

cristianoc
cristianoc reviewed Nov 20, 2022
View reviewed changes
jscomp/core/js_dump.ml Outdated Show resolved Hide resolved
cristianoc
cristianoc reviewed Nov 20, 2022
View reviewed changes
Copy link
Collaborator

@cristianoc cristianoc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks finished to me.
I think it could go into 10.1 as it's a low-risk change and replaces a type error that surprises users anyway.
If we go with 10.1, this should be based on that branch.
And once the PR is merged, then branch 10.1 should be merged into master again so this last PR is added.

jscomp/test/record_regression.res Outdated
let foo1 = Foo({name: "foo"})
let foo2 = Foo({name: "foo", age: 3})

// should be type error
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is not needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed 651ba2b

jscomp/test/record_regression.res Outdated
@@ -100,3 +100,47 @@ let setAA = (ao: option<int>) => {aa: @ns.optional ao, bb: None}
// @ns.optional bb: int,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pre-existing, but these @ns.optional in the file above here are unnecessary since there's proper syntax for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed 651ba2b

@mununki mununki force-pushed the fix-optional-pcstr-record branch from 71d3033 to 651ba2b Compare November 20, 2022 08:53
@mununki mununki changed the base branch from master to 10.1_release November 20, 2022 08:54
@mununki
Copy link
Member Author

mununki commented Nov 20, 2022

There were too many conflicts to rebase to 10.1_release, then I cherry-picked.
During cherry-picking, there was a conflict in the change log, I mistakenly merge the latest change logs from master. So I removed it in 95f47f9

Now it is rebased to the 10.1_release.

cristianoc
cristianoc reviewed Nov 20, 2022
View reviewed changes
CHANGELOG.md Outdated

#### :bug: Bug Fix

- Fix issue where optional field in the inlined record caused the type error https://github.com/rescript-lang/rescript-compiler/pull/5827
Copy link
Collaborator

@cristianoc cristianoc Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fix issue where optional fields in inline records were not supported and would cause type errors.

Copy link
Member Author

@mununki mununki Nov 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks bbf2397 👍

EDIT: amend git message
dedc12f

@mununki mununki force-pushed the fix-optional-pcstr-record branch from bbf2397 to dedc12f Compare November 20, 2022 09:22
@mununki mununki merged commit 7ea13a4 into 10.1_release Nov 20, 2022
@mununki mununki deleted the fix-optional-pcstr-record branch November 20, 2022 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

Cannot instantiate variant with inline record that has optional fields
2 participants
@mununki @cristianoc