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

Refactor the derive macro code #2090

Merged
merged 1 commit into from
May 3, 2024
Merged

Conversation

bendk
Copy link
Contributor

@bendk bendk commented May 2, 2024

This is prep work for implementing the system for remote / custom / external types as described in #2087. The generated code should stay exactly the same, this just refactors how it's generated.

Consolidated the derive_*_for_udl macros into a single udl_derive macro and added the DeriveOptions struct. I think that we can update UDL-generation to create blanket FFI trait impls by just tweaking which flags we set. We can also re-use the same pattern to implement remote types (which will have generate_metadata: true, local_tag: true).

Changed the specific derive code to use DeriveOptions plus an item struct. For example RecordItem, which represents the parsed DeriveInput for the record. This will make future changes easier since we only need to modify one of these structs, not update all the function signatures.

Changed the UDL derives to parse attributes exactly like regular derives. For example, we now wrap enums with #[uniffi(flat_error)] rather than calling derive_error_for_udl!(flat_error). This is prep work for remote types -- in that case I think we should tell the user to define the item exactly like it was a regular derive and not special case the attributes.

Removed the #[uniffi(non_exhaustive)] attribute and parse the real #[non_exhaustive] attribute instead. This is easy to do now with the new system and also will be good for remote types.

Made EnumItem also parse error attributes. I think this is simpler than creating a separate EnumItem vs ErrorItem, at least for now.

@bendk bendk requested review from mhammond and jplatte May 2, 2024 18:19
@bendk bendk requested a review from a team as a code owner May 2, 2024 18:19
This is prep work for implementing the system for remote / custom /
external types as described in mozilla#2087.  The generated code should stay
exactly the same, this just refactors how it's generated.

Consolidated the `derive_*_for_udl` macros into a single `udl_derive`
macro and added the `DeriveOptions` struct.  I think that we can update
UDL-generation to create blanket FFI trait impls by just tweaking which
flags we set.  We can also re-use the same pattern to implement remote
types (which will have `generate_metadata: true, local_tag: true`).

Changed the specific derive code to use `DeriveOptions` plus an item
struct.  For example  RecordItem, which represents the parsed
DeriveInput for the record. This will make future changes easier since
we only need to modify one of these structs, not update all the function
signatures.

Changed the UDL derives to parse attributes exactly like regular
derives.  For example, we now wrap enums with `#[uniffi(flat_error)]`
rather than calling `derive_error_for_udl!(flat_error)`.  This is prep
work for remote types -- in that case I think we should tell the user to
define the item exactly like it was a regular derive and not special
case the attributes.

Removed the `#[uniffi(non_exhaustive)]` attribute and parse the real
`#[non_exhaustive]` attribute instead.  This is easy to do now with the
new system and also will be good for remote types.

Made EnumItem also parse error attributes.  I think this is simpler than
creating a separate EnumItem vs ErrorItem, at least for now.
Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Only skimmed, but didn't notice anything wrong. The underlying idea seems very good!

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

thanks!

@bendk bendk merged commit b79ede9 into mozilla:main May 3, 2024
5 checks passed
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.

3 participants