/diesel

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

Let's consider going back to macros for stable #99

Closed
sgrif opened this Issue Jan 15, 2016 · 4 comments

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
  0.7
2 participants
@sgrif @brendanzab
@sgrif
Member

sgrif commented Jan 15, 2016

Syntex has proven to be pretty painful to track for supporting our compiler plugins on Stable. I think it's probably worth keeping support for, but we've been having issues with it causing released versions to break after the fact. I'd like to keep it as an option, but remove it by default in favor of our old "put your struct definition in these macros" solution as the "will definitely work on this version of Rust until the end of time" option.
@brendanzab

This comment has been minimized.

Sign in to view

brendanzab commented Mar 12, 2016

The reliance on Syntex scares me, as somebody who wished to introduce Rust for one of our data analytics projects. I would have definitely been more confident if there were macro alternatives available, in lieu of the stabilization of syntax extensions :(
@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Mar 12, 2016

The reliance on Syntex scares me

I wouldn't call it reliance. Every single thing that we use syntax extensions for is completely reasonable to write by hand (which I often do when I don't want to use syntex, or can't use codegen).
@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Apr 23, 2016

Putting this on the 0.7 milestone. I've been working on this for a few days now. Still need to audit a few more cases, but I'm fairly certain that every single one of our syntax extensions (with the exception of infer_schema! and infer_table_from_schema!) can be handled with a normal macro.

The overall plan is to add macros for each of our custom derive cases (all of our annotations are basically custom derive. The ones that aren't explicitly a #[derive] annotation are because they need arguments.) I'm thinking I'd like to structure the macros so that they are compatible with the custom-derive crate, but I see no reason to enforce using that crate.

I think it makes sense to keep the syntax extension form, but simply have them call the macro from the main Diesel crate. Aside from the "this eventually becomes stable" argument, we can also provide much better error messages from a procedural macro. For example, if Insertable is derived on a tuple struct, all fields must be annotated with the name of the column. We simply can't communicate that effectively from a normal macro.

I plan on opening a PR moving #[insertable_into] over to a normal macro this weekend, and I think we should declare this as a blocker for 1.0.

@sgrif sgrif added breaking change accepted labels Apr 23, 2016

@sgrif sgrif added this to the 0.7 milestone Apr 23, 2016

sgrif added a commit that referenced this issue Apr 24, 2016

@sgrif
Provide a syntax extension free alternative to `#[insertable_into]` 
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.

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
0dacf6e

sgrif added a commit that referenced this issue Apr 24, 2016

@sgrif
Provide a syntax extension free alternative to `#[insertable_into]` 
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
07c08c4

@sgrif sgrif referenced this issue Apr 24, 2016

Merged

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

sgrif added a commit that referenced this issue Apr 24, 2016

@sgrif
Provide a syntax extension free alternative to `#[insertable_into]` 
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
d8ce020

sgrif added a commit that referenced this issue Apr 25, 2016

@sgrif
Provide a syntax extension free alternative to `#[insertable_into]` (#… 
…303)

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

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
2b8c067
@sgrif

This comment has been minimized.

Sign in to view
Member

sgrif commented Aug 11, 2016

These have been implemented wherever possible.

@sgrif sgrif closed this Aug 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment