WIP: Allow #[serde(rename=str|bool|int)] for enum variants. by Phaiax · Pull Request #973 · serde-rs/serde

Phaiax · 2017-07-01T08:47:56Z

(This commit contains untested code and a dirty hacks so that the rename="str" still works)

#745 Issue for this PR

I got started and it was not too hard, serde derive is written very well. (I'm still confused by the deserializiation part, but I have not looked into it to much)

Some questions:

Non string literals are not stabilized yet. I think we should support a fallback with #[serde(rename="1", rename_as="int")]. This is necessary since serde supports old rustc versions anyway, so even with stabilization this will not go away.

error: non-string literals in attributes, or string literals in top-level positions, are experimental (see issue #34981)
   --> tests/test_macros.rs:713:9
    |
713 |         #[serde(rename=true)]
    |         ^^^^^^^^^^^^^^^^^^^^^
    |
    = help: add #![feature(attr_literals)] to the crate attributes to enable

Currently I implemented a type StrBoolInt, but I had to define it twice, once in serde_derive_internals and once again in serde_derive, so that I can implement ToTokens.
The second StrBoolInt is stuffed into fragment.rs. Should I use a new file or is that ok?
I decided to only allow non-string renames for internally and adjacently tagged enums. Correct?
As far as I understood it, Serializers have the choice to serialize either with the name of the variant or with the id. What happens if rename=2 is stated for attribute with field number 1? Should I just replace the whole the derserialize_u32 part as soon as an int-rename is encountered?
What is that [#serde(field_identifier/variant_identifier)] setting? It is not mentioned in the docs.

Thx, Daniel

(This commit contains untested code and a dirty hacks so that the rename="str" still works)

…pport non-str attribute literals

…(libc)].

Phaiax · 2017-07-05T11:44:41Z

Hey @dtolnay .

I think this is ready for a first review.

As for my Questions.

The implementation treats #[serde(rename=3)] the same as #[serde(rename="3", rename_as="int")] and gives errors if the parsing fails or the rename_as is used with a non string literal for rename.
still same question as above.
still same question as above.
still same question as above.
I implemented it so that deserializing by index is deactivated as soon as one rename has an int type.
still same question as above.
This PR will collide with Fix identifier deserialization from non-u32 #963 (same lines edited). I use u64 for the int type.
Naming of structs. See my own review.

After review I will merge the commits (except for the last) into one.

Phaiax

Questions / Notes

Phaiax · 2017-07-05T11:51:05Z

serde/src/export.rs

+    }
+
+    #[cfg(not(any(feature = "std", feature = "alloc")))]
+    pub fn from_int(i: u64) -> [u8; 20] {


the int/bool to string is needed for fn deserialize_identifier(..., fallthrough_arm : Option<Tokens>) where the fallthrough arm expects a local '&str' value, so writing to stdout is not possible.
It feels a bit too hacky :D

This seems fine. Nicely done.

Phaiax · 2017-07-05T11:54:56Z

serde/src/private/ser.rs

    t
 }

+pub enum VariantName {


Note: This enum is defined three times. The other two times it is named StrBoolInt.
Should it be named after its purpose or after its content?

In the serde_derive_internals crate, the name VariantName is already occupied by the struct VariantName { deserialize: StrBoolInt, serialize: StrBoolInt }. Maybe rename the VariantName into SerDeVariantName and StrBoolInt into VariantName?

Phaiax · 2017-07-05T11:57:20Z

serde_derive/src/de.rs

        .enumerate()
        .filter(|&(_, variant)| !variant.attrs.skip_deserializing())
-        .map(|(i, variant)| (variant.attrs.name().deserialize_name(), field_i(i)),)
+        .map(|(i, variant)| (StrBoolInt::from(variant.attrs.name().deserialize_name()), field_i(i)))


Note: These StrBoolInt::from() calls convert from serde_derive_internals::StrBoolInt into serde_derive::StrBoolInt (at least in enum/variant related functions, not in struct/field related functions).

Phaiax · 2017-07-05T11:59:37Z

serde_derive/src/de.rs

 fn deserialize_identifier(
    this: Tokens,
-    fields: &[(String, Ident)],
+    fields: &[(StrBoolInt, Ident)],


Maybe renaming StrBoolInt into VariantName is not so good because it can also represent a FieldName, (in this case it is always a string).

Phaiax · 2017-07-05T12:01:15Z

serde_derive/src/de.rs

-
-    let constructors: &Vec<_> = &fields
+    let field_strs = fields.iter().filter_map(|&(ref n, _)| n.as_str());
+    let field_bytes = fields.iter().filter_map(|&(ref n, _)| n.as_str().map(|n| quote::ByteStr(n)));


maybe rename into matcharms_str. (field_strs was the original name)

Phaiax · 2017-07-05T12:06:34Z

serde_derive/src/fragment.rs

+    ///
+    /// It will be used as second argument in
+    /// `_serde::Serializer::serialize_struct(__serializer, name, len)`
+    /// while serializing the inner struct in adjacently tagged enums.


This may be a problem, depending on how a serializer serializes a struct name. But I guess that formats that require a tagged enum do not serialize the variant/struct name. (This case is part of the tests and has a comment there also)

I am okay with the behavior as you implemented it.

Phaiax · 2017-07-05T12:07:17Z

serde_derive/src/fragment.rs

+
+/// An extended name, used for the type tag for enums.
+#[derive(Debug, Clone)]
+pub enum StrBoolInt {


Correct file for this struct?

Phaiax · 2017-07-05T12:08:31Z

serde_derive/src/ser.rs

-    let variant_name = variant.attrs.name().serialize_name();
+    let variant_name = match variant.attrs.name().serialize_name() {
+        attr::StrBoolInt::Str(s) => s,
+        _ => unreachable!(),


check.rs prevents this case

Phaiax · 2017-07-05T12:12:37Z

test_suite/tests/test_macros.rs

+            Token::Str("type"),
+            Token::Bool(false)
+        ],
+        "unknown variant `false`, expected one of `abc`, `true`, `3`, `4`, `5`, `6`",


the error message doesn't differentiate between string literals and non string literals ('3' vs 3). Should be ok anyway I hope.

That's fine.

Phaiax · 2017-07-05T12:13:44Z

test_suite/tests/test_macros.rs

+            Token::Str("t"),
+            Token::Bool(true),
+
+            Token::Str("c"),


that is the comment I referred to earlier in the review.

dtolnay

I would prefer not to implement this with rename_as because we will need to support that for long after the better attribute way is stable.

How would you feel about implementing just the attr_literals way and putting some pressure on the lang team to stabilize rust-lang/rust#34981? As far as I can tell there haven't been objections or changes to it over the past 11 months so it should be just about ready.

Alternatively I would be okay with a temporary fork of serde_derive under a different name like serde_derive_experimental that provides the rename_as workaround.

dtolnay · 2017-07-30T18:30:02Z

Currently I implemented a type StrBoolInt, but I had to define it twice, once in serde_derive_internals and once again in serde_derive, so that I can implement ToTokens.

serde_derive depends on serde_derive_internals so it should have access to the StrBoolInt in serde_derive_internals. We can add a dependency of serde_derive_internals on quote if necessary and provide the impl there.

The second StrBoolInt is stuffed into fragment.rs. Should I use a new file or is that ok?

It should be in serde_derive_internals.

I decided to only allow non-string renames for internally and adjacently tagged enums. Correct?

Yes I agree with this choice.

What is that [#serde(field_identifier/variant_identifier)] setting? It is not mentioned in the docs.

They generate a Deserialize impl for reading field names in a struct or variant names in an enum in a way that is compatible with how derive(Deserialize) behaves on structs and externally tagged enums. See the example of field_identifier in Manually implementing Deserialize for a struct. This is helpful when you want custom Deserialize logic but the field names / variant names do not need to be custom.

Ten0 · 2020-04-02T14:24:30Z

What happened to this ? Oh nvm it's replaced by #1392 right?

Phaiax added 5 commits July 1, 2017 01:51

WIP: Allow #[serde(rename=str|bool|int)] for enum variants.

18a5c23

(This commit contains untested code and a dirty hacks so that the rename="str" still works)

WIP: Add #[serde(rename_as)) as fallback for old rustc that do not su…

9576150

…pport non-str attribute literals

Fix #[serde(rename_as)] serialize for non struct variants

bbabd5b

Deserialize with #[serde(rename=bool|int)]

face89c

Add tests for adjacently tagged enum rename

f1cd5a2

Phaiax force-pushed the enum_tag_int branch from bd877df to 26c8b83 Compare July 4, 2017 12:51

Phaiax added 2 commits July 4, 2017 16:26

Fix no_std Bugs, clippy errors (#[serde(rename=int|bool)])

8f1d86c

Fix no_std libc error serde-rs#966. Use cargos libc and not #[feature…

ea177de

…(libc)].

Phaiax force-pushed the enum_tag_int branch from 26c8b83 to ea177de Compare July 4, 2017 14:33

Phaiax commented Jul 5, 2017

View reviewed changes

dtolnay reviewed Jul 30, 2017

View reviewed changes

Geal mentioned this pull request Sep 5, 2017

configuration message versioning sozu-proxy/sozu#240

Closed

dtolnay added the wip label Sep 9, 2017

dtolnay closed this Nov 7, 2017

Geal mentioned this pull request Dec 21, 2017

command message versioning sozu-proxy/sozu#104

Open

dtolnay removed the wip label Mar 27, 2018

This was referenced Sep 14, 2018

Allow integer tags for internally tagged enums #745

Open

Allow rename with str|bool|int type for internally tagged enums #1392

Closed

tonycdot mentioned this pull request Sep 9, 2021

Unify request/response types at the FacTec API client humanode-network/humanode#120

Merged

8 tasks

Uh oh!

Conversation

Phaiax commented Jul 1, 2017

Uh oh!

Phaiax commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Phaiax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

dtolnay commented Jul 30, 2017

Uh oh!

Ten0 commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Phaiax commented Jul 5, 2017 •

edited

Loading

Ten0 commented Apr 2, 2020 •

edited

Loading