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

Revisit of the representation of adjacently tagged enums tag #2505

Merged

Conversation

Baptistemontan
Copy link
Contributor

@Baptistemontan Baptistemontan commented Jul 11, 2023

This PR is a follow on #2496.

This is a proof of concept to evaluate if such feature would be interesting.

I've run some tests with serde_json and the output is the same, but still allow some format to take advantage of variant_index during serialization.

If it looks promising I can continue to work on it

@Baptistemontan Baptistemontan marked this pull request as draft July 11, 2023 17:56
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.

I can confirm this looks promising. Thanks!

let expecting = format!("enum {}", tag);
quote_block! {
#[doc(hidden)]
struct __FieldVariant(pub __Field);
Copy link
Member

Choose a reason for hiding this comment

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

I would expect this whole type to go in serde::__private::de instead of emitting a whole new type and set of trait impls into every single use of adjacently tagged enums.

pub struct AdjacentlyTaggedEnumVariant<F> {
    tag: &'static str,
    variants: &'static [&'static str],
    fields_enum: PhantomData<F>,
}

impl<'de, F> DeserializeSeed<'de> for AdjacentlyTaggedEnumVariant<F>
where
    F: Deserialize<'de>,
{
    type Value = F;
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's what I wanted to do in the first place, but I just looked at Deserialize and because it don't take self I just moved on and did not thought of DeserializeSeed, I did something like this for serialization

@Baptistemontan
Copy link
Contributor Author

I did not realised that DeserializeSeed was imported only with feature = "std" so I had some trouble fixing the import.

Does something like this seams more appropriate ?

@Baptistemontan
Copy link
Contributor Author

Baptistemontan commented Jul 12, 2023

Is there a crate implementing a binary format that use variant_index to serialize enums and support adjacently tagged enums deserialization ?

I've looked into postcard but it does'nt support deserialize_identifier and deserialize_any so adjacently tagged enums are not supported (maybe #2475 broke it?).
And I looked at ciborium but it serialize enums with the variant name.

@dtolnay
Copy link
Member

dtolnay commented Jul 17, 2023

Is there a crate implementing a binary format that use variant_index to serialize enums and support adjacently tagged enums deserialization ?

I don't know of any. But if there is, then yes, we would have heard about it breaking from #2475.

@dtolnay
Copy link
Member

dtolnay commented Jul 30, 2023

This PR is still marked as draft. Are there concrete changes planned (what are they?), or was this supposed to be ready for review?

@Baptistemontan
Copy link
Contributor Author

@dtolnay I just forgot to open it for review, there is no changes planned

@Baptistemontan Baptistemontan marked this pull request as ready for review July 31, 2023 18:07
@Baptistemontan
Copy link
Contributor Author

There is conflicts needed to be resolved, I'm gonna need some time to understand why thoses changes has been made and what can I do to not break anything.

@Baptistemontan Baptistemontan changed the title POC on the revisit of the representation of adjacently tagged enums tag Revisit of the representation of adjacently tagged enums tag Jul 31, 2023
@Baptistemontan
Copy link
Contributor Author

All conflicts were related to the removal of try! in #2539

@dtolnay dtolnay linked an issue Aug 2, 2023 that may be closed by this pull request
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.

Thank you!

@dtolnay dtolnay force-pushed the rework_adjacently_tagged_enum branch from 5671c6b to 957ef20 Compare August 2, 2023 05:22
@dtolnay dtolnay merged commit 43035f6 into serde-rs:master Aug 2, 2023
16 checks passed
juntyr added a commit to juntyr/ron that referenced this pull request Aug 6, 2023
juntyr added a commit to ron-rs/ron that referenced this pull request Aug 6, 2023
@Mingun
Copy link
Contributor

Mingun commented Aug 14, 2023

This PR has some breaking changes that results in failing tests in quick-xml (tafia/quick-xml#630)

martin-g added a commit to apache/avro that referenced this pull request Aug 15, 2023
Also see serde-rs/serde#2496

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g added a commit to apache/avro that referenced this pull request Aug 15, 2023
* Bump serde from 1.0.180 to 1.0.183 in /lang/rust

Bumps [serde](https://github.com/serde-rs/serde) from 1.0.180 to 1.0.183.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.180...v1.0.183)

---
updated-dependencies:
- dependency-name: serde
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update the Serde impl after serde-rs/serde#2505

Also see serde-rs/serde#2496

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g pushed a commit to apache/avro that referenced this pull request Aug 15, 2023
* Bump serde from 1.0.180 to 1.0.183 in /lang/rust

Bumps [serde](https://github.com/serde-rs/serde) from 1.0.180 to 1.0.183.
- [Release notes](https://github.com/serde-rs/serde/releases)
- [Commits](serde-rs/serde@v1.0.180...v1.0.183)

---
updated-dependencies:
- dependency-name: serde
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update the Serde impl after serde-rs/serde#2505

Also see serde-rs/serde#2496

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
(cherry picked from commit e26943f)
tgeoghegan added a commit to divviup/janus that referenced this pull request Aug 15, 2023
Recent Serde changes ([1], [2]) changed the internal representation of
enums, breaking our test.

[1]: serde-rs/serde#2496
[2]: serde-rs/serde#2505
tgeoghegan added a commit to divviup/janus that referenced this pull request Aug 15, 2023
Recent Serde changes ([1], [2]) changed the internal representation of
enums, breaking our test.

[1]: serde-rs/serde#2496
[2]: serde-rs/serde#2505
@Baptistemontan Baptistemontan deleted the rework_adjacently_tagged_enum branch August 15, 2023 18:17
juntyr added a commit to juntyr/ron that referenced this pull request Aug 15, 2023
juntyr added a commit to juntyr/ron that referenced this pull request Aug 16, 2023
@jost-s
Copy link

jost-s commented Nov 10, 2023

Since this is a breaking change, why was it released as a patch instead of a minor version?

v1.0.180

In my case, an enum

#[derive(Serialize, Deserialize, Debug)]
#serde(tag = "type", content = "data")]
enum Message {
  A(i32),
}

let m = Message::A(100);

was rmp_serded to

[130, 164, 116, 121, 112, 101, 161, 65, 164, 100, 97, 116, 97, 100]

which the JS msgpack package converts to JSON as

{
  "type": "A",
  "data": 100
}

with serde v1.0.180.

v1.0.181

Same enum and attributes, rmp_serde spits out

[130, 164, 116, 121, 112, 101, 129, 161, 65, 192, 164, 100, 97, 116, 97, 100]

and JS msgpack converts to

{
  "type": {
    "A": null
  },
  "data": 100
}

serde_json

What is further confusing this is that converting this to JSON with serde_json results in

{\"type\":\"A\",\"data\":100}

regardless of the serde version.

rmp_serde

With serde v1.0.180 the result of rmp_serde::to_vec_named(&m) is

[130, 164, 116, 121, 112, 101, 129, 0, 192, 164, 100, 97, 116, 97, 129, 165, 112, 97, 114, 97, 109, 100]

JS msgpack decodes that to JSON like so

{
  "type": "request",
  "data": {
    "param": 100
  }
}

And with serde v1.0.181 the msgpack encoded byte array is

[130, 164, 116, 121, 112, 101, 129, 0, 192, 164, 100, 97, 116, 97, 129, 165, 112, 97, 114, 97, 109, 100]

which JS msgpack decodes to

{
  "type": {
    "0": null
  },
  "data": {
    "param": 100
  }
}

Can you please shed light on this? @dtolnay @Baptistemontan

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.

Revisit the representation of variant id for adjacently tagged enums
4 participants