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

Add @as support for obj ppx #6412

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

DZakh
Copy link
Contributor

@DZakh DZakh commented Sep 22, 2023

After we removed field mangling, it happened that it was not possible to migrate to rescript@11 because rescript-relay relied on it to support queriend data having names as reserved keywords. So, as an alternative solution for the field mangling, I think we should allow creating field aliases in the obj ppx, as it works in other places.

@zth Will you be able to update rescript-relay to use @as instead of prefixing reserved keywords with underscore? Or should I do this? It's currently the only blocker for us to start using rescript@11 at Carla.

@DZakh DZakh changed the title Support field aliases for obj ppx Add @as support for obj ppx Sep 22, 2023
@@ -1,21 +1,38 @@
@@jsxConfig({version: 3})

module C30 = {
@obj external makeProps: (~_open: 'T_open, ~key: string=?, unit) => {"_open": 'T_open} = ""
module C3A0 = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not affected in the PR

@DZakh
Copy link
Contributor Author

DZakh commented Sep 22, 2023

Could somebody help me with the CI error?

@fhammerschmidt
Copy link
Contributor

Had the same issue in my PR, here is the fix: ba56ffc

@zth
Copy link
Collaborator

zth commented Sep 23, 2023

After we removed field mangling, it happened that it was not possible to migrate to rescript@11 because rescript-relay relied on it to support queriend data having names as reserved keywords. So, as an alternative solution for the field mangling, I think we should allow creating field aliases in the obj ppx, as it works in other places.

@zth Will you be able to update rescript-relay to use @as instead of prefixing reserved keywords with underscore? Or should I do this? It's currently the only blocker for us to start using rescript@11 at Carla.

I can update it when I get a chance. You're still on 2.x, right, not 3.x?

@cristianoc opinions on this PR? Looks to me like it's just implementing what should already be working but wasn't.

@@ -449,11 +475,16 @@ let process_obj (loc : Location.t) (st : external_desc) (prim_name : string)
| Extern_unit -> assert false
| Poly_var _ ->
Location.raise_errorf ~loc
"%@obj label %s does not support such arg type" name
"%@obj label %s does not support such arg type" label
Copy link
Collaborator

Choose a reason for hiding this comment

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

Separate from this PR, but these errors would be good to clean up too at some point.

@cristianoc
Copy link
Collaborator

cristianoc commented Sep 23, 2023

@DZakh
This is only for external right?
Just double checking. If so, it looks good to me.

@DZakh
Copy link
Contributor Author

DZakh commented Sep 23, 2023

This is only for external right?
Just double checking. If so, it looks good to me.

It should be only for external.

@DZakh
Copy link
Contributor Author

DZakh commented Sep 23, 2023

I can update it when I get a chance. You're still on 2.x, right, not 3.x?

Yes we are. Actually, it might be even easier for us to migrate to 3.x. But I think it would be nice to make 2.x work with V11 for other people. I'm currently on vacation, but if I have time, I'll prepare a PR.

Comment on lines 357 to 371
PStr
[
{
pstr_desc =
Pstr_eval
( {
pexp_desc = Pexp_constant (Pconst_string (alias, _));
_;
},
_ );
_;
};
] ) ->
Some alias
| _ -> None)
Copy link
Contributor

Choose a reason for hiding this comment

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

@DZakh
Copy link
Contributor Author

DZakh commented Sep 27, 2023

@aspeddro Thank you. I've found an even better helper.

@DZakh DZakh requested review from zth and aspeddro September 27, 2023 16:45
@DZakh
Copy link
Contributor Author

DZakh commented Sep 27, 2023

It should be ready now

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.

Looks great. One day we can hopefully get rid of this all together, but it does have some interesting use cases still.

@zth zth merged commit f755a1c into rescript-lang:master Sep 27, 2023
14 checks passed
@DZakh
Copy link
Contributor Author

DZakh commented Sep 27, 2023

Yep, it would be great. But for now, it's nice to have for backward compatibility

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.

None yet

5 participants