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

Generate From impls for event (content) enums. #693

Merged
merged 5 commits into from
Aug 17, 2021

Conversation

Frinksy
Copy link
Contributor

@Frinksy Frinksy commented Aug 14, 2021

Resolves: #245

Tasks:

  • From<SpecificEventContent> for Any[kind]EventContent
  • From<SpecificEvent> for Any[kind]Event
  • From impls for the manually defined enums.

I'm not sure what other For implementations which could be useful, but I'd gladly add those too (if any).

@Frinksy
Copy link
Contributor Author

Frinksy commented Aug 15, 2021

I decided to create a derive macro for the manually defined enums. I'm not sure if this the best or desired way to do this, and there is probably currently some code duplication that I could refactor.

@Frinksy Frinksy marked this pull request as ready for review August 15, 2021 19:56
@Frinksy Frinksy requested a review from jplatte as a code owner August 15, 2021 19:56
Copy link
Member

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

I wouldn't personally have used a proc-macro when it's only used in four places, but I suppose we would have somewhat boilerplate-y code there otherwise.

let var_ident = &variant.ident.to_token_stream();
let id = &input.ident;
quote! {
impl From<#inner_struct> for #id {
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead implement From<T> for #id where T: Into<#inner_struct>? That would allow both AnySyncMessageEvent => AnySyncRoomEvent (using the blanket From<T> for T impl from std to fulfill the bound) and SyncMessageEvent<MessageEventContent> => AnySyncRoomEvent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure we can implement From<T> for #id where T: Into<#inner_struct>.

When I tried, I got a bunch of

conflicting implementations of trait `std::convert::From<_>` for type `enums::AnyRoomEvent`
first implementation here (.....)

I'm guessing that the compiler is being conservative and assumes that someone could implement From<OneVariant> for AnotherVariant, causing an overlap in the impls?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right. Somebody in an external crate could implement Into<AnySyncMessageEvent> as well as Into<AnySyncStateEvent> for the same type. Hmm. In that case let's leave this as-is, it doesn't hurt though we should have another issue for allowing more conversions here in one way or another.

crates/ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
crates/ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
crates/ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
crates/ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
crates/ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
crates/ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
crates/ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
Copy link
Member

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

Would be nice to have a changelog entry for this, otherwise LGTM.

@jplatte jplatte enabled auto-merge (squash) August 17, 2021 15:36
@jplatte jplatte merged commit d746244 into ruma:main Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Generate From implementations for event (content) enums
2 participants