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

Attribute for handwritten where clauses #352

Merged
merged 9 commits into from
Jun 6, 2016
Merged

Attribute for handwritten where clauses #352

merged 9 commits into from
Jun 6, 2016

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jun 4, 2016

Addresses (2) and (3) in #336 (comment).

  • If there is a #[serde(bound="...")] attribute on the type, use the union of that and the actual type's where clause as the where clause for the impl and do not attempt to generate any additional where clauses whatsoever.
  • If there is a #[serde(bound="...")] attribute on a field, use that and do not attempt to generate any additional where clauses for the field.

The bound attribute behaves similar to rename in that you can specify a single attribute that applies to both ser and de, or individual ones.

#[serde(bound="D: Serialize + Deserialize")]

#[serde(bound(serialize="D: Serialize", deserialize="D: Deserialize"))]

EDIT: now addresses (4) from #336 (comment) as well.

  • If a field contains direct recursion, do not generate any bounds based on that field except from bound attributes.

@dtolnay
Copy link
Member Author

dtolnay commented Jun 4, 2016

I failed a handful of clippy lints, working on them now.

@mcarton
Copy link

mcarton commented Jun 4, 2016

FYI, clippy 0.0.72 was just published, fixing the false positives in this PR 😄

@dtolnay
Copy link
Member Author

dtolnay commented Jun 5, 2016

@oli-obk @erickt this is ready for review.

@@ -2,6 +2,7 @@
#![cfg_attr(feature = "nightly-testing", feature(plugin))]
#![cfg_attr(feature = "nightly-testing", allow(too_many_arguments))]
#![cfg_attr(feature = "nightly-testing", allow(used_underscore_binding))]
#![cfg_attr(feature = "nightly-testing", allow(useless_let_if_seq))]
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Should be fixed in Clippy 0.0.73 just published. Thanks again for the report.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Martin, that solved it.

fields.iter()
.enumerate()
.map(|(i, field)| {
let attrs = try!(FieldAttrs::from_field(cx, i, field));
Ok((field, attrs))
Ok((field.clone(), attrs))
Copy link
Member

Choose a reason for hiding this comment

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

why was this changed from a reference to a value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because in all_fields_with_attrs the Vec<ast::StructField> does not live long enough to return references into it.

@oli-obk
Copy link
Member

oli-obk commented Jun 6, 2016

Implementation lgtm.

Design wise I think it's good, too. We can always add more magic later if it seems helpful/necessary, but the current version gives the user a lot of control to fix whatever issues they have with codegen.

Discovering this feature is hard though... But I'm not sure what to do about it... It would be great if we had some attribute to attach to generated ast-nodes that would cause rustc to emit some custom link/info if an error occurs inside that generated node. Let's see how often people complain, and adding a bound is the solution.

@homu r+

@homu homu merged commit bdffaf3 into serde-rs:master Jun 6, 2016
homu added a commit that referenced this pull request Jun 6, 2016
Attribute for handwritten where clauses

Addresses (2) and (3) in #336 (comment).

- If there is a `#[serde(bound="...")]` attribute on the type, use the union of that and the actual type's `where` clause as the `where` clause for the impl and do not attempt to generate any additional `where` clauses whatsoever.
- If there is a `#[serde(bound="...")]` attribute on a field, use that and do not attempt to generate any additional `where` clauses for the field.

The `bound` attribute behaves similar to `rename` in that you can specify a single attribute that applies to both ser and de, or individual ones.

```
#[serde(bound="D: Serialize + Deserialize")]

#[serde(bound(serialize="D: Serialize", deserialize="D: Deserialize"))]
```

EDIT: now addresses (4) from #336 (comment) as well.

- If a field contains direct recursion, do not generate any bounds based on that field except from `bound` attributes.
@dtolnay dtolnay deleted the where branch June 6, 2016 08:47
@alexbool
Copy link

alexbool commented Jun 6, 2016

Thanks! It would be great to bump and publish a version

@dtolnay
Copy link
Member Author

dtolnay commented Jun 6, 2016

@alexbool
Copy link

alexbool commented Jun 6, 2016

Thanks!

@dtolnay dtolnay added this to the v0.7.8 milestone Jun 9, 2016
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.

None yet

5 participants