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

Provide a syntax extension free alternative to #[insertable_into] #303

Merged
merged 3 commits into from
Apr 25, 2016

Conversation

sgrif
Copy link
Member

@sgrif sgrif commented Apr 24, 2016

This provides a pure stable alternative to the #[insertable_into]
annotation. The intention is to change the annotation to call this
macro, rather than impl Insertable directly. However, there are some
unaddressed issues for that, and I will submit that as a separate PR to
attempt to keep the PR size reasonable.

The tests for this are a bit messy, as the actual test body doesn't
change much -- Just the struct definition. I moved the most common case
out into a macro, but I opted to just leave the duplication for the
remaining 4-5 cases that didn't fit, instead of trying to make it so dry
it chafes.

We will continue to support syntex as an option on stable, as we can
provide much better error messages from a procedural macro. I would like
to improve the error messages in some cases if possible though (in
particular, we want to handle the case where a unit struct is passed or
where a tuple struct has unannotated fields).

The structure of the macro is intended to be compatible with the
custom_derive crate. This is untested, but will be fully tested once
I've moved all our annotations to stable macros. The goal is for any
struct definition to be copy pasted into this macro, and the macro
parses the struct body to create the proper implementation. For
sufficiently large structs, we can hit the recursion limit, but there's
really no way around that. People will just need to bump the limit.

One case that this macro doesn't handle is when there are annotations
on struct fields other than #[column_name]. I had originally planned
to handle these, but I realized that the only recognized annotation that
could be there on stable is #[cfg], and we are not handling cfg
attributes. We might handle that in the future, but it'd look really
ugly.

Related to #99

This provides a pure stable alternative to the `#[insertable_into]`
annotation. The intention is to change the annotation to call this
macro, rather than `impl Insertable` directly. However, there are some
unaddressed issues for that, and I will submit that as a separate PR to
attempt to keep the PR size reasonable.

The tests for this are a bit messy, as the actual test body doesn't
change much -- Just the struct definition. I moved the most common case
out into a macro, but I opted to just leave the duplication for the
remaining 4-5 cases that didn't fit, instead of trying to make it so dry
it chafes.

We will continue to support syntex as an option on stable, as we can
provide much better error messages from a procedural macro. I would like
to improve the error messages in some cases if possible though (in
particular, we want to handle the case where a unit struct is passed or
where a tuple struct has unannotated fields).

The structure of the macro is intended to be compatible with the
`custom_derive` crate. This is untested, but will be fully tested once
I've moved all our annotations to stable macros. The goal is for any
struct definition to be copy pasted into this macro, and the macro
parses the struct body to create the proper implementation. For
sufficiently large structs, we can hit the recursion limit, but there's
really no way around that. People will just need to bump the limit.

One case that this macro *doesn't* handle is when there are annotations
on struct fields other than `#[column_name]`. I had originally planned
to handle these, but I realized that the only recognized annotation that
could be there on stable is `#[cfg]`, and we are *not* handling cfg
attributes. We might handle that in the future, but it'd look *really*
ugly.

Related to #99
@sgrif sgrif force-pushed the sg-stable-insertable-into branch from 07c08c4 to d8ce020 Compare April 24, 2016 23:23
@sgrif
Copy link
Member Author

sgrif commented Apr 24, 2016

Review? @diesel-rs/core

/cc @aturon @nikomatsakis Seeing some cases of this sort of thing out in the wild is likely relevant to the lang team.

@sgrif
Copy link
Member Author

sgrif commented Apr 24, 2016

The build failures are because the code assumes rust-lang/rust#31776. I need to address that, but the changes are minor and shouldn't need to affect review.

@sgrif sgrif force-pushed the sg-stable-insertable-into branch from 0fbf8a9 to 4e5fca9 Compare April 24, 2016 23:46
}
};

// Handle struct with lifetimes
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the few places where I really appreciate in-code comments. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. Would normally restructure this code to not need it, but that's basically impossible with this type of macro.

This isn't valid until Rust 1.9. See the comment in the code for details
@mfpiccolo
Copy link
Contributor

Macros for days! Nice work Sean. Once travis is happy I am happy. 👍

@tessgriffin
Copy link
Contributor

Documentation seems very reasonable. Looks good to me!

@sgrif sgrif merged commit 2b8c067 into master Apr 25, 2016
@sgrif sgrif deleted the sg-stable-insertable-into branch April 25, 2016 00:35
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