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
Add codegen expr magic for default, skip serializing, and alternative serializers #238
Conversation
One thing I can't tell is a great idea or not is that for They are also a bit wordy, but I'm not sure if I want to impose my |
Also would address #213. |
on the readability part I'd prefer to only allow entering a function name. Code in strings is confusing to me. |
cx, | ||
&field.node.ty, | ||
generics, | ||
try!(parse_lit_into_expr(cx, name, lit)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this allow calling functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it allows arbitrary expressions.
} | ||
}; | ||
|
||
let expr = parse::parse_expr_from_source_str("<lit expansion>".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this fails, the error reporting will be very confusing. is there a way to give a span?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I'll add it.
lgtm. Had to dig into the aster source to grok some of it. Are the aster-docs somewhere online? |
I agree with the "code in strings" bit. For the simple default value and boolean check expression, it seems natural to me. The |
@oli-obk: Yeah, code in strings is a bit ugly. It feels worth it for #[derive(Serialize, Deserialize)]
struct Struct {
#[serde(default="default_a")]
a: i32,
#[serde(skip_serializing_if="skip_serializing_b")]
b: i32,
#[serde(serialize_with="serialize_with_c")]
c: i32,
#[serde(deserialize_with="deserialize_with_d")]
d: i32,
}
fn default_a() -> i32 { 123 }
fn skip_serializing_b(value: &Struct) -> bool { value.b == 123 }
fn serialize_with_c<S>(serializer: &mut S,
value: &Struct) -> Result<(), S::Error>
where S: serde::Serializer
{
(value.b == 123).serialize(serializer)
}
fn deserialize_with_d<D>(deserializer: &mut D) -> Result<i32, S::Error>
where D: serde::Deserializer
{
Ok(if try!(bool::deserialize(deserializer) { 123) } else { 0 })
} But it would only be slightly more typing to do this with expressions: #[derive(Serialize, Deserialize)]
struct Struct {
#[serde(default="default_a()")]
a: i32,
#[serde(skip_serializing_if="skip_serializing_b(self)")]
b: i32,
#[serde(serialize_with="serialize_with_c(serializer, self)")]
c: i32,
#[serde(deserialize_with="deserialize_with_d(deserializer)")]
d: i32,
} I expect we'd still have to parse out these strings as a path, so that we can reference other crates, and possibly bundle in types, a la |
This feature adds support for the default to be specified to be some expression (which unfortunately needs to be parsed from a string) without needing this value to have an implementation of `Default`, or for a new-type wrapper in order to provide an alternative implementation. This expression is run in a function, and therefore has no access to any of the internal state of the deserializer.
This allows end users to use an arbitrary expression to decide whether or not to serialize some field. This expression has access to all the fields in the struct, but none of the internal state of the Serialize implementation. For structs, serde implements this by creating a temporary trait and implementing the struct for it. For struct variants, the fields are copied by reference into a temporary struct first before implementing the temporary trait. This also fixes a bug where the serde_codegen wasn't making calls to Serializer::serialize_{tuple,struct}_variant{,_elt}.
This allows a field to be serialized with an expression instead of the default serializer. This simplifies serializing a struct or enum that contains an external type that doesn't implement `serde::Serialize`. This expression is passed a variable `serializer` that needs to be used to serialize the expression.
This allows a field to be deserialized with an expression instead of the default deserializer. This simplifies deserializing a struct or enum that contains an external type that doesn't implement `serde::Deserialize`. This expression is passed a variable `deserializer` that needs to be used to deserialize the expression.
`#[serde(skip_serializing_if="...")]` can replace this functionality.
I've updated the PR to pickup the recent nightly breakage, and to remove |
I suggest to simply make that what's shown in the examples and docs, even if more complex expressions are allowed. |
@oli-obk: I've been talking it over with a few people, and I'm going to experiment with switching over to using the idents / paths to functions in order to avoid hygiene issues. Since we're breaking new ground here, maybe it'd be better to be a bit conservative. We could always switch back to full expressions in a future release. |
This is getting really really awkward fast. Take the use case of So we have a few solutions to that...
Same arguments and approaches go for Now,
|
Note that the hybrid approach's goal is to essentially make everyone use paths, but in the exceptional case where you need access to the other fields of |
Updated the patch to give better error messages if there's a syntax error in the expression. Still working on the other approaches. @arcnmx: You enumerated it pretty well. I think your right that access to |
Mm, though it could be useful in some cases (and it's something that's possible for us to expose). I think the reason why I entertain it is because it's awkward to implement impl Serialize for X {
fn serialize<S: Serializer>(&self, s: &mut S) -> Result<(), S::Error> {
// Custom modifications go in here
#[derive(Serialize)]
struct Inner<'a> {
field1: Newtype<'a>,
field2: Option<&'a Y>,
}
Inner { field1: Newtype(&self.field1), field2: Some(&self.field2) }.serialize(s)
}
} ... but maybe the pattern is rare enough that we shouldn't need worry about it; I'm kind of on the fence about it as I think access to Either way, I'm leaning toward the path approach, with maybe Also as a random aside, would it make sense to instead name the attribute |
@arcnmx: I got to step out for a bit, but I just pushed up a PR that switches from expressions to paths. I'm currently just passing in the field. When do you think the As to |
Cool, I feel that's probably the best approach to take for now!
default can't access the whole struct, the only two that can are
Mm, it's not difficult. Doing it for every structure is a huge pain though, and why I made the PR to avoid newtypes in the first place! You'll have to fall back to that if you need access to the whole struct which (ideally) isn't nearly as common a situation as needing a newtype so it should be fine. |
@arcnmx: I just realized we can't pass in the whole container into these path functions, because we are re-wrapping enum structs in a hidden struct, so there would be no way for a user to reference those types. So we would either need to have expressions, or only pass in field references to paths. My vote is for the latter in order to be hygienic. Also I did think about |
Hmm... Would it not be possible to thread an instance of |
@arcnmx: We could try to pass in the original type, but then we'd also have to pass in some string to identify the specific variant. That seems a bit tricky to me. We could revisit this if an rfc like rust-lang/rfcs#1450 lands that exposes a concrete type for a specific variant. |
Well, you would pass a reference to the whole type, and to the particular field. Then just let the implementation choose what it needs / wants to do. For the most part it would ignore the enum field entirely. It'd be more type-safe if we had enum variant types, sure, but I don't see it as a major problem without them. |
@arcnmx: Yeah, I think we could revisit this if it turns out to be an issue. |
Either way I think just passing the field reference alone is fine for now. I'm fine with either |
@arcnmx: I'm going to err with conservatism here. I'm going to merge this in. Thanks for the reviews! |
Add codegen expr magic for default, skip serializing, and alternative serializers
Seems I'm late for the party. |
I was kind of hoping we could shorten it to
Not sure what you mean here... You should be able to create helper functions and reuse those. Do you mean Considering that this has been merged now we should probably discuss on a new issue about any ergonomic concerns with it. |
traits: Remove pattern in trait's method signature This use of `mut` was nonsensical for now, but is proposed to be disallowed by rustc in the future (as a bugfix). See rust-lang/rust/pull/37378 for context.
This is an alternative implementation of #214, #198, #208 and implements #90 and #216. This allows one to write:
cc @oli-obk, @target-san, @arcnmx