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

Fix build error where aliasing argument to _ in make function with jsx v4 #5881

Merged
merged 3 commits into from
Dec 8, 2022

Conversation

mununki
Copy link
Member

@mununki mununki commented Dec 7, 2022

This PR fixes #5880

@mununki mununki changed the title Fix alias props to any in jsx v4 Fix alias argument to _ in make function with jsx v4 Dec 7, 2022
@mununki mununki changed the title Fix alias argument to _ in make function with jsx v4 Fix build error where aliasing argument to _ in make function with jsx v4 Dec 7, 2022
ppat_desc =
Ppat_construct ({txt = Lident "()"}, _) | Ppat_any;
},
{ppat_desc = Ppat_construct ({txt = Lident "()"}, _)},
Copy link
Member Author

Choose a reason for hiding this comment

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

A build error was caused here when meeting Ppat_any stops the loop.

@cristianoc
Copy link
Collaborator

@cknitt this should go into branch 10.1 ?

@cknitt
Copy link
Member

cknitt commented Dec 7, 2022

Right, we need that in 10.1, too.
And on master there is a Windows CI issue when the syntax has changed, will look into that.

@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

Need to rebase to 10.1_release?

@cknitt
Copy link
Member

cknitt commented Dec 7, 2022

@mununki Yes, please, that would be great (but for 10.1 the change needs to be made in the syntax repo).

@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

@mununki Yes, please, that would be great (but for 10.1 the change needs to be made in the syntax repo).

Not clear to me. Do I need to make another PR in the syntax repo? I thought the syntax repo was archived.

@cknitt
Copy link
Member

cknitt commented Dec 7, 2022

Yes, you would need to make another PR in the syntax repo first, as the 10.1 release branch still has the syntax submodule.

@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

Yes, you would need to make another PR in the syntax repo first, as the 10.1 release branch still has the syntax submodule.

Ah! I got it, remember that.

@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

I made another PR in the syntax repo rescript-lang/syntax#720

@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

@cknitt still does this branch need to be rebased to 10.1_release?

@cknitt
Copy link
Member

cknitt commented Dec 7, 2022

You would need to create a new PR for the 10.1_release branch that uses the updated syntax (once your syntax PR is merged).

@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

You would need to create a new PR for the 10.1_release branch that uses the updated syntax (once your syntax PR is merged).

If so, then I'm going to close this PR.

@mununki mununki closed this Dec 7, 2022
@mununki mununki deleted the fix-jsx-v4-alias branch December 7, 2022 16:32
@cknitt
Copy link
Member

cknitt commented Dec 7, 2022

Well, we still need the fix on master, too. :-)

@mununki mununki restored the fix-jsx-v4-alias branch December 7, 2022 16:34
@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

Ooops, I reopen it 😄

@mununki mununki reopened this Dec 7, 2022
@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

The updating change log seems already done in #5884 (comment). If this PR doesn't need to update the change log in the compiler, let me know.

@cknitt
Copy link
Member

cknitt commented Dec 7, 2022

Please rebase to latest master, the Windows build should work now.

@mununki
Copy link
Member Author

mununki commented Dec 7, 2022

Please rebase to latest master, the Windows build should work now.

Thanks. Rebased to master.

@cknitt cknitt merged commit 04eb5bd into master Dec 8, 2022
@cknitt cknitt deleted the fix-jsx-v4-alias branch December 8, 2022 06:12
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.

JSX4: compile error with ignored prop + prop with default value
3 participants