Skip to content

Add a derive macro for Prism #101

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

Merged
merged 1 commit into from
Jan 13, 2022
Merged

Conversation

jplatte
Copy link
Member

@jplatte jplatte commented Jan 11, 2022

cc @xarvic 🙂

@jplatte jplatte force-pushed the prism-derive branch 3 times, most recently from f00c5da to 3995b0e Compare January 11, 2022 09:56
@jplatte
Copy link
Member Author

jplatte commented Jan 11, 2022

That was the last force-push, testing whether existing where clauses or duplicate Clone predicates cause any issues (they don't seem to!).


Ok(quote! {
#[derive(Clone)]
#enum_vis struct #name;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we make it mod $enum_name { pub struct $variant_name; } instead of struct $enum_name$variant_name?
this is done in derive Lens

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that! I thought this kind of thing would require inherent associated types, great to hear it doesn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately this doesn't work because enum variants already exist in the value namespace (Option::Some in the value namespace is a function from T to Option<T>).

Because of rust-lang/rust#76347, I only got errors after fully implementing this idea 🙄

Copy link
Member Author

@jplatte jplatte Jan 11, 2022

Choose a reason for hiding this comment

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

Oh by the way the way druid does it / what almost worked wasn't mod $enum_name, it was mod $mod_name {} + impl #enum_name { pub const Some: $mod_name = $mod_name::Some; }.

The first approach immediately resulted in amibiguity errors between the enum and module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the broken code that uses associated constants: jplatte@7a62a9b

Copy link
Collaborator

@richard-uk1 richard-uk1 left a comment

Choose a reason for hiding this comment

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

Cool, personally I think it's worth having a derive crate for the nursery, because there will probably be other codegen we want to experiment with, and because druid already requires syn/quote. We should make sure we use the same versions, so users don't have to build them twice.

The couple of small issues that @maan2003 identified can either be fixed before merge, or in a follow-up PR, either is fine.

@jplatte
Copy link
Member Author

jplatte commented Jan 11, 2022

We should make sure we use the same versions, so users don't have to build them twice.

Cargo.toml dep versions aren't exact by degault. Virtually everything uses syn & quote 1.x.y, so only one will be compiled.

@richard-uk1
Copy link
Collaborator

We should make sure we use the same versions, so users don't have to build them twice.

Cargo.toml dep versions aren't exact by degault. Virtually everything uses syn & quote 1.x.y, so only one will be compiled.

Yes sorry was more just thinking out loud. I don't expect it to be an issue.

@jplatte
Copy link
Member Author

jplatte commented Jan 11, 2022

Fixed the panic message and did some other refactorings, ready for another review :)

@xarvic
Copy link
Collaborator

xarvic commented Jan 12, 2022

This is awesome! @jplatte, thank you so much :)

@jplatte
Copy link
Member Author

jplatte commented Jan 13, 2022

@maan2003 can this be merged? :)

@maan2003
Copy link
Collaborator

Sure!

@maan2003 maan2003 merged commit e0b8dc5 into linebender:master Jan 13, 2022
@jplatte jplatte deleted the prism-derive branch January 13, 2022 15:22
maurerdietmar pushed a commit to maurerdietmar/druid-widget-nursery that referenced this pull request Jan 22, 2022
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.

4 participants