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

de: Make flattened structs and aliases interact correctly. #1519

Closed
wants to merge 2 commits into from

Conversation

emilio
Copy link

@emilio emilio commented May 4, 2019

The issue is that FlatStructAccess has no access to the aliases of the struct
it's deserializing.

Ideally we'd try to serialize the key rather than checking whether we're going
to use it before-hand, then actually take the value out, but that happens to be
tricky with the current seed API.

So we need to somehow get the aliased names back to FlatStructAccess. Introduce
a serialize_struct-like API that takes them in a backwards-compatible way. For
parallelism, and since we also support aliases on enum variants, also extend the
struct_variant API in a similar way. I'm open to better ways to fix it, but I
can't think of any other that isn't a breaking change...

Fixes #1504.

@emilio
Copy link
Author

emilio commented May 4, 2019

r? @dtolnay

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks! Would it be sufficient to pass the aliases in the fields given to deserialize_struct instead? I would prefer to avoid adding new Deserializer methods for this.

@emilio
Copy link
Author

emilio commented May 4, 2019

That's sort-of a breaking change, plus other serialize_struct implementations that use the fields use it as a hint to pre-allocate storage, which in the case of aliases may vastly over-allocate.

The issue is that FlatStructAccess has no access to the aliases of the struct
it's deserializing.

Ideally we'd try to serialize the key rather than checking whether we're going
to use it before-hand, then actually take the value out, but that happens to be
tricky with the current seed API.

So we need to somehow get the aliased names back to FlatStructAccess. Introduce
a serialize_struct-like API that takes them in a backwards-compatible way. For
parallelism, and since we also support aliases on enum variants, also extend the
struct_variant API in a similar way. I'm open to better ways to fix it, but I
can't think of any other that isn't a breaking change...

Fixes serde-rs#1504.
@dtolnay
Copy link
Member

dtolnay commented May 4, 2019

Ah, I haven't heard of Deserializer impls pre-allocating storage based on deserialize_struct. Do you have an example of a format like that? Ordinarily it would be the other way around; the Deserializer impl is aware how much data is in the input, and the Deserialize impl needs to pre-allocate based on MapAccess::size_hint.

@emilio
Copy link
Author

emilio commented May 4, 2019

Blerg, I was misreading code (was reading Serializer impls indeed).

But searched for a bit and found bits that would look at best fishy with that change. For example bincode serializes structs as tuples, and takes fields.len():

I haven't dug about whether this would break bincode for sure. I'm happy to dig out a bit. If it's just lying about a too large size hint it may be ok I guess....

emilio added a commit to emilio/aemet-visualizer that referenced this pull request May 4, 2019
This is gonna be useful to parse aggregate data.

Patch with serde-rs/serde#1519
to work-around / fix serde-rs/serde#1504.

That bound is needed due to deserialize_with, it's kind-of unfortunate that it
cannot be inferred.
@dmarcuse
Copy link

Is there any change to the status of this PR? I just ran into this issue in one of my projects, and it would be nice to have it fixed assuming this implementation covers all bases

@jnicholls
Copy link

Same here. It's August and no movement. @dtolnay @emilio can this conversation be resuscitated or should we take up the mantle?

@KeyboardDanni
Copy link

Ran into this while trying to make field name changes in a backward-compatible way.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Fixed a different way by #2387.

@dtolnay dtolnay closed this Mar 16, 2023
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.

Field aliases do not work in combination with flatten
5 participants